Re: [PATCH net-next v2 2/2] openvswitch: Support conntrack zone limit

[Pravin Shelar <pshelar@ovn.org>]

On Wed, Apr 25, 2018 at 2:51 PM, Yi-Hung Wei <yihung.wei@gmail.com> wrote:
>>> +#if    IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT)
>>> +#define OVS_CT_LIMIT_UNLIMITED 0
>>> +#define OVS_CT_LIMIT_DEFAULT OVS_CT_LIMIT_UNLIMITED
>>> +#define CT_LIMIT_HASH_BUCKETS 512
>>> +
>> Can you use static key when the limit is not set.
>> This would avoid overhead in datapath when these limits are not used.
>>
> Thanks Parvin for the review.  I'm not sure about the 'static key'
> part, are you suggesting that say if we can have a flag to check if no
> one has ever set the ct_limit?   If the ct_limit feature is not used
> so far, then instead of look up the hash table for the zone limit, we
> just return the default unlimited value.  So that we can avoid the
> overhead of checking ct_limit.
>

Right, here documentaion of static keys:
https://www.kernel.org/doc/Documentation/static-keys.txt

>>> +struct ovs_ct_limit {
>>> +       /* Elements in ovs_ct_limit_info->limits hash table */
>>> +       struct hlist_node hlist_node;
>>> +       struct rcu_head rcu;
>>> +       u16 zone;
>>> +       u32 limit;
>>> +};
>>> +
>> ...
>
>
>>>  /* Lookup connection and confirm if unconfirmed. */
>>>  static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
>>>                          const struct ovs_conntrack_info *info,
>>> @@ -1054,6 +1176,13 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
>>>         if (!ct)
>>>                 return 0;
>>>
>>> +#if    IS_ENABLED(CONFIG_NETFILTER_CONNCOUNT)
>>> +       err = ovs_ct_check_limit(net, info,
>>> +                                &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple);
>>> +       if (err)
>>> +               return err;
>>> +#endif
>>> +
>>
>> This could be checked during flow install time, so that only permitted
>> flows would have 'ct commit' action, we can avoid per packet cost
>> checking the limit.
> It seems to me that it would be hard to check the # of connections of
> a flow in the flow installation stage.  For example, if we have a
> datapath flow that performs “ct commit” action on all UDP traffic from
> in_port 1, then it could be various combinations of 5-tuple that form
> various # of connections.  Therefore, it would be hard to do the
> admission control there.
>

Ok, I see the issue.
I think we could still avoid the lookup every time by checking the
limits for unconfirmed connections (ref: nf_ct_is_confirmed()).
So once connection confirmed and is under limit  we should allow all
packets for given connection.
This way we can avoid connection disruption when we are reaching near
connection limit.

>
>> returning error code form ovs_ct_commit() is lost in datapath and it
>> would be hard to debug packet lost in case of the limit is reached. So
>> another advantage of checking the limit in flow install be better
>> traceability. datapath would return error to usespace and it can log
>> the error code.
> Thanks for pointing out the error code from ovs_ct_commit() will be
> lost in the datapath.  In this case, shall we consider to report the
> packet drop by some rate_limit logging such as net_warn_ratelimited()
> or net_info_ratelimited()?
>

ok, that would be fine.