Message-ID: <df74d6e8-41cc-4840-8aca-ad7e57d387ce@gmail.com>
Date: Mon, 28 Jul 2025 19:18:36 +0100
From: Pavel Begunkov <asml.silence@...il.com>
To: Stanislav Fomichev <stfomichev@...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, almasrymina@...gle.com, dw@...idwei.uk,
 michael.chan@...adcom.com, dtatulea@...dia.com, ap420073@...il.com
Subject: Re: [RFC v1 00/22] Large rx buffer support for zcrx

On 7/28/25 18:13, Stanislav Fomichev wrote:
> On 07/28, Pavel Begunkov wrote:
>> This series implements large rx buffer support for io_uring/zcrx on
>> top of Jakub's queue configuration changes, but it can also be used
>> by other memory providers. Large rx buffers can be drastically
>> beneficial with high-end hw-gro enabled cards that can coalesce traffic
>> into larger pages, reducing the number of frags traversing the network
>> stack and resuling in larger contiguous chunks of data for the
>> userspace. Benchamrks showed up to ~30% improvement in CPU util.
>>
>> For example, for 200Gbit broadcom NIC, 4K vs 32K buffers, and napi and
>> userspace pinned to the same CPU:
>>
>> packets=23987040 (MB=2745098), rps=199559 (MB/s=22837)
>> CPU    %usr   %nice    %sys %iowait    %irq   %soft   %idle
>>    0    1.53    0.00   27.78    2.72    1.31   66.45    0.22
>> packets=24078368 (MB=2755550), rps=200319 (MB/s=22924)
>> CPU    %usr   %nice    %sys %iowait    %irq   %soft   %idle
>>    0    0.69    0.00    8.26   31.65    1.83   57.00    0.57
>>
>> And for napi and userspace on different CPUs:
>>
>> packets=10725082 (MB=1227388), rps=198285 (MB/s=22692)
>> CPU    %usr   %nice    %sys %iowait    %irq   %soft   %idle
>>    0    0.10    0.00    0.50    0.00    0.50   74.50    24.40
>>    1    4.51    0.00   44.33   47.22    2.08    1.85    0.00
>> packets=14026235 (MB=1605175), rps=198388 (MB/s=22703)
>> CPU    %usr   %nice    %sys %iowait    %irq   %soft   %idle
>>    0    0.10    0.00    0.70    0.00    1.00   43.78   54.42
>>    1    1.09    0.00   31.95   62.91    1.42    2.63    0.00
>>
>> Patch 19 allows to pass queue config from a memory provider. The
>> zcrx changes are contained in a single patch as I already queued
>> most of work making it size agnostic into my zcrx branch. The
>> uAPI is simple and imperative, it'll use the exact value (if)
>> specified by the user. In the future we might extend it to
>> "choose the best size in a given range".
>>
>> The rest (first 20) patches are from Jakub's series implementing
>> per queue configuration. Quoting Jakub:
>>
>> "... The direct motivation for the series is that zero-copy Rx queues would
>> like to use larger Rx buffers. Most modern high-speed NICs support HW-GRO,
>> and can coalesce payloads into pages much larger than than the MTU.
>> Enabling larger buffers globally is a bit precarious as it exposes us
>> to potentially very inefficient memory use. Also allocating large
>> buffers may not be easy or cheap under load. Zero-copy queues service
>> only select traffic and have pre-allocated memory so the concerns don't
>> apply as much.
>>
>> The per-queue config has to address 3 problems:
>> - user API
>> - driver API
>> - memory provider API
>>
>> For user API the main question is whether we expose the config via
>> ethtool or netdev nl. I picked the latter - via queue GET/SET, rather
>> than extending the ethtool RINGS_GET API. I worry slightly that queue
>> GET/SET will turn in a monster like SETLINK. OTOH the only per-queue
>> settings we have in ethtool which are not going via RINGS_SET is
>> IRQ coalescing.
>>
>> My goal for the driver API was to avoid complexity in the drivers.
>> The queue management API has gained two ops, responsible for preparing
>> configuration for a given queue, and validating whether the config
>> is supported. The validating is used both for NIC-wide and per-queue
>> changes. Queue alloc/start ops have a new "config" argument which
>> contains the current config for a given queue (we use queue restart
>> to apply per-queue settings). Outside of queue reset paths drivers
>> can call netdev_queue_config() which returns the config for an arbitrary
>> queue. Long story short I anticipate it to be used during ndo_open.
>>
>> In the core I extended struct netdev_config with per queue settings.
>> All in all this isn't too far from what was there in my "queue API
>> prototype" a few years ago ..."
> 
> Supporting big buffers is the right direction, but I have the same
> feedback: 

Let me actually check the feedback for the queue config RFC...

it would be nice to fit a cohesive story for the devmem as well.

Only the last patch is zcrx specific, the rest is agnostic,
devmem can absolutely reuse that. I don't think there are any
issues wiring up devmem?

> We should also aim for another use-case where we allocate page pool
> chunks from the huge page(s), 

Separate huge page pool is a bit beyond the scope of this series.

this should push the perf even more.

And not sure about "even more" is from, you can already
register a huge page with zcrx, and this will allow to chunk
them to 32K or so for hardware. Is it in terms of applicability
or you have some perf optimisation ideas?

> We need some way to express these things from the UAPI point of view.

Can you elaborate?

> Flipping the rx-buf-len value seems too fragile - there needs to be
> something to request 32K chunks only for devmem case, not for the (default)
> CPU memory. And the queues should go back to default 4K pages when the dmabuf
> is detached from the queue.

That's what the per-queue config is solving. It's not default, zcrx
configures it only for the specific queue it allocated, and the value
is cleared on restart in netdev_rx_queue_restart(), if not even too
aggressively. Maybe I should just stash it into mp_params to make
sure it's not cleared if a provider is still attached on a spurious
restart.

-- 
Pavel Begunkov

Powered by blists - more mailing lists