Re: [PATCH v2 net-next] net: core: use listified Rx for GRO_NORMAL in napi_gro_receive()

Alexander Lobakin
2019-11-25T12:02:43Z

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,
ᚷ ᛖ ᚢ ᚦ ᚠ ᚱ