| Message ID | 20201102120702.173016-1-roid@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] netdev-offload-tc: Use single thread for probing tc features | expand |
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. > > Signed-off-by: Roi Dayan <roid@nvidia.com> > Reviewed-by: Paul Blakey <paulb@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); >
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@nvidia.com> > > Reviewed-by: Paul Blakey <paulb@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); > > >
On 2020-11-02 2:07 PM, Roi Dayan wrote: > There is no need for a thread start per probe. > > Signed-off-by: Roi Dayan <roid@nvidia.com> > Reviewed-by: Paul Blakey <paulb@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)) { > 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); > Hi, Any comments, can we merge this small change? Thanks, Roi
On 11/10/20 1:39 PM, Roi Dayan wrote: > > > On 2020-11-02 2:07 PM, Roi Dayan wrote: >> There is no need for a thread start per probe. >> >> Signed-off-by: Roi Dayan <roid@nvidia.com> >> Reviewed-by: Paul Blakey <paulb@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)) { >> 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); >> > > > Hi, > > Any comments, can we merge this small change? Have you missed previous replies from me and Simon? They are available on mail list and in patchwork: https://patchwork.ozlabs.org/project/openvswitch/patch/20201102120702.173016-1-roid@nvidia.com/ Best regards, Ilya Maximets.
On 2020-11-10 2:44 PM, Ilya Maximets wrote: > On 11/10/20 1:39 PM, Roi Dayan wrote: >> >> >> On 2020-11-02 2:07 PM, Roi Dayan wrote: >>> There is no need for a thread start per probe. >>> >>> Signed-off-by: Roi Dayan <roid@nvidia.com> >>> Reviewed-by: Paul Blakey <paulb@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)) { >>> 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); >>> >> >> >> Hi, >> >> Any comments, can we merge this small change? > > Have you missed previous replies from me and Simon? > > They are available on mail list and in patchwork: > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fproject%2Fopenvswitch%2Fpatch%2F20201102120702.173016-1-roid%40nvidia.com%2F&data=04%7C01%7Croid%40nvidia.com%7C16c65742faeb4d1d02bd08d8857671a5%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637406091043689812%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=SuBh3JKTlwuHxNg58VFE8FIl%2Bpc%2FO0Zm49VgIs3y%2BC4%3D&reserved=0 > > Best regards, Ilya Maximets. > I did miss them! strange. they didnt get into my inbox. I'll do the update. thanks!
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)) { 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);