netdev - Re: [PATCH net-next 2/3] net: skbuff: cache one skb

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: <ef9ab8960763289e990b0010ee2aa761c3ee80a3.camel@redhat.com>
Date:   Wed, 15 Feb 2023 09:41:13 +0100
From:   Paolo Abeni <pabeni@...hat.com>
To:     Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc:     netdev@...r.kernel.org, edumazet@...gle.com, willemb@...gle.com,
        fw@...len.de
Subject: Re: [PATCH net-next 2/3] net: skbuff: cache one skb_ext for use by
 GRO

On Tue, 2023-02-14 at 19:43 -0800, Jakub Kicinski wrote:
> On the driver -> GRO path we can avoid thrashing the kmemcache
> by holding onto one skb_ext.
> 
> Drivers usually report static data, so don't bother trying to
> hold onto the skb_ext if the ext has contents which require
> a destructor.
> 
> With a single flow and SW GRO adding a tc_skb_ext to every
> frame costs around 16.6% of performance (21.2Gbps -> 17.6Gbps,
> yes it's a relatively slow CPU). Using the cache reduces
> the loss to 9.3%, (-> 19.2Gbps) although obviously in real
> life the recycling will be less effective.
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
>  include/linux/skbuff.h |  1 +
>  net/core/skbuff.c      | 79 +++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 75 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index d5602b15c714..e68cb0a777b9 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -4622,6 +4622,7 @@ struct skb_ext *__skb_ext_alloc(gfp_t flags);
>  void *__skb_ext_set(struct sk_buff *skb, enum skb_ext_id id,
>  		    struct skb_ext *ext);
>  void *skb_ext_add(struct sk_buff *skb, enum skb_ext_id id);
> +void *napi_skb_ext_add(struct sk_buff *skb, enum skb_ext_id id);
>  void __skb_ext_del(struct sk_buff *skb, enum skb_ext_id id);
>  void __skb_ext_put(struct skb_ext *ext);
>  
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 6f0fc1f09536..feb5034b13ad 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -224,6 +224,9 @@ static void *page_frag_alloc_1k(struct page_frag_1k *nc, gfp_t gfp_mask)
>  struct napi_alloc_cache {
>  	struct page_frag_cache page;
>  	struct page_frag_1k page_small;
> +#ifdef CONFIG_SKB_EXTENSIONS
> +	struct skb_ext *ext;
> +#endif
>  	unsigned int skb_count;
>  	void *skb_cache[NAPI_SKB_CACHE_SIZE];
>  };
> @@ -1228,6 +1231,43 @@ static void napi_skb_cache_put(struct sk_buff *skb)
>  	}
>  }
>  
> +static bool skb_ext_needs_destruct(const struct skb_ext *ext)
> +{
> +	bool needs_destruct = false;
> +
> +#ifdef CONFIG_XFRM
> +	needs_destruct |= __skb_ext_exist(ext, SKB_EXT_SEC_PATH);
> +#endif
> +#ifdef CONFIG_MCTP_FLOWS
> +	needs_destruct |= __skb_ext_exist(ext, SKB_EXT_MCTP);
> +#endif
> +
> +	return needs_destruct;
> +}
> +
> +static void napi_skb_ext_put(struct sk_buff *skb)
> +{
> +#ifdef CONFIG_SKB_EXTENSIONS
> +	struct skb_ext *ext;
> +
> +	if (!skb->active_extensions)
> +		return;
> +
> +	ext = skb->extensions;
> +	if (!skb_ext_needs_destruct(ext)) {
> +		struct napi_alloc_cache *nc = this_cpu_ptr(&napi_alloc_cache);
> +
> +		if (refcount_read(&ext->refcnt) == 1 && !nc->ext) {
> +			kasan_poison_object_data(skbuff_ext_cache, ext);
> +			nc->ext = ext;
> +			return;
> +		}
> +	}
> +
> +	__skb_ext_put(ext);
> +#endif
> +}
> +
>  void __kfree_skb_defer(struct sk_buff *skb)

I'm wondering if napi_reuse_skb() should be touched, too? Even it's not
directly used by the following patch...

Cheers,

Paolo