Re: [PATCH net-next v3 05/12] selftests: ncdevmem: Remove default arguments

Re: [PATCH net-next v3 05/12] selftests: ncdevmem: Remove default arguments

[Mina Almasry]

On Wed, Oct 9, 2024 at 10:13 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> 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.
>
> Move the invocation comment to the top, add missing -s to the client
> and cleanup the client invocation a bit to make more readable.
>
> Cc: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> ---
>  tools/testing/selftests/net/ncdevmem.c | 49 ++++++++++++++------------
>  1 file changed, 27 insertions(+), 22 deletions(-)
>
> diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> index 2ee7b4eb9f71..99ae3a595787 100644
> --- a/tools/testing/selftests/net/ncdevmem.c
> +++ b/tools/testing/selftests/net/ncdevmem.c
> @@ -1,4 +1,19 @@
>  // SPDX-License-Identifier: GPL-2.0
> +/*
> + * 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
> + *
> + *     On client:
> + *     echo -n "hello\nworld" | nc -s <server IP> 5201 -p 5201
> + *

No need to remove the documentation telling users how to do validation
when moving these docs. Please have a secondary section that retains
the docs for the validation:

* Usage:

(what you have)

* Test data validation:

(What I had before)

With that:

Reviewed-by: Mina Almasry <almasrymina@google.com>

-- 
Thanks,
Mina

Re: [PATCH net-next v3 09/12] selftests: ncdevmem: Remove hard-coded queue numbers

[Mina Almasry]

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

Sorry I thought agreed that multi-queue binding test coverage is important.

Can you please leave the default of num_queues to be 8 queues, or
rxq_num / 2? You can override num_queues to 1 in your test invocations
if you want. I would like by default an unaware tester that doesn't
set num_queues explicitly to get multi-queue test coverage.
> 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 02ba3f368888..90aacfb3433f 100644
> --- a/tools/testing/selftests/net/ncdevmem.c
> +++ b/tools/testing/selftests/net/ncdevmem.c
> @@ -63,8 +63,8 @@ static char *server_ip;
>  static char *client_ip;
>  static char *port;
>  static size_t do_validation;
> -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;
> @@ -196,6 +196,33 @@ void validate_buffer(void *line, size_t size)
>         fprintf(stdout, "Validated buffer\n");
>  }
>
> +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];                                      \
> @@ -690,6 +717,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.47.0
>


--
Thanks,
Mina

Re: [PATCH net-next v3 09/12] selftests: ncdevmem: Remove hard-coded queue numbers

[Stanislav Fomichev]

On 10/12, Mina Almasry wrote:
> On Wed, Oct 9, 2024 at 10:13 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > Use single last queue of the device and probe it dynamically.
> >
> 
> Sorry I thought agreed that multi-queue binding test coverage is important.
> 
> Can you please leave the default of num_queues to be 8 queues, or
> rxq_num / 2? You can override num_queues to 1 in your test invocations
> if you want. I would like by default an unaware tester that doesn't
> set num_queues explicitly to get multi-queue test coverage.

I might have misunderstood the agreement :-) I though you were ok with
the following arrangement:

1. use num_queues / 2 in the selftest mode to make sure binding to multiple
   queues works (and this gets exercised from the python kselftest)
2. use single queue for the actual data path test (since we are
   installing single flow steering rule, having multiple queues here is
   confusing)

The num_queues / 2 part is here:
https://lore.kernel.org/netdev/20241009171252.2328284-11-sdf@fomichev.me/

Anything I'm missing?

Re: [PATCH net-next v3 05/12] selftests: ncdevmem: Remove default arguments

[Stanislav Fomichev]

On 10/12, Mina Almasry wrote:
> On Wed, Oct 9, 2024 at 10:13 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > 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.
> >
> > Move the invocation comment to the top, add missing -s to the client
> > and cleanup the client invocation a bit to make more readable.
> >
> > Cc: Mina Almasry <almasrymina@google.com>
> > Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> > ---
> >  tools/testing/selftests/net/ncdevmem.c | 49 ++++++++++++++------------
> >  1 file changed, 27 insertions(+), 22 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> > index 2ee7b4eb9f71..99ae3a595787 100644
> > --- a/tools/testing/selftests/net/ncdevmem.c
> > +++ b/tools/testing/selftests/net/ncdevmem.c
> > @@ -1,4 +1,19 @@
> >  // SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * 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
> > + *
> > + *     On client:
> > + *     echo -n "hello\nworld" | nc -s <server IP> 5201 -p 5201
> > + *
> 
> No need to remove the documentation telling users how to do validation
> when moving these docs. Please have a secondary section that retains
> the docs for the validation:
> 
> * Usage:
> 
> (what you have)
> 
> * Test data validation:
> 
> (What I had before)
> 
> With that:
> 
> Reviewed-by: Mina Almasry <almasrymina@google.com>

SG, will do, thanks!

Re: [PATCH net-next v3 10/12] selftests: ncdevmem: Run selftest when none of the -s or -c has been provided

[Mina Almasry]

On Wed, Oct 9, 2024 at 8:13 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> This will be used as a 'probe' mode in the selftest to check whether
> the device supports the devmem or not. Use hard-coded queue layout
> (two last queues) and prevent user from passing custom -q and/or -t.
>
> Cc: Mina Almasry <almasrymina@google.com>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> ---
>  tools/testing/selftests/net/ncdevmem.c | 42 ++++++++++++++++++++------
>  1 file changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/selftests/net/ncdevmem.c b/tools/testing/selftests/net/ncdevmem.c
> index 90aacfb3433f..3a456c058241 100644
> --- a/tools/testing/selftests/net/ncdevmem.c
> +++ b/tools/testing/selftests/net/ncdevmem.c
> @@ -64,7 +64,7 @@ static char *client_ip;
>  static char *port;
>  static size_t do_validation;
>  static int start_queue = -1;
> -static int num_queues = 1;
> +static int num_queues = -1;
>  static char *ifname;
>  static unsigned int ifindex;
>  static unsigned int dmabuf_id;
> @@ -706,19 +706,31 @@ 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);
>
> -       if (start_queue < 0) {
> -               start_queue = rxq_num(ifindex) - 1;
> +       if (!server_ip && !client_ip) {
> +               if (start_queue < 0 && num_queues < 0) {
> +                       num_queues = rxq_num(ifindex);
> +                       if (num_queues < 0)
> +                               error(1, 0, "couldn't detect number of queues\n");
> +                       /* make sure can bind to multiple queues */
> +                       start_queues = num_queues / 2;
> +                       num_queues /= 2;
> +               }
> +
> +               if (start_queue < 0 || num_queues < 0)
> +                       error(1, 0, "Both -t and -q are requred\n");
> +
> +               run_devmem_tests();
> +               return 0;
> +       }
> +
> +       if (start_queue < 0 && num_queues < 0) {
> +               num_queues = 1;
> +               start_queue = rxq_num(ifindex) - num_queues;
>

Nit: this can be written into the more readable:

// set start_queues = rxq_num / 2;
if (!server_ip && !client_ip) {
  // set num_queues rxq_num/2
  run_devmem_tests();
} else {
  // set num_queue = 1.
  run_server();
}

With build error fixed:

Reviewed-by: Mina Almasry <almasrymina@google.com>


-- 
Thanks,
Mina

Re: [PATCH net-next v3 09/12] selftests: ncdevmem: Remove hard-coded queue numbers

[Mina Almasry]

On Mon, Oct 14, 2024 at 5:47 PM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 10/12, Mina Almasry wrote:
> > On Wed, Oct 9, 2024 at 10:13 AM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > >
> > > Use single last queue of the device and probe it dynamically.
> > >
> >
> > Sorry I thought agreed that multi-queue binding test coverage is important.
> >
> > Can you please leave the default of num_queues to be 8 queues, or
> > rxq_num / 2? You can override num_queues to 1 in your test invocations
> > if you want. I would like by default an unaware tester that doesn't
> > set num_queues explicitly to get multi-queue test coverage.
>
> I might have misunderstood the agreement :-) I though you were ok with
> the following arrangement:
>
> 1. use num_queues / 2 in the selftest mode to make sure binding to multiple
>    queues works (and this gets exercised from the python kselftest)
> 2. use single queue for the actual data path test (since we are
>    installing single flow steering rule, having multiple queues here is
>    confusing)
>
> The num_queues / 2 part is here:
> https://lore.kernel.org/netdev/20241009171252.2328284-11-sdf@fomichev.me/
>
> Anything I'm missing?

Sorry, I indeed missed that this is reworked in patch 10/12. Squashing
the patches could be OK, because the changes to num_queues here are
essentially reworked in the next patch, but this is also fine.

Reviewed-by: Mina Almasry <almasrymina@google.com>

-- 
Thanks,
Mina