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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zv8XrPEG6g_W_k7O@mini-arch>
Date: Thu, 3 Oct 2024 15:16:12 -0700
From: Stanislav Fomichev <stfomichev@...il.com>
To: Mina Almasry <almasrymina@...gle.com>
Cc: Stanislav Fomichev <sdf@...ichev.me>, netdev@...r.kernel.org,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com
Subject: Re: [PATCH net-next v2 09/12] selftests: ncdevmem: Remove hard-coded
 queue numbers

On 10/03, Mina Almasry wrote:
> On Thu, Oct 3, 2024 at 10:02 AM Stanislav Fomichev <stfomichev@...il.com> wrote:
> >
> > On 10/03, Mina Almasry wrote:
> > > On Mon, Sep 30, 2024 at 10:18 AM Stanislav Fomichev <sdf@...ichev.me> wrote:
> > > >
> > > > Use single last queue of the device and probe it dynamically.
> > > >
> > >
> > > Sorry I know there was a pending discussion in the last iteration that
> > > I didn't respond to. Been a rough week with me out sick a bit.
> > >
> > > For this, the issue I see is that by default only 1 queue binding will
> > > be tested, but I feel like test coverage for the multiple queues case
> > > by default is very nice because I actually ran into some issues making
> > > multi-queue binding work.
> > >
> > > Can we change this so that, by default, it binds to the last rxq_num/2
> > > queues of the device?
> >
> > I'm probably missing something, but why do you think exercising this from
> > the probe/selftest mode is not enough? It might be confusing for the readers
> > to understand why we bind to half of the queues and flow steer into them
> > when in reality there is only single tcp flow.
> >
> > IOW, can we keep these two modes:
> > 1. server / client - use single queue
> > 2. selftest / probe - use more than 1 queue by default (and I'll remove the
> >    checks that enforce the number of queues for this mode to let the
> >    users override)
> 
> Ah, I see. Thanks for the explanation.
> 
> My paranoia here is that we don't notice multi-queue binding
> regressions because the tests are often run in data path mode and we
> don't use or notice failures in the probe mode.
> 
> I will concede my paranoia is just that and this is not very likely to
> happen, but also if it is confusing to bind multi-queues and then just
> use one, then we could remedy that with a comment and keep the
> accidental test coverage. It also makes the test simpler to always
> bind the same # of queues rather than special case data and control
> path tests.
> 
> But your 2 mode approach sounds fine as well. But to implement that
> you need more than to remove the checks that enforce the number of
> queues, right? In probe mode num_queues should be rxq_num/2, and in
> server mode num_queues should be 1, yes?

Yes, I'll follow your suggestion with `start_queues = num_queues / 2`
for the selftest part.

Tentatively (default to 1/2 queues, if want to override - provide both
-t an -q):

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;