Re: [PATCH v2 11/12] tap: Don't size pool_tap[46] for the maximum number of packets

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 20 Dec 2024 19:35:34 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Currently we attempt to size pool_tap[46] so they have room for the maximum
> possible number of packets that could fit in pkt_buf, TAP_MSGS.  However,
> the calculation isn't quite correct: TAP_MSGS is based on ETH_ZLEN (60) as
> the minimum possible L2 frame size.  But, we don't enforce that L2 frames
> are at least ETH_ZLEN when we receive them from the tap backend, and since
> we're dealing with virtual interfaces we don't have the physical Ethernet
> limitations requiring that length.  Indeed it is possible to generate a
> legitimate frame smaller than that (e.g. a zero-payload UDP/IPv4 frame on
> the 'pasta' backend is only 42 bytes long).
> 
> It's also unclear if this limit is sufficient for vhost-user which isn't
> limited by the size of pkt_buf as the other modes are.
> 
> We could attempt to correct the calculation, but that would leave us with
> even larger arrays, which in practice rarely accumulate more than a handful
> of packets.  So, instead, put an arbitrary cap on the number of packets we
> can put in a batch, and if we run out of space, process and flush the
> batch.

I ran a few more tests with this, keeping TAP_MSGS at 256, and in
general I couldn't really see a difference in latency (especially for
UDP streams with small packets) or throughput. Figures from short
throughput tests (such as the ones from the test suite) look a bit more
variable, but I don't have any statistically meaningful data.

Then I looked into how many messages we might have in the array without
this change, and I realised that, with the throughput tests from the
suite, we very easily exceed the 256 limit.

Perhaps surprisingly we get the highest buffer counts with TCP transfers
and intermediate MTUs: we're at about 4000-5000 with 1500 bytes (and
more like ~1000 with 1280 bytes) meaning that we move 6 to 8 megabytes
in one shot, every 5-10ms (at 8 Gbps). With that kind of time interval,
the extra system call overhead from forcibly flushing batches might
become rather relevant.

With lower MTUs, it looks like we have a lower CPU load and
transmissions are scheduled differently (resulting in smaller batches),
but I didn't really trace things.

So I start thinking that this has the *potential* to introduce a
performance regression in some cases and we shouldn't just assume that
some arbitrary 256 limit is good enough. I didn't check with perf(1),
though.

Right now that array takes, effectively, less than 100 KiB (it's ~5000
copies of struct iovec, 16 bytes each), and in theory that could be
~2.5 MiB (at 161319 items). Even if we double or triple that (let's
assume we use 2 * ETH_ALEN to keep it simple) it's not much... and will
have no practical effect anyway.

All in all, I think we shouldn't change this limit without a deeper
understanding of the practical impact. While this change doesn't bring
any practical advantage, the current behaviour is somewhat tested by
now, and a small limit isn't.

-- 
Stefano