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

From: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
To: Edward Cree <ecree@solarflare.com>
Cc: Alexander Lobakin <alobakin@dlink.ru>,
	David Miller <davem@davemloft.net>,
	"jiri@mellanox.com" <jiri@mellanox.com>,
	"edumazet@google.com" <edumazet@google.com>,
	"idosch@mellanox.com" <idosch@mellanox.com>,
	"pabeni@redhat.com" <pabeni@redhat.com>,
	"petrm@mellanox.com" <petrm@mellanox.com>,
	"sd@queasysnail.net" <sd@queasysnail.net>,
	"f.fainelli@gmail.com" <f.fainelli@gmail.com>,
	"jaswinder.singh@linaro.org" <jaswinder.singh@linaro.org>,
	"ilias.apalodimas@linaro.org" <ilias.apalodimas@linaro.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"johannes.berg@intel.com" <johannes.berg@intel.com>,
	"emmanuel.grumbach@intel.com" <emmanuel.grumbach@intel.com>,
	"luciano.coelho@intel.com" <luciano.coelho@intel.com>,
	"linuxwifi@intel.com" <linuxwifi@intel.com>,
	"kvalo@codeaurora.org" <kvalo@codeaurora.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH v2 net-next] net: core: use listified Rx for GRO_NORMAL in napi_gro_receive()
Date: Mon, 25 Nov 2019 13:11:18 +0000	[thread overview]
Message-ID: <PSXP216MB0438DBC8CF65DCA2DD0E3441804A0@PSXP216MB0438.KORP216.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <cc08834c-ccb3-263a-2967-f72a9d72535a@solarflare.com>

On Mon, Nov 25, 2019 at 10:31:12AM +0000, Edward Cree wrote:
> 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?
> 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...
Pardon my lack of understanding, but is it unusual that something that 
the drivers should not be calling be exposed to the drivers? Could it be 
hidden from the drivers so that it is out of scope, once the current 
drivers are modified to not use it?

> 
> -Ed
> 
> [1]: https://elixir.bootlin.com/linux/latest/ident/napi_gro_flush
Kind regards,
Nicholas