[PATCH net-next] gro: move the tc_ext comparison to a helper

[PATCH net-next] gro: move the tc_ext comparison to a helper

[Jakub Kicinski]

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)
+{
+#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

Re: [PATCH net-next] gro: move the tc_ext comparison to a helper

[Simon Horman]

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
> 
> 

Re: [PATCH net-next] gro: move the tc_ext comparison to a helper

[Yunsheng Lin]

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;
> 

Re: [PATCH net-next] gro: move the tc_ext comparison to a helper

[Jakub Kicinski]

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.

Re: [PATCH net-next] gro: move the tc_ext comparison to a helper

[Jakub Kicinski]

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.
-- 
pw-bot: cr

Re: [PATCH net-next] gro: move the tc_ext comparison to a helper

[Simon Horman]

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 :)