From: Eric Dumazet <eric.dumazet@gmail•com>
To: Jakub Kicinski <kuba@kernel•org>, davem@davemloft•net
Cc: netdev@vger•kernel.org, eric.dumazet@gmail•com, kernel-team@fb•com
Subject: Re: [PATCH net-next 1/3] net: remove napi_hash_del() from driver-facing API
Date: Tue, 24 Nov 2020 19:00:50 +0100 [thread overview]
Message-ID: <8735d11e-e734-2ba9-7ced-d047682f9f3e@gmail.com> (raw)
In-Reply-To: <20200909173753.229124-2-kuba@kernel.org>
On 9/9/20 7:37 PM, Jakub Kicinski wrote:
> We allow drivers to call napi_hash_del() before calling
> netif_napi_del() to batch RCU grace periods. This makes
> the API asymmetric and leaks internal implementation details.
> Soon we will want the grace period to protect more than just
> the NAPI hash table.
>
> Restructure the API and have drivers call a new function -
> __netif_napi_del() if they want to take care of RCU waits.
>
> Note that only core was checking the return status from
> napi_hash_del() so the new helper does not report if the
> NAPI was actually deleted.
>
> Some notes on driver oddness:
> - veth observed the grace period before calling netif_napi_del()
> but that should not matter
> - myri10ge observed normal RCU flavor
> - bnx2x and enic did not actually observe the grace period
> (unless they did so implicitly)
> - virtio_net and enic only unhashed Rx NAPIs
>
> The last two points seem to indicate that the calls to
> napi_hash_del() were a left over rather than an optimization.
> Regardless, it's easy enough to correct them.
>
> This patch may introduce extra synchronize_net() calls for
> interfaces which set NAPI_STATE_NO_BUSY_POLL and depend on
> free_netdev() to call netif_napi_del(). This seems inevitable
> since we want to use RCU for netpoll dev->napi_list traversal,
> and almost no drivers set IFF_DISABLE_NETPOLL.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel•org>
After this patch, gro_cells_destroy() became damn slow
on hosts with a lot of cores.
After your change, we have one additional synchronize_net() per cpu as
you stated in your changelog.
gro_cells_init() is setting NAPI_STATE_NO_BUSY_POLL, and this was enough
to not have one synchronize_net() call per netif_napi_del()
I will test something like :
I am not yet convinced the synchronize_net() is needed, since these
NAPI structs are not involved in busy polling.
diff --git a/net/core/gro_cells.c b/net/core/gro_cells.c
index e095fb871d9120787bfdf62149f4d82e0e3b0a51..8cfa6ce0738977290cc9f76a3f5daa617308e107 100644
--- a/net/core/gro_cells.c
+++ b/net/core/gro_cells.c
@@ -99,9 +99,10 @@ void gro_cells_destroy(struct gro_cells *gcells)
struct gro_cell *cell = per_cpu_ptr(gcells->cells, i);
napi_disable(&cell->napi);
- netif_napi_del(&cell->napi);
+ __netif_napi_del(&cell->napi);
__skb_queue_purge(&cell->napi_skbs);
}
+ synchronize_net();
free_percpu(gcells->cells);
gcells->cells = NULL;
}
next prev parent reply other threads:[~2020-11-24 18:00 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-09 17:37 [PATCH net-next 0/3] netpoll: make sure napi_list is safe for RCU traversal Jakub Kicinski
2020-09-09 17:37 ` [PATCH net-next 1/3] net: remove napi_hash_del() from driver-facing API Jakub Kicinski
2020-11-24 18:00 ` Eric Dumazet [this message]
2020-11-24 18:54 ` Jakub Kicinski
2020-11-24 19:11 ` Eric Dumazet
2020-11-24 19:36 ` Jakub Kicinski
2020-09-09 17:37 ` [PATCH net-next 2/3] net: manage napi add/del idempotence explicitly Jakub Kicinski
2020-09-09 17:37 ` [PATCH net-next 3/3] net: make sure napi_list is safe for RCU traversal Jakub Kicinski
2020-09-10 20:09 ` [PATCH net-next 0/3] netpoll: " David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8735d11e-e734-2ba9-7ced-d047682f9f3e@gmail.com \
--to=eric.dumazet@gmail$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=kernel-team@fb$(echo .)com \
--cc=kuba@kernel$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox