[xdp-hints] Re: [PATCH RFC bpf-next 7/9] i40e: add XDP-hints handling

[Maryam Tahhan <mtahhan@redhat.com>]

On 28/06/2022 17:31, Jesper Dangaard Brouer wrote:

<snip>

> +
> +static inline void i40e_process_xdp_hints(struct i40e_ring *rx_ring,
> +					  union i40e_rx_desc *rx_desc,
> +					  struct xdp_buff *xdp,
> +					  u64 qword)
> +{
> +	u32 rx_status = (qword & I40E_RXD_QW1_STATUS_MASK) >>
> +			I40E_RXD_QW1_STATUS_SHIFT;
> +	u32 tsynvalid = rx_status & I40E_RXD_QW1_STATUS_TSYNVALID_MASK;
> +	u32 tsyn = (rx_status & I40E_RXD_QW1_STATUS_TSYNINDX_MASK) >>
> +		   I40E_RXD_QW1_STATUS_TSYNINDX_SHIFT;
> +	u64 tsyn_ts;
> +
> +	struct i40e_rx_ptype_decoded ptype;
> +	struct xdp_hints_i40e *xdp_hints;
> +	struct xdp_hints_common *common;
> +	u32 btf_id = btf_id_xdp_hints_i40e;
> +	u32 btf_sz = sizeof(*xdp_hints);
> +	u32 f1 = 0, f2, f3, f4, f5 = 0;
> +	u8 rx_ptype;
> +
> +	if (!(rx_ring->netdev->features & NETIF_F_XDP_HINTS))
> +		return;
> +
> +	/* Driver have xdp headroom when using build_skb */
> +	if (unlikely(!ring_uses_build_skb(rx_ring)))
> +		return;
> +
> +	xdp_hints = xdp->data - btf_sz;
> +	common = &xdp_hints->common;
> +
> +	if (unlikely(tsynvalid)) {
> +		struct xdp_hints_i40e_timestamp *hints;
> +
> +		tsyn_ts = i40e_ptp_rx_hwtstamp_raw(rx_ring->vsi->back, tsyn);
> +		btf_id = btf_id_xdp_hints_i40e_timestamp;
> +		btf_sz = sizeof(*hints);
> +		hints = xdp->data - btf_sz;
> +		hints->rx_timestamp = ns_to_ktime(tsyn_ts);
> +		f1 = HINT_FLAG_RX_TIMESTAMP;
> +	}
> +
> +	/* ptype needed by both hash and checksum code */
> +	rx_ptype = (qword & I40E_RXD_QW1_PTYPE_MASK) >> I40E_RXD_QW1_PTYPE_SHIFT;
> +	ptype = decode_rx_desc_ptype(rx_ptype);
> +
> +	f2 = i40e_rx_hash_xdp(rx_ring, rx_desc, xdp, qword, xdp_hints, ptype);
> +	f3 = i40e_rx_checksum_xdp(rx_ring->vsi, qword, xdp_hints, ptype);
> +	f4 = xdp_hints_set_rxq(common, rx_ring->queue_index);
> +
> +	if (unlikely(qword & BIT(I40E_RX_DESC_STATUS_L2TAG1P_SHIFT))) {
> +		__le16 vlan_tag = rx_desc->wb.qword0.lo_dword.l2tag1;
> +
> +		f5 = xdp_hints_set_vlan(common, le16_to_cpu(vlan_tag),
> +				   htons(ETH_P_8021Q));
> +	}
> +
> +	xdp_hints_set_flags(common, (f1 | f2 | f3 | f4 | f5));
> +	common->btf_id = btf_id;
> +	xdp->data_meta = xdp->data - btf_sz;

I think it might be worth considering leaving a predefined size space in 
the headroom before the data (but after the metadata) for encapsulation 
headers that may be applied to the packet as it transitions to it's 
final destination through a host. In other words starting the metadata 
further up so that the BTF id resides at a known offset from data.

Say for example a bpf program that inserts vlan/vxlan tags/headers on 
received packets on a host should have enough space to apply that vlan 
tag without having to copy the metadata and shift it before it does that.

Or maybe there was something that was already accounting for this in the 
design and I missed it. Would really appreciate a pointer in that case.