[PATCH net-next 00/13] selftests: ncdevmem: Add ncdevmem to ksft

[PATCH net-next 00/13] selftests: ncdevmem: Add ncdevmem to ksft

[Stanislav Fomichev]

The goal of the series is to simplify and make it possible to use
ncdevmem in an automated way from the ksft python wrapper.

ncdevmem is slowly mutated into a state where it uses stdout
to print the payload and the python wrapper is added to
make sure the arrived payload matches the expected one.

Cc: Mina Almasry <almasrymina@google.com>

Stanislav Fomichev (13):
  selftests: ncdevmem: Add a flag for the selftest
  selftests: ncdevmem: Remove validation
  selftests: ncdevmem: Redirect all non-payload output to stderr
  selftests: ncdevmem: Separate out dmabuf provider
  selftests: ncdevmem: Unify error handling
  selftests: ncdevmem: Remove client_ip
  selftests: ncdevmem: Remove default arguments
  selftests: ncdevmem: Switch to AF_INET6
  selftests: ncdevmem: Properly reset flow steering
  selftests: ncdevmem: Use YNL to enable TCP header split
  selftests: ncdevmem: Remove hard-coded queue numbers
  selftests: ncdevmem: Move ncdevmem under drivers/net
  selftests: ncdevmem: Add automated test

 .../testing/selftests/drivers/net/.gitignore  |   1 +
 tools/testing/selftests/drivers/net/Makefile  |  10 +
 tools/testing/selftests/drivers/net/devmem.py |  46 ++
 .../testing/selftests/drivers/net/ncdevmem.c  | 682 ++++++++++++++++++
 tools/testing/selftests/net/.gitignore        |   1 -
 tools/testing/selftests/net/Makefile          |   9 -
 tools/testing/selftests/net/ncdevmem.c        | 570 ---------------
 7 files changed, 739 insertions(+), 580 deletions(-)
 create mode 100644 tools/testing/selftests/drivers/net/.gitignore
 create mode 100755 tools/testing/selftests/drivers/net/devmem.py
 create mode 100644 tools/testing/selftests/drivers/net/ncdevmem.c
 delete mode 100644 tools/testing/selftests/net/ncdevmem.c

-- 
2.46.0

[PATCH net-next 01/13] selftests: ncdevmem: Add a flag for the selftest

[Stanislav Fomichev]

And rename it to 'probing'. This is gonna be used in the selftests
to probe devmem functionality.

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/ncdevmem.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
index 64d6805381c5..352dba211fb0 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -523,8 +523,9 @@ void run_devmem_tests(void)
 int main(int argc, char *argv[])
 {
 	int is_server = 0, opt;
+	int probe = 0;
 
-	while ((opt = getopt(argc, argv, "ls:c:p:v:q:t:f:")) != -1) {
+	while ((opt = getopt(argc, argv, "ls:c:p:v:q:t:f:P")) != -1) {
 		switch (opt) {
 		case 'l':
 			is_server = 1;
@@ -550,6 +551,9 @@ int main(int argc, char *argv[])
 		case 'f':
 			ifname = optarg;
 			break;
+		case 'P':
+			probe = 1;
+			break;
 		case '?':
 			printf("unknown option: %c\n", optopt);
 			break;
@@ -561,7 +565,10 @@ int main(int argc, char *argv[])
 	for (; optind < argc; optind++)
 		printf("extra arguments: %s\n", argv[optind]);
 
-	run_devmem_tests();
+	if (probe) {
+		run_devmem_tests();
+		return 0;
+	}
 
 	if (is_server)
 		return do_server();
-- 
2.46.0

Re: [PATCH net-next 01/13] selftests: ncdevmem: Add a flag for the selftest

[Mina Almasry]

On Thu, Sep 12, 2024 at 10:12 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> And rename it to 'probing'. This is gonna be used in the selftests
> to probe devmem functionality.
>
> Cc: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> ---
>  tools/testing/selftests/net/ncdevmem.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> index 64d6805381c5..352dba211fb0 100644
> --- a/tools/testing/selftests/net/ncdevmem.c
> +++ b/tools/testing/selftests/net/ncdevmem.c
> @@ -523,8 +523,9 @@ void run_devmem_tests(void)
>  int main(int argc, char *argv[])
>  {
>         int is_server = 0, opt;
> +       int probe = 0;
>
> -       while ((opt = getopt(argc, argv, "ls:c:p:v:q:t:f:")) != -1) {
> +       while ((opt = getopt(argc, argv, "ls:c:p:v:q:t:f:P")) != -1) {
>                 switch (opt) {
>                 case 'l':
>                         is_server = 1;
> @@ -550,6 +551,9 @@ int main(int argc, char *argv[])
>                 case 'f':
>                         ifname = optarg;
>                         break;
> +               case 'P':
> +                       probe = 1;
> +                       break;
>                 case '?':
>                         printf("unknown option: %c\n", optopt);
>                         break;
> @@ -561,7 +565,10 @@ int main(int argc, char *argv[])
>         for (; optind < argc; optind++)
>                 printf("extra arguments: %s\n", argv[optind]);
>
> -       run_devmem_tests();
> +       if (probe) {
> +               run_devmem_tests();
> +               return 0;
> +       }
>

Before this change:
./ncdevmem (runs run_devmem_tests() and exits)
./ncdevmem -l: runs devmem tests and listens

And I plan to add, for the tx path:

./ncdevmem -c: runs devmem tests and does a devmem client.

After this change, running ncdevmem with no flags just exits without
doing anything; a bit weird IMO, but I'm not opposed if you see an
upside.

Is your intention with this change to not run the devmem tests on
listen? Maybe something like:

if (is_server)
  return do_server();
else if (is_client) /* to be added */
  return do_client();
else
  run_devmem_tests();

return 0;

?

But, I'm not totally opposed if you see an upside. Maybe use -p
instead of -P for consistency.



--
Thanks,
Mina

Re: [PATCH net-next 01/13] selftests: ncdevmem: Add a flag for the selftest

[Stanislav Fomichev]

On 09/12, Mina Almasry wrote:
> On Thu, Sep 12, 2024 at 10:12 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > And rename it to 'probing'. This is gonna be used in the selftests
> > to probe devmem functionality.
> >
> > Cc: Mina Almasry <almasrymina@google.com>
> > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > ---
> >  tools/testing/selftests/net/ncdevmem.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> > index 64d6805381c5..352dba211fb0 100644
> > --- a/tools/testing/selftests/net/ncdevmem.c
> > +++ b/tools/testing/selftests/net/ncdevmem.c
> > @@ -523,8 +523,9 @@ void run_devmem_tests(void)
> >  int main(int argc, char *argv[])
> >  {
> >         int is_server = 0, opt;
> > +       int probe = 0;
> >
> > -       while ((opt = getopt(argc, argv, "ls:c:p:v:q:t:f:")) != -1) {
> > +       while ((opt = getopt(argc, argv, "ls:c:p:v:q:t:f:P")) != -1) {
> >                 switch (opt) {
> >                 case 'l':
> >                         is_server = 1;
> > @@ -550,6 +551,9 @@ int main(int argc, char *argv[])
> >                 case 'f':
> >                         ifname = optarg;
> >                         break;
> > +               case 'P':
> > +                       probe = 1;
> > +                       break;
> >                 case '?':
> >                         printf("unknown option: %c\n", optopt);
> >                         break;
> > @@ -561,7 +565,10 @@ int main(int argc, char *argv[])
> >         for (; optind < argc; optind++)
> >                 printf("extra arguments: %s\n", argv[optind]);
> >
> > -       run_devmem_tests();
> > +       if (probe) {
> > +               run_devmem_tests();
> > +               return 0;
> > +       }
> >
> 
> Before this change:
> ./ncdevmem (runs run_devmem_tests() and exits)
> ./ncdevmem -l: runs devmem tests and listens
> 
> And I plan to add, for the tx path:
> 
> ./ncdevmem -c: runs devmem tests and does a devmem client.
> 
> After this change, running ncdevmem with no flags just exits without
> doing anything; a bit weird IMO, but I'm not opposed if you see an
> upside.
> 
> Is your intention with this change to not run the devmem tests on
> listen? Maybe something like:
> 
> if (is_server)
>   return do_server();
> else if (is_client) /* to be added */
>   return do_client();
> else
>   run_devmem_tests();
> 
> return 0;
> 
> ?

SG! That's even better without the extra flag.

[PATCH net-next 02/13] selftests: ncdevmem: Remove validation

[Stanislav Fomichev]

ncdevmem should (see next patches) print the payload on the stdout.
The validation can and should be done by the callers:

$ ncdevmem -l ... > file
$ sha256sum file

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/ncdevmem.c | 56 +++-----------------------
 1 file changed, 6 insertions(+), 50 deletions(-)

diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
index 352dba211fb0..3712296d997b 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -64,24 +64,13 @@
 static char *server_ip = "192.168.1.4";
 static char *client_ip = "192.168.1.2";
 static char *port = "5201";
-static size_t do_validation;
 static int start_queue = 8;
 static int num_queues = 8;
 static char *ifname = "eth1";
 static unsigned int ifindex;
 static unsigned int dmabuf_id;
 
-void print_bytes(void *ptr, size_t size)
-{
-	unsigned char *p = ptr;
-	int i;
-
-	for (i = 0; i < size; i++)
-		printf("%02hhX ", p[i]);
-	printf("\n");
-}
-
-void print_nonzero_bytes(void *ptr, size_t size)
+static void print_nonzero_bytes(void *ptr, size_t size)
 {
 	unsigned char *p = ptr;
 	unsigned int i;
@@ -91,30 +80,6 @@ void print_nonzero_bytes(void *ptr, size_t size)
 	printf("\n");
 }
 
-void validate_buffer(void *line, size_t size)
-{
-	static unsigned char seed = 1;
-	unsigned char *ptr = line;
-	int errors = 0;
-	size_t i;
-
-	for (i = 0; i < size; i++) {
-		if (ptr[i] != seed) {
-			fprintf(stderr,
-				"Failed validation: expected=%u, actual=%u, index=%lu\n",
-				seed, ptr[i], i);
-			errors++;
-			if (errors > 20)
-				error(1, 0, "validation failed.");
-		}
-		seed++;
-		if (seed == do_validation)
-			seed = 0;
-	}
-
-	fprintf(stdout, "Validated buffer\n");
-}
-
 #define run_command(cmd, ...)                                           \
 	({                                                              \
 		char command[256];                                      \
@@ -414,16 +379,10 @@ int do_server(void)
 			sync.flags = DMA_BUF_SYNC_READ | DMA_BUF_SYNC_START;
 			ioctl(buf, DMA_BUF_IOCTL_SYNC, &sync);
 
-			if (do_validation)
-				validate_buffer(
-					((unsigned char *)buf_mem) +
-						dmabuf_cmsg->frag_offset,
-					dmabuf_cmsg->frag_size);
-			else
-				print_nonzero_bytes(
-					((unsigned char *)buf_mem) +
-						dmabuf_cmsg->frag_offset,
-					dmabuf_cmsg->frag_size);
+			print_nonzero_bytes(
+				((unsigned char *)buf_mem) +
+					dmabuf_cmsg->frag_offset,
+				dmabuf_cmsg->frag_size);
 
 			sync.flags = DMA_BUF_SYNC_READ | DMA_BUF_SYNC_END;
 			ioctl(buf, DMA_BUF_IOCTL_SYNC, &sync);
@@ -525,7 +484,7 @@ int main(int argc, char *argv[])
 	int is_server = 0, opt;
 	int probe = 0;
 
-	while ((opt = getopt(argc, argv, "ls:c:p:v:q:t:f:P")) != -1) {
+	while ((opt = getopt(argc, argv, "ls:c:p:q:t:f:P")) != -1) {
 		switch (opt) {
 		case 'l':
 			is_server = 1;
@@ -539,9 +498,6 @@ int main(int argc, char *argv[])
 		case 'p':
 			port = optarg;
 			break;
-		case 'v':
-			do_validation = atoll(optarg);
-			break;
 		case 'q':
 			num_queues = atoi(optarg);
 			break;
-- 
2.46.0

Re: [PATCH net-next 02/13] selftests: ncdevmem: Remove validation

[Mina Almasry]

On Thu, Sep 12, 2024 at 10:12 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> ncdevmem should (see next patches) print the payload on the stdout.
> The validation can and should be done by the callers:
>
> $ ncdevmem -l ... > file
> $ sha256sum file
>
> Cc: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> ---
>  tools/testing/selftests/net/ncdevmem.c | 56 +++-----------------------
>  1 file changed, 6 insertions(+), 50 deletions(-)
>
> diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> index 352dba211fb0..3712296d997b 100644
> --- a/tools/testing/selftests/net/ncdevmem.c
> +++ b/tools/testing/selftests/net/ncdevmem.c
> @@ -64,24 +64,13 @@
>  static char *server_ip = "192.168.1.4";
>  static char *client_ip = "192.168.1.2";
>  static char *port = "5201";
> -static size_t do_validation;
>  static int start_queue = 8;
>  static int num_queues = 8;
>  static char *ifname = "eth1";
>  static unsigned int ifindex;
>  static unsigned int dmabuf_id;
>
> -void print_bytes(void *ptr, size_t size)
> -{
> -       unsigned char *p = ptr;
> -       int i;
> -
> -       for (i = 0; i < size; i++)
> -               printf("%02hhX ", p[i]);
> -       printf("\n");
> -}
> -
> -void print_nonzero_bytes(void *ptr, size_t size)
> +static void print_nonzero_bytes(void *ptr, size_t size)
>  {
>         unsigned char *p = ptr;
>         unsigned int i;
> @@ -91,30 +80,6 @@ void print_nonzero_bytes(void *ptr, size_t size)
>         printf("\n");
>  }
>
> -void validate_buffer(void *line, size_t size)
> -{
> -       static unsigned char seed = 1;
> -       unsigned char *ptr = line;
> -       int errors = 0;
> -       size_t i;
> -
> -       for (i = 0; i < size; i++) {
> -               if (ptr[i] != seed) {
> -                       fprintf(stderr,
> -                               "Failed validation: expected=%u, actual=%u, index=%lu\n",
> -                               seed, ptr[i], i);
> -                       errors++;

FWIW the index at where the validation started to fail often gives
critical clues about where the bug is, along with this line, which I'm
glad is not removed:

printf("received frag_page=%llu, in_page_offset=%llu,
frag_offset=%llu, frag_size=%u, token=%u, total_received=%lu,
dmabuf_id=%u\n",

I think we can ensure that what is doing the validation above ncdevmem
prints enough context about the error. Although, just to understand
your thinking a bit, why not have this binary do the validation
itself?

--
Thanks,
Mina

Re: [PATCH net-next 02/13] selftests: ncdevmem: Remove validation

[Stanislav Fomichev]

On 09/12, Mina Almasry wrote:
> On Thu, Sep 12, 2024 at 10:12 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > ncdevmem should (see next patches) print the payload on the stdout.
> > The validation can and should be done by the callers:
> >
> > $ ncdevmem -l ... > file
> > $ sha256sum file
> >
> > Cc: Mina Almasry <almasrymina@google.com>
> > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > ---
> >  tools/testing/selftests/net/ncdevmem.c | 56 +++-----------------------
> >  1 file changed, 6 insertions(+), 50 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> > index 352dba211fb0..3712296d997b 100644
> > --- a/tools/testing/selftests/net/ncdevmem.c
> > +++ b/tools/testing/selftests/net/ncdevmem.c
> > @@ -64,24 +64,13 @@
> >  static char *server_ip = "192.168.1.4";
> >  static char *client_ip = "192.168.1.2";
> >  static char *port = "5201";
> > -static size_t do_validation;
> >  static int start_queue = 8;
> >  static int num_queues = 8;
> >  static char *ifname = "eth1";
> >  static unsigned int ifindex;
> >  static unsigned int dmabuf_id;
> >
> > -void print_bytes(void *ptr, size_t size)
> > -{
> > -       unsigned char *p = ptr;
> > -       int i;
> > -
> > -       for (i = 0; i < size; i++)
> > -               printf("%02hhX ", p[i]);
> > -       printf("\n");
> > -}
> > -
> > -void print_nonzero_bytes(void *ptr, size_t size)
> > +static void print_nonzero_bytes(void *ptr, size_t size)
> >  {
> >         unsigned char *p = ptr;
> >         unsigned int i;
> > @@ -91,30 +80,6 @@ void print_nonzero_bytes(void *ptr, size_t size)
> >         printf("\n");
> >  }
> >
> > -void validate_buffer(void *line, size_t size)
> > -{
> > -       static unsigned char seed = 1;
> > -       unsigned char *ptr = line;
> > -       int errors = 0;
> > -       size_t i;
> > -
> > -       for (i = 0; i < size; i++) {
> > -               if (ptr[i] != seed) {
> > -                       fprintf(stderr,
> > -                               "Failed validation: expected=%u, actual=%u, index=%lu\n",
> > -                               seed, ptr[i], i);
> > -                       errors++;
> 
> FWIW the index at where the validation started to fail often gives
> critical clues about where the bug is, along with this line, which I'm
> glad is not removed:
> 
> printf("received frag_page=%llu, in_page_offset=%llu,
> frag_offset=%llu, frag_size=%u, token=%u, total_received=%lu,
> dmabuf_id=%u\n",
> 
> I think we can ensure that what is doing the validation above ncdevmem
> prints enough context about the error. Although, just to understand
> your thinking a bit, why not have this binary do the validation
> itself?

Right, the debugging stuff will still be there to track the offending
part. And the caller can print out the idx of data where the validation
failed.

The reason I removed it from the tool is to be able to do the validation
with regular nc on the other side. I just do:

- echo "expected payload" | nc ..
- ncdevmem -s .. > expected_payload.txt
- [ "expected payload" != $(cat expected_payload.txt) ] && fail

If I were to use the validation on the ncdevmem side, I'd need to bother
with generating the payload that it expects which seems like
an unnecessary complication? For the (to be posted) txrx test I basically
do the following (sha256sum to validate a bunch of random bytes):

def check_txrx(cfg) -> None:
    cfg.require_v6()
    require_devmem(cfg)

    cmd(f"cat /dev/urandom | tr -dc '[:print:]' | head -c 1M > random_file.txt", host=cfg.remote, shell=True)
    want_sha = cmd(f"sha256sum random_file.txt", host=cfg.remote, shell=True).stdout.strip()

    port = rand_port()
    listen_cmd = f"./ncdevmem -l -f {cfg.ifname} -s {cfg.v6} -p {port} | tee random_file.txt | sha256sum -"

    pwd = cmd(f"pwd").stdout.strip()
    with bkg(listen_cmd, exit_wait=True) as nc:
        wait_port_listen(port)
        cmd(f"cat random_file.txt | {pwd}/ncdevmem -f {cfg.ifname} -s {cfg.v6} -p {port}", host=cfg.remote, shell=True)

    ksft_eq(nc.stdout.strip().split(" ")[0], want_sha.split(" ")[0])

Re: [PATCH net-next 02/13] selftests: ncdevmem: Remove validation

[Mina Almasry]

On Thu, Sep 12, 2024 at 2:57 PM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 09/12, Mina Almasry wrote:
> > On Thu, Sep 12, 2024 at 10:12 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > >
> > > ncdevmem should (see next patches) print the payload on the stdout.
> > > The validation can and should be done by the callers:
> > >
> > > $ ncdevmem -l ... > file
> > > $ sha256sum file
> > >
> > > Cc: Mina Almasry <almasrymina@google.com>
> > > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > > ---
> > >  tools/testing/selftests/net/ncdevmem.c | 56 +++-----------------------
> > >  1 file changed, 6 insertions(+), 50 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> > > index 352dba211fb0..3712296d997b 100644
> > > --- a/tools/testing/selftests/net/ncdevmem.c
> > > +++ b/tools/testing/selftests/net/ncdevmem.c
> > > @@ -64,24 +64,13 @@
> > >  static char *server_ip = "192.168.1.4";
> > >  static char *client_ip = "192.168.1.2";
> > >  static char *port = "5201";
> > > -static size_t do_validation;
> > >  static int start_queue = 8;
> > >  static int num_queues = 8;
> > >  static char *ifname = "eth1";
> > >  static unsigned int ifindex;
> > >  static unsigned int dmabuf_id;
> > >
> > > -void print_bytes(void *ptr, size_t size)
> > > -{
> > > -       unsigned char *p = ptr;
> > > -       int i;
> > > -
> > > -       for (i = 0; i < size; i++)
> > > -               printf("%02hhX ", p[i]);
> > > -       printf("\n");
> > > -}
> > > -
> > > -void print_nonzero_bytes(void *ptr, size_t size)
> > > +static void print_nonzero_bytes(void *ptr, size_t size)
> > >  {
> > >         unsigned char *p = ptr;
> > >         unsigned int i;
> > > @@ -91,30 +80,6 @@ void print_nonzero_bytes(void *ptr, size_t size)
> > >         printf("\n");
> > >  }
> > >
> > > -void validate_buffer(void *line, size_t size)
> > > -{
> > > -       static unsigned char seed = 1;
> > > -       unsigned char *ptr = line;
> > > -       int errors = 0;
> > > -       size_t i;
> > > -
> > > -       for (i = 0; i < size; i++) {
> > > -               if (ptr[i] != seed) {
> > > -                       fprintf(stderr,
> > > -                               "Failed validation: expected=%u, actual=%u, index=%lu\n",
> > > -                               seed, ptr[i], i);
> > > -                       errors++;
> >
> > FWIW the index at where the validation started to fail often gives
> > critical clues about where the bug is, along with this line, which I'm
> > glad is not removed:
> >
> > printf("received frag_page=%llu, in_page_offset=%llu,
> > frag_offset=%llu, frag_size=%u, token=%u, total_received=%lu,
> > dmabuf_id=%u\n",
> >
> > I think we can ensure that what is doing the validation above ncdevmem
> > prints enough context about the error. Although, just to understand
> > your thinking a bit, why not have this binary do the validation
> > itself?
>
> Right, the debugging stuff will still be there to track the offending
> part. And the caller can print out the idx of data where the validation
> failed.
>

Sorry to harp on this, but on second thought I don't think just
printing out the idx is good enough. In many cases all the context
printed by ncdevmem validation (page_frag/offset/dmabuf_id/etc) is
useful, and it's useful to have it inline with where the check failed.

IIUC after your changes the frag_page/offset/dmabuf_id will go to
stderr output of ncdevmem, but the validation check fail will go to a
different log by the parent checker. Matching the failure in the 2
logs in megs of frag output will be annoying.

> The reason I removed it from the tool is to be able to do the validation
> with regular nc on the other side. I just do:
>

To be honest I don't think the ncdevmem validation gets in the way of that.

To test rx, we can set up regular nc on the client side, and have
ncdevmem do validation on rx.
To test tx, we can have ncdevmem do tx (no validation), and have a
script on top of nc do validation on the rx side.

I guess you may find the lack of symmetry annoying, but IMO it's less
annoying than losing some debuggability when the test fails.

I think probably there are 3 hopefully agreeable things we can do here:

1. Keep the validation as is and do the rx/tx test as I described above.
2. keep the ncdevmem validation, dump it to stderr, and ignore it in
the test. Leave it be for folks wanting to run the test manually to
debug it.
3. Move the validation to a parent process of ncdevmem, but that
parent process needs to print full context of the failure in 1 log.

I prefer #1 TBH.

-- 
Thanks,
Mina

Re: [PATCH net-next 02/13] selftests: ncdevmem: Remove validation

[Stanislav Fomichev]

On 09/13, Mina Almasry wrote:
> On Thu, Sep 12, 2024 at 2:57 PM Stanislav Fomichev <stfomichev@gmail.com> wrote:
> >
> > On 09/12, Mina Almasry wrote:
> > > On Thu, Sep 12, 2024 at 10:12 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > > >
> > > > ncdevmem should (see next patches) print the payload on the stdout.
> > > > The validation can and should be done by the callers:
> > > >
> > > > $ ncdevmem -l ... > file
> > > > $ sha256sum file
> > > >
> > > > Cc: Mina Almasry <almasrymina@google.com>
> > > > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > > > ---
> > > >  tools/testing/selftests/net/ncdevmem.c | 56 +++-----------------------
> > > >  1 file changed, 6 insertions(+), 50 deletions(-)
> > > >
> > > > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> > > > index 352dba211fb0..3712296d997b 100644
> > > > --- a/tools/testing/selftests/net/ncdevmem.c
> > > > +++ b/tools/testing/selftests/net/ncdevmem.c
> > > > @@ -64,24 +64,13 @@
> > > >  static char *server_ip = "192.168.1.4";
> > > >  static char *client_ip = "192.168.1.2";
> > > >  static char *port = "5201";
> > > > -static size_t do_validation;
> > > >  static int start_queue = 8;
> > > >  static int num_queues = 8;
> > > >  static char *ifname = "eth1";
> > > >  static unsigned int ifindex;
> > > >  static unsigned int dmabuf_id;
> > > >
> > > > -void print_bytes(void *ptr, size_t size)
> > > > -{
> > > > -       unsigned char *p = ptr;
> > > > -       int i;
> > > > -
> > > > -       for (i = 0; i < size; i++)
> > > > -               printf("%02hhX ", p[i]);
> > > > -       printf("\n");
> > > > -}
> > > > -
> > > > -void print_nonzero_bytes(void *ptr, size_t size)
> > > > +static void print_nonzero_bytes(void *ptr, size_t size)
> > > >  {
> > > >         unsigned char *p = ptr;
> > > >         unsigned int i;
> > > > @@ -91,30 +80,6 @@ void print_nonzero_bytes(void *ptr, size_t size)
> > > >         printf("\n");
> > > >  }
> > > >
> > > > -void validate_buffer(void *line, size_t size)
> > > > -{
> > > > -       static unsigned char seed = 1;
> > > > -       unsigned char *ptr = line;
> > > > -       int errors = 0;
> > > > -       size_t i;
> > > > -
> > > > -       for (i = 0; i < size; i++) {
> > > > -               if (ptr[i] != seed) {
> > > > -                       fprintf(stderr,
> > > > -                               "Failed validation: expected=%u, actual=%u, index=%lu\n",
> > > > -                               seed, ptr[i], i);
> > > > -                       errors++;
> > >
> > > FWIW the index at where the validation started to fail often gives
> > > critical clues about where the bug is, along with this line, which I'm
> > > glad is not removed:
> > >
> > > printf("received frag_page=%llu, in_page_offset=%llu,
> > > frag_offset=%llu, frag_size=%u, token=%u, total_received=%lu,
> > > dmabuf_id=%u\n",
> > >
> > > I think we can ensure that what is doing the validation above ncdevmem
> > > prints enough context about the error. Although, just to understand
> > > your thinking a bit, why not have this binary do the validation
> > > itself?
> >
> > Right, the debugging stuff will still be there to track the offending
> > part. And the caller can print out the idx of data where the validation
> > failed.
> >
> 
> Sorry to harp on this, but on second thought I don't think just
> printing out the idx is good enough. In many cases all the context
> printed by ncdevmem validation (page_frag/offset/dmabuf_id/etc) is
> useful, and it's useful to have it inline with where the check failed.
> 
> IIUC after your changes the frag_page/offset/dmabuf_id will go to
> stderr output of ncdevmem, but the validation check fail will go to a
> different log by the parent checker. Matching the failure in the 2
> logs in megs of frag output will be annoying.

Yes, it will definitely be more annoying to piece those two things
together. But I don't expect us to debug the payload validation issues
from the nipa dashboard logs. Even if you get a clear message of
"byte at position X is not expected" plus all the chunk info logs,
what do you really do with this info (especially if it's flaky)?

For development we can have some script to put those two things together
for debugging.

> > The reason I removed it from the tool is to be able to do the validation
> > with regular nc on the other side. I just do:
> >
> 
> To be honest I don't think the ncdevmem validation gets in the way of that.
> 
> To test rx, we can set up regular nc on the client side, and have
> ncdevmem do validation on rx.
> To test tx, we can have ncdevmem do tx (no validation), and have a
> script on top of nc do validation on the rx side.
> 
> I guess you may find the lack of symmetry annoying, but IMO it's less
> annoying than losing some debuggability when the test fails.
> 
> I think probably there are 3 hopefully agreeable things we can do here:
> 
> 1. Keep the validation as is and do the rx/tx test as I described above.
> 2. keep the ncdevmem validation, dump it to stderr, and ignore it in
> the test. Leave it be for folks wanting to run the test manually to
> debug it.
> 3. Move the validation to a parent process of ncdevmem, but that
> parent process needs to print full context of the failure in 1 log.
> 
> I prefer #1 TBH.

TBH, I find the existing ncdevmem validation scheme a bit lacking. It is
basically the same payload over and over (or maybe I misread how it
works). Maybe we should implement a PRNG-like sequence for validation
if you prefer to keep it internal?

How about we start with what I have (simple 'hello\nworld') and once
you do tx path, we can add the internal validation back? Both sides,
with a proper selftest this time. For the time being, we can use the existing
rx selftest as a smoke test.

Or I can just drop this patch from the series and let you follow up
on the validation in the selftests (i.e., convert from 'hello\nworld'
to whatever you prefer)

Re: [PATCH net-next 02/13] selftests: ncdevmem: Remove validation

[Mina Almasry]

On Fri, Sep 13, 2024 at 10:15 AM Stanislav Fomichev
<stfomichev@gmail.com> wrote:
>
> On 09/13, Mina Almasry wrote:
> > On Thu, Sep 12, 2024 at 2:57 PM Stanislav Fomichev <stfomichev@gmail.com> wrote:
> > >
> > > On 09/12, Mina Almasry wrote:
> > > > On Thu, Sep 12, 2024 at 10:12 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > > > >
> > > > > ncdevmem should (see next patches) print the payload on the stdout.
> > > > > The validation can and should be done by the callers:
> > > > >
> > > > > $ ncdevmem -l ... > file
> > > > > $ sha256sum file
> > > > >
> > > > > Cc: Mina Almasry <almasrymina@google.com>
> > > > > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > > > > ---
> > > > >  tools/testing/selftests/net/ncdevmem.c | 56 +++-----------------------
> > > > >  1 file changed, 6 insertions(+), 50 deletions(-)
> > > > >
> > > > > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> > > > > index 352dba211fb0..3712296d997b 100644
> > > > > --- a/tools/testing/selftests/net/ncdevmem.c
> > > > > +++ b/tools/testing/selftests/net/ncdevmem.c
> > > > > @@ -64,24 +64,13 @@
> > > > >  static char *server_ip = "192.168.1.4";
> > > > >  static char *client_ip = "192.168.1.2";
> > > > >  static char *port = "5201";
> > > > > -static size_t do_validation;
> > > > >  static int start_queue = 8;
> > > > >  static int num_queues = 8;
> > > > >  static char *ifname = "eth1";
> > > > >  static unsigned int ifindex;
> > > > >  static unsigned int dmabuf_id;
> > > > >
> > > > > -void print_bytes(void *ptr, size_t size)
> > > > > -{
> > > > > -       unsigned char *p = ptr;
> > > > > -       int i;
> > > > > -
> > > > > -       for (i = 0; i < size; i++)
> > > > > -               printf("%02hhX ", p[i]);
> > > > > -       printf("\n");
> > > > > -}
> > > > > -
> > > > > -void print_nonzero_bytes(void *ptr, size_t size)
> > > > > +static void print_nonzero_bytes(void *ptr, size_t size)
> > > > >  {
> > > > >         unsigned char *p = ptr;
> > > > >         unsigned int i;
> > > > > @@ -91,30 +80,6 @@ void print_nonzero_bytes(void *ptr, size_t size)
> > > > >         printf("\n");
> > > > >  }
> > > > >
> > > > > -void validate_buffer(void *line, size_t size)
> > > > > -{
> > > > > -       static unsigned char seed = 1;
> > > > > -       unsigned char *ptr = line;
> > > > > -       int errors = 0;
> > > > > -       size_t i;
> > > > > -
> > > > > -       for (i = 0; i < size; i++) {
> > > > > -               if (ptr[i] != seed) {
> > > > > -                       fprintf(stderr,
> > > > > -                               "Failed validation: expected=%u, actual=%u, index=%lu\n",
> > > > > -                               seed, ptr[i], i);
> > > > > -                       errors++;
> > > >
> > > > FWIW the index at where the validation started to fail often gives
> > > > critical clues about where the bug is, along with this line, which I'm
> > > > glad is not removed:
> > > >
> > > > printf("received frag_page=%llu, in_page_offset=%llu,
> > > > frag_offset=%llu, frag_size=%u, token=%u, total_received=%lu,
> > > > dmabuf_id=%u\n",
> > > >
> > > > I think we can ensure that what is doing the validation above ncdevmem
> > > > prints enough context about the error. Although, just to understand
> > > > your thinking a bit, why not have this binary do the validation
> > > > itself?
> > >
> > > Right, the debugging stuff will still be there to track the offending
> > > part. And the caller can print out the idx of data where the validation
> > > failed.
> > >
> >
> > Sorry to harp on this, but on second thought I don't think just
> > printing out the idx is good enough. In many cases all the context
> > printed by ncdevmem validation (page_frag/offset/dmabuf_id/etc) is
> > useful, and it's useful to have it inline with where the check failed.
> >
> > IIUC after your changes the frag_page/offset/dmabuf_id will go to
> > stderr output of ncdevmem, but the validation check fail will go to a
> > different log by the parent checker. Matching the failure in the 2
> > logs in megs of frag output will be annoying.
>
> Yes, it will definitely be more annoying to piece those two things
> together. But I don't expect us to debug the payload validation issues
> from the nipa dashboard logs. Even if you get a clear message of
> "byte at position X is not expected" plus all the chunk info logs,
> what do you really do with this info (especially if it's flaky)?
>

Oh, they provide important clues, even for flakes. See an example of
Taehee using these clues to arrive at a root cause here:
https://lore.kernel.org/netdev/CAMArcTUXm13xJO9XqcT=0uQAn_ZQOQ=Y49EPpHqV+jkkhihMcw@mail.gmail.com/

> For development we can have some script to put those two things together
> for debugging.
>
> > > The reason I removed it from the tool is to be able to do the validation
> > > with regular nc on the other side. I just do:
> > >
> >
> > To be honest I don't think the ncdevmem validation gets in the way of that.
> >
> > To test rx, we can set up regular nc on the client side, and have
> > ncdevmem do validation on rx.
> > To test tx, we can have ncdevmem do tx (no validation), and have a
> > script on top of nc do validation on the rx side.
> >
> > I guess you may find the lack of symmetry annoying, but IMO it's less
> > annoying than losing some debuggability when the test fails.
> >
> > I think probably there are 3 hopefully agreeable things we can do here:
> >
> > 1. Keep the validation as is and do the rx/tx test as I described above.
> > 2. keep the ncdevmem validation, dump it to stderr, and ignore it in
> > the test. Leave it be for folks wanting to run the test manually to
> > debug it.
> > 3. Move the validation to a parent process of ncdevmem, but that
> > parent process needs to print full context of the failure in 1 log.
> >
> > I prefer #1 TBH.
>
> TBH, I find the existing ncdevmem validation scheme a bit lacking. It is
> basically the same payload over and over (or maybe I misread how it
> works). Maybe we should implement a PRNG-like sequence for validation
> if you prefer to keep it internal?
>

It's just a repeating sequence that's easy to implement. Technically
it would not detect if we drop an N iterations in the sequence, but in
practice it's hard to be that unlucky repeatedly, especially if the
number of repeating bytes is not a power of 2. Improving it to a more
robust sequence sounds fine to me.

> How about we start with what I have (simple 'hello\nworld') and once
> you do tx path, we can add the internal validation back? Both sides,
> with a proper selftest this time. For the time being, we can use the existing
> rx selftest as a smoke test.
>
> Or I can just drop this patch from the series and let you follow up
> on the validation in the selftests (i.e., convert from 'hello\nworld'
> to whatever you prefer)

To be honest the validation has been too important in the testing so
far in root causing issues that I would not like it temporarily
removed. Dropping this patch and letting me add validation to NIPA
later on sounds good to me.

-- 
Thanks,
Mina

[PATCH net-next 03/13] selftests: ncdevmem: Redirect all non-payload output to stderr

[Stanislav Fomichev]

That should make it possible to do expected payload validation on
the caller side.

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/ncdevmem.c | 61 +++++++++++++-------------
 1 file changed, 30 insertions(+), 31 deletions(-)

diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
index 3712296d997b..7fb930571ff9 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -77,7 +77,6 @@ static void print_nonzero_bytes(void *ptr, size_t size)
 
 	for (i = 0; i < size; i++)
 		putchar(p[i]);
-	printf("\n");
 }
 
 #define run_command(cmd, ...)                                           \
@@ -85,7 +84,7 @@ static void print_nonzero_bytes(void *ptr, size_t size)
 		char command[256];                                      \
 		memset(command, 0, sizeof(command));                    \
 		snprintf(command, sizeof(command), cmd, ##__VA_ARGS__); \
-		printf("Running: %s\n", command);                       \
+		fprintf(stderr, "Running: %s\n", command);                       \
 		system(command);                                        \
 	})
 
@@ -93,22 +92,22 @@ static int reset_flow_steering(void)
 {
 	int ret = 0;
 
-	ret = run_command("sudo ethtool -K %s ntuple off", ifname);
+	ret = run_command("sudo ethtool -K %s ntuple off >&2", ifname);
 	if (ret)
 		return ret;
 
-	return run_command("sudo ethtool -K %s ntuple on", ifname);
+	return run_command("sudo ethtool -K %s ntuple on >&2", ifname);
 }
 
 static int configure_headersplit(bool on)
 {
-	return run_command("sudo ethtool -G %s tcp-data-split %s", ifname,
+	return run_command("sudo ethtool -G %s tcp-data-split %s >&2", ifname,
 			   on ? "on" : "off");
 }
 
 static int configure_rss(void)
 {
-	return run_command("sudo ethtool -X %s equal %d", ifname, start_queue);
+	return run_command("sudo ethtool -X %s equal %d >&2", ifname, start_queue);
 }
 
 static int configure_channels(unsigned int rx, unsigned int tx)
@@ -118,7 +117,7 @@ static int configure_channels(unsigned int rx, unsigned int tx)
 
 static int configure_flow_steering(void)
 {
-	return run_command("sudo ethtool -N %s flow-type tcp4 src-ip %s dst-ip %s src-port %s dst-port %s queue %d",
+	return run_command("sudo ethtool -N %s flow-type tcp4 src-ip %s dst-ip %s src-port %s dst-port %s queue %d >&2",
 			   ifname, client_ip, server_ip, port, port, start_queue);
 }
 
@@ -152,7 +151,7 @@ static int bind_rx_queue(unsigned int ifindex, unsigned int dmabuf_fd,
 		goto err_close;
 	}
 
-	printf("got dmabuf id=%d\n", rsp->id);
+	fprintf(stderr, "got dmabuf id=%d\n", rsp->id);
 	dmabuf_id = rsp->id;
 
 	netdev_bind_rx_req_free(req);
@@ -279,8 +278,8 @@ int do_server(void)
 	if (ret)
 		error(errno, errno, "%s: [FAIL, set sock opt]\n", TEST_PREFIX);
 
-	printf("binding to address %s:%d\n", server_ip,
-	       ntohs(server_sin.sin_port));
+	fprintf(stderr, "binding to address %s:%d\n", server_ip,
+		ntohs(server_sin.sin_port));
 
 	ret = bind(socket_fd, &server_sin, sizeof(server_sin));
 	if (ret)
@@ -294,14 +293,14 @@ int do_server(void)
 
 	inet_ntop(server_sin.sin_family, &server_sin.sin_addr, buffer,
 		  sizeof(buffer));
-	printf("Waiting or connection on %s:%d\n", buffer,
-	       ntohs(server_sin.sin_port));
+	fprintf(stderr, "Waiting or connection on %s:%d\n", buffer,
+		ntohs(server_sin.sin_port));
 	client_fd = accept(socket_fd, &client_addr, &client_addr_len);
 
 	inet_ntop(client_addr.sin_family, &client_addr.sin_addr, buffer,
 		  sizeof(buffer));
-	printf("Got connection from %s:%d\n", buffer,
-	       ntohs(client_addr.sin_port));
+	fprintf(stderr, "Got connection from %s:%d\n", buffer,
+		ntohs(client_addr.sin_port));
 
 	while (1) {
 		struct iovec iov = { .iov_base = iobuf,
@@ -314,14 +313,13 @@ int do_server(void)
 		ssize_t ret;
 
 		is_devmem = false;
-		printf("\n\n");
 
 		msg.msg_iov = &iov;
 		msg.msg_iovlen = 1;
 		msg.msg_control = ctrl_data;
 		msg.msg_controllen = sizeof(ctrl_data);
 		ret = recvmsg(client_fd, &msg, MSG_SOCK_DEVMEM);
-		printf("recvmsg ret=%ld\n", ret);
+		fprintf(stderr, "recvmsg ret=%ld\n", ret);
 		if (ret < 0 && (errno == EAGAIN || errno == EWOULDBLOCK))
 			continue;
 		if (ret < 0) {
@@ -329,7 +327,7 @@ int do_server(void)
 			continue;
 		}
 		if (ret == 0) {
-			printf("client exited\n");
+			fprintf(stderr, "client exited\n");
 			goto cleanup;
 		}
 
@@ -338,7 +336,7 @@ int do_server(void)
 			if (cm->cmsg_level != SOL_SOCKET ||
 			    (cm->cmsg_type != SCM_DEVMEM_DMABUF &&
 			     cm->cmsg_type != SCM_DEVMEM_LINEAR)) {
-				fprintf(stdout, "skipping non-devmem cmsg\n");
+				fprintf(stderr, "skipping non-devmem cmsg\n");
 				continue;
 			}
 
@@ -349,7 +347,7 @@ int do_server(void)
 				/* TODO: process data copied from skb's linear
 				 * buffer.
 				 */
-				fprintf(stdout,
+				fprintf(stderr,
 					"SCM_DEVMEM_LINEAR. dmabuf_cmsg->frag_size=%u\n",
 					dmabuf_cmsg->frag_size);
 
@@ -360,12 +358,13 @@ int do_server(void)
 			token.token_count = 1;
 
 			total_received += dmabuf_cmsg->frag_size;
-			printf("received frag_page=%llu, in_page_offset=%llu, frag_offset=%llu, frag_size=%u, token=%u, total_received=%lu, dmabuf_id=%u\n",
-			       dmabuf_cmsg->frag_offset >> PAGE_SHIFT,
-			       dmabuf_cmsg->frag_offset % getpagesize(),
-			       dmabuf_cmsg->frag_offset, dmabuf_cmsg->frag_size,
-			       dmabuf_cmsg->frag_token, total_received,
-			       dmabuf_cmsg->dmabuf_id);
+			fprintf(stderr,
+				"received frag_page=%llu, in_page_offset=%llu, frag_offset=%llu, frag_size=%u, token=%u, total_received=%lu, dmabuf_id=%u\n",
+				dmabuf_cmsg->frag_offset >> PAGE_SHIFT,
+				dmabuf_cmsg->frag_offset % getpagesize(),
+				dmabuf_cmsg->frag_offset,
+				dmabuf_cmsg->frag_size, dmabuf_cmsg->frag_token,
+				total_received, dmabuf_cmsg->dmabuf_id);
 
 			if (dmabuf_cmsg->dmabuf_id != dmabuf_id)
 				error(1, 0,
@@ -397,15 +396,15 @@ int do_server(void)
 		if (!is_devmem)
 			error(1, 0, "flow steering error\n");
 
-		printf("total_received=%lu\n", total_received);
+		fprintf(stderr, "total_received=%lu\n", total_received);
 	}
 
-	fprintf(stdout, "%s: ok\n", TEST_PREFIX);
+	fprintf(stderr, "%s: ok\n", TEST_PREFIX);
 
-	fprintf(stdout, "page_aligned_frags=%lu, non_page_aligned_frags=%lu\n",
+	fprintf(stderr, "page_aligned_frags=%lu, non_page_aligned_frags=%lu\n",
 		page_aligned_frags, non_page_aligned_frags);
 
-	fprintf(stdout, "page_aligned_frags=%lu, non_page_aligned_frags=%lu\n",
+	fprintf(stderr, "page_aligned_frags=%lu, non_page_aligned_frags=%lu\n",
 		page_aligned_frags, non_page_aligned_frags);
 
 cleanup:
@@ -511,7 +510,7 @@ int main(int argc, char *argv[])
 			probe = 1;
 			break;
 		case '?':
-			printf("unknown option: %c\n", optopt);
+			fprintf(stderr, "unknown option: %c\n", optopt);
 			break;
 		}
 	}
@@ -519,7 +518,7 @@ int main(int argc, char *argv[])
 	ifindex = if_nametoindex(ifname);
 
 	for (; optind < argc; optind++)
-		printf("extra arguments: %s\n", argv[optind]);
+		fprintf(stderr, "extra arguments: %s\n", argv[optind]);
 
 	if (probe) {
 		run_devmem_tests();
-- 
2.46.0

[PATCH net-next 04/13] selftests: ncdevmem: Separate out dmabuf provider

[Stanislav Fomichev]

So we can plug the other ones in the future if needed.

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/ncdevmem.c | 209 ++++++++++++++++---------
 1 file changed, 136 insertions(+), 73 deletions(-)

diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
index 7fb930571ff9..a20f40adfde8 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -70,6 +70,117 @@ static char *ifname = "eth1";
 static unsigned int ifindex;
 static unsigned int dmabuf_id;
 
+struct memory_buffer {
+	int fd;
+	size_t size;
+
+	int devfd;
+	int memfd;
+	char *buf_mem;
+};
+
+struct memory_provider {
+	struct memory_buffer *(*alloc)(size_t size);
+	void (*free)(struct memory_buffer *ctx);
+	void (*memcpy_to_device)(struct memory_buffer *dst, size_t off,
+				 void *src, int n);
+	void (*memcpy_from_device)(void *dst, struct memory_buffer *src,
+				   size_t off, int n);
+};
+
+static struct memory_buffer *udmabuf_alloc(size_t size)
+{
+	struct udmabuf_create create;
+	struct memory_buffer *ctx;
+	int ret;
+
+	ctx = malloc(sizeof(*ctx));
+	if (!ctx)
+		error(1, ENOMEM, "malloc failed");
+
+	ctx->size = size;
+
+	ctx->devfd = open("/dev/udmabuf", O_RDWR);
+	if (ctx->devfd < 0)
+		error(1, errno,
+		      "%s: [skip,no-udmabuf: Unable to access DMA buffer device file]\n",
+		      TEST_PREFIX);
+
+	ctx->memfd = memfd_create("udmabuf-test", MFD_ALLOW_SEALING);
+	if (ctx->memfd < 0)
+		error(1, errno, "%s: [skip,no-memfd]\n", TEST_PREFIX);
+
+	ret = fcntl(ctx->memfd, F_ADD_SEALS, F_SEAL_SHRINK);
+	if (ret < 0)
+		error(1, errno, "%s: [skip,fcntl-add-seals]\n", TEST_PREFIX);
+
+	ret = ftruncate(ctx->memfd, size);
+	if (ret == -1)
+		error(1, errno, "%s: [FAIL,memfd-truncate]\n", TEST_PREFIX);
+
+	memset(&create, 0, sizeof(create));
+
+	create.memfd = ctx->memfd;
+	create.offset = 0;
+	create.size = size;
+	ctx->fd = ioctl(ctx->devfd, UDMABUF_CREATE, &create);
+	if (ctx->fd < 0)
+		error(1, errno, "%s: [FAIL, create udmabuf]\n", TEST_PREFIX);
+
+	ctx->buf_mem = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED,
+			    ctx->fd, 0);
+	if (ctx->buf_mem == MAP_FAILED)
+		error(1, errno, "%s: [FAIL, map udmabuf]\n", TEST_PREFIX);
+
+	return ctx;
+}
+
+static void udmabuf_free(struct memory_buffer *ctx)
+{
+	munmap(ctx->buf_mem, ctx->size);
+	close(ctx->fd);
+	close(ctx->memfd);
+	close(ctx->devfd);
+	free(ctx);
+}
+
+static void udmabuf_memcpy_to_device(struct memory_buffer *dst, size_t off,
+				     void *src, int n)
+{
+	struct dma_buf_sync sync = {};
+
+	sync.flags = DMA_BUF_SYNC_START | DMA_BUF_SYNC_WRITE;
+	ioctl(dst->fd, DMA_BUF_IOCTL_SYNC, &sync);
+
+	memcpy(dst->buf_mem + off, src, n);
+
+	sync.flags = DMA_BUF_SYNC_END | DMA_BUF_SYNC_WRITE;
+	ioctl(dst->fd, DMA_BUF_IOCTL_SYNC, &sync);
+}
+
+static void udmabuf_memcpy_from_device(void *dst, struct memory_buffer *src,
+				       size_t off, int n)
+{
+	struct dma_buf_sync sync = {};
+
+	sync.flags = DMA_BUF_SYNC_START;
+	ioctl(src->fd, DMA_BUF_IOCTL_SYNC, &sync);
+
+	memcpy(dst, src->buf_mem + off, n);
+
+	sync.flags = DMA_BUF_SYNC_END;
+	ioctl(src->fd, DMA_BUF_IOCTL_SYNC, &sync);
+}
+
+static struct memory_provider udmabuf_memory_provider = {
+	.alloc = udmabuf_alloc,
+	.free = udmabuf_free,
+	.memcpy_to_device = udmabuf_memcpy_to_device,
+	.memcpy_from_device = udmabuf_memcpy_from_device,
+};
+
+static struct memory_provider *provider = &udmabuf_memory_provider;
+
 static void print_nonzero_bytes(void *ptr, size_t size)
 {
 	unsigned char *p = ptr;
@@ -166,42 +277,7 @@ static int bind_rx_queue(unsigned int ifindex, unsigned int dmabuf_fd,
 	return -1;
 }
 
-static void create_udmabuf(int *devfd, int *memfd, int *buf, size_t dmabuf_size)
-{
-	struct udmabuf_create create;
-	int ret;
-
-	*devfd = open("/dev/udmabuf", O_RDWR);
-	if (*devfd < 0) {
-		error(70, 0,
-		      "%s: [skip,no-udmabuf: Unable to access DMA buffer device file]\n",
-		      TEST_PREFIX);
-	}
-
-	*memfd = memfd_create("udmabuf-test", MFD_ALLOW_SEALING);
-	if (*memfd < 0)
-		error(70, 0, "%s: [skip,no-memfd]\n", TEST_PREFIX);
-
-	/* Required for udmabuf */
-	ret = fcntl(*memfd, F_ADD_SEALS, F_SEAL_SHRINK);
-	if (ret < 0)
-		error(73, 0, "%s: [skip,fcntl-add-seals]\n", TEST_PREFIX);
-
-	ret = ftruncate(*memfd, dmabuf_size);
-	if (ret == -1)
-		error(74, 0, "%s: [FAIL,memfd-truncate]\n", TEST_PREFIX);
-
-	memset(&create, 0, sizeof(create));
-
-	create.memfd = *memfd;
-	create.offset = 0;
-	create.size = dmabuf_size;
-	*buf = ioctl(*devfd, UDMABUF_CREATE, &create);
-	if (*buf < 0)
-		error(75, 0, "%s: [FAIL, create udmabuf]\n", TEST_PREFIX);
-}
-
-int do_server(void)
+int do_server(struct memory_buffer *mem)
 {
 	char ctrl_data[sizeof(int) * 20000];
 	struct netdev_queue_id *queues;
@@ -209,23 +285,18 @@ int do_server(void)
 	struct sockaddr_in client_addr;
 	struct sockaddr_in server_sin;
 	size_t page_aligned_frags = 0;
-	int devfd, memfd, buf, ret;
 	size_t total_received = 0;
 	socklen_t client_addr_len;
 	bool is_devmem = false;
-	char *buf_mem = NULL;
+	char *tmp_mem = NULL;
 	struct ynl_sock *ys;
-	size_t dmabuf_size;
 	char iobuf[819200];
 	char buffer[256];
 	int socket_fd;
 	int client_fd;
 	size_t i = 0;
 	int opt = 1;
-
-	dmabuf_size = getpagesize() * NUM_PAGES;
-
-	create_udmabuf(&devfd, &memfd, &buf, dmabuf_size);
+	int ret;
 
 	if (reset_flow_steering())
 		error(1, 0, "Failed to reset flow steering\n");
@@ -249,13 +320,12 @@ int do_server(void)
 		queues[i].id = start_queue + i;
 	}
 
-	if (bind_rx_queue(ifindex, buf, queues, num_queues, &ys))
+	if (bind_rx_queue(ifindex, mem->fd, queues, num_queues, &ys))
 		error(1, 0, "Failed to bind\n");
 
-	buf_mem = mmap(NULL, dmabuf_size, PROT_READ | PROT_WRITE, MAP_SHARED,
-		       buf, 0);
-	if (buf_mem == MAP_FAILED)
-		error(1, 0, "mmap()");
+	tmp_mem = malloc(mem->size);
+	if (!tmp_mem)
+		error(1, ENOMEM, "malloc failed");
 
 	server_sin.sin_family = AF_INET;
 	server_sin.sin_port = htons(atoi(port));
@@ -306,7 +376,6 @@ int do_server(void)
 		struct iovec iov = { .iov_base = iobuf,
 				     .iov_len = sizeof(iobuf) };
 		struct dmabuf_cmsg *dmabuf_cmsg = NULL;
-		struct dma_buf_sync sync = { 0 };
 		struct cmsghdr *cm = NULL;
 		struct msghdr msg = { 0 };
 		struct dmabuf_token token;
@@ -375,16 +444,11 @@ int do_server(void)
 			else
 				page_aligned_frags++;
 
-			sync.flags = DMA_BUF_SYNC_READ | DMA_BUF_SYNC_START;
-			ioctl(buf, DMA_BUF_IOCTL_SYNC, &sync);
-
-			print_nonzero_bytes(
-				((unsigned char *)buf_mem) +
-					dmabuf_cmsg->frag_offset,
-				dmabuf_cmsg->frag_size);
+			provider->memcpy_from_device(tmp_mem, mem,
+						     dmabuf_cmsg->frag_offset,
+						     dmabuf_cmsg->frag_size);
 
-			sync.flags = DMA_BUF_SYNC_READ | DMA_BUF_SYNC_END;
-			ioctl(buf, DMA_BUF_IOCTL_SYNC, &sync);
+			print_nonzero_bytes(tmp_mem, dmabuf_cmsg->frag_size);
 
 			ret = setsockopt(client_fd, SOL_SOCKET,
 					 SO_DEVMEM_DONTNEED, &token,
@@ -409,12 +473,9 @@ int do_server(void)
 
 cleanup:
 
-	munmap(buf_mem, dmabuf_size);
+	free(tmp_mem);
 	close(client_fd);
 	close(socket_fd);
-	close(buf);
-	close(memfd);
-	close(devfd);
 	ynl_sock_destroy(ys);
 
 	return 0;
@@ -423,14 +484,11 @@ int do_server(void)
 void run_devmem_tests(void)
 {
 	struct netdev_queue_id *queues;
-	int devfd, memfd, buf;
+	struct memory_buffer *mem;
 	struct ynl_sock *ys;
-	size_t dmabuf_size;
 	size_t i = 0;
 
-	dmabuf_size = getpagesize() * NUM_PAGES;
-
-	create_udmabuf(&devfd, &memfd, &buf, dmabuf_size);
+	mem = provider->alloc(getpagesize() * NUM_PAGES);
 
 	/* Configure RSS to divert all traffic from our devmem queues */
 	if (configure_rss())
@@ -441,7 +499,7 @@ void run_devmem_tests(void)
 	if (configure_headersplit(1))
 		error(1, 0, "Failed to configure header split\n");
 
-	if (!bind_rx_queue(ifindex, buf, queues, num_queues, &ys))
+	if (!bind_rx_queue(ifindex, mem->fd, queues, num_queues, &ys))
 		error(1, 0, "Binding empty queues array should have failed\n");
 
 	for (i = 0; i < num_queues; i++) {
@@ -454,7 +512,7 @@ void run_devmem_tests(void)
 	if (configure_headersplit(0))
 		error(1, 0, "Failed to configure header split\n");
 
-	if (!bind_rx_queue(ifindex, buf, queues, num_queues, &ys))
+	if (!bind_rx_queue(ifindex, mem->fd, queues, num_queues, &ys))
 		error(1, 0, "Configure dmabuf with header split off should have failed\n");
 
 	if (configure_headersplit(1))
@@ -467,7 +525,7 @@ void run_devmem_tests(void)
 		queues[i].id = start_queue + i;
 	}
 
-	if (bind_rx_queue(ifindex, buf, queues, num_queues, &ys))
+	if (bind_rx_queue(ifindex, mem->fd, queues, num_queues, &ys))
 		error(1, 0, "Failed to bind\n");
 
 	/* Deactivating a bound queue should not be legal */
@@ -476,12 +534,16 @@ void run_devmem_tests(void)
 
 	/* Closing the netlink socket does an implicit unbind */
 	ynl_sock_destroy(ys);
+
+	provider->free(mem);
 }
 
 int main(int argc, char *argv[])
 {
+	struct memory_buffer *mem;
 	int is_server = 0, opt;
 	int probe = 0;
+	int ret;
 
 	while ((opt = getopt(argc, argv, "ls:c:p:q:t:f:P")) != -1) {
 		switch (opt) {
@@ -525,8 +587,9 @@ int main(int argc, char *argv[])
 		return 0;
 	}
 
-	if (is_server)
-		return do_server();
+	mem = provider->alloc(getpagesize() * NUM_PAGES);
+	ret = is_server ? do_server(mem) : 1;
+	provider->free(mem);
 
-	return 0;
+	return ret;
 }
-- 
2.46.0

Re: [PATCH net-next 04/13] selftests: ncdevmem: Separate out dmabuf provider

[Mina Almasry]

On Thu, Sep 12, 2024 at 10:13 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> So we can plug the other ones in the future if needed.
>
> Cc: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>

Absolutely wonderful to see! It will make it much much easier to plug
in io_uring mem or others in the future.

Probably maybe at that point to rename to ncpm (netcat memory
provider), since it could be more generic than just devmem.

I'll take a closer look and test and provide reviewed-by


--
Thanks,
Mina

[PATCH net-next 05/13] selftests: ncdevmem: Unify error handling

[Stanislav Fomichev]

There is a bunch of places where error() calls look out of place.
Use the same error(1, errno, ...) pattern everywhere.

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/ncdevmem.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
index a20f40adfde8..c0da2b2e077f 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -332,32 +332,32 @@ int do_server(struct memory_buffer *mem)
 
 	ret = inet_pton(server_sin.sin_family, server_ip, &server_sin.sin_addr);
 	if (socket < 0)
-		error(79, 0, "%s: [FAIL, create socket]\n", TEST_PREFIX);
+		error(1, 0, "%s: [FAIL, create socket]\n", TEST_PREFIX);
 
 	socket_fd = socket(server_sin.sin_family, SOCK_STREAM, 0);
 	if (socket < 0)
-		error(errno, errno, "%s: [FAIL, create socket]\n", TEST_PREFIX);
+		error(1, errno, "%s: [FAIL, create socket]\n", TEST_PREFIX);
 
 	ret = setsockopt(socket_fd, SOL_SOCKET, SO_REUSEPORT, &opt,
 			 sizeof(opt));
 	if (ret)
-		error(errno, errno, "%s: [FAIL, set sock opt]\n", TEST_PREFIX);
+		error(1, errno, "%s: [FAIL, set sock opt]\n", TEST_PREFIX);
 
 	ret = setsockopt(socket_fd, SOL_SOCKET, SO_REUSEADDR, &opt,
 			 sizeof(opt));
 	if (ret)
-		error(errno, errno, "%s: [FAIL, set sock opt]\n", TEST_PREFIX);
+		error(1, errno, "%s: [FAIL, set sock opt]\n", TEST_PREFIX);
 
 	fprintf(stderr, "binding to address %s:%d\n", server_ip,
 		ntohs(server_sin.sin_port));
 
 	ret = bind(socket_fd, &server_sin, sizeof(server_sin));
 	if (ret)
-		error(errno, errno, "%s: [FAIL, bind]\n", TEST_PREFIX);
+		error(1, errno, "%s: [FAIL, bind]\n", TEST_PREFIX);
 
 	ret = listen(socket_fd, 1);
 	if (ret)
-		error(errno, errno, "%s: [FAIL, listen]\n", TEST_PREFIX);
+		error(1, errno, "%s: [FAIL, listen]\n", TEST_PREFIX);
 
 	client_addr_len = sizeof(client_addr);
 
-- 
2.46.0

Re: [PATCH net-next 05/13] selftests: ncdevmem: Unify error handling

[Mina Almasry]

On Thu, Sep 12, 2024 at 10:13 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> There is a bunch of places where error() calls look out of place.
> Use the same error(1, errno, ...) pattern everywhere.
>
> Cc: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> ---
>  tools/testing/selftests/net/ncdevmem.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> index a20f40adfde8..c0da2b2e077f 100644
> --- a/tools/testing/selftests/net/ncdevmem.c
> +++ b/tools/testing/selftests/net/ncdevmem.c
> @@ -332,32 +332,32 @@ int do_server(struct memory_buffer *mem)
>
>         ret = inet_pton(server_sin.sin_family, server_ip, &server_sin.sin_addr);
>         if (socket < 0)
> -               error(79, 0, "%s: [FAIL, create socket]\n", TEST_PREFIX);
> +               error(1, 0, "%s: [FAIL, create socket]\n", TEST_PREFIX);
>
>         socket_fd = socket(server_sin.sin_family, SOCK_STREAM, 0);
>         if (socket < 0)
> -               error(errno, errno, "%s: [FAIL, create socket]\n", TEST_PREFIX);
> +               error(1, errno, "%s: [FAIL, create socket]\n", TEST_PREFIX);
>

To be honest this was a bit intentional. For example here, I want to
see what errno socket() failed with; it's a clue to why it failed.

I guess you're not actually removing that, right? You're making the
return code of ncdevmem 1, but it will still print the errno of the
subfailure? That sounds fine.

Isn't it a bit more standard for the sub errno to be the return of the
parent process. But not opposed. If you think this is better we can go
for it.


--
Thanks,
Mina

Re: [PATCH net-next 05/13] selftests: ncdevmem: Unify error handling

[Stanislav Fomichev]

On 09/12, Mina Almasry wrote:
> On Thu, Sep 12, 2024 at 10:13 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > There is a bunch of places where error() calls look out of place.
> > Use the same error(1, errno, ...) pattern everywhere.
> >
> > Cc: Mina Almasry <almasrymina@google.com>
> > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > ---
> >  tools/testing/selftests/net/ncdevmem.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> > index a20f40adfde8..c0da2b2e077f 100644
> > --- a/tools/testing/selftests/net/ncdevmem.c
> > +++ b/tools/testing/selftests/net/ncdevmem.c
> > @@ -332,32 +332,32 @@ int do_server(struct memory_buffer *mem)
> >
> >         ret = inet_pton(server_sin.sin_family, server_ip, &server_sin.sin_addr);
> >         if (socket < 0)
> > -               error(79, 0, "%s: [FAIL, create socket]\n", TEST_PREFIX);
> > +               error(1, 0, "%s: [FAIL, create socket]\n", TEST_PREFIX);
> >
> >         socket_fd = socket(server_sin.sin_family, SOCK_STREAM, 0);
> >         if (socket < 0)
> > -               error(errno, errno, "%s: [FAIL, create socket]\n", TEST_PREFIX);
> > +               error(1, errno, "%s: [FAIL, create socket]\n", TEST_PREFIX);
> >
> 
> To be honest this was a bit intentional. For example here, I want to
> see what errno socket() failed with; it's a clue to why it failed.
> 
> I guess you're not actually removing that, right? You're making the
> return code of ncdevmem 1, but it will still print the errno of the
> subfailure? That sounds fine.
> 
> Isn't it a bit more standard for the sub errno to be the return of the
> parent process. But not opposed. If you think this is better we can go
> for it.

Yes, this will still print the correct errno.

[PATCH net-next 06/13] selftests: ncdevmem: Remove client_ip

[Stanislav Fomichev]

It's used only in ntuple filter, but having dst address/port should
be enough.

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/ncdevmem.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
index c0da2b2e077f..77f6cb166ada 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -62,7 +62,6 @@
  */
 
 static char *server_ip = "192.168.1.4";
-static char *client_ip = "192.168.1.2";
 static char *port = "5201";
 static int start_queue = 8;
 static int num_queues = 8;
@@ -228,8 +227,8 @@ static int configure_channels(unsigned int rx, unsigned int tx)
 
 static int configure_flow_steering(void)
 {
-	return run_command("sudo ethtool -N %s flow-type tcp4 src-ip %s dst-ip %s src-port %s dst-port %s queue %d >&2",
-			   ifname, client_ip, server_ip, port, port, start_queue);
+	return run_command("sudo ethtool -N %s flow-type tcp4 dst-ip %s dst-port %s queue %d >&2",
+			   ifname, server_ip, port, start_queue);
 }
 
 static int bind_rx_queue(unsigned int ifindex, unsigned int dmabuf_fd,
@@ -553,9 +552,6 @@ int main(int argc, char *argv[])
 		case 's':
 			server_ip = optarg;
 			break;
-		case 'c':
-			client_ip = optarg;
-			break;
 		case 'p':
 			port = optarg;
 			break;
-- 
2.46.0

Re: [PATCH net-next 06/13] selftests: ncdevmem: Remove client_ip

[Mina Almasry]

On Thu, Sep 12, 2024 at 10:13 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> It's used only in ntuple filter, but having dst address/port should
> be enough.
>
> Cc: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> ---
>  tools/testing/selftests/net/ncdevmem.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> index c0da2b2e077f..77f6cb166ada 100644
> --- a/tools/testing/selftests/net/ncdevmem.c
> +++ b/tools/testing/selftests/net/ncdevmem.c
> @@ -62,7 +62,6 @@
>   */
>
>  static char *server_ip = "192.168.1.4";
> -static char *client_ip = "192.168.1.2";
>  static char *port = "5201";
>  static int start_queue = 8;
>  static int num_queues = 8;
> @@ -228,8 +227,8 @@ static int configure_channels(unsigned int rx, unsigned int tx)
>
>  static int configure_flow_steering(void)
>  {
> -       return run_command("sudo ethtool -N %s flow-type tcp4 src-ip %s dst-ip %s src-port %s dst-port %s queue %d >&2",
> -                          ifname, client_ip, server_ip, port, port, start_queue);
> +       return run_command("sudo ethtool -N %s flow-type tcp4 dst-ip %s dst-port %s queue %d >&2",
> +                          ifname, server_ip, port, start_queue);
>  }

Oh, sorry. I need 5-tuple rules here. Unfortunately GVE doesn't (yet)
support 3 tuple rules that you're converting to here, AFAIR. Other
drivers may also have a similar limitation.

If you would like to add support for 3-tuples rules because your
driver needs it, that's more than fine, but I would ask that this be
configurable via a flag, or auto-detected by ncdevmem.

Re: [PATCH net-next 06/13] selftests: ncdevmem: Remove client_ip

[Stanislav Fomichev]

On 09/12, Mina Almasry wrote:
> On Thu, Sep 12, 2024 at 10:13 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > It's used only in ntuple filter, but having dst address/port should
> > be enough.
> >
> > Cc: Mina Almasry <almasrymina@google.com>
> > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > ---
> >  tools/testing/selftests/net/ncdevmem.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> > index c0da2b2e077f..77f6cb166ada 100644
> > --- a/tools/testing/selftests/net/ncdevmem.c
> > +++ b/tools/testing/selftests/net/ncdevmem.c
> > @@ -62,7 +62,6 @@
> >   */
> >
> >  static char *server_ip = "192.168.1.4";
> > -static char *client_ip = "192.168.1.2";
> >  static char *port = "5201";
> >  static int start_queue = 8;
> >  static int num_queues = 8;
> > @@ -228,8 +227,8 @@ static int configure_channels(unsigned int rx, unsigned int tx)
> >
> >  static int configure_flow_steering(void)
> >  {
> > -       return run_command("sudo ethtool -N %s flow-type tcp4 src-ip %s dst-ip %s src-port %s dst-port %s queue %d >&2",
> > -                          ifname, client_ip, server_ip, port, port, start_queue);
> > +       return run_command("sudo ethtool -N %s flow-type tcp4 dst-ip %s dst-port %s queue %d >&2",
> > +                          ifname, server_ip, port, start_queue);
> >  }
> 
> Oh, sorry. I need 5-tuple rules here. Unfortunately GVE doesn't (yet)
> support 3 tuple rules that you're converting to here, AFAIR. Other
> drivers may also have a similar limitation.
> 
> If you would like to add support for 3-tuples rules because your
> driver needs it, that's more than fine, but I would ask that this be
> configurable via a flag, or auto-detected by ncdevmem.

Ah, cool, in this case I'll make client ip optional (will keep existing
behavior when client_ip is provided).

[PATCH net-next 07/13] selftests: ncdevmem: Remove default arguments

[Stanislav Fomichev]

To make it clear what's required and what's not. Also, some of the
values don't seem like a good defaults; for example eth1.

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/ncdevmem.c | 34 +++++++++-----------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
index 77f6cb166ada..829a7066387a 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -42,30 +42,11 @@
 #define MSG_SOCK_DEVMEM 0x2000000
 #endif
 
-/*
- * tcpdevmem netcat. Works similarly to netcat but does device memory TCP
- * instead of regular TCP. Uses udmabuf to mock a dmabuf provider.
- *
- * Usage:
- *
- *	On server:
- *	ncdevmem -s <server IP> -c <client IP> -f eth1 -l -p 5201 -v 7
- *
- *	On client:
- *	yes $(echo -e \\x01\\x02\\x03\\x04\\x05\\x06) | \
- *		tr \\n \\0 | \
- *		head -c 5G | \
- *		nc <server IP> 5201 -p 5201
- *
- * Note this is compatible with regular netcat. i.e. the sender or receiver can
- * be replaced with regular netcat to test the RX or TX path in isolation.
- */
-
-static char *server_ip = "192.168.1.4";
-static char *port = "5201";
+static char *server_ip;
+static char *port;
 static int start_queue = 8;
 static int num_queues = 8;
-static char *ifname = "eth1";
+static char *ifname;
 static unsigned int ifindex;
 static unsigned int dmabuf_id;
 
@@ -573,6 +554,15 @@ int main(int argc, char *argv[])
 		}
 	}
 
+	if (!server_ip)
+		error(1, 0, "Missing -s argument\n");
+
+	if (!port)
+		error(1, 0, "Missing -p argument\n");
+
+	if (!ifname)
+		error(1, 0, "Missing -f argument\n");
+
 	ifindex = if_nametoindex(ifname);
 
 	for (; optind < argc; optind++)
-- 
2.46.0

[PATCH net-next 08/13] selftests: ncdevmem: Switch to AF_INET6

[Stanislav Fomichev]

Use dualstack socket to support both v4 and v6. v4-mapped-v6 address
can be used to do v4.

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/ncdevmem.c | 84 +++++++++++++++++---------
 1 file changed, 56 insertions(+), 28 deletions(-)

diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
index 829a7066387a..d82e550369c0 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -206,10 +206,18 @@ static int configure_channels(unsigned int rx, unsigned int tx)
 	return run_command("sudo ethtool -L %s rx %u tx %u", ifname, rx, tx);
 }
 
-static int configure_flow_steering(void)
+static int configure_flow_steering(struct sockaddr_in6 *server_sin)
 {
-	return run_command("sudo ethtool -N %s flow-type tcp4 dst-ip %s dst-port %s queue %d >&2",
-			   ifname, server_ip, port, start_queue);
+	const char *addr = server_ip;
+	const char *type = "tcp6";
+
+	if (IN6_IS_ADDR_V4MAPPED(&server_sin->sin6_addr)) {
+		type = "tcp4";
+		addr = strrchr(server_ip, ':') + 1;
+	}
+
+	return run_command("sudo ethtool -N %s flow-type %s dst-ip %s dst-port %s queue %d >&2",
+			   ifname, type, addr, port, start_queue);
 }
 
 static int bind_rx_queue(unsigned int ifindex, unsigned int dmabuf_fd,
@@ -257,13 +265,43 @@ static int bind_rx_queue(unsigned int ifindex, unsigned int dmabuf_fd,
 	return -1;
 }
 
+static int enable_reuseaddr(int fd)
+{
+	int opt = 1;
+	int ret;
+
+	ret = setsockopt(fd, SOL_SOCKET, SO_REUSEPORT, &opt, sizeof(opt));
+	if (ret)
+		return -errno;
+
+	ret = setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt));
+	if (ret)
+		return -errno;
+
+	return 0;
+}
+
+static int parse_address(const char *str, int port, struct sockaddr_in6 *sin6)
+{
+	int ret;
+
+	sin6->sin6_family = AF_INET6;
+	sin6->sin6_port = htons(port);
+
+	ret = inet_pton(sin6->sin6_family, str, &sin6->sin6_addr);
+	if (ret < 0)
+		return -1;
+
+	return 0;
+}
+
 int do_server(struct memory_buffer *mem)
 {
 	char ctrl_data[sizeof(int) * 20000];
 	struct netdev_queue_id *queues;
 	size_t non_page_aligned_frags = 0;
-	struct sockaddr_in client_addr;
-	struct sockaddr_in server_sin;
+	struct sockaddr_in6 client_addr;
+	struct sockaddr_in6 server_sin;
 	size_t page_aligned_frags = 0;
 	size_t total_received = 0;
 	socklen_t client_addr_len;
@@ -275,9 +313,12 @@ int do_server(struct memory_buffer *mem)
 	int socket_fd;
 	int client_fd;
 	size_t i = 0;
-	int opt = 1;
 	int ret;
 
+	ret = parse_address(server_ip, atoi(port), &server_sin);
+	if (ret < 0)
+		error(1, 0, "parse server address");
+
 	if (reset_flow_steering())
 		error(1, 0, "Failed to reset flow steering\n");
 
@@ -286,7 +327,7 @@ int do_server(struct memory_buffer *mem)
 		error(1, 0, "Failed to configure rss\n");
 
 	/* Flow steer our devmem flows to start_queue */
-	if (configure_flow_steering())
+	if (configure_flow_steering(&server_sin))
 		error(1, 0, "Failed to configure flow steering\n");
 
 	sleep(1);
@@ -307,29 +348,16 @@ int do_server(struct memory_buffer *mem)
 	if (!tmp_mem)
 		error(1, ENOMEM, "malloc failed");
 
-	server_sin.sin_family = AF_INET;
-	server_sin.sin_port = htons(atoi(port));
-
-	ret = inet_pton(server_sin.sin_family, server_ip, &server_sin.sin_addr);
-	if (socket < 0)
-		error(1, 0, "%s: [FAIL, create socket]\n", TEST_PREFIX);
-
-	socket_fd = socket(server_sin.sin_family, SOCK_STREAM, 0);
+	socket_fd = socket(AF_INET6, SOCK_STREAM, 0);
 	if (socket < 0)
 		error(1, errno, "%s: [FAIL, create socket]\n", TEST_PREFIX);
 
-	ret = setsockopt(socket_fd, SOL_SOCKET, SO_REUSEPORT, &opt,
-			 sizeof(opt));
-	if (ret)
-		error(1, errno, "%s: [FAIL, set sock opt]\n", TEST_PREFIX);
-
-	ret = setsockopt(socket_fd, SOL_SOCKET, SO_REUSEADDR, &opt,
-			 sizeof(opt));
+	ret = enable_reuseaddr(socket_fd);
 	if (ret)
-		error(1, errno, "%s: [FAIL, set sock opt]\n", TEST_PREFIX);
+		error(1, errno, "%s: [FAIL, reuseaddr]\n", TEST_PREFIX);
 
 	fprintf(stderr, "binding to address %s:%d\n", server_ip,
-		ntohs(server_sin.sin_port));
+		ntohs(server_sin.sin6_port));
 
 	ret = bind(socket_fd, &server_sin, sizeof(server_sin));
 	if (ret)
@@ -341,16 +369,16 @@ int do_server(struct memory_buffer *mem)
 
 	client_addr_len = sizeof(client_addr);
 
-	inet_ntop(server_sin.sin_family, &server_sin.sin_addr, buffer,
+	inet_ntop(AF_INET6, &server_sin.sin6_addr, buffer,
 		  sizeof(buffer));
 	fprintf(stderr, "Waiting or connection on %s:%d\n", buffer,
-		ntohs(server_sin.sin_port));
+		ntohs(server_sin.sin6_port));
 	client_fd = accept(socket_fd, &client_addr, &client_addr_len);
 
-	inet_ntop(client_addr.sin_family, &client_addr.sin_addr, buffer,
+	inet_ntop(AF_INET6, &client_addr.sin6_addr, buffer,
 		  sizeof(buffer));
 	fprintf(stderr, "Got connection from %s:%d\n", buffer,
-		ntohs(client_addr.sin_port));
+		ntohs(client_addr.sin6_port));
 
 	while (1) {
 		struct iovec iov = { .iov_base = iobuf,
-- 
2.46.0

[PATCH net-next 09/13] selftests: ncdevmem: Properly reset flow steering

[Stanislav Fomichev]

ntuple off/on might be not enough to do it on all NICs.
Add a bunch of shell crap to explicitly remove the rules.

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/ncdevmem.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
index d82e550369c0..c5b4d9069a83 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -181,13 +181,12 @@ static void print_nonzero_bytes(void *ptr, size_t size)
 
 static int reset_flow_steering(void)
 {
-	int ret = 0;
-
-	ret = run_command("sudo ethtool -K %s ntuple off >&2", ifname);
-	if (ret)
-		return ret;
-
-	return run_command("sudo ethtool -K %s ntuple on >&2", ifname);
+	run_command("sudo ethtool -K %s ntuple off >&2", ifname);
+	run_command("sudo ethtool -K %s ntuple on >&2", ifname);
+	run_command(
+		"sudo ethtool -n %s | grep 'Filter:' | awk '{print $2}' | xargs -n1 ethtool -N %s delete >&2",
+		ifname, ifname);
+	return 0;
 }
 
 static int configure_headersplit(bool on)
-- 
2.46.0

[PATCH net-next 10/13] selftests: ncdevmem: Use YNL to enable TCP header split

[Stanislav Fomichev]

In the next patch the hard-coded queue numbers are gonna be removed.
So introduce some initial support for ethtool YNL and use
it to enable header split.

Also, tcp-data-split requires latest ethtool which is unlikely
to be present in the distros right now.

(ideally, we should not shell out to ethtool at all).

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/Makefile   |  2 +-
 tools/testing/selftests/net/ncdevmem.c | 42 ++++++++++++++++++++++++--
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 649f1fe0dc46..9c970e96ed33 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -112,7 +112,7 @@ TEST_INCLUDES := forwarding/lib.sh
 include ../lib.mk
 
 # YNL build
-YNL_GENS := netdev
+YNL_GENS := ethtool netdev
 include ynl.mk
 
 $(OUTPUT)/epoll_busy_poll: LDLIBS += -lcap
diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
index c5b4d9069a83..f5cfaafb6509 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -32,6 +32,7 @@
 #include <net/if.h>
 
 #include "netdev-user.h"
+#include "ethtool-user.h"
 #include <ynl.h>
 
 #define PAGE_SHIFT 12
@@ -191,8 +192,42 @@ static int reset_flow_steering(void)
 
 static int configure_headersplit(bool on)
 {
-	return run_command("sudo ethtool -G %s tcp-data-split %s >&2", ifname,
-			   on ? "on" : "off");
+	struct ethtool_rings_set_req *req;
+	struct ynl_error yerr;
+	struct ynl_sock *ys;
+	int ret;
+
+	ys = ynl_sock_create(&ynl_ethtool_family, &yerr);
+	if (!ys) {
+		fprintf(stderr, "YNL: %s\n", yerr.msg);
+		return -1;
+	}
+
+	req = ethtool_rings_set_req_alloc();
+	ethtool_rings_set_req_set_header_dev_index(req, ifindex);
+	ethtool_rings_set_req_set_tcp_data_split(req, on ? 2 : 0);
+	ret = ethtool_rings_set(ys, req);
+	if (ret < 0)
+		fprintf(stderr, "YNL failed: %s\n", ys->err.msg);
+	ethtool_rings_set_req_free(req);
+
+	{
+		struct ethtool_rings_get_req *req;
+		struct ethtool_rings_get_rsp *rsp;
+
+		req = ethtool_rings_get_req_alloc();
+		ethtool_rings_get_req_set_header_dev_index(req, ifindex);
+		rsp = ethtool_rings_get(ys, req);
+		ethtool_rings_get_req_free(req);
+		if (rsp)
+			fprintf(stderr, "TCP header split: %d\n",
+				rsp->tcp_data_split);
+		ethtool_rings_get_rsp_free(rsp);
+	}
+
+	ynl_sock_destroy(ys);
+
+	return ret;
 }
 
 static int configure_rss(void)
@@ -321,6 +356,9 @@ int do_server(struct memory_buffer *mem)
 	if (reset_flow_steering())
 		error(1, 0, "Failed to reset flow steering\n");
 
+	if (configure_headersplit(1))
+		error(1, 0, "Failed to enable TCP header split\n");
+
 	/* Configure RSS to divert all traffic from our devmem queues */
 	if (configure_rss())
 		error(1, 0, "Failed to configure rss\n");
-- 
2.46.0

Re: [PATCH net-next 10/13] selftests: ncdevmem: Use YNL to enable TCP header split

[kernel test robot]

Hi Stanislav,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Stanislav-Fomichev/selftests-ncdevmem-Add-a-flag-for-the-selftest/20240913-011631
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240912171251.937743-11-sdf%40fomichev.me
patch subject: [PATCH net-next 10/13] selftests: ncdevmem: Use YNL to enable TCP header split
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409142047.UOZ425m7-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/r/202409142047.UOZ425m7-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from ncdevmem.c:35:
>> tools/testing/selftests/../../../tools/net/ynl/generated/ethtool-user.h:23:43: warning: declaration of 'enum ethtool_header_flags' will not be visible outside of this function [-Wvisibility]
      23 | const char *ethtool_header_flags_str(enum ethtool_header_flags value);
         |                                           ^
   1 warning generated.

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH net-next 11/13] selftests: ncdevmem: Remove hard-coded queue numbers

[Stanislav Fomichev]

Use single last queue of the device and probe it dynamically.

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/net/ncdevmem.c | 40 ++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
index f5cfaafb6509..3883a67d387f 100644
--- a/tools/testing/selftests/net/ncdevmem.c
+++ b/tools/testing/selftests/net/ncdevmem.c
@@ -45,8 +45,8 @@
 
 static char *server_ip;
 static char *port;
-static int start_queue = 8;
-static int num_queues = 8;
+static int start_queue = -1;
+static int num_queues = 1;
 static char *ifname;
 static unsigned int ifindex;
 static unsigned int dmabuf_id;
@@ -171,6 +171,33 @@ static void print_nonzero_bytes(void *ptr, size_t size)
 		putchar(p[i]);
 }
 
+static int rxq_num(int ifindex)
+{
+	struct ethtool_channels_get_req *req;
+	struct ethtool_channels_get_rsp *rsp;
+	struct ynl_error yerr;
+	struct ynl_sock *ys;
+	int num = -1;
+
+	ys = ynl_sock_create(&ynl_ethtool_family, &yerr);
+	if (!ys) {
+		fprintf(stderr, "YNL: %s\n", yerr.msg);
+		return -1;
+	}
+
+	req = ethtool_channels_get_req_alloc();
+	ethtool_channels_get_req_set_header_dev_index(req, ifindex);
+	rsp = ethtool_channels_get(ys, req);
+	if (rsp)
+		num = rsp->rx_count + rsp->combined_count;
+	ethtool_channels_get_req_free(req);
+	ethtool_channels_get_rsp_free(rsp);
+
+	ynl_sock_destroy(ys);
+
+	return num;
+}
+
 #define run_command(cmd, ...)                                           \
 	({                                                              \
 		char command[256];                                      \
@@ -630,6 +657,15 @@ int main(int argc, char *argv[])
 
 	ifindex = if_nametoindex(ifname);
 
+	if (start_queue < 0) {
+		start_queue = rxq_num(ifindex) - 1;
+
+		if (start_queue < 0)
+			error(1, 0, "couldn't detect number of queues\n");
+
+		fprintf(stderr, "using queues %d..%d\n", start_queue, start_queue + num_queues);
+	}
+
 	for (; optind < argc; optind++)
 		fprintf(stderr, "extra arguments: %s\n", argv[optind]);
 
-- 
2.46.0

Re: [PATCH net-next 11/13] selftests: ncdevmem: Remove hard-coded queue numbers

[Mina Almasry]

On Thu, Sep 12, 2024 at 10:13 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> Use single last queue of the device and probe it dynamically.
>

Can we use the last N queues, instead of 1? Or the last half of the queues?

Test coverage that we can bind multiple queues at once is important, I think.

--
Thanks,
Mina

Re: [PATCH net-next 11/13] selftests: ncdevmem: Remove hard-coded queue numbers

[Stanislav Fomichev]

On 09/12, Mina Almasry wrote:
> On Thu, Sep 12, 2024 at 10:13 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > Use single last queue of the device and probe it dynamically.
> >
> 
> Can we use the last N queues, instead of 1? Or the last half of the queues?
> 
> Test coverage that we can bind multiple queues at once is important, I think.

Anything against doing this in the selftest/probe part?

if (probe) {
	if (start_queue > 1) {
		/* make sure can bind to multiple queues */
		start_queue -= 1;
		num_queues +=1;
	}
	run_devmem_tests();
	return 0;
}

Re: [PATCH net-next 11/13] selftests: ncdevmem: Remove hard-coded queue numbers

[Mina Almasry]

On Thu, Sep 12, 2024 at 2:47 PM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 09/12, Mina Almasry wrote:
> > On Thu, Sep 12, 2024 at 10:13 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > >
> > > Use single last queue of the device and probe it dynamically.
> > >
> >
> > Can we use the last N queues, instead of 1? Or the last half of the queues?
> >
> > Test coverage that we can bind multiple queues at once is important, I think.
>
> Anything against doing this in the selftest/probe part?
>
> if (probe) {
>         if (start_queue > 1) {
>                 /* make sure can bind to multiple queues */
>                 start_queue -= 1;
>                 num_queues +=1;

Sorry for the late reply, this particular thread slipped my inbox.

Overriding user-provided configs here doesn't seem great. It's nice to
be able to launch ncdevmem requesting 1 queue to be bound or multiple,
and I had the idea that in the future the tests can be improved to
verify that multiple concurrent connections on multiple queues can be
handled correctly, in case we run into any bugs that can only be
reproduced in this setup.

-- 
Thanks,
Mina

Re: [PATCH net-next 11/13] selftests: ncdevmem: Remove hard-coded queue numbers

[Stanislav Fomichev]

On 09/26, Mina Almasry wrote:
> On Thu, Sep 12, 2024 at 2:47 PM Stanislav Fomichev <stfomichev@gmail.com> wrote:
> >
> > On 09/12, Mina Almasry wrote:
> > > On Thu, Sep 12, 2024 at 10:13 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > > >
> > > > Use single last queue of the device and probe it dynamically.
> > > >
> > >
> > > Can we use the last N queues, instead of 1? Or the last half of the queues?
> > >
> > > Test coverage that we can bind multiple queues at once is important, I think.
> >
> > Anything against doing this in the selftest/probe part?
> >
> > if (probe) {
> >         if (start_queue > 1) {
> >                 /* make sure can bind to multiple queues */
> >                 start_queue -= 1;
> >                 num_queues +=1;
> 
> Sorry for the late reply, this particular thread slipped my inbox.

So what's better? Hard-coding start_queue and num_queues to 8?
This is only for the purpose of self testing, not sure we really care.
 
> Overriding user-provided configs here doesn't seem great. It's nice to
> be able to launch ncdevmem requesting 1 queue to be bound or multiple,
> and I had the idea that in the future the tests can be improved to
> verify that multiple concurrent connections on multiple queues can be
> handled correctly, in case we run into any bugs that can only be
> reproduced in this setup.

Currently, having multiple queues doesn't make any sense because there
is only a single receiver. I have some patches to have a thread per
receiver, can post them if you're interested (after we sort out this
series).

[PATCH net-next 12/13] selftests: ncdevmem: Move ncdevmem under drivers/net

[Stanislav Fomichev]

This is where all the tests that depend on the HW functionality live in
and this is where the automated test is gonna be added in the next
patch.

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/drivers/net/.gitignore       | 1 +
 tools/testing/selftests/drivers/net/Makefile         | 9 +++++++++
 tools/testing/selftests/{ => drivers}/net/ncdevmem.c | 0
 tools/testing/selftests/net/.gitignore               | 1 -
 tools/testing/selftests/net/Makefile                 | 9 ---------
 5 files changed, 10 insertions(+), 10 deletions(-)
 create mode 100644 tools/testing/selftests/drivers/net/.gitignore
 rename tools/testing/selftests/{ => drivers}/net/ncdevmem.c (100%)

diff --git a/tools/testing/selftests/drivers/net/.gitignore b/tools/testing/selftests/drivers/net/.gitignore
new file mode 100644
index 000000000000..e9fe6ede681a
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/.gitignore
@@ -0,0 +1 @@
+ncdevmem
diff --git a/tools/testing/selftests/drivers/net/Makefile b/tools/testing/selftests/drivers/net/Makefile
index 39fb97a8c1df..bb8f7374942e 100644
--- a/tools/testing/selftests/drivers/net/Makefile
+++ b/tools/testing/selftests/drivers/net/Makefile
@@ -11,4 +11,13 @@ TEST_PROGS := \
 	stats.py \
 # end of TEST_PROGS
 
+# YNL files, must be before "include ..lib.mk"
+EXTRA_CLEAN += $(OUTPUT)/libynl.a $(OUTPUT)/ncdevmem
+YNL_GEN_FILES := ncdevmem
+TEST_GEN_FILES += $(YNL_GEN_FILES)
+
 include ../../lib.mk
+
+# YNL build
+YNL_GENS := ethtool netdev
+include ../../net/ynl.mk
diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/drivers/net/ncdevmem.c
similarity index 100%
rename from tools/testing/selftests/net/ncdevmem.c
rename to tools/testing/selftests/drivers/net/ncdevmem.c
diff --git a/tools/testing/selftests/net/.gitignore b/tools/testing/selftests/net/.gitignore
index 1c04c780db66..923bf098e2eb 100644
--- a/tools/testing/selftests/net/.gitignore
+++ b/tools/testing/selftests/net/.gitignore
@@ -17,7 +17,6 @@ ipv6_flowlabel
 ipv6_flowlabel_mgr
 log.txt
 msg_zerocopy
-ncdevmem
 nettest
 psock_fanout
 psock_snd
diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 9c970e96ed33..22a5d6a7c3f3 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -97,11 +97,6 @@ TEST_PROGS += fq_band_pktlimit.sh
 TEST_PROGS += vlan_hw_filter.sh
 TEST_PROGS += bpf_offload.py
 
-# YNL files, must be before "include ..lib.mk"
-EXTRA_CLEAN += $(OUTPUT)/libynl.a
-YNL_GEN_FILES := ncdevmem
-TEST_GEN_FILES += $(YNL_GEN_FILES)
-
 TEST_FILES := settings
 TEST_FILES += in_netns.sh lib.sh net_helper.sh setup_loopback.sh setup_veth.sh
 
@@ -111,10 +106,6 @@ TEST_INCLUDES := forwarding/lib.sh
 
 include ../lib.mk
 
-# YNL build
-YNL_GENS := ethtool netdev
-include ynl.mk
-
 $(OUTPUT)/epoll_busy_poll: LDLIBS += -lcap
 $(OUTPUT)/reuseport_bpf_numa: LDLIBS += -lnuma
 $(OUTPUT)/tcp_mmap: LDLIBS += -lpthread -lcrypto
-- 
2.46.0

Re: [PATCH net-next 12/13] selftests: ncdevmem: Move ncdevmem under drivers/net

[Mina Almasry]

On Thu, Sep 12, 2024 at 10:13 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> This is where all the tests that depend on the HW functionality live in

Is this true? My impression is that selftests/net verifies core
functionality and drivers/net verifies more lower level driver
(specific?) functionality. There are tests in selftests/net that
depend on HW functionality, I think like bpf_offload.py.

devmem tcp in my mind is primarily a core functionality and a lot of
effort was put into making the driver bits of it as minimal as
possible. Is there a need to move it or is this preference?

-- 
Thanks,
Mina

Re: [PATCH net-next 12/13] selftests: ncdevmem: Move ncdevmem under drivers/net

[Stanislav Fomichev]

On 09/13, Mina Almasry wrote:
> On Thu, Sep 12, 2024 at 10:13 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > This is where all the tests that depend on the HW functionality live in
> 
> Is this true? My impression is that selftests/net verifies core
> functionality and drivers/net verifies more lower level driver
> (specific?) functionality. There are tests in selftests/net that
> depend on HW functionality, I think like bpf_offload.py.
> 
> devmem tcp in my mind is primarily a core functionality and a lot of
> effort was put into making the driver bits of it as minimal as
> possible. Is there a need to move it or is this preference?

I'm wrong and it is actually drivers/net/hw, not drivers/net. But yes,
the driver/net/hw is the directory for the tests that depend on some
HW functionality (NIPA doesn't run the tests in this directory; but
the vendors eventually will). And until/if we get to loopback mode [1],
there is no way to run ncdevmem without special HW :-(

1: https://lore.kernel.org/netdev/20240913150913.1280238-5-sdf@fomichev.me/T/#m4fc52463857c19ad432db656c433cffdf74477a4

[PATCH net-next 13/13] selftests: ncdevmem: Add automated test

[Stanislav Fomichev]

Only RX side for now and small message to test the setup.
In the future, we can extend it to TX side and to testing
both sides with a couple of megs of data.

  make \
  	-C tools/testing/selftests \
  	TARGETS="drivers/net" \
  	install INSTALL_PATH=~/tmp/ksft

  scp ~/tmp/ksft ${HOST}:
  scp ~/tmp/ksft ${PEER}:

  cfg+="NETIF=${DEV}\n"
  cfg+="LOCAL_V6=${HOST_IP}\n"
  cfg+="REMOTE_V6=${PEER_IP}\n"
  cfg+="REMOTE_TYPE=ssh\n"
  cfg+="REMOTE_ARGS=root@${PEER}\n"

  echo -e "$cfg" | ssh root@${HOST} "cat > ksft/drivers/net/net.config"
  ssh root@${HOST} "cd ksft && ./run_kselftest.sh -t drivers/net:devmem.py"

Cc: Mina Almasry <almasrymina@google.com>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
 tools/testing/selftests/drivers/net/Makefile  |  1 +
 tools/testing/selftests/drivers/net/devmem.py | 46 +++++++++++++++++++
 2 files changed, 47 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/net/devmem.py

diff --git a/tools/testing/selftests/drivers/net/Makefile b/tools/testing/selftests/drivers/net/Makefile
index bb8f7374942e..00da59970a76 100644
--- a/tools/testing/selftests/drivers/net/Makefile
+++ b/tools/testing/selftests/drivers/net/Makefile
@@ -5,6 +5,7 @@ TEST_INCLUDES := $(wildcard lib/py/*.py) \
 		 ../../net/lib.sh \
 
 TEST_PROGS := \
+	devmem.py \
 	netcons_basic.sh \
 	ping.py \
 	queues.py \
diff --git a/tools/testing/selftests/drivers/net/devmem.py b/tools/testing/selftests/drivers/net/devmem.py
new file mode 100755
index 000000000000..bbd32e0b0fe2
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/devmem.py
@@ -0,0 +1,46 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: GPL-2.0
+
+import errno
+from lib.py import ksft_run, ksft_exit
+from lib.py import ksft_eq, KsftSkipEx
+from lib.py import NetDrvEpEnv
+from lib.py import bkg, cmd, rand_port, wait_port_listen
+from lib.py import ksft_disruptive
+
+
+def require_devmem(cfg):
+    if not hasattr(cfg, "_devmem_probed"):
+        port = rand_port()
+        probe_command = f"./ncdevmem -P -f {cfg.ifname} -s {cfg.v6} -p {port}"
+        cfg._devmem_supported = cmd(probe_command, fail=False, shell=True).ret == 0
+        cfg._devmem_probed = True
+
+    if not cfg._devmem_supported:
+        raise KsftSkipEx("Test requires devmem support")
+
+
+@ksft_disruptive
+def check_rx(cfg) -> None:
+    cfg.require_v6()
+    require_devmem(cfg)
+
+    port = rand_port()
+    listen_cmd = f"./ncdevmem -l -f {cfg.ifname} -s {cfg.v6} -p {port}"
+
+    with bkg(listen_cmd) as nc:
+        wait_port_listen(port)
+        cmd(f"echo -e \"hello\\nworld\"| nc {cfg.v6} {port}", host=cfg.remote, shell=True)
+
+    ksft_eq(nc.stdout.strip(), "hello\nworld")
+
+
+def main() -> None:
+    with NetDrvEpEnv(__file__) as cfg:
+        ksft_run([check_rx],
+                 args=(cfg, ))
+    ksft_exit()
+
+
+if __name__ == "__main__":
+    main()
-- 
2.46.0

Re: [PATCH net-next 13/13] selftests: ncdevmem: Add automated test

[Mina Almasry]

On Thu, Sep 12, 2024 at 10:13 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> Only RX side for now and small message to test the setup.
> In the future, we can extend it to TX side and to testing
> both sides with a couple of megs of data.
>
>   make \
>         -C tools/testing/selftests \
>         TARGETS="drivers/net" \
>         install INSTALL_PATH=~/tmp/ksft
>
>   scp ~/tmp/ksft ${HOST}:
>   scp ~/tmp/ksft ${PEER}:
>
>   cfg+="NETIF=${DEV}\n"
>   cfg+="LOCAL_V6=${HOST_IP}\n"
>   cfg+="REMOTE_V6=${PEER_IP}\n"
>   cfg+="REMOTE_TYPE=ssh\n"
>   cfg+="REMOTE_ARGS=root@${PEER}\n"
>
>   echo -e "$cfg" | ssh root@${HOST} "cat > ksft/drivers/net/net.config"
>   ssh root@${HOST} "cd ksft && ./run_kselftest.sh -t drivers/net:devmem.py"
>
> Cc: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>

Thank you _very_ much. I had an action item to figure this out, and
awesome to see you beat me to it!

I'll take a deeper look and test and provide reviewed-by.


> ---
>  tools/testing/selftests/drivers/net/Makefile  |  1 +
>  tools/testing/selftests/drivers/net/devmem.py | 46 +++++++++++++++++++
>  2 files changed, 47 insertions(+)
>  create mode 100755 tools/testing/selftests/drivers/net/devmem.py
>
> diff --git a/tools/testing/selftests/drivers/net/Makefile b/tools/testing/selftests/drivers/net/Makefile
> index bb8f7374942e..00da59970a76 100644
> --- a/tools/testing/selftests/drivers/net/Makefile
> +++ b/tools/testing/selftests/drivers/net/Makefile
> @@ -5,6 +5,7 @@ TEST_INCLUDES := $(wildcard lib/py/*.py) \
>                  ../../net/lib.sh \
>
>  TEST_PROGS := \
> +       devmem.py \
>         netcons_basic.sh \
>         ping.py \
>         queues.py \
> diff --git a/tools/testing/selftests/drivers/net/devmem.py b/tools/testing/selftests/drivers/net/devmem.py
> new file mode 100755
> index 000000000000..bbd32e0b0fe2
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/devmem.py
> @@ -0,0 +1,46 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0
> +
> +import errno
> +from lib.py import ksft_run, ksft_exit
> +from lib.py import ksft_eq, KsftSkipEx
> +from lib.py import NetDrvEpEnv
> +from lib.py import bkg, cmd, rand_port, wait_port_listen
> +from lib.py import ksft_disruptive
> +
> +
> +def require_devmem(cfg):
> +    if not hasattr(cfg, "_devmem_probed"):
> +        port = rand_port()
> +        probe_command = f"./ncdevmem -P -f {cfg.ifname} -s {cfg.v6} -p {port}"
> +        cfg._devmem_supported = cmd(probe_command, fail=False, shell=True).ret == 0
> +        cfg._devmem_probed = True
> +
> +    if not cfg._devmem_supported:
> +        raise KsftSkipEx("Test requires devmem support")
> +
> +
> +@ksft_disruptive
> +def check_rx(cfg) -> None:
> +    cfg.require_v6()
> +    require_devmem(cfg)
> +
> +    port = rand_port()
> +    listen_cmd = f"./ncdevmem -l -f {cfg.ifname} -s {cfg.v6} -p {port}"
> +
> +    with bkg(listen_cmd) as nc:
> +        wait_port_listen(port)
> +        cmd(f"echo -e \"hello\\nworld\"| nc {cfg.v6} {port}", host=cfg.remote, shell=True)
> +
> +    ksft_eq(nc.stdout.strip(), "hello\nworld")
> +
> +
> +def main() -> None:
> +    with NetDrvEpEnv(__file__) as cfg:
> +        ksft_run([check_rx],
> +                 args=(cfg, ))
> +    ksft_exit()
> +
> +
> +if __name__ == "__main__":
> +    main()
> --
> 2.46.0
>


--
Thanks,
Mina

Re: [PATCH net-next 00/13] selftests: ncdevmem: Add ncdevmem to ksft

[Stanislav Fomichev]

On 09/12, Stanislav Fomichev wrote:
> The goal of the series is to simplify and make it possible to use
> ncdevmem in an automated way from the ksft python wrapper.
> 
> ncdevmem is slowly mutated into a state where it uses stdout
> to print the payload and the python wrapper is added to
> make sure the arrived payload matches the expected one.

Mina, what's your plan/progress on the upstreamable TX side? I hope
you're still gonna finish it up?

I have a PoC based on your old RFC over here (plus all the selftests to
verify it). I also have a 'loopback' mode to test/verify UAPI parts on qemu
without real HW.

Should I post it as an RFC once the merge window closes? Or maybe send
it off list? I don't intent to push those patches further. If you already
have the implementation on your side, maybe at least the selftests will be
helpful to reuse? 

Re: [PATCH net-next 00/13] selftests: ncdevmem: Add ncdevmem to ksft

[Mina Almasry]

On Thu, Sep 12, 2024 at 12:48 PM Stanislav Fomichev
<stfomichev@gmail.com> wrote:
>
> On 09/12, Stanislav Fomichev wrote:
> > The goal of the series is to simplify and make it possible to use
> > ncdevmem in an automated way from the ksft python wrapper.
> >
> > ncdevmem is slowly mutated into a state where it uses stdout
> > to print the payload and the python wrapper is added to
> > make sure the arrived payload matches the expected one.
>
> Mina, what's your plan/progress on the upstreamable TX side? I hope
> you're still gonna finish it up?
>

I'm very open to someone pushing the TX side, but there is a bit of a
need here to get the TX side done sooner than later. In reality I
don't think anyone cares as much as me to push this ASAP so I
plan/hope to look into it. I have made some progress but a bit to be
worked through at the moment. I hope to have something ready as the
merge window reopens; very likely doable.

> I have a PoC based on your old RFC over here (plus all the selftests to
> verify it). I also have a 'loopback' mode to test/verify UAPI parts on qemu
> without real HW.
>

This sounds excellent. Where is here? Is there a link missing?

I'm happy to push those patches forward, giving you full credit of
course (either signed-off-by both of us or 'Based on work by
stfomichev@gmail.com' etc).

> Should I post it as an RFC once the merge window closes? Or maybe send
> it off list? I don't intent to push those patches further. If you already
> have the implementation on your side, maybe at least the selftests will be
> helpful to reuse?

Everything would be useful to reuse. Thank you very much, this is amazing.

BTW, I was planning on looking into both TX and improving tests and
you have looked into both, which is amazing, thank you. Would you
Pavel/David/Taehee others would be interested in, say, a monthly
face-to-face call to discuss future work? I am under the impression
Taehee will push devmem support for bnxt and Pavel for io uring ZC, so
we may have a bunch of details to discuss.

Any sync would be hosted on bbb, public to anyone to join.

No pressure, of course. Communicating on-list via email has worked so
far, so in a sense there is no reason to change things :D

-- 
Thanks,
Mina

Re: [PATCH net-next 00/13] selftests: ncdevmem: Add ncdevmem to ksft

[Stanislav Fomichev]

On 09/12, Mina Almasry wrote:
> On Thu, Sep 12, 2024 at 12:48 PM Stanislav Fomichev
> <stfomichev@gmail.com> wrote:
> >
> > On 09/12, Stanislav Fomichev wrote:
> > > The goal of the series is to simplify and make it possible to use
> > > ncdevmem in an automated way from the ksft python wrapper.
> > >
> > > ncdevmem is slowly mutated into a state where it uses stdout
> > > to print the payload and the python wrapper is added to
> > > make sure the arrived payload matches the expected one.
> >
> > Mina, what's your plan/progress on the upstreamable TX side? I hope
> > you're still gonna finish it up?
> >
> 
> I'm very open to someone pushing the TX side, but there is a bit of a
> need here to get the TX side done sooner than later. In reality I
> don't think anyone cares as much as me to push this ASAP so I
> plan/hope to look into it. I have made some progress but a bit to be
> worked through at the moment. I hope to have something ready as the
> merge window reopens; very likely doable.

Perfect!

> > I have a PoC based on your old RFC over here (plus all the selftests to
> > verify it). I also have a 'loopback' mode to test/verify UAPI parts on qemu
> > without real HW.
> >
> 
> This sounds excellent. Where is here? Is there a link missing?
> 
> I'm happy to push those patches forward, giving you full credit of
> course (either signed-off-by both of us or 'Based on work by
> stfomichev@gmail.com' etc).

Great, let me clean them up a bit and I'll post an RFC to the list for
you to take over sometime tomorrow (or ignore or whatever).
Didn't want to send out anything without asking you first..
And this will be based on top of this series.

> > Should I post it as an RFC once the merge window closes? Or maybe send
> > it off list? I don't intent to push those patches further. If you already
> > have the implementation on your side, maybe at least the selftests will be
> > helpful to reuse?
> 
> Everything would be useful to reuse. Thank you very much, this is amazing.
> 
> BTW, I was planning on looking into both TX and improving tests and
> you have looked into both, which is amazing, thank you. Would you
> Pavel/David/Taehee others would be interested in, say, a monthly
> face-to-face call to discuss future work? I am under the impression
> Taehee will push devmem support for bnxt and Pavel for io uring ZC, so
> we may have a bunch of details to discuss.
> 
> Any sync would be hosted on bbb, public to anyone to join.
> 
> No pressure, of course. Communicating on-list via email has worked so
> far, so in a sense there is no reason to change things :D

If you think it's helpful to coordinate - sure! Doing something once
a month is not a huge commitment :-D Set the dates and send out a link.

Re: [PATCH net-next 00/13] selftests: ncdevmem: Add ncdevmem to ksft

[Stanislav Fomichev]

On 09/12, Stanislav Fomichev wrote:
> On 09/12, Mina Almasry wrote:
> > On Thu, Sep 12, 2024 at 12:48 PM Stanislav Fomichev
> > <stfomichev@gmail.com> wrote:
> > >
> > > On 09/12, Stanislav Fomichev wrote:
> > > > The goal of the series is to simplify and make it possible to use
> > > > ncdevmem in an automated way from the ksft python wrapper.
> > > >
> > > > ncdevmem is slowly mutated into a state where it uses stdout
> > > > to print the payload and the python wrapper is added to
> > > > make sure the arrived payload matches the expected one.
> > >
> > > Mina, what's your plan/progress on the upstreamable TX side? I hope
> > > you're still gonna finish it up?
> > >
> > 
> > I'm very open to someone pushing the TX side, but there is a bit of a
> > need here to get the TX side done sooner than later. In reality I
> > don't think anyone cares as much as me to push this ASAP so I
> > plan/hope to look into it. I have made some progress but a bit to be
> > worked through at the moment. I hope to have something ready as the
> > merge window reopens; very likely doable.
> 
> Perfect!

Hey Mina,

Any updates on this? Any chance getting something out this merge window?

I was hoping you'd post something in the previous merge window (late Sep),
but if you're still busy with other things, should I post a v2 RFC? I have
moved to the mode which you suggested where tx dmabuf is associated
with the socket; this lets me drop all tx ref counts (socket holds
dmabuf, skb holds socket until tx completion). I also moved to a
more performant offset->dma_addr resolution logic in tcp_sendmsg and
fixed a bunch of things on the test side...

Re: [PATCH net-next 00/13] selftests: ncdevmem: Add ncdevmem to ksft

[Mina Almasry]

On Thu, Dec 12, 2024 at 5:19 PM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 09/12, Stanislav Fomichev wrote:
> > On 09/12, Mina Almasry wrote:
> > > On Thu, Sep 12, 2024 at 12:48 PM Stanislav Fomichev
> > > <stfomichev@gmail.com> wrote:
> > > >
> > > > On 09/12, Stanislav Fomichev wrote:
> > > > > The goal of the series is to simplify and make it possible to use
> > > > > ncdevmem in an automated way from the ksft python wrapper.
> > > > >
> > > > > ncdevmem is slowly mutated into a state where it uses stdout
> > > > > to print the payload and the python wrapper is added to
> > > > > make sure the arrived payload matches the expected one.
> > > >
> > > > Mina, what's your plan/progress on the upstreamable TX side? I hope
> > > > you're still gonna finish it up?
> > > >
> > >
> > > I'm very open to someone pushing the TX side, but there is a bit of a
> > > need here to get the TX side done sooner than later. In reality I
> > > don't think anyone cares as much as me to push this ASAP so I
> > > plan/hope to look into it. I have made some progress but a bit to be
> > > worked through at the moment. I hope to have something ready as the
> > > merge window reopens; very likely doable.
> >
> > Perfect!
>
> Hey Mina,
>
> Any updates on this? Any chance getting something out this merge window?
>
> I was hoping you'd post something in the previous merge window (late Sep),
> but if you're still busy with other things, should I post a v2 RFC? I have
> moved to the mode which you suggested where tx dmabuf is associated
> with the socket; this lets me drop all tx ref counts (socket holds
> dmabuf, skb holds socket until tx completion). I also moved to a
> more performant offset->dma_addr resolution logic in tcp_sendmsg and
> fixed a bunch of things on the test side...

My apologies for the delay. I have the TX path ready, but having
trouble running the performance tests unrelated to the patch itself.
I'm going to send the TX path after a round of internal reviews,
likely by the end of next week.


-- 
Thanks,
Mina

Re: [PATCH net-next 00/13] selftests: ncdevmem: Add ncdevmem to ksft

[Stanislav Fomichev]

On 12/13, Mina Almasry wrote:
> On Thu, Dec 12, 2024 at 5:19 PM Stanislav Fomichev <stfomichev@gmail.com> wrote:
> >
> > On 09/12, Stanislav Fomichev wrote:
> > > On 09/12, Mina Almasry wrote:
> > > > On Thu, Sep 12, 2024 at 12:48 PM Stanislav Fomichev
> > > > <stfomichev@gmail.com> wrote:
> > > > >
> > > > > On 09/12, Stanislav Fomichev wrote:
> > > > > > The goal of the series is to simplify and make it possible to use
> > > > > > ncdevmem in an automated way from the ksft python wrapper.
> > > > > >
> > > > > > ncdevmem is slowly mutated into a state where it uses stdout
> > > > > > to print the payload and the python wrapper is added to
> > > > > > make sure the arrived payload matches the expected one.
> > > > >
> > > > > Mina, what's your plan/progress on the upstreamable TX side? I hope
> > > > > you're still gonna finish it up?
> > > > >
> > > >
> > > > I'm very open to someone pushing the TX side, but there is a bit of a
> > > > need here to get the TX side done sooner than later. In reality I
> > > > don't think anyone cares as much as me to push this ASAP so I
> > > > plan/hope to look into it. I have made some progress but a bit to be
> > > > worked through at the moment. I hope to have something ready as the
> > > > merge window reopens; very likely doable.
> > >
> > > Perfect!
> >
> > Hey Mina,
> >
> > Any updates on this? Any chance getting something out this merge window?
> >
> > I was hoping you'd post something in the previous merge window (late Sep),
> > but if you're still busy with other things, should I post a v2 RFC? I have
> > moved to the mode which you suggested where tx dmabuf is associated
> > with the socket; this lets me drop all tx ref counts (socket holds
> > dmabuf, skb holds socket until tx completion). I also moved to a
> > more performant offset->dma_addr resolution logic in tcp_sendmsg and
> > fixed a bunch of things on the test side...
> 
> My apologies for the delay. I have the TX path ready, but having
> trouble running the performance tests unrelated to the patch itself.
> I'm going to send the TX path after a round of internal reviews,
> likely by the end of next week.

Awesome, looking forward to it! I put whatever I have on my side on gh
as well: https://github.com/fomichev/linux/commits/upstream/net-next/tx/2/

Take a look in case something makes sense to update on your side. I
reworked the test to a more conventional 'cfg.remote.deploy' model
and I also simplified lookup of dma_addr on tx (my previous tx code
was super slow and buggy).