| 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
| ||
|
Date: Sat, 13 Feb 2021 13:56:40 +0000
From: Alexander Lobakin <alobakin@...me>
To: Alexander Duyck <alexander.duyck@...il.com>
Cc: Alexander Lobakin <alobakin@...me>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Jonathan Lemon <jonathan.lemon@...il.com>,
Eric Dumazet <edumazet@...gle.com>,
Dmitry Vyukov <dvyukov@...gle.com>,
Willem de Bruijn <willemb@...gle.com>,
Randy Dunlap <rdunlap@...radead.org>,
Kevin Hao <haokexin@...il.com>,
Pablo Neira Ayuso <pablo@...filter.org>,
Jakub Sitnicki <jakub@...udflare.com>,
Marco Elver <elver@...gle.com>,
Dexuan Cui <decui@...rosoft.com>,
Paolo Abeni <pabeni@...hat.com>,
Jesper Dangaard Brouer <brouer@...hat.com>,
Alexander Duyck <alexanderduyck@...com>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andriin@...com>,
Taehee Yoo <ap420073@...il.com>,
Cong Wang <xiyou.wangcong@...il.com>,
Björn Töpel <bjorn@...nel.org>,
Miaohe Lin <linmiaohe@...wei.com>,
Guillaume Nault <gnault@...hat.com>,
Yonghong Song <yhs@...com>, zhudi <zhudi21@...wei.com>,
Michal Kubecek <mkubecek@...e.cz>,
Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
Dmitry Safonov <0x7f454c46@...il.com>,
Yang Yingliang <yangyingliang@...wei.com>,
Florian Westphal <fw@...len.de>,
Edward Cree <ecree.xilinx@...il.com>,
LKML <linux-kernel@...r.kernel.org>,
Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH v5 net-next 06/11] skbuff: remove __kfree_skb_flush()
From: Alexander Duyck <alexander.duyck@...il.com>
Date: Thu, 11 Feb 2021 19:28:52 -0800
> On Thu, Feb 11, 2021 at 10:57 AM Alexander Lobakin <alobakin@...me> wrote:
> >
> > This function isn't much needed as NAPI skb queue gets bulk-freed
> > anyway when there's no more room, and even may reduce the efficiency
> > of bulk operations.
> > It will be even less needed after reusing skb cache on allocation path,
> > so remove it and this way lighten network softirqs a bit.
> >
> > Suggested-by: Eric Dumazet <edumazet@...gle.com>
> > Signed-off-by: Alexander Lobakin <alobakin@...me>
>
> I'm wondering if you have any actual gains to show from this patch?
>
> The reason why I ask is because the flushing was happening at the end
> of the softirq before the system basically gave control back over to
> something else. As such there is a good chance for the memory to be
> dropped from the cache by the time we come back to it. So it may be
> just as expensive if not more so than accessing memory that was just
> freed elsewhere and placed in the slab cache.
Just retested after readding this function (and changing the logics so
it would drop the second half of the cache, like napi_skb_cache_put()
does) and got 10 Mbps drawback with napi_build_skb() +
napi_gro_receive().
So seems like getting a pointer from an array instead of calling
kmem_cache_alloc() is cheaper even if the given object was pulled
out of CPU caches.
> > ---
> > include/linux/skbuff.h | 1 -
> > net/core/dev.c | 7 +------
> > net/core/skbuff.c | 12 ------------
> > 3 files changed, 1 insertion(+), 19 deletions(-)
> >
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 0a4e91a2f873..0e0707296098 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -2919,7 +2919,6 @@ static inline struct sk_buff *napi_alloc_skb(struct napi_struct *napi,
> > }
> > void napi_consume_skb(struct sk_buff *skb, int budget);
> >
> > -void __kfree_skb_flush(void);
> > void __kfree_skb_defer(struct sk_buff *skb);
> >
> > /**
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 321d41a110e7..4154d4683bb9 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -4944,8 +4944,6 @@ static __latent_entropy void net_tx_action(struct softirq_action *h)
> > else
> > __kfree_skb_defer(skb);
> > }
> > -
> > - __kfree_skb_flush();
> > }
> >
> > if (sd->output_queue) {
> > @@ -7012,7 +7010,6 @@ static int napi_threaded_poll(void *data)
> > __napi_poll(napi, &repoll);
> > netpoll_poll_unlock(have);
> >
> > - __kfree_skb_flush();
> > local_bh_enable();
> >
> > if (!repoll)
>
> So it looks like this is the one exception to my comment above. Here
> we should probably be adding a "if (!repoll)" before calling
> __kfree_skb_flush().
>
> > @@ -7042,7 +7039,7 @@ static __latent_entropy void net_rx_action(struct softirq_action *h)
> >
> > if (list_empty(&list)) {
> > if (!sd_has_rps_ipi_waiting(sd) && list_empty(&repoll))
> > - goto out;
> > + return;
> > break;
> > }
> >
> > @@ -7069,8 +7066,6 @@ static __latent_entropy void net_rx_action(struct softirq_action *h)
> > __raise_softirq_irqoff(NET_RX_SOFTIRQ);
> >
> > net_rps_action_and_irq_enable(sd);
> > -out:
> > - __kfree_skb_flush();
> > }
> >
> > struct netdev_adjacent {
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 1c6f6ef70339..4be2bb969535 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -838,18 +838,6 @@ void __consume_stateless_skb(struct sk_buff *skb)
> > kfree_skbmem(skb);
> > }
> >
> > -void __kfree_skb_flush(void)
> > -{
> > - struct napi_alloc_cache *nc = this_cpu_ptr(&napi_alloc_cache);
> > -
> > - /* flush skb_cache if containing objects */
> > - if (nc->skb_count) {
> > - kmem_cache_free_bulk(skbuff_head_cache, nc->skb_count,
> > - nc->skb_cache);
> > - nc->skb_count = 0;
> > - }
> > -}
> > -
> > static inline void _kfree_skb_defer(struct sk_buff *skb)
> > {
> > struct napi_alloc_cache *nc = this_cpu_ptr(&napi_alloc_cache);
> > --
> > 2.30.1
Al
Powered by blists - more mailing lists