netdev - Re: [PATCH net-next] gro: move the tc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <81d765b3-8545-8791-4d1e-3b0f0dba39c0@huawei.com>
Date: Wed, 14 Jun 2023 20:58:53 +0800
From: Yunsheng Lin <linyunsheng@...wei.com>
To: Jakub Kicinski <kuba@...nel.org>, <davem@...emloft.net>
CC: <netdev@...r.kernel.org>, <edumazet@...gle.com>, <pabeni@...hat.com>,
	<richardbgobert@...il.com>
Subject: Re: [PATCH net-next] gro: move the tc_ext comparison to a helper

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@...nel.org>
> ---
> CC: richardbgobert@...il.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;
>