| Message ID | 20230613205105.1996166-1-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] gro: move the tc_ext comparison to a helper | expand |
On Tue, Jun 13, 2023 at 01:51:05PM -0700, Jakub Kicinski wrote: > The double ifdefs are quite aesthetically displeasing. > Use a helper function to make the code more compact. > The resulting machine code looks the same (with minor > movement of some basic blocks). > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> Hi Jakub, I like your patch, and I think it is a good improvement, but I find the patch description slightly confusing. In my understanding of things this patch is doing two things: 1) Moving code into a helper 2) Eliminating a check on CONFIG_SKB_EXTENSIONS, presumably because it is selected by NET_TC_SKB_EXT. But the patch description seems to conflate these. In any case, code looks good. Reviewed-by: Simon Horman <simon.horman@corigine.com> > --- > CC: richardbgobert@gmail.com > --- > net/core/gro.c | 31 ++++++++++++++++++------------- > 1 file changed, 18 insertions(+), 13 deletions(-) > > diff --git a/net/core/gro.c b/net/core/gro.c > index ab9a447dfba7..90889e1f3f9a 100644 > --- a/net/core/gro.c > +++ b/net/core/gro.c > @@ -305,6 +305,23 @@ void napi_gro_flush(struct napi_struct *napi, bool flush_old) > } > EXPORT_SYMBOL(napi_gro_flush); > > +static void gro_list_prepare_tc_ext(const struct sk_buff *skb, > + const struct sk_buff *p, > + unsigned long *diffs) > +{ > +#if IS_ENABLED(CONFIG_NET_TC_SKB_EXT) > + struct tc_skb_ext *skb_ext; > + struct tc_skb_ext *p_ext; > + > + skb_ext = skb_ext_find(skb, TC_SKB_EXT); > + p_ext = skb_ext_find(p, TC_SKB_EXT); > + > + *diffs |= (!!p_ext) ^ (!!skb_ext); > + if (!*diffs && unlikely(skb_ext)) > + *diffs |= p_ext->chain ^ skb_ext->chain; > +#endif > +} > + > static void gro_list_prepare(const struct list_head *head, > const struct sk_buff *skb) > { > @@ -339,23 +356,11 @@ static void gro_list_prepare(const struct list_head *head, > * avoid trying too hard to skip each of them individually > */ > if (!diffs && unlikely(skb->slow_gro | p->slow_gro)) { > -#if IS_ENABLED(CONFIG_SKB_EXTENSIONS) && IS_ENABLED(CONFIG_NET_TC_SKB_EXT) > - struct tc_skb_ext *skb_ext; > - struct tc_skb_ext *p_ext; > -#endif > - > diffs |= p->sk != skb->sk; > diffs |= skb_metadata_dst_cmp(p, skb); > diffs |= skb_get_nfct(p) ^ skb_get_nfct(skb); > > -#if IS_ENABLED(CONFIG_SKB_EXTENSIONS) && IS_ENABLED(CONFIG_NET_TC_SKB_EXT) > - skb_ext = skb_ext_find(skb, TC_SKB_EXT); > - p_ext = skb_ext_find(p, TC_SKB_EXT); > - > - diffs |= (!!p_ext) ^ (!!skb_ext); > - if (!diffs && unlikely(skb_ext)) > - diffs |= p_ext->chain ^ skb_ext->chain; > -#endif > + gro_list_prepare_tc_ext(skb, p, &diffs); > } > > NAPI_GRO_CB(p)->same_flow = !diffs; > -- > 2.40.1 > >
On 2023/6/14 4:51, Jakub Kicinski wrote: > The double ifdefs are quite aesthetically displeasing. > Use a helper function to make the code more compact. > The resulting machine code looks the same (with minor > movement of some basic blocks). > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > CC: richardbgobert@gmail.com > --- > net/core/gro.c | 31 ++++++++++++++++++------------- > 1 file changed, 18 insertions(+), 13 deletions(-) > > diff --git a/net/core/gro.c b/net/core/gro.c > index ab9a447dfba7..90889e1f3f9a 100644 > --- a/net/core/gro.c > +++ b/net/core/gro.c > @@ -305,6 +305,23 @@ void napi_gro_flush(struct napi_struct *napi, bool flush_old) > } > EXPORT_SYMBOL(napi_gro_flush); > > +static void gro_list_prepare_tc_ext(const struct sk_buff *skb, > + const struct sk_buff *p, > + unsigned long *diffs) Isn't it more common to do something like below? static unsigned long gro_list_prepare_tc_ext(const struct sk_buff *skb, const struct sk_buff *p, unsigned long diffs) Is it because the resulting machine code is bigger for the above case? > +{ > +#if IS_ENABLED(CONFIG_NET_TC_SKB_EXT) > + struct tc_skb_ext *skb_ext; > + struct tc_skb_ext *p_ext; > + > + skb_ext = skb_ext_find(skb, TC_SKB_EXT); > + p_ext = skb_ext_find(p, TC_SKB_EXT); > + > + *diffs |= (!!p_ext) ^ (!!skb_ext); > + if (!*diffs && unlikely(skb_ext)) > + *diffs |= p_ext->chain ^ skb_ext->chain; > +#endif > +} > + > static void gro_list_prepare(const struct list_head *head, > const struct sk_buff *skb) > { > @@ -339,23 +356,11 @@ static void gro_list_prepare(const struct list_head *head, > * avoid trying too hard to skip each of them individually > */ > if (!diffs && unlikely(skb->slow_gro | p->slow_gro)) { > -#if IS_ENABLED(CONFIG_SKB_EXTENSIONS) && IS_ENABLED(CONFIG_NET_TC_SKB_EXT) > - struct tc_skb_ext *skb_ext; > - struct tc_skb_ext *p_ext; > -#endif > - > diffs |= p->sk != skb->sk; > diffs |= skb_metadata_dst_cmp(p, skb); > diffs |= skb_get_nfct(p) ^ skb_get_nfct(skb); > > -#if IS_ENABLED(CONFIG_SKB_EXTENSIONS) && IS_ENABLED(CONFIG_NET_TC_SKB_EXT) > - skb_ext = skb_ext_find(skb, TC_SKB_EXT); > - p_ext = skb_ext_find(p, TC_SKB_EXT); > - > - diffs |= (!!p_ext) ^ (!!skb_ext); > - if (!diffs && unlikely(skb_ext)) > - diffs |= p_ext->chain ^ skb_ext->chain; > -#endif > + gro_list_prepare_tc_ext(skb, p, &diffs); > } > > NAPI_GRO_CB(p)->same_flow = !diffs; >
On Wed, 14 Jun 2023 14:23:21 +0200 Simon Horman wrote: > I like your patch, and I think it is a good improvement, > but I find the patch description slightly confusing. > > In my understanding of things this patch is doing two things: > > 1) Moving code into a helper > 2) Eliminating a check on CONFIG_SKB_EXTENSIONS, > presumably because it is selected by NET_TC_SKB_EXT. Ah, I thought removing the check for CONFIG_SKB_EXTENSIONS is not worth mentioning. The double ifdefs I was talking about was the fact that we need an ifdef both around the variable declarations and the comparisons themselves. > But the patch description seems to conflate these. > > In any case, code looks good.
On Wed, 14 Jun 2023 20:58:53 +0800 Yunsheng Lin wrote: > > +static void gro_list_prepare_tc_ext(const struct sk_buff *skb, > > + const struct sk_buff *p, > > + unsigned long *diffs) > > Isn't it more common to do something like below? > > static unsigned long gro_list_prepare_tc_ext(const struct sk_buff *skb, > const struct sk_buff *p, > unsigned long diffs) > > Is it because the resulting machine code is bigger for the above > case? Not really, just laziness - the above is more typing. I guess avoiding output args is worth while, so between this and Simon's comment - let me respin.
On Wed, Jun 14, 2023 at 10:44:49AM -0700, Jakub Kicinski wrote: > On Wed, 14 Jun 2023 14:23:21 +0200 Simon Horman wrote: > > I like your patch, and I think it is a good improvement, > > but I find the patch description slightly confusing. > > > > In my understanding of things this patch is doing two things: > > > > 1) Moving code into a helper > > 2) Eliminating a check on CONFIG_SKB_EXTENSIONS, > > presumably because it is selected by NET_TC_SKB_EXT. > > Ah, I thought removing the check for CONFIG_SKB_EXTENSIONS > is not worth mentioning. The double ifdefs I was talking about > was the fact that we need an ifdef both around the variable > declarations and the comparisons themselves. Ok, now I understand. Looks good :)
diff --git a/net/core/gro.c b/net/core/gro.c index ab9a447dfba7..90889e1f3f9a 100644 --- a/net/core/gro.c +++ b/net/core/gro.c @@ -305,6 +305,23 @@ void napi_gro_flush(struct napi_struct *napi, bool flush_old) } EXPORT_SYMBOL(napi_gro_flush); +static void gro_list_prepare_tc_ext(const struct sk_buff *skb, + const struct sk_buff *p, + unsigned long *diffs) +{ +#if IS_ENABLED(CONFIG_NET_TC_SKB_EXT) + struct tc_skb_ext *skb_ext; + struct tc_skb_ext *p_ext; + + skb_ext = skb_ext_find(skb, TC_SKB_EXT); + p_ext = skb_ext_find(p, TC_SKB_EXT); + + *diffs |= (!!p_ext) ^ (!!skb_ext); + if (!*diffs && unlikely(skb_ext)) + *diffs |= p_ext->chain ^ skb_ext->chain; +#endif +} + static void gro_list_prepare(const struct list_head *head, const struct sk_buff *skb) { @@ -339,23 +356,11 @@ static void gro_list_prepare(const struct list_head *head, * avoid trying too hard to skip each of them individually */ if (!diffs && unlikely(skb->slow_gro | p->slow_gro)) { -#if IS_ENABLED(CONFIG_SKB_EXTENSIONS) && IS_ENABLED(CONFIG_NET_TC_SKB_EXT) - struct tc_skb_ext *skb_ext; - struct tc_skb_ext *p_ext; -#endif - diffs |= p->sk != skb->sk; diffs |= skb_metadata_dst_cmp(p, skb); diffs |= skb_get_nfct(p) ^ skb_get_nfct(skb); -#if IS_ENABLED(CONFIG_SKB_EXTENSIONS) && IS_ENABLED(CONFIG_NET_TC_SKB_EXT) - skb_ext = skb_ext_find(skb, TC_SKB_EXT); - p_ext = skb_ext_find(p, TC_SKB_EXT); - - diffs |= (!!p_ext) ^ (!!skb_ext); - if (!diffs && unlikely(skb_ext)) - diffs |= p_ext->chain ^ skb_ext->chain; -#endif + gro_list_prepare_tc_ext(skb, p, &diffs); } NAPI_GRO_CB(p)->same_flow = !diffs;
The double ifdefs are quite aesthetically displeasing. Use a helper function to make the code more compact. The resulting machine code looks the same (with minor movement of some basic blocks). Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- CC: richardbgobert@gmail.com --- net/core/gro.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-)