Messages in this thread Patch in this message |  | | Date | Mon, 25 Nov 2019 15:02:24 +0300 | | From | Alexander Lobakin <> | | Subject | Re: [PATCH v2 net-next] net: core: use listified Rx for GRO_NORMAL in napi_gro_receive() |
| |
Paolo Abeni wrote 25.11.2019 14:42: > On Mon, 2019-11-25 at 12:05 +0100, Johannes Berg wrote: >> On Mon, 2019-11-25 at 13:58 +0300, Alexander Lobakin wrote: >> > Edward Cree wrote 25.11.2019 13:31: >> > > On 25/11/2019 09:09, Nicholas Johnson wrote: >> > > > The default value of /proc/sys/net/core/gro_normal_batch was 8. >> > > > Setting it to 1 allowed it to connect to Wi-Fi network. >> > > > >> > > > Setting it back to 8 did not kill the connection. >> > > > >> > > > But when I disconnected and tried to reconnect, it did not re-connect. >> > > > >> > > > Hence, it appears that the problem only affects the initial handshake >> > > > when associating with a network, and not normal packet flow. >> > > That sounds like the GRO batch isn't getting flushed at the endof the >> > > NAPI — maybe the driver isn't calling napi_complete_done() at the >> > > appropriate time? >> > >> > Yes, this was the first reason I thought about, but didn't look at >> > iwlwifi yet. I already knew this driver has some tricky parts, but >> > this 'fake NAPI' solution seems rather strange to me. >> >> Truth be told, we kinda just fudged it until we got GRO, since that's >> what we really want on wifi (to reduce the costly TCP ACKs if >> possible). >> >> Maybe we should call napi_complete_done() instead? But as Edward noted >> (below), we don't actually really do NAPI polling, we just fake it for >> each interrupt since we will often get a lot of frames in one >> interrupt >> if there's high throughput (A-MPDUs are basically coming in all at the >> same time). I've never really looked too much at what exactly happens >> here, beyond seeing the difference from GRO. >> >> >> > > Indeed, from digging through the layers of iwlwifi I eventually get to >> > > iwl_pcie_rx_handle() which doesn't really have a NAPI poll (the >> > > napi->poll function is iwl_pcie_dummy_napi_poll() { WARN_ON(1); >> > > return 0; }) and instead calls napi_gro_flush() at the end of its RX >> > > handling. Unfortunately, napi_gro_flush() is no longer enough, >> > > because it doesn't call gro_normal_list() so the packets on the >> > > GRO_NORMAL list just sit there indefinitely. >> > > >> > > It was seeing drivers calling napi_gro_flush() directly that had me >> > > worried in the first place about whether listifying napi_gro_receive() >> > > was safe and where the gro_normal_list() should go. >> > > I wondered if other drivers that show up in [1] needed fixing with a >> > > gro_normal_list() next to their napi_gro_flush() call. From a cursory >> > > check: >> > > brocade/bna: has a real poller, calls napi_complete_done() so is OK. >> > > cortina/gemini: calls napi_complete_done() straight after >> > > napi_gro_flush(), so is OK. >> > > hisilicon/hns3: calls napi_complete(), so is _probably_ OK. >> > > But it's far from clear to me why *any* of those drivers are calling >> > > napi_gro_flush() themselves... >> > >> > Agree. I mean, we _can_ handle this particular problem from networking >> > core side, but from my point of view only rethinking driver's logic is >> > the correct way to solve this and other issues that may potentionally >> > appear in future. >> >> Do tell what you think it should be doing :) >> >> One additional wrinkle is that we have firmware notifications, command >> completions and actual RX interleaved, so I think we do want to have >> interrupts for the notifications and command completions? > > I think it would be nice moving the iwlwifi driver to full/plain NAPI > mode. The interrupt handler could keep processing extra work as it does > now and queue real pkts on some internal queue, and than schedule the > relevant napi, which in turn could process such queue in the napi poll > method. Likely I missed tons of details and/or oversimplified it...
Yep, full NAPI is the best variant, but I may miss a lot too.
> For -net, I *think* something as dumb and hacky as the following could > possibly work: > ---- > diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/rx.c > b/drivers/net/wireless/intel/iwlwifi/pcie/rx.c > index 4bba6b8a863c..df82fad96cbb 100644 > --- a/drivers/net/wireless/intel/iwlwifi/pcie/rx.c > +++ b/drivers/net/wireless/intel/iwlwifi/pcie/rx.c > @@ -1527,7 +1527,7 @@ static void iwl_pcie_rx_handle(struct iwl_trans > *trans, int queue) > iwl_pcie_rxq_alloc_rbs(trans, GFP_ATOMIC, rxq); > > if (rxq->napi.poll) > - napi_gro_flush(&rxq->napi, false); > + napi_complete_done(&rxq->napi, 0); > > iwl_pcie_rxq_restock(trans, rxq); > } > ---
napi_complete_done(napi, 0) has an equivalent static inline napi_complete(napi). I'm not sure it will work without any issues as iwlwifi doesn't _really_ turn NAPI into scheduling state.
I'm not very familiar with iwlwifi, but as a work around manual napi_gro_flush() you can also manually flush napi->rx_list to prevent packets from stalling:
diff -Naur a/drivers/net/wireless/intel/iwlwifi/pcie/rx.c b/drivers/net/wireless/intel/iwlwifi/pcie/rx.c --- a/drivers/net/wireless/intel/iwlwifi/pcie/rx.c 2019-11-25 14:55:03.610355230 +0300 +++ b/drivers/net/wireless/intel/iwlwifi/pcie/rx.c 2019-11-25 14:57:29.399556868 +0300 @@ -1526,8 +1526,16 @@ if (unlikely(emergency && count)) iwl_pcie_rxq_alloc_rbs(trans, GFP_ATOMIC, rxq);
- if (rxq->napi.poll) + if (rxq->napi.poll) { + if (rxq->napi.rx_count) { + netif_receive_skb_list(&rxq->napi.rx_list); + + INIT_LIST_HEAD(&rxq->napi.rx_list); + rxq->napi.rx_count = 0; + } + napi_gro_flush(&rxq->napi, false); + }
iwl_pcie_rxq_restock(trans, rxq); } > Cheers, > > Paolo
Regards, ᚷ ᛖ ᚢ ᚦ ᚠ ᚱ
|  |