Date:   Fri, 9 Sep 2022 14:44:37 +0200
From:   Magnus Karlsson <magnus.karlsson@...il.com>
To:     Jesper Dangaard Brouer <jbrouer@...hat.com>
Cc:     brouer@...hat.com, Maryam Tahhan <mtahhan@...hat.com>,
        bpf@...r.kernel.org, netdev@...r.kernel.org,
        xdp-hints@...-project.net, larysa.zaremba@...el.com,
        memxor@...il.com, Lorenzo Bianconi <lorenzo@...nel.org>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Daniel Borkmann <borkmann@...earbox.net>,
        Andrii Nakryiko <andrii.nakryiko@...il.com>,
        dave@...cker.co.uk, Magnus Karlsson <magnus.karlsson@...el.com>,
        bjorn@...nel.org, Alexander Lobakin <alexandr.lobakin@...el.com>
Subject: Re: [xdp-hints] Re: [PATCH RFCv2 bpf-next 17/18] xsk: AF_XDP
 xdp-hints support in desc options

On Fri, Sep 9, 2022 at 2:35 PM Jesper Dangaard Brouer
<jbrouer@...hat.com> wrote:
>
>
>
> On 09/09/2022 12.14, Magnus Karlsson wrote:
> > On Fri, Sep 9, 2022 at 11:42 AM Jesper Dangaard Brouer
> > <jbrouer@...hat.com> wrote:
> >>
> >>
> >> On 09/09/2022 10.12, Maryam Tahhan wrote:
> >>> <snip>
> >>>>>>>
> >>>>>>> * Instead encode this information into each metadata entry in the
> >>>>>>> metadata area, in some way so that a flags field is not needed (-1
> >>>>>>> signifies not valid, or whatever happens to make sense). This has the
> >>>>>>> drawback that the user might have to look at a large number of entries
> >>>>>>> just to find out there is nothing valid to read. To alleviate this, it
> >>>>>>> could be combined with the next suggestion.
> >>>>>>>
> >>>>>>> * Dedicate one bit in the options field to indicate that there is at
> >>>>>>> least one valid metadata entry in the metadata area. This could be
> >>>>>>> combined with the two approaches above. However, depending on what
> >>>>>>> metadata you have enabled, this bit might be pointless. If some
> >>>>>>> metadata is always valid, then it serves no purpose. But it might if
> >>>>>>> all enabled metadata is rarely valid, e.g., if you get an Rx timestamp
> >>>>>>> on one packet out of one thousand.
> >>>>>>>
> >>>>>
> >>>>> I like this option better! Except that I have hoped to get 2 bits ;-)
> >>>>
> >>>> I will give you two if you need it Jesper, no problem :-).
> >>>>
> >>>
> >>> Ok I will look at implementing and testing this and post an update.
> >>
> >> Perfect if you Maryam have cycles to work on this.
> >>
> >> Let me explain what I wanted the 2nd bit for.  I simply wanted to also
> >> transfer the XDP_FLAGS_HINTS_COMPAT_COMMON flag.  One could argue that
> >> is it redundant information as userspace AF_XDP will have to BTF decode
> >> all the know XDP-hints. Thus, it could know if a BTF type ID is
> >> compatible with the common struct.   This problem is performance as my
> >> userspace AF_XDP code will have to do more code (switch/jump-table or
> >> table lookup) to map IDs to common compat (to e.g. extract the RX-csum
> >> indication).  Getting this extra "common-compat" bit is actually a
> >> micro-optimization.  It is up to AF_XDP maintainers if they can spare
> >> this bit.
> >>
> >>
> >>> Thanks folks
> >>>
> >>>>> The performance advantage is that the AF_XDP descriptor bits will
> >>>>> already be cache-hot, and if it indicates no-metadata-hints the AF_XDP
> >>>>> application can avoid reading the metadata cache-line :-).
> >>>>
> >>>> Agreed. I prefer if we can keep it simple and fast like this.
> >>>>
> >>
> >> Great, lets proceed this way then.
> >>
> >>> <snip>
> >>>
> >>
> >> Thinking ahead: We will likely need 3 bits.
> >>
> >> The idea is that for TX-side, we set a bit indicating that AF_XDP have
> >> provided a valid XDP-hints layout (incl corresponding BTF ID). (I would
> >> overload and reuse "common-compat" bit if TX gets a common struct).
> >
> > I think we should reuse the "Rx metadata valid" flag for this since
> > this will not be used in the Tx case by definition. In the Tx case,
> > this bit would instead mean that the user has provided a valid
> > XDP-hints layout. It has a nice symmetry, on Rx it is set by the
> > kernel when it has put something relevant in the metadata area. On Tx,
> > it is set by user-space if it has put something relevant in the
> > metadata area.
>
> I generally like reusing the bit, *BUT* there is the problem of
> (existing) applications ignoring the desc-options bit and forwarding
> packets.  This would cause the "Rx metadata valid" flag to be seen as
> userspace having set the "TX-hints-bit" and kernel would use what is
> provided in metadata area (leftovers from RX-hints).  IMHO that will be
> hard to debug for end-users and likely break existing applications.

Good point. I buy this. We need separate Rx and Tx bits.

> > We can also reuse this bit when we get a notification
> > in the completion queue to indicate if the kernel has produced some
> > metadata on tx completions. This could be a Tx timestamp for example.
> >
>
> Big YES, reuse "Rx metadata valid" bit when we get a TX notification in
> completion queue.  This will be okay because it cannot be forgotten and
> misinterpreted as the kernel will have responsibility to update this bit.
>
> > So hopefully we could live with only two bits :-).
> >
>
> I still think we need three bits ;-)
> That should be enough to cover the 6 states:
>   - RX hints
>   - RX hints and compat
>   - TX hints
>   - TX hints and compat
>   - TX completion
>   - TX completion and compat
>
>
> >> But lets land RX-side first, but make sure we can easily extend for the
> >> TX-side.
>

Powered by blists - more mailing lists