netdev - Re: [RFC v1 05/22] net: add rx_buf

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: <CAHS8izOx5p2hw7OxhKZNUUmC5uJaM0PKw_4UdELe8LQ1QkuLyw@mail.gmail.com>
Date: Mon, 28 Jul 2025 14:50:12 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Pavel Begunkov <asml.silence@...il.com>
Cc: Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org, io-uring@...r.kernel.org, 
	Eric Dumazet <edumazet@...gle.com>, Willem de Bruijn <willemb@...gle.com>, 
	Paolo Abeni <pabeni@...hat.com>, andrew+netdev@...n.ch, horms@...nel.org, 
	davem@...emloft.net, sdf@...ichev.me, dw@...idwei.uk, 
	michael.chan@...adcom.com, dtatulea@...dia.com, ap420073@...il.com
Subject: Re: [RFC v1 05/22] net: add rx_buf_len to netdev config

On Mon, Jul 28, 2025 at 4:03 AM Pavel Begunkov <asml.silence@...il.com> wrote:
>
> From: Jakub Kicinski <kuba@...nel.org>
>
> Add rx_buf_len to configuration maintained by the core.
> Use "three-state" semantics where 0 means "driver default".
>

What are three states in the semantics here?

- 0 = driver default.
- non-zero means value set by userspace

What is the 3rd state here?

> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> Signed-off-by: Pavel Begunkov <asml.silence@...il.com>
> ---
>  include/net/netdev_queues.h | 4 ++++
>  net/ethtool/common.c        | 1 +
>  net/ethtool/rings.c         | 2 ++
>  3 files changed, 7 insertions(+)
>
> diff --git a/include/net/netdev_queues.h b/include/net/netdev_queues.h
> index 81df0794d84c..eb3a5ac823e6 100644
> --- a/include/net/netdev_queues.h
> +++ b/include/net/netdev_queues.h
> @@ -24,6 +24,10 @@ struct netdev_config {
>          * If "unset" driver is free to decide, and may change its choice
>          * as other parameters change.
>          */
> +       /** @rx_buf_len: Size of buffers on the Rx ring
> +        *               (ETHTOOL_A_RINGS_RX_BUF_LEN).
> +        */
> +       u32     rx_buf_len;
>         /** @hds_config: HDS enabled (ETHTOOL_A_RINGS_TCP_DATA_SPLIT).
>          */
>         u8      hds_config;
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index a87298f659f5..8fdffc77e981 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -832,6 +832,7 @@ void ethtool_ringparam_get_cfg(struct net_device *dev,
>
>         /* Driver gives us current state, we want to return current config */
>         kparam->tcp_data_split = dev->cfg->hds_config;
> +       kparam->rx_buf_len = dev->cfg->rx_buf_len;

I'm confused that struct netdev_config is defined in netdev_queues.h,
and is documented to be a queue-related configuration, but doesn't
seem to be actually per queue? This line is grabbing the current
config for this queue from dev->cfg which looks like a shared value.

I don't think rx_buf_len should be a shared value between all the
queues. I strongly think it should a per-queue value. The
devmem/io_uring queues will probably want large rx_buf_len, but normal
queues will want 0 buf len, me thinks.

-- 
Thanks,
Mina