Tianyi, Jesper (added to Cc),
Tianyi got in touch off-list offering to help get this series merged,
and sent a draft v1 built on the comments in this thread. Thanks, and
sorry for the slow reply. I would be glad to work on it together, and
I would like to keep the discussion on the list from here so everyone
can follow it.
Tianyi, you have a second use case for this: an XDP program on a veth
peer. Please describe it here, as it bears directly on the points
raised above.
I suggest you post v1, with patches 1-4 from me and 5-8 from you, and
the cover letter under your own name, since the veth XDP case and the
new measurements are yours. Please drop my Co-developed-by and
Signed-off-by from patch 5. It is your test, and little of the RFC test
it replaces is left.
I rebased your draft on net-next 0822d2fe3826 and ran it on x86_64 and
arm64. tx_extra_headroom.py passes on both, and with patch 2 reverted
the push and XDP cases fail as expected. A few things to fold in before
posting:
- The note below the --- that native XDP on veth still copies belongs
in the body. The motivation says veth copies every packet, and this
series only removes the copy for generic XDP.
- checkpatch --strict reports "Macro argument reuse 'dev'" twice on
patch 1, for LL_ALLOC_SPACE_EX(). It is the same pattern as
LL_RESERVED_SPACE_EX() and I am fine leaving it, but NIPA will flag
it, so a line under the --- may save a question.
- The comment above main() in xdp_helper.c still says the helper does
not process packets. Something like "with -t it sends one frame
through the TX ring and exits once the frame completes" would match
patch 6.
- The iproute2 patch does not build against iproute2-next yet, because
its uapi copy has no IFLA_TX_EXTRA_HEADROOM. Worth saying in that
patch that it depends on the kernel side and a header sync.
On the questions you raised:
1. Should removing LL_RESERVED_SPACE_EXTRA() go in patch 1? Yes. The
reason it cannot be reused is tied to the new macro, so it reads
well there.
2. Should new fragments from ip_do_fragment() and ip6_fragment() also
get the extra headroom? No, leave them. A larger value would send
more packets to the slow path, which copies them.
3. Is patch 5 fine with you as author and my Co-developed-by? As
above, please drop my trailers from patch 5.
4. Should there be a test that TCP is not reallocated? Yes, I have
one. It runs one TCP connection over IPv4 and one over IPv6 with
tx_extra_headroom 0 and 512, and checks that the headroom of the
TCP packets is the same. With ip_finish_output2() changed to use
LL_ALLOC_SPACE(), the IPv4 case fails (headroom 246-254 becomes
566-574). It applies on top of your patches, so I will send it as a
reply to v1.
Daniel, Dragos, David: unless you see a reason to hold off, the plan is
for Tianyi to post v1, addressing the comments in this thread.
Thanks,
David