Message-ID: <87tu1xeeh1.fsf@toke.dk>
Date:   Thu, 15 Dec 2022 00:46:02 +0100
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Stanislav Fomichev <sdf@...gle.com>, bpf@...r.kernel.org
Cc:     ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
        martin.lau@...ux.dev, song@...nel.org, yhs@...com,
        john.fastabend@...il.com, kpsingh@...nel.org, sdf@...gle.com,
        haoluo@...gle.com, jolsa@...nel.org,
        David Ahern <dsahern@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Willem de Bruijn <willemb@...gle.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Anatoly Burakov <anatoly.burakov@...el.com>,
        Alexander Lobakin <alexandr.lobakin@...el.com>,
        Magnus Karlsson <magnus.karlsson@...il.com>,
        Maryam Tahhan <mtahhan@...hat.com>, xdp-hints@...-project.net,
        netdev@...r.kernel.org
Subject: Re: [xdp-hints] [PATCH bpf-next v4 01/15] bpf: Document XDP RX
 metadata

Stanislav Fomichev <sdf@...gle.com> writes:

> Document all current use-cases and assumptions.

Below is a set of slightly more constructive suggestions for how to edit
this so it's not confusing the metadata area description with the kfunc
list:

> Cc: John Fastabend <john.fastabend@...il.com>
> Cc: David Ahern <dsahern@...il.com>
> Cc: Martin KaFai Lau <martin.lau@...ux.dev>
> Cc: Jakub Kicinski <kuba@...nel.org>
> Cc: Willem de Bruijn <willemb@...gle.com>
> Cc: Jesper Dangaard Brouer <brouer@...hat.com>
> Cc: Anatoly Burakov <anatoly.burakov@...el.com>
> Cc: Alexander Lobakin <alexandr.lobakin@...el.com>
> Cc: Magnus Karlsson <magnus.karlsson@...il.com>
> Cc: Maryam Tahhan <mtahhan@...hat.com>
> Cc: xdp-hints@...-project.net
> Cc: netdev@...r.kernel.org
> Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> ---
>  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
> @@ -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:
> +
> +1. ``AF_XDP`` consumer.
> +2. Kernel core stack via ``XDP_PASS``.
> +3. Another device via ``bpf_redirect_map``.
> +4. Other BPF programs via ``bpf_tail_call``.

I'd replace the above with a short introduction, like:

"This document describes how an XDP program can access hardware metadata
related to a packet using a set of helper functions, and how it can pass
that metadata on to other consumers."

> +General Design
> +==============
> +
> +XDP has access to a set of kfuncs to manipulate the metadata. Every
> +device driver implements these kfuncs. The set of kfuncs is
> +declared in ``include/net/xdp.h`` via ``XDP_METADATA_KFUNC_xxx``.
> +
> +Currently, the following kfuncs are supported. In the future, as more
> +metadata is supported, this set will grow:
> +
> +- ``bpf_xdp_metadata_rx_timestamp_supported`` returns true/false to
> +  indicate whether the device supports RX timestamps
> +- ``bpf_xdp_metadata_rx_timestamp`` returns packet RX timestamp
> +- ``bpf_xdp_metadata_rx_hash_supported`` returns true/false to
> +  indicate whether the device supports RX hash
> +- ``bpf_xdp_metadata_rx_hash`` returns packet RX hash

Keep the above (with David's comments), then add a bit of extra text,
here:

"The XDP program can use these kfuncs to read the metadata into stack
variables for its own consumption. Or, to pass the metadata on to other
consumers, an XDP program can store it into the metadata area carried
ahead of the packet.

> +Within the XDP frame, the metadata layout is as follows::
> +
> +  +----------+-----------------+------+
> +  | headroom | custom metadata | data |
> +  +----------+-----------------+------+
> +             ^                 ^
> +             |                 |
> +   xdp_buff->data_meta   xdp_buff->data

Add:

"The XDP program can store individual metadata items into this data_meta
area in whichever format it chooses. Later consumers of the metadata
will have to agree on the format by some out of band contract (like for
the AF_XDP use case, see below)."

> +AF_XDP
> +======
> +
> +``AF_XDP`` use-case implies that there is a contract between the BPF program
> +that redirects XDP frames into the ``XSK`` and the final consumer.
> +Thus the BPF program manually allocates a fixed number of
> +bytes out of metadata via ``bpf_xdp_adjust_meta`` and calls a subset
> +of kfuncs to populate it. User-space ``XSK`` consumer, looks
> +at ``xsk_umem__get_data() - METADATA_SIZE`` to locate its metadata.
> +
> +Here is the ``AF_XDP`` consumer layout (note missing ``data_meta`` pointer)::
> +
> +  +----------+-----------------+------+
> +  | headroom | custom metadata | data |
> +  +----------+-----------------+------+
> +                               ^
> +                               |
> +                        rx_desc->address
> +
> +XDP_PASS
> +========
> +
> +This is the path where the packets processed by the XDP program are passed
> +into the kernel. The kernel creates ``skb`` out of the ``xdp_buff`` contents.
> +Currently, every driver has a custom kernel code to parse the descriptors and
> +populate ``skb`` metadata when doing this ``xdp_buff->skb`` conversion.

Add: ", and the XDP metadata is not used by the kernel when building
skbs. However, TC-BPF programs can access the XDP metadata area using
the data_meta pointer."

> +In the future, we'd like to support a case where XDP program can override
> +some of that metadata.

s/some of that metadata/some of the metadata used for building skbs/.

> +The plan of record is to make this path similar to ``bpf_redirect_map``
> +so the program can control which metadata is passed to the skb layer.

I'm not sure we are quite agreed on this part, just drop for now (it's
sorta covered by the above)?

> +bpf_redirect_map
> +================
> +
> +``bpf_redirect_map`` can redirect the frame to a different device.
> +In this case we don't know ahead of time whether that final consumer
> +will further redirect to an ``XSK`` or pass it to the kernel via ``XDP_PASS``.
> +Additionally, the final consumer doesn't have access to the original
> +hardware descriptor and can't access any of the original metadata.

Replace this paragraph with: "``bpf_redirect_map`` can redirect the
frame to a different device. Some devices (like virtual ethernet links)
support running a second XDP program after the redirect. However, the
final consumer doesn't have access to the original hardware descriptor
and can't access any of the original metadata. The same applies to XDP
programs installed into devmaps and cpumaps."

> +For this use-case, only custom metadata is currently supported. If
> +the frame is eventually passed to the kernel, the skb created from such
> +a frame won't have any skb metadata. The ``XSK`` consumer will only
> +have access to the custom metadata.

Reword as:

"This means that for redirected packets only custom metadata is
currently supported, which has to be prepared by the initial XDP program
before redirect. If +the frame is eventually passed to the kernel, the
skb created from such a frame won't have any hardware metadata populated
in its skb. And if such a packet is later redirected into an ``XSK``,
that will also only have access to the custom metadata."

> +bpf_tail_call
> +=============
> +
> +No special handling here. Tail-called program operates on the same context
> +as the original one.

Replace this with a statement that it is in fact *not* supported in tail
maps :)

-Toke

Powered by blists - more mailing lists