On Tue, 24 Nov 2020 19:00:50 +0100 Eric Dumazet wrote: > 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 <[email protected]> > > 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.
Sorry :S I hope it didn't waste too much of your time.. > 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. IDK how this squares against netpoll, though? > 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; > } > >
