On Wed, Apr 25, 2018 at 2:51 PM, Yi-Hung Wei <yihung....@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.
