Stanislav Fomichev <sdf@xxxxxxxxxx> writes:
> On Thu, Jun 29, 2023 at 5:01 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
>>
>> Magnus Karlsson <magnus.karlsson@xxxxxxxxx> writes:
>>
>> > On Thu, 29 Jun 2023 at 13:30, Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
>> >>
>> >> Stanislav Fomichev <sdf@xxxxxxxxxx> writes:
>> >>
>> >> > On Wed, Jun 28, 2023 at 1:09 AM Magnus Karlsson
>> >> > <magnus.karlsson@xxxxxxxxx> wrote:
>> >> >>
>> >> >> On Mon, 26 Jun 2023 at 19:06, Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
>> >> >> >
>> >> >> > On Sat, Jun 24, 2023 at 2:02 AM Jesper Dangaard Brouer
>> >> >> > <jbrouer@xxxxxxxxxx> wrote:
>> >> >> > >
>> >> >> > >
>> >> >> > >
>> >> >> > > On 23/06/2023 19.41, Stanislav Fomichev wrote:
>> >> >> > > > On Fri, Jun 23, 2023 at 3:24 AM Jesper Dangaard Brouer
>> >> >> > > > <jbrouer@xxxxxxxxxx> wrote:
>> >> >> > > >>
>> >> >> > > >>
>> >> >> > > >>
>> >> >> > > >> On 22/06/2023 19.55, Stanislav Fomichev wrote:
>> >> >> > > >>> On Thu, Jun 22, 2023 at 2:11 AM Jesper D. Brouer <netdev@xxxxxxxxxx> wrote:
>> >> >> > > >>>>
>> >> >> > > >>>>
>> >> >> > > >>>> This needs to be reviewed by AF_XDP maintainers Magnus and Bjørn (Cc)
>> >> >> > > >>>>
>> >> >> > > >>>> On 21/06/2023 19.02, Stanislav Fomichev wrote:
>> >> >> > > >>>>> For zerocopy mode, tx_desc->addr can point to the arbitrary offset
>> >> >> > > >>>>> and carry some TX metadata in the headroom. For copy mode, there
>> >> >> > > >>>>> is no way currently to populate skb metadata.
>> >> >> > > >>>>>
>> >> >> > > >>>>> Introduce new XDP_TX_METADATA_LEN that indicates how many bytes
>> >> >> > > >>>>> to treat as metadata. Metadata bytes come prior to tx_desc address
>> >> >> > > >>>>> (same as in RX case).
>> >> >> > > >>>>
>> >> >> > > >>>> From looking at the code, this introduces a socket option for this TX
>> >> >> > > >>>> metadata length (tx_metadata_len).
>> >> >> > > >>>> This implies the same fixed TX metadata size is used for all packets.
>> >> >> > > >>>> Maybe describe this in patch desc.
>> >> >> > > >>>
>> >> >> > > >>> I was planning to do a proper documentation page once we settle on all
>> >> >> > > >>> the details (similar to the one we have for rx).
>> >> >> > > >>>
>> >> >> > > >>>> What is the plan for dealing with cases that doesn't populate same/full
>> >> >> > > >>>> TX metadata size ?
>> >> >> > > >>>
>> >> >> > > >>> Do we need to support that? I was assuming that the TX layout would be
>> >> >> > > >>> fixed between the userspace and BPF.
>> >> >> > > >>
>> >> >> > > >> I hope you don't mean fixed layout, as the whole point is adding
>> >> >> > > >> flexibility and extensibility.
>> >> >> > > >
>> >> >> > > > I do mean a fixed layout between the userspace (af_xdp) and devtx program.
>> >> >> > > > At least fixed max size of the metadata. The userspace and the bpf
>> >> >> > > > prog can then use this fixed space to implement some flexibility
>> >> >> > > > (btf_ids, versioned structs, bitmasks, tlv, etc).
>> >> >> > > > If we were to make the metalen vary per packet, we'd have to signal
>> >> >> > > > its size per packet. Probably not worth it?
>> >> >> > >
>> >> >> > > Existing XDP metadata implementation also expand in a fixed/limited
>> >> >> > > sized memory area, but communicate size per packet in this area (also
>> >> >> > > for validation purposes). BUT for AF_XDP we don't have room for another
>> >> >> > > pointer or size in the AF_XDP descriptor (see struct xdp_desc).
>> >> >> > >
>> >> >> > >
>> >> >> > > >
>> >> >> > > >>> If every packet would have a different metadata length, it seems like
>> >> >> > > >>> a nightmare to parse?
>> >> >> > > >>>
>> >> >> > > >>
>> >> >> > > >> No parsing is really needed. We can simply use BTF IDs and type cast in
>> >> >> > > >> BPF-prog. Both BPF-prog and userspace have access to the local BTF ids,
>> >> >> > > >> see [1] and [2].
>> >> >> > > >>
>> >> >> > > >> It seems we are talking slightly past each-other(?). Let me rephrase
>> >> >> > > >> and reframe the question, what is your *plan* for dealing with different
>> >> >> > > >> *types* of TX metadata. The different struct *types* will of-cause have
>> >> >> > > >> different sizes, but that is okay as long as they fit into the maximum
>> >> >> > > >> size set by this new socket option XDP_TX_METADATA_LEN.
>> >> >> > > >> Thus, in principle I'm fine with XSK having configured a fixed headroom
>> >> >> > > >> for metadata, but we need a plan for handling more than one type and
>> >> >> > > >> perhaps a xsk desc indicator/flag for knowing TX metadata isn't random
>> >> >> > > >> data ("leftover" since last time this mem was used).
>> >> >> > > >
>> >> >> > > > Yeah, I think the above correctly catches my expectation here. Some
>> >> >> > > > headroom is reserved via XDP_TX_METADATA_LEN and the flexibility is
>> >> >> > > > offloaded to the bpf program via btf_id/tlv/etc.
>> >> >> > > >
>> >> >> > > > Regarding leftover metadata: can we assume the userspace will take
>> >> >> > > > care of setting it up?
>> >> >> > > >
>> >> >> > > >> With this kfunc approach, then things in-principle, becomes a contract
>> >> >> > > >> between the "local" TX-hook BPF-prog and AF_XDP userspace. These two
>> >> >> > > >> components can as illustrated here [1]+[2] can coordinate based on local
>> >> >> > > >> BPF-prog BTF IDs. This approach works as-is today, but patchset
>> >> >> > > >> selftests examples don't use this and instead have a very static
>> >> >> > > >> approach (that people will copy-paste).
>> >> >> > > >>
>> >> >> > > >> An unsolved problem with TX-hook is that it can also get packets from
>> >> >> > > >> XDP_REDIRECT and even normal SKBs gets processed (right?). How does the
>> >> >> > > >> BPF-prog know if metadata is valid and intended to be used for e.g.
>> >> >> > > >> requesting the timestamp? (imagine metadata size happen to match)
>> >> >> > > >
>> >> >> > > > My assumption was the bpf program can do ifindex/netns filtering. Plus
>> >> >> > > > maybe check that the meta_len is the one that's expected.
>> >> >> > > > Will that be enough to handle XDP_REDIRECT?
>> >> >> > >
>> >> >> > > I don't think so, using the meta_len (+ ifindex/netns) to communicate
>> >> >> > > activation of TX hardware hints is too weak and not enough. This is an
>> >> >> > > implicit API for BPF-programmers to understand and can lead to implicit
>> >> >> > > activation.
>> >> >> > >
>> >> >> > > Think about what will happen for your AF_XDP send use-case. For
>> >> >> > > performance reasons AF_XDP don't zero out frame memory. Thus, meta_len
>> >> >> > > is fixed even if not used (and can contain garbage), it can by accident
>> >> >> > > create hard-to-debug situations. As discussed with Magnus+Maryam
>> >> >> > > before, we found it was practical (and faster than mem zero) to extend
>> >> >> > > AF_XDP descriptor (see struct xdp_desc) with some flags to
>> >> >> > > indicate/communicate this frame comes with TX metadata hints.
>> >> >> >
>> >> >> > What is that "if not used" situation? Can the metadata itself have
>> >> >> > is_used bit? The userspace has to initialize at least that bit.
>> >> >> > We can definitely add that extra "has_metadata" bit to the descriptor,
>> >> >> > but I'm trying to understand whether we can do without it.
>> >> >>
>> >> >> To me, this "has_metadata" bit in the descriptor is just an
>> >> >> optimization. If it is 0, then there is no need to go and check the
>> >> >> metadata field and you save some performance. Regardless of this bit,
>> >> >> you need some way to say "is_used" for each metadata entry (at least
>> >> >> when the number of metadata entries is >1). Three options come to mind
>> >> >> each with their pros and cons.
>> >> >>
>> >> >> #1: Let each metadata entry have an invalid state. Not possible for
>> >> >> every metadata and requires the user/kernel to go scan through every
>> >> >> entry for every packet.
>> >> >>
>> >> >> #2: Have a field of bits at the start of the metadata section (closest
>> >> >> to packet data) that signifies if a metadata entry is valid or not. If
>> >> >> there are N metadata entries in the metadata area, then N bits in this
>> >> >> field would be used to signify if the corresponding metadata is used
>> >> >> or not. Only requires the user/kernel to scan the valid entries plus
>> >> >> one access for the "is_used" bits.
>> >> >>
>> >> >> #3: Have N bits in the AF_XDP descriptor options field instead of the
>> >> >> N bits in the metadata area of #2. Faster but would consume many
>> >> >> precious bits in the fixed descriptor and cap the number of metadata
>> >> >> entries possible at around 8. E.g., 8 for Rx, 8 for Tx, 1 for the
>> >> >> multi-buffer work, and 15 for some future use. Depends on how daring
>> >> >> we are.
>> >> >>
>> >> >> The "has_metadata" bit suggestion can be combined with 1 or 2.
>> >> >> Approach 3 is just a fine grained extension of the idea itself.
>> >> >>
>> >> >> IMO, the best approach unfortunately depends on the metadata itself.
>> >> >> If it is rarely valid, you want something like the "has_metadata" bit.
>> >> >> If it is nearly always valid and used, approach #1 (if possible for
>> >> >> the metadata) should be the fastest. The decision also depends on the
>> >> >> number of metadata entries you have per packet. Sorry that I do not
>> >> >> have a good answer. My feeling is that we need something like #1 or
>> >> >> #2, or maybe both, then if needed we can add the "has_metadata" bit or
>> >> >> bits (#3) optimization. Can we do this encoding and choice (#1, #2, or
>> >> >> a combo) in the eBPF program itself? Would provide us with the
>> >> >> flexibility, if possible.
>> >> >
>> >> > Here is my take on it, lmk if I'm missing something:
>> >> >
>> >> > af_xdp users call this new setsockopt(XDP_TX_METADATA_LEN) when they
>> >> > plan to use metadata on tx.
>> >> > This essentially requires allocating a fixed headroom to carry the metadata.
>> >> > af_xdp machinery exports this fixed len into the bpf programs somehow
>> >> > (devtx_frame.meta_len in this series).
>> >> > Then it's up to the userspace and bpf program to agree on the layout.
>> >> > If not every packet is expected to carry the metadata, there might be
>> >> > some bitmask in the metadata area to indicate that.
>> >> >
>> >> > Iow, the metadata isn't interpreted by the kernel. It's up to the prog
>> >> > to interpret it and call appropriate kfunc to enable some offload.
>> >>
>> >> The reason for the flag on RX is mostly performance: there's a
>> >> substantial performance hit from reading the metadata area because it's
>> >> not cache-hot; we want to avoid that when no metadata is in use. Putting
>> >> the flag inside the metadata area itself doesn't work for this, because
>> >> then you incur the cache miss just to read the flag.
>> >
>> > Not necessarily. Let us say that the flag is 4 bytes. Increase the
>> > start address of the packet buffer with 4 and the flags field will be
>> > on the same cache line as the first 60 bytes of the packet data
>> > (assuming a 64 byte cache line size and the flags field is closest to
>> > the start of the packet data). As long as you write something in those
>> > first 60 bytes of packet data that cache line will be brought in and
>> > will likely be in the cache when you access the bits in the metadata
>> > field. The trick works similarly for Rx by setting the umem headroom
>> > accordingly.
>>
>> Yeah, a trick like that was what I was alluding to with the "could" in
>> this bit:
>>
>> >> but I see no reason it could not also occur on TX (it'll mostly
>> >> depend on data alignment I guess?).
>>
>> right below the text you quoted ;)
>>
>> > But you are correct in that dedicating a bit in the descriptor will
>> > make sure it is always hot, while the trick above is dependent on the
>> > app wanting to read or write the first cache line worth of packet
>> > data.
>>
>> Exactly; which is why I think it's worth the flag bit :)
>
> Ack. Let me add this to the list of things to follow up on. I'm
> assuming it's fair to start without the flag and add it later as a
> performance optimization?
> We have a fair bit of core things we need to agree on first :-D
Certainly no objection as long as we are doing RFC patches, but I think
we should probably add this before merging something; no reason to
change API more than we have to :)
-Toke
