* [Intel-wired-lan] "Missing unregister, handled but fix driver" when changing ring settings on igc interface @ 2022-01-10 16:07 Lennert Buytenhek 2022-01-13 0:40 ` [Intel-wired-lan] [PATCH net-queue v1] igc: Fix trying to register an already registered xdp_rxq Vinicius Costa Gomes 0 siblings, 1 reply; 6+ messages in thread From: Lennert Buytenhek @ 2022-01-10 16:07 UTC (permalink / raw) To: intel-wired-lan 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Intel-wired-lan] [PATCH net-queue v1] igc: Fix trying to register an already registered xdp_rxq 2022-01-10 16:07 [Intel-wired-lan] "Missing unregister, handled but fix driver" when changing ring settings on igc interface Lennert Buytenhek @ 2022-01-13 0:40 ` Vinicius Costa Gomes 2022-01-13 0:55 ` Lennert Buytenhek 2022-01-13 12:03 ` Alexander Lobakin 0 siblings, 2 replies; 6+ messages in thread From: Vinicius Costa Gomes @ 2022-01-13 0:40 UTC (permalink / raw) To: intel-wired-lan 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Intel-wired-lan] [PATCH net-queue v1] igc: Fix trying to register an already registered xdp_rxq 2022-01-13 0:40 ` [Intel-wired-lan] [PATCH net-queue v1] igc: Fix trying to register an already registered xdp_rxq Vinicius Costa Gomes @ 2022-01-13 0:55 ` Lennert Buytenhek 2022-01-13 10:03 ` Maciej Fijalkowski 2022-01-13 12:03 ` Alexander Lobakin 1 sibling, 1 reply; 6+ messages in thread From: Lennert Buytenhek @ 2022-01-13 0:55 UTC (permalink / raw) To: intel-wired-lan 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Intel-wired-lan] [PATCH net-queue v1] igc: Fix trying to register an already registered xdp_rxq 2022-01-13 0:55 ` Lennert Buytenhek @ 2022-01-13 10:03 ` Maciej Fijalkowski 0 siblings, 0 replies; 6+ messages in thread From: Maciej Fijalkowski @ 2022-01-13 10:03 UTC (permalink / raw) To: intel-wired-lan 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Intel-wired-lan] [PATCH net-queue v1] igc: Fix trying to register an already registered xdp_rxq 2022-01-13 0:40 ` [Intel-wired-lan] [PATCH net-queue v1] igc: Fix trying to register an already registered xdp_rxq Vinicius Costa Gomes 2022-01-13 0:55 ` Lennert Buytenhek @ 2022-01-13 12:03 ` Alexander Lobakin 2022-01-14 0:47 ` Vinicius Costa Gomes 1 sibling, 1 reply; 6+ messages in thread From: Alexander Lobakin @ 2022-01-13 12:03 UTC (permalink / raw) To: intel-wired-lan 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Intel-wired-lan] [PATCH net-queue v1] igc: Fix trying to register an already registered xdp_rxq 2022-01-13 12:03 ` Alexander Lobakin @ 2022-01-14 0:47 ` Vinicius Costa Gomes 0 siblings, 0 replies; 6+ messages in thread From: Vinicius Costa Gomes @ 2022-01-14 0:47 UTC (permalink / raw) To: intel-wired-lan 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-14 0:47 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-10 16:07 [Intel-wired-lan] "Missing unregister, handled but fix driver" when changing ring settings on igc interface Lennert Buytenhek 2022-01-13 0:40 ` [Intel-wired-lan] [PATCH net-queue v1] igc: Fix trying to register an already registered xdp_rxq Vinicius Costa Gomes 2022-01-13 0:55 ` Lennert Buytenhek 2022-01-13 10:03 ` Maciej Fijalkowski 2022-01-13 12:03 ` Alexander Lobakin 2022-01-14 0:47 ` Vinicius Costa Gomes
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.