Re: [PATCH net-next 1/8] net: add tx_extra_headroom and LL_ALLOC_SPACE()

[Stephen Hemminger <stephen@networkplumber.org>]

On Sun, 08 Nov 2026 10:13:03 -0500
Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:

> Tianyi Gao wrote:
> > On Sat, Nov 07, 2026 at 09:51:07AM -0500, Willem de Bruijn wrote:  
> > > Tianyi Gao wrote:  
> > > > From: David Keogh <dkeogh@amazon.com>
> > > >
> > > > Add a tx_extra_headroom field to struct net_device for that purpose,  
> > >
> > > Main high level concern is just adding an extra field. What's next,
> > > do we need to add a field extra_extra_headroom?  
> > 
> > Thanks for the review.
> > 
> > A user needs more room either in every packet or only in new skbs.
> > needed_headroom covers the first case and tx_extra_headroom the
> > second. I do not see a third case.
> > 
> > needed_headroom does not only set the headroom of new skbs. It is also
> > the minimum that ip_finish_output2() and ip_forward() require of
> > existing skbs. Raising it for new skbs also raises that minimum. With
> > XDP on a veth peer and needed_headroom raised to 256, throughput drops
> > by 2.8% to 5.8% for local TCP and by 4.0% to 20.8% for forwarded
> > traffic. The cover letter has the detailed analysis. veth also cannot
> > set needed_headroom from user space today.
> > 
> > pahole on x86_64 (defconfig, net/core/dev.o) shows that the field fills
> > the 2-byte hole after needed_headroom, so struct net_device does not
> > grow:
> > 
> > before:
> > 	unsigned short             needed_headroom;      /*    60     2 */
> > 	/* XXX 2 bytes hole, try to pack */
> > 	/* size: 2688, cachelines: 42, members: 187 */
> > 
> > after:
> > 	unsigned short             needed_headroom;      /*    60     2 */
> > 	unsigned short             tx_extra_headroom;    /*    62     2 */
> > 	/* size: 2688, cachelines: 42, members: 188 */  
> 
> Thanks.
> 
> Pushing back on another design choice: the UAPI change. That both cannot be
> undone, and is fragile and laborious, requiring manual admin arithmetic.


I agree with Willem. This is heading the wrong direction.


The more wordy AI analysis is from the iproute2 patch side:


Network configuration should not require pre-guessing this value. To set
it correctly an administrator must know which TC egress programs are
attached, how many bytes each pushes, and whether XDP is attached on the
veth peer. That is a kernel-internal sizing decision being pushed out to
user space, where the information is not reliably available. Worse, it
goes stale silently: attach another encap action or move the peer to a
different netns, and the value that was right at "ip link add" time is
now wrong with nothing reporting it.

The reply that a stale value only costs performance is true but does not
rescue the design. The entire purpose of the field is performance. A
setting that quietly stops delivering the benefit it exists for is one
operators carry indefinitely without knowing whether it still helps.
Failing quietly in the only dimension the knob was added for is the cost,
not a mitigation.

Since the motivating case is XDP, the mechanism should let the XDP program
ask for the padding it needs, rather than making the operator translate
the program's requirement into a device setting by hand. XDP already
guarantees XDP_PACKET_HEADROOM to its programs. The requirement is known
to the kernel, in the right units, at the moment of attach. Propagating it
from the attach point beats an administrator's arithmetic and cannot go
stale, because detach undoes it.

Your own reply already works this out:

> - XDP on a veth peer: veth_xdp_set() and generic_xdp_install() can
>   increase the headroom of the peer to XDP_PACKET_HEADROOM on attach
>   and reset it on detach.

That is the mechanism, and it needs no uapi, no operator arithmetic, and
no stale state. I would rather see it implemented for the cases it covers
than add a permanent uapi that removes the incentive to do so.

To be clear on what I am not asking for: this is not auto-tuning. Your
objections to auto-tuning are good ones. The kmalloc-2048 to kmalloc-4096
cliff being crossed without anyone deciding to is a real problem, and the
reset-on-detach bookkeeping across namespaces is real work. Attach-time
propagation is a different thing: a known requirement, declared at a known
point, with a natural reset.

On the argument that program-side declaration is itself new uapi, and that
the labeled device is often not the device the skb was allocated on:
granted for the general TC egress case. It does not apply to the
attach-time propagation above, which is in-kernel plumbing rather than
uapi.

Which leaves the genuinely hard case, the one you describe at Cloudflare: a
service in a netns sends through a veth pair, the host forwards to the
uplink, and a TC egress program there encapsulates, after ip_rcv_core()
has orphaned the skb so the allocating device cannot be identified. If
that is the real residue, make it the problem being solved and argue for
the narrowest mechanism that solves it. As posted, the series leads with
cases that in-kernel propagation handles and uses them to justify an
interface sized for the case it does not.