Thanks Ben for your feedback, my replies inline.
> -----Original Message-----
> From: Ben Pfaff [mailto:b...@ovn.org]
> Sent: Tuesday, July 11, 2017 8:54 PM
> To: Fischetti, Antonio <antonio.fische...@intel.com>
> Cc: d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 3/3] dpctl: Add new 'ct-bkts' command.
>
> On Fri, Jun 23, 2017 at 01:28:22PM +0100, antonio.fische...@intel.com
> wrote:
> > From: Antonio Fischetti <antonio.fische...@intel.com>
> >
> > 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 <antonio.fische...@intel.com>
> > Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodire...@intel.com>
> > Co-authored-by: Bhanuprakash Bodireddy
> <bhanuprakash.bodire...@intel.com>
>
> Thanks for the patch!
>
> checkpatch reports a few style issues:
>
[AF] my bad, will fix
> WARNING: Line length is >79-characters long
> #145 FILE: lib/dpctl.c:1529:
> dpctl_print(dpctl_p, "\n %3d..%3d | ", i, i +
> NUM_BKTS_PER_ROW - 1);
>
> ERROR: Inappropriate bracing around statement
> #147 FILE: lib/dpctl.c:1531:
> if (conn_per_bkts[i] > gt)
>
>
> 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?
>
> I'd update NEWS to describe the new command.
>
> Thanks,
>
> Ben.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
