On Mon, Oct 28, 2024 at 07:42:54PM +0100, Stefano
Brivio wrote:
On Mon, 28 Oct 2024 20:40:44 +1100
David Gibson <david(a)gibson.dropbear.id.au> wrote:
Currently these expects both the TCP header and
payload in a single IOV,
and goes to some trouble to locate the checksum field within it. In the
current caller we've already know where the TCP header is, so we might as
well just pass it in. This will need to work a bit differently for
vhost-user, but that code already needs to locate the TCP header for other
reasons, so again we can just pass it in.
We couldn't do this, and also what you're now doing in 5/7, because
with vhost-user the TCP header is not aligned, so we can't pass it
around as a pointer, see:
<ZeUpxEY-sn64NLE5@zatzit>
https://archives.passt.top/passt-dev/ZeUpxEY-sn64NLE5@zatzit/
and following. That one is about IP headers, but the same applies to
TCP and UDP headers.
Hrm. I'm aware it theoretically need not be aligned, but I thought it
was in practice.. and that we were already relying on that.
In fact, I'm pretty sure the second part is true, although more subtly
than here. v8 of the vhost-user patches calls tcp_fill_headers[46]()
with the bp parameter set to the offset of the TCP header. If
creating a tcphdr * there is a problem, then creating a tcp_payload_t
* can't be any better.
Of course the current solution is not elegant and
it would be nice to
find another way to deal with it, but we couldn't come up with anything
better back then.
The rest of the series looks good to me, but I'm afraid that without
this one and 5/7 the other changes will be a bit more complicated to
implement (if at all possible).
Definitely. I have so ideas for approaches more robust to
misalignment, but they're substantially more complicated. I was
hoping we could avoid it at least for now.