netdev - Re: [RFC PATCH 2/3] veth: make queues nr configurable via kernel module params

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: <87fswnea9g.fsf@toke.dk>
Date:   Fri, 09 Jul 2021 18:12:59 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [RFC PATCH 2/3] veth: make queues nr configurable via kernel
 module params

Paolo Abeni <pabeni@...hat.com> writes:

> On Fri, 2021-07-09 at 12:24 +0200, Toke Høiland-Jørgensen wrote:
>> Paolo Abeni <pabeni@...hat.com> writes:
>> 
>> > This allows configuring the number of tx and rx queues at
>> > module load time. A single module parameter controls
>> > both the default number of RX and TX queues created
>> > at device registration time.
>> > 
>> > Signed-off-by: Paolo Abeni <pabeni@...hat.com>
>> > ---
>> >  drivers/net/veth.c | 21 +++++++++++++++++++++
>> >  1 file changed, 21 insertions(+)
>> > 
>> > diff --git a/drivers/net/veth.c b/drivers/net/veth.c
>> > index 10360228a06a..787b4ad2cc87 100644
>> > --- a/drivers/net/veth.c
>> > +++ b/drivers/net/veth.c
>> > @@ -27,6 +27,11 @@
>> >  #include <linux/bpf_trace.h>
>> >  #include <linux/net_tstamp.h>
>> >  
>> > +static int queues_nr	= 1;
>> > +
>> > +module_param(queues_nr, int, 0644);
>> > +MODULE_PARM_DESC(queues_nr, "Max number of RX and TX queues (default = 1)");
>> 
>> Adding new module parameters is generally discouraged. Also, it's sort
>> of a cumbersome API that you'll have to set this first, then re-create
>> the device, and then use channels to get the number you want.
>> 
>> So why not just default to allocating num_possible_cpus() number of
>> queues? Arguably that is the value that makes the most sense from a
>> scalability point of view anyway, but if we're concerned about behaviour
>> change (are we?), we could just default real_num_*_queues to 1, so that
>> the extra queues have to be explicitly enabled by ethtool?
>
> I was concerned by the amount of memory wasted memory (should be ~256
> bytes per rx queue, ~320 per tx, plus the sysfs entries).

I'm not too worried by that since it's per CPU; systems with a lot of
CPUs should hopefully also have plenty of memory. Or at least I think
the user friendliness outweighs the cost in memory.

> real_num_tx_queue > 1 will makes the xmit path slower, so we likely
> want to keep that to 1 by default - unless the userspace explicitly set
> numtxqueues via netlink.

Right, that's fine by me :)

> Finally, a default large num_tx_queue slows down device creation:
>
> cat << ENDL > run.sh
> #!/bin/sh
> MAX=$1
> for I in `seq 1 $MAX`; do
> 	ip link add name v$I type veth peer name pv$I
> done
> for I in `seq 1 $MAX`; do
> 	ip link del dev v$I
> done
> ENDL
> chmod a+x run.sh
>
> # with num_tx_queue == 1
> time ./run.sh 100 
> real	0m2.276s
> user	0m0.107s
> sys	0m0.162s
>
> # with num_tx_queue == 128
> time ./run.sh 100 1
> real	0m4.199s
> user	0m0.091s
> sys	0m1.419s
>
> # with num_tx_queue == 4096
> time ./run.sh 100 
> real	0m24.519s
> user	0m0.089s
> sys	0m21.711s

So ~42 ms to create a device if there are 128 CPUs? And ~245 when
there's 4k CPUs? Doesn't seem too onerous to me...

> Still, if there is agreement I can switch to num_possible_cpus default,
> plus some trickery to keep real_num_{r,t}x_queue unchanged.
>
> WDYT?

SGTM :)

-Toke