On 09/09/2022 12.14, Magnus Karlsson wrote:
On Fri, Sep 9, 2022 at 11:42 AM Jesper Dangaard Brouer
<jbrouer@xxxxxxxxxx> 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.
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.
