> -----Original Message----- > From: Ben Pfaff [mailto:[email protected]] > Sent: Friday, July 14, 2017 7:29 PM > To: Fischetti, Antonio <[email protected]> > Cc: CC: <[email protected]> > Subject: Re: [ovs-dev] [PATCH 3/3] dpctl: Add new 'ct-bkts' command. > > On Fri, Jul 14, 2017 at 10:11:04AM +0000, Fischetti, Antonio wrote: > > Thanks Ben for your feedback, my replies inline. > > > > > -----Original Message----- > > > From: Ben Pfaff [mailto:[email protected]] > > > Sent: Tuesday, July 11, 2017 8:54 PM > > > To: Fischetti, Antonio <[email protected]> > > > Cc: [email protected] > > > Subject: Re: [ovs-dev] [PATCH 3/3] dpctl: Add new 'ct-bkts' command. > > > > > > On Fri, Jun 23, 2017 at 01:28:22PM +0100, [email protected] > > > wrote: > > > > From: Antonio Fischetti <[email protected]> > > > > > > > > With the command: > > > > ovs-appctl dpctl/ct-bkts > > > > shows the number of connections per bucket. > > > > > > > > By using a threshold: > > > > ovs-appctl dpctl/ct-bkts gt=N > > > > for each bucket shows the number of connections when they > > > > are greater than N. > > > > > > > > Signed-off-by: Antonio Fischetti <[email protected]> > > > > Signed-off-by: Bhanuprakash Bodireddy > <[email protected]> > > > > Co-authored-by: Bhanuprakash Bodireddy > > > <[email protected]> > > > > > > Is this concept of buckets one that the kernel conntracker has too? > If > > > so, then I'm concerned about the definition of conn_per_buckets[] in > > > dpctl_ct_bkts(), since it has a hardcoded CONNTRACK_BUCKETS size and > > > nothing checks whether cte.bkt is in the right range. If not, then > > > should this command be one that is limited to the userspace > conntracker? > > > > [AF] You're right, I didn't realize the concept of buckets > > is in the kernel CT too, I did this implementation with the > > userspace ConnTracker in mind. > > > > Would it be ok to have a first implementation limited to the userspace > CT like: > > > > dpctl_ct_bkts(...) > > { > > if (!dpctl_p->is_appctl) { /* Not called by ovs-appctl command => > exit. */ > > dpctl_print(dpctl_p, "Command is available for UserSpace > ConnTracker only.\n"); > > return 0; > > } > > > > < implementation for Userspace CT here > > > ... > > > > } > > > > so that later on it could be extended to cover also the kernel CT case? > > Is it much work to extend it to cover the kernel? If not, it would be > better to cover both. Otherwise, sure, this approach is an OK way to > start.
[AF] I'd prefer to start with an implementation for userspace only. I did some investigation on how to cover the kernel case too but - as I'm not enough familiar with the details of the kernel CT implementation - I'd limit this patch to the userspace CT for now. I'll post a v2 soon. Thanks, Antonio _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
