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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Tianyi Gao wrote:
> From: David Keogh <dkeogh@amazon.com>
> 
> Programs attached at TC egress commonly push additional headers onto a
> packet, for example to encapsulate it. When the packet does not carry
> enough headroom the BPF helpers reallocate the head, which costs an
> allocation and a copy per packet.
> 
> Locally generated UDP, raw and AF_PACKET traffic is allocated with
> exactly LL_RESERVED_SPACE() of headroom, which only covers the
> link-layer header and the driver's needed_headroom. An administrator can
> only ask for more headroom at allocation time if the driver supports it,
> as netkit does with IFLA_NETKIT_HEADROOM. veth does not.
> 
> 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?

> together with LL_ALLOC_SPACE() and LL_ALLOC_SPACE_EX(). These add the

Since this already changes callers from LL_RESERVED_SPACE to
LL_ALLOC_SPACE, could this additional use case be integrated more in
the struct fields and helpers that are already there?

Admittedly, I did not spend much time looking at that deeply, and you
likely did, so the answer is no. But I want to poke a little at that
before we add yet another optional length field.

> extra reservation to the header length before the HH_DATA_MOD alignment,
> so the alignment of the link-layer header does not change.
> 
> The new macros are intended only for sites that allocate new packets.
> LL_RESERVED_SPACE() keeps its current meaning and remains the threshold
> used when checking whether an existing packet must be expanded, so the
> extra reservation never forces a reallocation on its own: packets that
> already have enough room for the headers being pushed, such as TCP
> segments built with MAX_TCP_HEADER, are left alone.
> 
> Keep the extra reservation separate from needed_headroom so that driver
> updates of their own requirement do not clobber it and so that it can
> be cleared independently. Place the field in the padding that follows
> needed_headroom, which keeps it in the TX read-mostly cacheline group.
> 
> The field is not set anywhere yet. A later patch exposes it through
> rtnetlink.
> 
> Also remove LL_RESERVED_SPACE_EXTRA(). It has had no user since
> commit 6d5431555de0 ("caif: remove CAIF NETWORK LAYER"). It could add
> the extra headroom, but it reads dev->hard_header_len itself. AF_PACKET
> must use the value that it read once, see commit 03390aa32e66 ("packet:
> use consistent hard_header_len in non-ring send paths").
> LL_ALLOC_SPACE_EX() takes the header length as an argument instead.
> 
> Signed-off-by: David Keogh <dkeogh@amazon.com>
> [tianyi: remove LL_RESERVED_SPACE_EXTRA()]
> Signed-off-by: Tianyi Gao <tianyi@cloudflare.com>
> ---
> checkpatch --strict reports "Macro argument reuse 'dev'" for
> LL_ALLOC_SPACE_EX() and LL_ALLOC_SPACE(). It reports the same for
> LL_RESERVED_SPACE().
> ---
>  .../networking/net_cachelines/net_device.rst  |  1 +
>  include/linux/netdevice.h                     | 36 +++++++++++++++++--
>  net/core/dev.c                                |  2 ++
>  3 files changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/networking/net_cachelines/net_device.rst b/Documentation/networking/net_cachelines/net_device.rst
> index 8eceaa80b686..a7e311fb7875 100644
> --- a/Documentation/networking/net_cachelines/net_device.rst
> +++ b/Documentation/networking/net_cachelines/net_device.rst
> @@ -35,6 +35,7 @@ unsigned_short                      gflags
>  unsigned_short                      hard_header_len             read_mostly         read_mostly         ip6_xmit(tx);gro_list_prepare(rx)
>  unsigned_int                        mtu                         read_mostly                             ip_finish_output2
>  unsigned_short                      needed_headroom             read_mostly                             LL_RESERVED_SPACE/ip_finish_output2
> +unsigned_short                      tx_extra_headroom           read_mostly                             LL_ALLOC_SPACE/__ip_append_data
>  unsigned_short                      needed_tailroom
>  netdev_features_t                   features                    read_mostly         read_mostly         HARD_TX_LOCK,netif_skb_features,sk_setup_caps(tx);netif_elide_gro(rx)
>  netdev_features_t                   hw_features
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 435be7880d7e..98d1501d6fee 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -311,9 +311,24 @@ struct hh_cache {
>  	  & ~(HH_DATA_MOD - 1)) + HH_DATA_MOD)
>  #define LL_RESERVED_SPACE(dev) \
>  	LL_RESERVED_SPACE_EX(dev, (dev)->hard_header_len)
> -#define LL_RESERVED_SPACE_EXTRA(dev,extra) \
> -	((((dev)->hard_header_len + READ_ONCE((dev)->needed_headroom) + (extra)) \
> -	  & ~(HH_DATA_MOD - 1)) + HH_DATA_MOD)
> +
> +/*
> + * Headroom to reserve when allocating a locally generated packet for
> + * transmission through a device. On top of LL_RESERVED_SPACE() this adds
> + * the administrator-configured tx_extra_headroom, folded in before the
> + * alignment so that the link-layer header placement does not change.
> + *
> + * Only use this for new allocations. Code that checks whether an existing
> + * packet has enough headroom must keep using LL_RESERVED_SPACE(), so that
> + * the extra reservation can never force a reallocation by itself.
> + */
> +#define LL_ALLOC_SPACE_EX(dev, hlen) \
> +	LL_RESERVED_SPACE_EX(dev, (hlen) + READ_ONCE((dev)->tx_extra_headroom))
> +#define LL_ALLOC_SPACE(dev) \
> +	LL_ALLOC_SPACE_EX(dev, (dev)->hard_header_len)
> +

If having to modify/add these helpers, it's probably a good time to
switch to actual, type safe and readable, functions instead.