From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8363FC433E6 for ; Sat, 13 Feb 2021 13:57:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 4BB8F64E35 for ; Sat, 13 Feb 2021 13:57:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229691AbhBMN5k (ORCPT ); Sat, 13 Feb 2021 08:57:40 -0500 Received: from mail-40131.protonmail.ch ([185.70.40.131]:49790 "EHLO mail-40131.protonmail.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229584AbhBMN5h (ORCPT ); Sat, 13 Feb 2021 08:57:37 -0500 Date: Sat, 13 Feb 2021 13:56:40 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pm.me; s=protonmail; t=1613224611; bh=T6KIeabV5sI7XCFa/PJx1BokACoH/f2hbL4xDuJnHrw=; h=Date:To:From:Cc:Reply-To:Subject:In-Reply-To:References:From; b=paJxQJHQ6QsKrY/FqcS3AXMj5XvUuQ90BRqLHfuUsi9sWtk2cLIaYm+tHM3cNPOXE hWNhJER6x7rJCLYwz7ifoh6QseyWydbnO7bNjI88HMlCcJuxtVZ+MRy4O9mAcI75i9 1xvBPF74nQtOT4vaJUsbWYhFmQTIDt7XTILs8FPjL1Yf3gWTDr7d84paGVEQHkZGbj nB8qpQdsijf1m6XNi4NEDHZTsO4eURX0Aqu3IZ+86+YhUdb0550UMZLqiETXmiV9+P ABbATPXQXpOmD6yhsv6GAC40z5OW4KErtHtpdp2RKfVdek7WuoNsVsV8FccVS7p6gM 9ha3bAL1AC/rA== To: Alexander Duyck From: Alexander Lobakin Cc: Alexander Lobakin , "David S. Miller" , Jakub Kicinski , Jonathan Lemon , Eric Dumazet , Dmitry Vyukov , Willem de Bruijn , Randy Dunlap , Kevin Hao , Pablo Neira Ayuso , Jakub Sitnicki , Marco Elver , Dexuan Cui , Paolo Abeni , Jesper Dangaard Brouer , Alexander Duyck , Alexei Starovoitov , Daniel Borkmann , Andrii Nakryiko , Taehee Yoo , Cong Wang , =?utf-8?Q?Bj=C3=B6rn_T=C3=B6pel?= , Miaohe Lin , Guillaume Nault , Yonghong Song , zhudi , Michal Kubecek , Marcelo Ricardo Leitner , Dmitry Safonov <0x7f454c46@gmail.com>, Yang Yingliang , Florian Westphal , Edward Cree , LKML , Netdev Reply-To: Alexander Lobakin Subject: Re: [PATCH v5 net-next 06/11] skbuff: remove __kfree_skb_flush() Message-ID: <20210213135604.86581-1-alobakin@pm.me> In-Reply-To: References: <20210211185220.9753-1-alobakin@pm.me> <20210211185220.9753-7-alobakin@pm.me> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org From: Alexander Duyck Date: Thu, 11 Feb 2021 19:28:52 -0800 > On Thu, Feb 11, 2021 at 10:57 AM Alexander Lobakin 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 > > Signed-off-by: Alexander Lobakin >=20 > I'm wondering if you have any actual gains to show from this patch? >=20 > 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(stru= ct 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) >=20 > 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(). >=20 > > @@ -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 =3D 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 =3D 0; > > - } > > -} > > - > > static inline void _kfree_skb_defer(struct sk_buff *skb) > > { > > struct napi_alloc_cache *nc =3D this_cpu_ptr(&napi_alloc_cache)= ; > > -- > > 2.30.1 Al