diff mbox series

[iwl-net] igc: Fix XSK queue NAPI ID mapping

Message ID 20250305180901.128286-1-jdamato@fastly.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series [iwl-net] igc: Fix XSK queue NAPI ID mapping | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 15 of 15 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 14 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Joe Damato March 5, 2025, 6:09 p.m. UTC
In commit b65969856d4f ("igc: Link queues to NAPI instances"), the XSK
queues were incorrectly unmapped from their NAPI instances. After
discussion on the mailing list and the introduction of a test to codify
the expected behavior, we can see that the unmapping causes the
check_xsk test to fail:

NETIF=enp86s0 ./tools/testing/selftests/drivers/net/queues.py

[...]
  # Check|     ksft_eq(q.get('xsk', None), {},
  # Check failed None != {} xsk attr on queue we configured
  not ok 4 queues.check_xsk

After this commit, the test passes:

  ok 4 queues.check_xsk

Note that the test itself is only in net-next, so I tested this change
by applying it to my local net-next tree, booting, and running the test.

Cc: stable@vger.kernel.org
Fixes: b65969856d4f ("igc: Link queues to NAPI instances")
Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 drivers/net/ethernet/intel/igc/igc_xdp.c | 2 --
 1 file changed, 2 deletions(-)


base-commit: 3c9231ea6497dfc50ac0ef69fff484da27d0df66

Comments

Gerhard Engleder March 5, 2025, 7:15 p.m. UTC | #1
On 05.03.25 19:09, Joe Damato wrote:
> In commit b65969856d4f ("igc: Link queues to NAPI instances"), the XSK
> queues were incorrectly unmapped from their NAPI instances. After
> discussion on the mailing list and the introduction of a test to codify
> the expected behavior, we can see that the unmapping causes the
> check_xsk test to fail:
> 
> NETIF=enp86s0 ./tools/testing/selftests/drivers/net/queues.py
> 
> [...]
>    # Check|     ksft_eq(q.get('xsk', None), {},
>    # Check failed None != {} xsk attr on queue we configured
>    not ok 4 queues.check_xsk
> 
> After this commit, the test passes:
> 
>    ok 4 queues.check_xsk
> 
> Note that the test itself is only in net-next, so I tested this change
> by applying it to my local net-next tree, booting, and running the test.
> 
> Cc: stable@vger.kernel.org
> Fixes: b65969856d4f ("igc: Link queues to NAPI instances")
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> ---
>   drivers/net/ethernet/intel/igc/igc_xdp.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_xdp.c b/drivers/net/ethernet/intel/igc/igc_xdp.c
> index 13bbd3346e01..869815f48ac1 100644
> --- a/drivers/net/ethernet/intel/igc/igc_xdp.c
> +++ b/drivers/net/ethernet/intel/igc/igc_xdp.c
> @@ -86,7 +86,6 @@ static int igc_xdp_enable_pool(struct igc_adapter *adapter,
>   		napi_disable(napi);
>   	}
>   
> -	igc_set_queue_napi(adapter, queue_id, NULL);
>   	set_bit(IGC_RING_FLAG_AF_XDP_ZC, &rx_ring->flags);
>   	set_bit(IGC_RING_FLAG_AF_XDP_ZC, &tx_ring->flags);
>   
> @@ -136,7 +135,6 @@ static int igc_xdp_disable_pool(struct igc_adapter *adapter, u16 queue_id)
>   	xsk_pool_dma_unmap(pool, IGC_RX_DMA_ATTR);
>   	clear_bit(IGC_RING_FLAG_AF_XDP_ZC, &rx_ring->flags);
>   	clear_bit(IGC_RING_FLAG_AF_XDP_ZC, &tx_ring->flags);
> -	igc_set_queue_napi(adapter, queue_id, napi);
>   
>   	if (needs_reset) {
>   		napi_enable(napi);
> 
> base-commit: 3c9231ea6497dfc50ac0ef69fff484da27d0df66

igc_set_queue_napi() could be made static as it only used within
igc_main.c after this change.

Reviewed-by: Gerhard Engleder <gerhard@engleder-embedded.com>
Florian Bezdeka March 6, 2025, 4:27 p.m. UTC | #2
Hi Joe,

On 2025-03-05 19:09, Joe Damato wrote:
> In commit b65969856d4f ("igc: Link queues to NAPI instances"), the XSK
> queues were incorrectly unmapped from their NAPI instances. After
> discussion on the mailing list and the introduction of a test to codify
> the expected behavior, we can see that the unmapping causes the
> check_xsk test to fail:
> 
> NETIF=enp86s0 ./tools/testing/selftests/drivers/net/queues.py
> 
> [...]
>   # Check|     ksft_eq(q.get('xsk', None), {},
>   # Check failed None != {} xsk attr on queue we configured
>   not ok 4 queues.check_xsk
> 
> After this commit, the test passes:
> 
>   ok 4 queues.check_xsk
> 
> Note that the test itself is only in net-next, so I tested this change
> by applying it to my local net-next tree, booting, and running the 
> test.
> 
> Cc: stable@vger.kernel.org
> Fixes: b65969856d4f ("igc: Link queues to NAPI instances")
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_xdp.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_xdp.c
> b/drivers/net/ethernet/intel/igc/igc_xdp.c
> index 13bbd3346e01..869815f48ac1 100644
> --- a/drivers/net/ethernet/intel/igc/igc_xdp.c
> +++ b/drivers/net/ethernet/intel/igc/igc_xdp.c
> @@ -86,7 +86,6 @@ static int igc_xdp_enable_pool(struct igc_adapter 
> *adapter,
>  		napi_disable(napi);
>  	}
> 
> -	igc_set_queue_napi(adapter, queue_id, NULL);
>  	set_bit(IGC_RING_FLAG_AF_XDP_ZC, &rx_ring->flags);
>  	set_bit(IGC_RING_FLAG_AF_XDP_ZC, &tx_ring->flags);
> 
> @@ -136,7 +135,6 @@ static int igc_xdp_disable_pool(struct igc_adapter
> *adapter, u16 queue_id)
>  	xsk_pool_dma_unmap(pool, IGC_RX_DMA_ATTR);
>  	clear_bit(IGC_RING_FLAG_AF_XDP_ZC, &rx_ring->flags);
>  	clear_bit(IGC_RING_FLAG_AF_XDP_ZC, &tx_ring->flags);
> -	igc_set_queue_napi(adapter, queue_id, napi);
> 
>  	if (needs_reset) {
>  		napi_enable(napi);

That doesn't look correct to me. You removed both invocations of
igc_set_queue_napi() from igc_xdp.c. Where is the napi mapping now
done (in case XDP is enabled)?

To me it seems flipped. igc_xdp_enable_pool() should do the mapping
(previously did the unmapping) and igc_xdp_disable_pool() should do
the unmapping (previously did the mapping). No?

Btw: I got this patch via stable. It doesn't make sense to send it
to stable where this patch does not apply.

> 
> base-commit: 3c9231ea6497dfc50ac0ef69fff484da27d0df66
Joe Damato March 6, 2025, 4:45 p.m. UTC | #3
On Thu, Mar 06, 2025 at 05:27:38PM +0100, florian@bezdeka.de wrote:
> Hi Joe,
> 
> On 2025-03-05 19:09, Joe Damato wrote:
> > In commit b65969856d4f ("igc: Link queues to NAPI instances"), the XSK
> > queues were incorrectly unmapped from their NAPI instances. After
> > discussion on the mailing list and the introduction of a test to codify
> > the expected behavior, we can see that the unmapping causes the
> > check_xsk test to fail:
> > 
> > NETIF=enp86s0 ./tools/testing/selftests/drivers/net/queues.py
> > 
> > [...]
> >   # Check|     ksft_eq(q.get('xsk', None), {},
> >   # Check failed None != {} xsk attr on queue we configured
> >   not ok 4 queues.check_xsk
> > 
> > After this commit, the test passes:
> > 
> >   ok 4 queues.check_xsk
> > 
> > Note that the test itself is only in net-next, so I tested this change
> > by applying it to my local net-next tree, booting, and running the test.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: b65969856d4f ("igc: Link queues to NAPI instances")
> > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > ---
> >  drivers/net/ethernet/intel/igc/igc_xdp.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/igc/igc_xdp.c
> > b/drivers/net/ethernet/intel/igc/igc_xdp.c
> > index 13bbd3346e01..869815f48ac1 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_xdp.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_xdp.c
> > @@ -86,7 +86,6 @@ static int igc_xdp_enable_pool(struct igc_adapter
> > *adapter,
> >  		napi_disable(napi);
> >  	}
> > 
> > -	igc_set_queue_napi(adapter, queue_id, NULL);
> >  	set_bit(IGC_RING_FLAG_AF_XDP_ZC, &rx_ring->flags);
> >  	set_bit(IGC_RING_FLAG_AF_XDP_ZC, &tx_ring->flags);
> > 
> > @@ -136,7 +135,6 @@ static int igc_xdp_disable_pool(struct igc_adapter
> > *adapter, u16 queue_id)
> >  	xsk_pool_dma_unmap(pool, IGC_RX_DMA_ATTR);
> >  	clear_bit(IGC_RING_FLAG_AF_XDP_ZC, &rx_ring->flags);
> >  	clear_bit(IGC_RING_FLAG_AF_XDP_ZC, &tx_ring->flags);
> > -	igc_set_queue_napi(adapter, queue_id, napi);
> > 
> >  	if (needs_reset) {
> >  		napi_enable(napi);
> 
> That doesn't look correct to me. You removed both invocations of
> igc_set_queue_napi() from igc_xdp.c. Where is the napi mapping now
> done (in case XDP is enabled)?

igc_set_queue_napi is called when the queues are created (igc_up,
__igc_open). When the queues are created they'll be linked. Whether
or not XDP is enabled does not affect the queues being linked.

The test added for this (which I mentioned in the commit message)
confirms that this is the correct behavior, as does the
documentation in Documentation/netlink/specs/netdev.yaml.

See commit df524c8f5771 ("netdev-genl: Add an XSK attribute to
queues").

> To me it seems flipped. igc_xdp_enable_pool() should do the mapping
> (previously did the unmapping) and igc_xdp_disable_pool() should do
> the unmapping (previously did the mapping). No?

In igc, all queues get their NAPIs mapped in igc_up or __igc_open. I
had mistakenly added code to remove the mapping for XDP because I
was under the impression that NAPIs should not be mapped for XDP
queues. See the commit under fixes.

This was incorrect, so this commit removes the unmapping and
corrects the behavior.

With this change, all queues have their NAPIs mapped (whether or not
they are used for AF_XDP) and is the agreed upon behavior based on
prior conversations on the list and the documentation I mentioned
above.

> Btw: I got this patch via stable. It doesn't make sense to send it
> to stable where this patch does not apply.

Maybe I made a mistake, but as far as I can tell the commit under
fixes is in 6.14-rc*:

$ git tag --contains b65969856d4f
v6.14-rc1
v6.14-rc2
v6.14-rc3
v6.14-rc4

So, I think this change is:
  - Correct
  - Definitely a "fixes" and should go to iwl-net
  - But maybe does not need to CC stable ?

If the Intel folks would like me to resend with some change please
let me know.
Tony Nguyen March 6, 2025, 6:31 p.m. UTC | #4
On 3/6/2025 8:45 AM, Joe Damato wrote:
> On Thu, Mar 06, 2025 at 05:27:38PM +0100, florian@bezdeka.de wrote:

...

>> Btw: I got this patch via stable. It doesn't make sense to send it
>> to stable where this patch does not apply.
> 
> Maybe I made a mistake, but as far as I can tell the commit under
> fixes is in 6.14-rc*:
> 
> $ git tag --contains b65969856d4f
> v6.14-rc1
> v6.14-rc2
> v6.14-rc3
> v6.14-rc4
> 
> So, I think this change is:
>    - Correct
>    - Definitely a "fixes" and should go to iwl-net
>    - But maybe does not need to CC stable ?
> 
> If the Intel folks would like me to resend with some change please
> let me know.

Seems like the only change needed is the omission of stable. If so, no 
changes need. I can take care of that when sending on to netdev.

Thanks,
Tony
Tony Nguyen March 7, 2025, 9:37 p.m. UTC | #5
On 3/6/2025 10:31 AM, Tony Nguyen wrote:
> On 3/6/2025 8:45 AM, Joe Damato wrote:
>> On Thu, Mar 06, 2025 at 05:27:38PM +0100, florian@bezdeka.de wrote:
> 
> ...
> 
>>> Btw: I got this patch via stable. It doesn't make sense to send it
>>> to stable where this patch does not apply.
>>
>> Maybe I made a mistake, but as far as I can tell the commit under
>> fixes is in 6.14-rc*:
>>
>> $ git tag --contains b65969856d4f
>> v6.14-rc1
>> v6.14-rc2
>> v6.14-rc3
>> v6.14-rc4
>>
>> So, I think this change is:
>>    - Correct
>>    - Definitely a "fixes" and should go to iwl-net
>>    - But maybe does not need to CC stable ?
>>
>> If the Intel folks would like me to resend with some change please
>> let me know.
> 
> Seems like the only change needed is the omission of stable. If so, no 
> changes need. I can take care of that when sending on to netdev.

I missed this comment [1] when I responded; I can make this change though.

Thanks,
Tony

[1] 
https://lore.kernel.org/intel-wired-lan/9ddf6293-6cb0-47ea-a0e7-cad7d33c2535@intel.com/T/#m47dd425fb4de4c1738839c2f8253ec51718d299e
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/igc/igc_xdp.c b/drivers/net/ethernet/intel/igc/igc_xdp.c
index 13bbd3346e01..869815f48ac1 100644
--- a/drivers/net/ethernet/intel/igc/igc_xdp.c
+++ b/drivers/net/ethernet/intel/igc/igc_xdp.c
@@ -86,7 +86,6 @@  static int igc_xdp_enable_pool(struct igc_adapter *adapter,
 		napi_disable(napi);
 	}
 
-	igc_set_queue_napi(adapter, queue_id, NULL);
 	set_bit(IGC_RING_FLAG_AF_XDP_ZC, &rx_ring->flags);
 	set_bit(IGC_RING_FLAG_AF_XDP_ZC, &tx_ring->flags);
 
@@ -136,7 +135,6 @@  static int igc_xdp_disable_pool(struct igc_adapter *adapter, u16 queue_id)
 	xsk_pool_dma_unmap(pool, IGC_RX_DMA_ATTR);
 	clear_bit(IGC_RING_FLAG_AF_XDP_ZC, &rx_ring->flags);
 	clear_bit(IGC_RING_FLAG_AF_XDP_ZC, &tx_ring->flags);
-	igc_set_queue_napi(adapter, queue_id, napi);
 
 	if (needs_reset) {
 		napi_enable(napi);