[Intel-wired-lan] "Missing unregister, handled but fix driver" when changing ring settings on igc interface

[Intel-wired-lan] "Missing unregister, handled but fix driver" when changing ring settings on igc interface

[Lennert Buytenhek]

Hi,

I didn't see this reported yet.  When doing e.g. this on an igc
interface (I225-V):

	# ethtool -G enp3s0 rx 512

I get a few warnings like the following:

[  225.198467] ------------[ cut here ]------------
[  225.198473] Missing unregister, handled but fix driver
[  225.198485] WARNING: CPU: 7 PID: 959 at net/core/xdp.c:168 xdp_rxq_info_reg+0x79/0xd0
[...]
[  225.198601] Call Trace:
[  225.198604]  <TASK>
[  225.198609]  igc_setup_rx_resources+0x3f/0xe0 [igc]
[  225.198617]  igc_ethtool_set_ringparam+0x30e/0x450 [igc]
[  225.198626]  ethnl_set_rings+0x18a/0x250
[  225.198631]  genl_family_rcv_msg_doit+0xca/0x110
[  225.198637]  genl_rcv_msg+0xce/0x1c0
[  225.198640]  ? rings_prepare_data+0x60/0x60
[  225.198644]  ? genl_get_cmd+0xd0/0xd0
[  225.198647]  netlink_rcv_skb+0x4e/0xf0
[  225.198652]  genl_rcv+0x24/0x40
[  225.198655]  netlink_unicast+0x20e/0x330
[  225.198659]  netlink_sendmsg+0x23f/0x480
[  225.198663]  sock_sendmsg+0x5b/0x60
[  225.198667]  __sys_sendto+0xf0/0x160
[  225.198671]  ? handle_mm_fault+0xb2/0x280
[  225.198676]  ? do_user_addr_fault+0x1eb/0x690
[  225.198680]  __x64_sys_sendto+0x20/0x30
[  225.198683]  do_syscall_64+0x38/0x90
[  225.198687]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[  225.198693] RIP: 0033:0x7f7ae38ac3aa

This is on vanilla 5.16.


Thanks,
Lennert

[Intel-wired-lan] [PATCH net-queue v1] igc: Fix trying to register an already registered xdp_rxq

[Vinicius Costa Gomes]

When changing the number of RX descriptors, for example, by doing

 $ ethtool -G enp3s0 rx 1024

the XDP RX queue information (xdp_rxq) may be already registered, if
it's registered there's no need to do any thing in relation to
xdp_rxq, none of it's parameters will change if we change the number
of descriptors, for example.

Fixes: 4609ffb9f615 ("igc: Refactor XDP rxq info registration")
Reported-by: Lennert Buytenhek <buytenh@wantstofly.org>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
Lennert, I added your name and email to the Reported-by tag, please
see if you are ok with it.

 drivers/net/ethernet/intel/igc/igc_main.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index e29aadbc6744..d163139161fc 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (c)  2018 Intel Corporation */
 
+#include "net/xdp.h"
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/if_vlan.h>
@@ -499,12 +500,14 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
 	u8 index = rx_ring->queue_index;
 	int size, desc_len, res;
 
-	res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
-			       rx_ring->q_vector->napi.napi_id);
-	if (res < 0) {
-		netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
-			   index);
-		return res;
+	if (!xdp_rxq_info_is_reg(&rx_ring->xdp_rxq)) {
+		res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
+				       rx_ring->q_vector->napi.napi_id);
+		if (res < 0) {
+			netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
+				   index);
+			return res;
+		}
 	}
 
 	size = sizeof(struct igc_rx_buffer) * rx_ring->count;
-- 
2.34.1

[Intel-wired-lan] [PATCH net-queue v1] igc: Fix trying to register an already registered xdp_rxq

[Lennert Buytenhek]

On Wed, Jan 12, 2022 at 04:40:15PM -0800, Vinicius Costa Gomes wrote:

> When changing the number of RX descriptors, for example, by doing
> 
>  $ ethtool -G enp3s0 rx 1024
> 
> the XDP RX queue information (xdp_rxq) may be already registered, if
> it's registered there's no need to do any thing in relation to
> xdp_rxq, none of it's parameters will change if we change the number
> of descriptors, for example.
> 
> Fixes: 4609ffb9f615 ("igc: Refactor XDP rxq info registration")
> Reported-by: Lennert Buytenhek <buytenh@wantstofly.org>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
> Lennert, I added your name and email to the Reported-by tag, please
> see if you are ok with it.

This patch seems to work -- thank you!

Tested-by: Lennert Buytenhek <buytenh@arista.com>

Could you use the same email address for Reported-by: ?


> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index e29aadbc6744..d163139161fc 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /* Copyright (c)  2018 Intel Corporation */
>  
> +#include "net/xdp.h"
>  #include <linux/module.h>
>  #include <linux/types.h>
>  #include <linux/if_vlan.h>
> @@ -499,12 +500,14 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
>  	u8 index = rx_ring->queue_index;
>  	int size, desc_len, res;
>  
> -	res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
> -			       rx_ring->q_vector->napi.napi_id);
> -	if (res < 0) {
> -		netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
> -			   index);
> -		return res;
> +	if (!xdp_rxq_info_is_reg(&rx_ring->xdp_rxq)) {
> +		res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
> +				       rx_ring->q_vector->napi.napi_id);
> +		if (res < 0) {
> +			netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
> +				   index);
> +			return res;
> +		}
>  	}
>  
>  	size = sizeof(struct igc_rx_buffer) * rx_ring->count;
> -- 
> 2.34.1

[Intel-wired-lan] [PATCH net-queue v1] igc: Fix trying to register an already registered xdp_rxq

[Maciej Fijalkowski]

On Thu, Jan 13, 2022 at 02:55:41AM +0200, Lennert Buytenhek wrote:
> On Wed, Jan 12, 2022 at 04:40:15PM -0800, Vinicius Costa Gomes wrote:
> 
> > When changing the number of RX descriptors, for example, by doing
> > 
> >  $ ethtool -G enp3s0 rx 1024
> > 
> > the XDP RX queue information (xdp_rxq) may be already registered, if
> > it's registered there's no need to do any thing in relation to
> > xdp_rxq, none of it's parameters will change if we change the number
> > of descriptors, for example.
> > 
> > Fixes: 4609ffb9f615 ("igc: Refactor XDP rxq info registration")
> > Reported-by: Lennert Buytenhek <buytenh@wantstofly.org>
> > Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> > ---
> > Lennert, I added your name and email to the Reported-by tag, please
> > see if you are ok with it.
> 
> This patch seems to work -- thank you!
> 
> Tested-by: Lennert Buytenhek <buytenh@arista.com>
> 
> Could you use the same email address for Reported-by: ?

I'm missing the context in here, fix itself is fine but it would be good
to include in commit message what Lennert actually reported TBH.

> 
> 
> > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> > index e29aadbc6744..d163139161fc 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_main.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> > @@ -1,6 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  /* Copyright (c)  2018 Intel Corporation */
> >  
> > +#include "net/xdp.h"
> >  #include <linux/module.h>
> >  #include <linux/types.h>
> >  #include <linux/if_vlan.h>
> > @@ -499,12 +500,14 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
> >  	u8 index = rx_ring->queue_index;
> >  	int size, desc_len, res;
> >  
> > -	res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
> > -			       rx_ring->q_vector->napi.napi_id);
> > -	if (res < 0) {
> > -		netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
> > -			   index);
> > -		return res;
> > +	if (!xdp_rxq_info_is_reg(&rx_ring->xdp_rxq)) {
> > +		res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
> > +				       rx_ring->q_vector->napi.napi_id);
> > +		if (res < 0) {
> > +			netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
> > +				   index);
> > +			return res;
> > +		}
> >  	}
> >  
> >  	size = sizeof(struct igc_rx_buffer) * rx_ring->count;
> > -- 
> > 2.34.1
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan at osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH net-queue v1] igc: Fix trying to register an already registered xdp_rxq

[Alexander Lobakin]

From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Date: Wed, 12 Jan 2022 16:40:15 -0800

> When changing the number of RX descriptors, for example, by doing
> 
>  $ ethtool -G enp3s0 rx 1024
> 
> the XDP RX queue information (xdp_rxq) may be already registered, if
> it's registered there's no need to do any thing in relation to
> xdp_rxq, none of it's parameters will change if we change the number
> of descriptors, for example.
> 
> Fixes: 4609ffb9f615 ("igc: Refactor XDP rxq info registration")
> Reported-by: Lennert Buytenhek <buytenh@wantstofly.org>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
> Lennert, I added your name and email to the Reported-by tag, please
> see if you are ok with it.
> 
>  drivers/net/ethernet/intel/igc/igc_main.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index e29aadbc6744..d163139161fc 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /* Copyright (c)  2018 Intel Corporation */
>  
> +#include "net/xdp.h"
>  #include <linux/module.h>
>  #include <linux/types.h>
>  #include <linux/if_vlan.h>
> @@ -499,12 +500,14 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
>  	u8 index = rx_ring->queue_index;
>  	int size, desc_len, res;
>  
> -	res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
> -			       rx_ring->q_vector->napi.napi_id);
> -	if (res < 0) {
> -		netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
> -			   index);
> -		return res;
> +	if (!xdp_rxq_info_is_reg(&rx_ring->xdp_rxq)) {
> +		res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
> +				       rx_ring->q_vector->napi.napi_id);
> +		if (res < 0) {
> +			netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
> +				   index);
> +			return res;
> +		}
>  	}

While the fix itself is correct, I don't really like the setup flow
in igc_ethtool_set_ringparam() (here: [0]).

Instead of unregistering/freeing Rx rings at first and then
allocating/registering the new ones, driver registers the new ones
at first and then unregister the olds.
It may cause other issues apart from this one. E.g. driver performs
Rxq info unregistering at the end, so netdevice may end up with no
Rxq info registered after changing the number of descriptors (Rxq
info is enumerated by queue index).

To me, the righteous approach would be changing this flow to more
common, i.e. do unregistering/freeing at first and then register
and allocate.

>  
>  	size = sizeof(struct igc_rx_buffer) * rx_ring->count;
> -- 
> 2.34.1

[0] https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/intel/igc/igc_ethtool.c#L664

Thanks,
Al

[Intel-wired-lan] [PATCH net-queue v1] igc: Fix trying to register an already registered xdp_rxq

[Vinicius Costa Gomes]

Alexander Lobakin <alexandr.lobakin@intel.com> writes:

> From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Date: Wed, 12 Jan 2022 16:40:15 -0800
>
>> When changing the number of RX descriptors, for example, by doing
>> 
>>  $ ethtool -G enp3s0 rx 1024
>> 
>> the XDP RX queue information (xdp_rxq) may be already registered, if
>> it's registered there's no need to do any thing in relation to
>> xdp_rxq, none of it's parameters will change if we change the number
>> of descriptors, for example.
>> 
>> Fixes: 4609ffb9f615 ("igc: Refactor XDP rxq info registration")
>> Reported-by: Lennert Buytenhek <buytenh@wantstofly.org>
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>> ---
>> Lennert, I added your name and email to the Reported-by tag, please
>> see if you are ok with it.
>> 
>>  drivers/net/ethernet/intel/igc/igc_main.c | 15 +++++++++------
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>> index e29aadbc6744..d163139161fc 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -1,6 +1,7 @@
>>  // SPDX-License-Identifier: GPL-2.0
>>  /* Copyright (c)  2018 Intel Corporation */
>>  
>> +#include "net/xdp.h"
>>  #include <linux/module.h>
>>  #include <linux/types.h>
>>  #include <linux/if_vlan.h>
>> @@ -499,12 +500,14 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
>>  	u8 index = rx_ring->queue_index;
>>  	int size, desc_len, res;
>>  
>> -	res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
>> -			       rx_ring->q_vector->napi.napi_id);
>> -	if (res < 0) {
>> -		netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
>> -			   index);
>> -		return res;
>> +	if (!xdp_rxq_info_is_reg(&rx_ring->xdp_rxq)) {
>> +		res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index,
>> +				       rx_ring->q_vector->napi.napi_id);
>> +		if (res < 0) {
>> +			netdev_err(ndev, "Failed to register xdp_rxq index %u\n",
>> +				   index);
>> +			return res;
>> +		}
>>  	}
>
> While the fix itself is correct, I don't really like the setup flow
> in igc_ethtool_set_ringparam() (here: [0]).
>
> Instead of unregistering/freeing Rx rings at first and then
> allocating/registering the new ones, driver registers the new ones
> at first and then unregister the olds.
> It may cause other issues apart from this one. E.g. driver performs
> Rxq info unregistering at the end, so netdevice may end up with no
> Rxq info registered after changing the number of descriptors (Rxq
> info is enumerated by queue index).
>
> To me, the righteous approach would be changing this flow to more
> common, i.e. do unregistering/freeing at first and then register
> and allocate.

Agreed. I was not that familiar with this part of the code, it seems to
be more complicated than it needs. i.e. doing something similar to what
ice does would be better. But I was a bit sleep deprived yesterday to do
that :-)


Cheers,
-- 
Vinicius