On Thursday, July 10, 2025 12:29 AM, Stanislav Fomichev <stfomic...@gmail.com>
wrote:
>On 07/09, Daniel Borkmann wrote:
>> On 7/7/25 5:03 PM, Stanislav Fomichev wrote:
>> > On 07/04, Daniel Borkmann wrote:
>> > > On 7/4/25 11:58 AM, Jesper Dangaard Brouer wrote:
>> > > > On 04/07/2025 03.17, Song, Yoong Siang wrote:
>> > > > > On Friday, July 4, 2025 1:05 AM, Jesper Dangaard Brouer
><h...@kernel.org> wrote:
>> > > > > > On 02/07/2025 18.57, Song Yoong Siang wrote:
>> > > > > > > Introduce the XDP_METADATA_SIZE macro as a conservative measure
>> > > > > > > to
>> > > > > > > accommodate any metadata areas reserved by Ethernet devices.
>> > > > > >
>> > > > > > This seems like a sloppy workaround :-(
>> > > > > >
>> > > > > > To me, the problem arise because AF_XDP is lacking the ability to
>> > > > > > communicate the size of the data_meta area. If we had this
>> > > > > > capability,
>> > > > > > then we could allow the IGC driver to take some of the space, have
>> > > > > > the
>> > > > > > BPF-prog expand it futher (bpf_xdp_adjust_meta) and then userspace
>> > > > > > AF_XDP would simply be able to see the size of the data_meta area,
>> > > > > > and
>> > > > > > apply the struct xdp_meta at right offset.
>> > > > > >
>> > > > > Thanks for your input.
>> > > > >
>> > > > > I agree with you that the implementation will be simple if user
>> > > > > application
>> > > > > able to get the size of data_meta area. The intention of this patch
>> > > > > set is to let
>> > > > > developer aware of such limitations before we have a perfect
>> > > > > solution.
>> > > > >
>> > > > > Btw, do you got any suggestion on how to expose the metadata length?
>> > > > > I not sure whether xdp_desc.options is a simple and good idea or not?
>> > > >
>> > > > That is a question to the AF_XDP maintainers... added them to this
>> > > > email.
>> > > >
>> > > > /* Rx/Tx descriptor */
>> > > > struct xdp_desc {
>> > > > __u64 addr;
>> > > > __u32 len;
>> > > > __u32 options;
>> > > > };
>> > > >
>> > > > As far as I know, the xdp_desc.options field isn't used, right?
>> > >
>> > > The options holds flags, see also XDP_PKT_CONTD and XDP_TX_METADATA.
>> > >
>> > > > (Please AF_XDP experts, please verify below statements:)
>> > > > Something else we likely want to document: The available headroom in
>> > > > the
>> > > > AF_XDP frame. When accessing the metadata in userspace AF_XDP we do a
>> > > > negative offset from the UMEM packet pointer. IIRC on RX the available
>> > > > headroom will be either 255 or 192 bytes (depending on NIC drivers).
>> > > >
>> > > > Slightly confusing when AF_XDP transmitting from userspace the UMEM
>> > > > headroom is default zero (XSK_UMEM__DEFAULT_FRAME_HEADROOM is
>zero).
>> > > > This is configurable via xsk_umem_config.frame_headroom, like I did in
>> > > > this example[1].
>> > > >
>> > > > Maybe I did something wrong in[1], because I see that the new method is
>> > > > setting xsk_umem_config.tx_metadata_len + flag
>> > > > XDP_UMEM_TX_METADATA_LEN.
>> > > > This is nicely documented in [2]. How does this interact with setting
>> > > > xsk_umem_config.frame_headroom ?
>> > >
>> > > If you request XDP_UMEM_TX_METADATA_LEN then on TX side you can fill
>> > > struct xsk_tx_metadata before the start of packet data, that is,
>> > > meta = data - sizeof(struct xsk_tx_metadata). The validity of the
>> > > latter is indicated via desc->options |= XDP_TX_METADATA and then
>> > > you fill meta->flags with things like XDP_TXMD_FLAGS_CHECKSUM to
>> > > tell that the related fields are valid (ex. request.csum_start,
>> > > request.csum_offset) and that you expect the driver to do the
>> > > offload with this info. This is also what I mentioned in the other
>> > > thread some time ago that imho it would make sense to have this also
>> > > on RX side somewhat similar to virtio_net_hdr..
>> >
>> > Let's at least document the current behavior where some (small minority of)
>> > drivers can reuse the rx metadata area for some of its state? If we want
>> > to improve on that by adding another knob, we can follow up?
>> > (but I remember last time it was discussed, about a year ago, people
>> > were not enthusiastic about another parameter exported as uapi)
>>
>> But its still fundamentally broken no? Unless there is no harm for BPF devs
>> to override that rx metadata area when the pkt later on goes up the stack,
>> but
>> it sounds this is not the case here. Iiuc, Yoong is trying a different
>> approach
>> now to prepend before data_hard_start [0]?
I plan to retrieve the timestamp from metadata area and put it in
xdp_buff_xsk.cb
area via struct igc_xdp_buff.
>> Then if BPF prog needs it, igc
>> already implements xmo_rx_timestamp callback which can copy it from there.
>>
>> [0] https://lore.kernel.org/bpf/20250707191742.662ae...@kernel.org/
>
>True, Jakub mentioned the same thread to me. This is, indeed, a better
>idea!
Would it be advisable to update the documentation to indicate that
drivers are expected to copy any device-reserved metadata from the
metadata area? This would ensure that xdp_buff->data_meta is equal
to xdp_buff->data before a BPF program is executed. This approach
would allow BPF programs to freely manipulate the metadata area
in XDP_REDIRECT scenarios.
Additionally, I am uncertain about the need to overriding metadata in
XDP_PASS scenarios. Should BPF programs refrain from overriding the
metadata in this case?
