[ovs-dev] [PATCH] netdev-offload-tc: Use single thread for probing tc features

Simon Horman simon.horman at netronome.com
Thu Nov 5 16:38:05 UTC 2020


On Tue, Nov 03, 2020 at 12:34:14PM +0100, Ilya Maximets wrote:
> On 11/2/20 1:07 PM, Roi Dayan wrote:
> > There is no need for a thread start per probe.
> 
> This sounds like we're actually starting some pthreads here.
> I think, it should be something like:
> "There is no need for a 'once' variable per probe."
> 
> Same for the patch name. E.g.
> "netdev-offload-tc: Use single 'once' variable for probing tc features."
> 
> One minor comment inline.
> 
> Best regards, Ilya Maximets.

Thanks,

this also looks good to me.

Roi, let me know if you want me to go ahead and apply this with or without
Ilya's suggestion below. It appears to apply cleanly back to branch-2.13.
If appropriate please consider supplying backports to older branches.

Kind regards,
Simon

> > Signed-off-by: Roi Dayan <roid at nvidia.com>
> > Reviewed-by: Paul Blakey <paulb at mellanox.com>
> > ---
> >  lib/netdev-offload-tc.c | 11 +++--------
> >  1 file changed, 3 insertions(+), 8 deletions(-)
> > 
> > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> > index e828a8683910..53662ef3f0e6 100644
> > --- a/lib/netdev-offload-tc.c
> > +++ b/lib/netdev-offload-tc.c
> > @@ -1988,8 +1988,7 @@ probe_tc_block_support(int ifindex)
> >  static int
> >  netdev_tc_init_flow_api(struct netdev *netdev)
> >  {
> > -    static struct ovsthread_once multi_mask_once = OVSTHREAD_ONCE_INITIALIZER;
> > -    static struct ovsthread_once block_once = OVSTHREAD_ONCE_INITIALIZER;
> > +    static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> >      enum tc_qdisc_hook hook = get_tc_qdisc_hook(netdev);
> >      uint32_t block_id = 0;
> >      struct tcf_id id;
> > @@ -2014,16 +2013,12 @@ netdev_tc_init_flow_api(struct netdev *netdev)
> >      /* make sure there is no ingress/egress qdisc */
> >      tc_add_del_qdisc(ifindex, false, 0, hook);
> >  
> > -    if (ovsthread_once_start(&block_once)) {
> > +    if (ovsthread_once_start(&once)) {
> >          probe_tc_block_support(ifindex);
> >          /* Need to re-fetch block id as it depends on feature availability. */
> >          block_id = get_block_id_from_netdev(netdev);
> > -        ovsthread_once_done(&block_once);
> > -    }
> > -
> > -    if (ovsthread_once_start(&multi_mask_once)) {
> 
> It might be good to have an empty line here to visually separate block-related
> things from the 'mask' ones.
> 
> >          probe_multi_mask_per_prio(ifindex);
> > -        ovsthread_once_done(&multi_mask_once);
> > +        ovsthread_once_done(&once);
> >      }
> >  
> >      error = tc_add_del_qdisc(ifindex, true, block_id, hook);
> > 
> 


More information about the dev mailing list