Re: [xdp-hints] Re: [PATCH bpf-next v4 01/15] bpf: Document XDP RX metadata

Stanislav Fomichev [off-list ref] writes:

On Wed, Dec 14, 2022 at 3:46 PM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted

Stanislav Fomichev [off-list ref] writes:

quoted

On Wed, Dec 14, 2022 at 2:34 AM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted

Stanislav Fomichev [off-list ref] writes:

quoted

On Tue, Dec 13, 2022 at 8:37 AM David Vernet [off-list ref] wrote:

quoted

On Mon, Dec 12, 2022 at 06:35:51PM -0800, Stanislav Fomichev wrote:

quoted

Document all current use-cases and assumptions.

Cc: John Fastabend <john.fastabend@gmail.com>
Cc: David Ahern <redacted>
Cc: Martin KaFai Lau <martin.lau@linux.dev>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Willem de Bruijn <willemb@google.com>
Cc: Jesper Dangaard Brouer <redacted>
Cc: Anatoly Burakov <redacted>
Cc: Alexander Lobakin <redacted>
Cc: Magnus Karlsson <redacted>
Cc: Maryam Tahhan <redacted>
Cc: xdp-hints@xdp-project.net
Cc: netdev@vger.kernel.org
Signed-off-by: Stanislav Fomichev <redacted>
---
 Documentation/bpf/xdp-rx-metadata.rst | 90 +++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)
 create mode 100644 Documentation/bpf/xdp-rx-metadata.rst

diff --git a/Documentation/bpf/xdp-rx-metadata.rst b/Documentation/bpf/xdp-rx-metadata.rst
new file mode 100644
index 000000000000..498eae718275
--- /dev/null
+++ b/Documentation/bpf/xdp-rx-metadata.rst

I think you need to add this to Documentation/bpf/index.rst. Or even
better, maybe it's time to add an xdp/ subdirectory and put all docs
there? Don't want to block your patchset from bikeshedding on this
point, so for now it's fine to just put it in
Documentation/bpf/index.rst until we figure that out.

Maybe let's put it under Documentation/networking/xdp-rx-metadata.rst
and reference form Documentation/networking/index.rst? Since it's more
relevant to networking than the core bpf?

quoted

quoted

@@ -0,0 +1,90 @@
+===============
+XDP RX Metadata
+===============
+
+XDP programs support creating and passing custom metadata via
+``bpf_xdp_adjust_meta``. This metadata can be consumed by the following
+entities:

Can you add a couple of sentences to this intro section that explains
what metadata is at a high level?

I'm gonna copy-paste here what I'm adding, feel free to reply back if
still unclear. (so we don't have to wait another week to discuss the
changes)

XDP programs support creating and passing custom metadata via
``bpf_xdp_adjust_meta``. The metadata can contain some extra information
about the packet: timestamps, hash, vlan and tunneling information, etc.
This metadata can be consumed by the following entities:

This is not really accurate, though? The metadata area itself can
contain whatever the XDP program wants it to, and I think you're
conflating the "old" usage for arbitrary storage with the driver-kfunc
metadata support.

I think we should clear separate the two: the metadata area is just a
place to store data (and is not consumed by the stack, except that
TC-BPF programs can access it), and the driver kfuncs are just a general
way to get data out of the drivers (and has nothing to do with the
metadata area, you can just get the data into stack variables).

While it would be good to have a documentation of the general metadata
area stuff somewhere, I don't think it necessarily have to be part of
this series, so maybe just stick to documenting the kfuncs?

Maybe I can reword to something like below?

The metadata can be used to store some extra information about the
packet timestamps, hash, vlan and tunneling information, etc.

This way we are not actually defining what it is, but hinting about
how it's commonly used?

Sent another reply to the original patch with some comments that are
hopefully a bit more constructive :)

Thanks, everything makes sense, will incorporate. I'll also try to
share the patches privately with you sometime tomorrow maybe; not
super comfortable sending them out with a bunch of changes on top of
your authorship without some kind of ack from you :-)

OK, sounds good. Tomorrow (Friday) is my last day before the holidays,
but happy to look things over before I sign off :)

-Toke