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

[Maryam Tahhan <mtahhan@redhat.com>]

On 18/07/2022 10:38, Maryam Tahhan wrote:
> 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 
I meant xdp_hints rather than metadata here...

> 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.

  xdp_hints here also

> 
> 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.
> 
>