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: <e425920ed8120597a3a2c129c5a19fa1bc4854a2.camel@redhat.com>
Date:   Fri, 09 Jul 2021 17:33:00 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Toke Høiland-Jørgensen <toke@...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

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).

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.

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

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?

Thanks!

Paolo