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

[Simon Horman <simon.horman@corigine.com>]

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