[RFC net 0/1] Fix netdevim to correctly mark NAPI IDs

[RFC net 0/1] Fix netdevim to correctly mark NAPI IDs

[Joe Damato]

Greetings:

Sending this as an RFC because I'm not sure if this is net or net-next
material?

If users are using netdevsim out in the wild to test userland apps, this
might be an net material, but LMK.

If this is net material: I can resend without the cover letter and when
net-next re-opens I'll update the busy_poller.c test to check the NAPI
ID isn't zero.

If this net-next material: I'll wait until it reopens and send this
patch + an update to busy_poller.c as described above.

Thanks,
Joe

Joe Damato (1):
  netdevsim: Mark NAPI ID on skb in nsim_rcv

 drivers/net/netdevsim/netdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


base-commit: 2ea396448f26d0d7d66224cb56500a6789c7ed07
-- 
2.43.0

[RFC net 1/1] netdevsim: Mark NAPI ID on skb in nsim_rcv

[Joe Damato]

Previously, nsim_rcv was not marking the NAPI ID on the skb, leading to
applications seeing a napi ID of 0 when using SO_INCOMING_NAPI_ID.

To add to the userland confusion, netlink appears to correctly report
the NAPI IDs for netdevsim queues but the resulting file descriptor from
a call to accept() was reporting a NAPI ID of 0.

Fixes: 3762ec05a9fb ("netdevsim: add NAPI support")
Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 drivers/net/netdevsim/netdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index b67af4651185..1c67030fba6a 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -29,7 +29,7 @@
 #include <net/pkt_cls.h>
 #include <net/rtnetlink.h>
 #include <net/udp_tunnel.h>
-
+#include <net/busy_poll.h>
 #include "netdevsim.h"
 
 MODULE_IMPORT_NS("NETDEV_INTERNAL");
@@ -357,6 +357,7 @@ static int nsim_rcv(struct nsim_rq *rq, int budget)
 			break;
 
 		skb = skb_dequeue(&rq->skb_queue);
+		skb_mark_napi_id(skb, &rq->napi);
 		netif_receive_skb(skb);
 	}
 
-- 
2.43.0

Re: [RFC net 0/1] Fix netdevim to correctly mark NAPI IDs

[Jakub Kicinski]

On Sat, 29 Mar 2025 00:00:28 +0000 Joe Damato wrote:
> If this net-next material: I'll wait until it reopens and send this
> patch + an update to busy_poller.c as described above.

Let's stick to net-next. Would it be possible / make sense to convert
the test to Python and move it to drivers/net ?

Re: [RFC net 0/1] Fix netdevim to correctly mark NAPI IDs

[Joe Damato]

On Mon, Mar 31, 2025 at 01:36:15PM -0700, Jakub Kicinski wrote:
> On Sat, 29 Mar 2025 00:00:28 +0000 Joe Damato wrote:
> > If this net-next material: I'll wait until it reopens and send this
> > patch + an update to busy_poller.c as described above.
> 
> Let's stick to net-next. 

Sure, sounds good. I'll drop the fixes tag when I resend when
net-next is open, of course.

> Would it be possible / make sense to convert the test to Python
> and move it to drivers/net ?

Hmm. We could; I think originally the busy_poller.c test was added
because it was requested by Paolo for IRQ suspension and netdevsim
was the only option that I could find that supported NAPI IDs at the
time.

busy_poller.c itself seems more like a selftests/net thing since
it's testing some functionality of the core networking code.

Maybe mixing the napi_id != 0 test into busy_poller.c is the wrong
way to go at a higher level. Maybe there should be a test for
netdevsim itself that checks napi_id != 0 and that test would make
more sense under drivers/net vs mixing a check into busy_poller.c?

Re: [RFC net 0/1] Fix netdevim to correctly mark NAPI IDs

[Jakub Kicinski]

On Mon, 31 Mar 2025 15:32:09 -0700 Joe Damato wrote:
> > Would it be possible / make sense to convert the test to Python
> > and move it to drivers/net ?  
> 
> Hmm. We could; I think originally the busy_poller.c test was added
> because it was requested by Paolo for IRQ suspension and netdevsim
> was the only option that I could find that supported NAPI IDs at the
> time.
> 
> busy_poller.c itself seems more like a selftests/net thing since
> it's testing some functionality of the core networking code.

I guess in my mind busy polling is tied to having IRQ-capable device.
Even if bulk of the logic resides in the core.

> Maybe mixing the napi_id != 0 test into busy_poller.c is the wrong
> way to go at a higher level. Maybe there should be a test for
> netdevsim itself that checks napi_id != 0 and that test would make
> more sense under drivers/net vs mixing a check into busy_poller.c?

Up to you. The patch make me wonder how many other corner cases / bugs
we may be missing in drivers. And therefore if we shouldn't flesh out
more device-related tests. But exercising the core code makes sense
in itself so no strong feelings.

Re: [RFC net 0/1] Fix netdevim to correctly mark NAPI IDs

[Joe Damato]

On Mon, Mar 31, 2025 at 04:39:17PM -0700, Jakub Kicinski wrote:
> On Mon, 31 Mar 2025 15:32:09 -0700 Joe Damato wrote:
> > > Would it be possible / make sense to convert the test to Python
> > > and move it to drivers/net ?  
> > 
> > Hmm. We could; I think originally the busy_poller.c test was added
> > because it was requested by Paolo for IRQ suspension and netdevsim
> > was the only option that I could find that supported NAPI IDs at the
> > time.
> > 
> > busy_poller.c itself seems more like a selftests/net thing since
> > it's testing some functionality of the core networking code.
> 
> I guess in my mind busy polling is tied to having IRQ-capable device.
> Even if bulk of the logic resides in the core.
> 
> > Maybe mixing the napi_id != 0 test into busy_poller.c is the wrong
> > way to go at a higher level. Maybe there should be a test for
> > netdevsim itself that checks napi_id != 0 and that test would make
> > more sense under drivers/net vs mixing a check into busy_poller.c?
> 
> Up to you. The patch make me wonder how many other corner cases / bugs
> we may be missing in drivers. And therefore if we shouldn't flesh out
> more device-related tests. But exercising the core code makes sense
> in itself so no strong feelings.

Sorry to revive this old thread, but I have a bit of time to get
this fixed now. I have a patch for netdevsim but am trying to figure
out what the best way to write a test for this is.

Locally, I've hacked up a tools/testing/selftests/drivers/net/napi_id.py

I'm using NetDrvEpEnv, but am not sure: is there an easy way in
Python to run stuff in a network namespace? Is there an example I
can look at?

In my Python code, I was thinking that I'd call fork and have each
python process (client and server) set their network namespace
according to the NetDrvEpEnv cfg... but wasn't sure if there was a
better/easier way ?

It looks like tools/testing/selftests/net/rds/test.py uses
LoadLibrary to call setns before creating a socket.

Should I go in that direction too?

Re: [RFC net 0/1] Fix netdevim to correctly mark NAPI IDs

[Jakub Kicinski]

On Tue, 15 Apr 2025 12:39:11 -0700 Joe Damato wrote:
> On Mon, Mar 31, 2025 at 04:39:17PM -0700, Jakub Kicinski wrote:
> > Up to you. The patch make me wonder how many other corner cases / bugs
> > we may be missing in drivers. And therefore if we shouldn't flesh out
> > more device-related tests. But exercising the core code makes sense
> > in itself so no strong feelings.  
> 
> Sorry to revive this old thread, but I have a bit of time to get
> this fixed now. I have a patch for netdevsim but am trying to figure
> out what the best way to write a test for this is.
> 
> Locally, I've hacked up a tools/testing/selftests/drivers/net/napi_id.py
> 
> I'm using NetDrvEpEnv, but am not sure: is there an easy way in
> Python to run stuff in a network namespace? Is there an example I
> can look at?
> 
> In my Python code, I was thinking that I'd call fork and have each
> python process (client and server) set their network namespace
> according to the NetDrvEpEnv cfg... but wasn't sure if there was a
> better/easier way ?
> 
> It looks like tools/testing/selftests/net/rds/test.py uses
> LoadLibrary to call setns before creating a socket.
> 
> Should I go in that direction too?

Why do you need a netns? The NetDrvEpEnv will create one for you
automatically and put one side of the netdevsim into it.
Do you mean that you need to adjust that other endpoint?
It's done the same way as if it was a remote machine:

	cmd(..., host=cfg.remote)

If you really need a netnes check out
tools/testing/selftests/net/lib/py/netns.py

Re: [RFC net 0/1] Fix netdevim to correctly mark NAPI IDs

[Joe Damato]

On Tue, Apr 15, 2025 at 05:11:54PM -0700, Jakub Kicinski wrote:
> On Tue, 15 Apr 2025 12:39:11 -0700 Joe Damato wrote:
> > On Mon, Mar 31, 2025 at 04:39:17PM -0700, Jakub Kicinski wrote:
> > > Up to you. The patch make me wonder how many other corner cases / bugs
> > > we may be missing in drivers. And therefore if we shouldn't flesh out
> > > more device-related tests. But exercising the core code makes sense
> > > in itself so no strong feelings.  
> > 
> > Sorry to revive this old thread, but I have a bit of time to get
> > this fixed now. I have a patch for netdevsim but am trying to figure
> > out what the best way to write a test for this is.
> > 
> > Locally, I've hacked up a tools/testing/selftests/drivers/net/napi_id.py
> > 
> > I'm using NetDrvEpEnv, but am not sure: is there an easy way in
> > Python to run stuff in a network namespace? Is there an example I
> > can look at?
> > 
> > In my Python code, I was thinking that I'd call fork and have each
> > python process (client and server) set their network namespace
> > according to the NetDrvEpEnv cfg... but wasn't sure if there was a
> > better/easier way ?
> > 
> > It looks like tools/testing/selftests/net/rds/test.py uses
> > LoadLibrary to call setns before creating a socket.
> > 
> > Should I go in that direction too?
> 
> Why do you need a netns? The NetDrvEpEnv will create one for you
> automatically and put one side of the netdevsim into it.
> Do you mean that you need to adjust that other endpoint?
> It's done the same way as if it was a remote machine:
> 
> 	cmd(..., host=cfg.remote)

Maybe I'm just thinking about it wrong and/or describing it poorly.

The idea was that napi_id.py test forks. One process does a
listen()/accept() and the other does a connect(). The accept side
checks that the napi ID is non-zero. For that to work, both
processes need their netdevsims to be able to talk to each other.

> If you really need a netnes check out
> tools/testing/selftests/net/lib/py/netns.py

I'll take a look, but I'm probably just missing something about how
to properly use NetDrvEpEnv.

Re: [RFC net 0/1] Fix netdevim to correctly mark NAPI IDs

[Joe Damato]

On Tue, Apr 15, 2025 at 06:39:27PM -0700, Joe Damato wrote:
> On Tue, Apr 15, 2025 at 05:11:54PM -0700, Jakub Kicinski wrote:
> > On Tue, 15 Apr 2025 12:39:11 -0700 Joe Damato wrote:
> > > On Mon, Mar 31, 2025 at 04:39:17PM -0700, Jakub Kicinski wrote:
> > > > Up to you. The patch make me wonder how many other corner cases / bugs
> > > > we may be missing in drivers. And therefore if we shouldn't flesh out
> > > > more device-related tests. But exercising the core code makes sense
> > > > in itself so no strong feelings.  
> > > 
> > > Sorry to revive this old thread, but I have a bit of time to get
> > > this fixed now. I have a patch for netdevsim but am trying to figure
> > > out what the best way to write a test for this is.
> > > 
> > > Locally, I've hacked up a tools/testing/selftests/drivers/net/napi_id.py
> > > 
> > > I'm using NetDrvEpEnv, but am not sure: is there an easy way in
> > > Python to run stuff in a network namespace? Is there an example I
> > > can look at?
> > > 
> > > In my Python code, I was thinking that I'd call fork and have each
> > > python process (client and server) set their network namespace
> > > according to the NetDrvEpEnv cfg... but wasn't sure if there was a
> > > better/easier way ?
> > > 
> > > It looks like tools/testing/selftests/net/rds/test.py uses
> > > LoadLibrary to call setns before creating a socket.
> > > 
> > > Should I go in that direction too?
> > 
> > Why do you need a netns? The NetDrvEpEnv will create one for you
> > automatically and put one side of the netdevsim into it.
> > Do you mean that you need to adjust that other endpoint?
> > It's done the same way as if it was a remote machine:
> > 
> > 	cmd(..., host=cfg.remote)
> 
> Maybe I'm just thinking about it wrong and/or describing it poorly.
> 
> The idea was that napi_id.py test forks. One process does a
> listen()/accept() and the other does a connect(). The accept side
> checks that the napi ID is non-zero. For that to work, both
> processes need their netdevsims to be able to talk to each other.

In retrospect, it's probably easier to just have the connect() side
be socat or something run in the background, like the ncdevmem test.

Sorry for the noise, I'll mess around a bit more.