Message-ID: <affeb1e3-69e6-9783-0012-6d917972ba30@redhat.com>
Date:   Mon, 16 Jan 2023 14:09:48 +0100
From:   Jesper Dangaard Brouer <jbrouer@...hat.com>
To:     Stanislav Fomichev <sdf@...gle.com>, bpf@...r.kernel.org
Cc:     brouer@...hat.com, 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,
        haoluo@...gle.com, jolsa@...nel.org,
        David Ahern <dsahern@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Willem de Bruijn <willemb@...gle.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, David Vernet <void@...ifault.com>
Subject: Re: [PATCH bpf-next v7 01/17] bpf: Document XDP RX metadata



On 12/01/2023 01.32, Stanislav Fomichev wrote:
> Document all current use-cases and assumptions.
> 
> 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
> Acked-by: David Vernet <void@...ifault.com>
> Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> ---
>   Documentation/networking/index.rst           |   1 +
>   Documentation/networking/xdp-rx-metadata.rst | 108 +++++++++++++++++++
>   2 files changed, 109 insertions(+)
>   create mode 100644 Documentation/networking/xdp-rx-metadata.rst
> 
> diff --git a/Documentation/networking/index.rst b/Documentation/networking/index.rst
> index 4f2d1f682a18..4ddcae33c336 100644
> --- a/Documentation/networking/index.rst
> +++ b/Documentation/networking/index.rst
> @@ -120,6 +120,7 @@ Refer to :ref:`netdev-FAQ` for a guide on netdev development process specifics.
>      xfrm_proc
>      xfrm_sync
>      xfrm_sysctl
> +   xdp-rx-metadata
>   
>   .. only::  subproject and html
>   
> diff --git a/Documentation/networking/xdp-rx-metadata.rst b/Documentation/networking/xdp-rx-metadata.rst
> new file mode 100644
> index 000000000000..b6c8c77937c4
> --- /dev/null
> +++ b/Documentation/networking/xdp-rx-metadata.rst
> @@ -0,0 +1,108 @@
> +===============
> +XDP RX Metadata
> +===============
> +
> +This document describes how an eXpress Data Path (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 in an XDP frame.
> +Every device driver that wishes to expose additional packet metadata can
> +implement 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:
> +
> +.. kernel-doc:: net/core/xdp.c
> +   :identifiers: bpf_xdp_metadata_rx_timestamp bpf_xdp_metadata_rx_hash
> +
> +An 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.
> +
> +Not all kfuncs have to be implemented by the device driver; when not
> +implemented, the default ones that return ``-EOPNOTSUPP`` will be used.
> +
> +Within an XDP frame, the metadata layout is as follows::

Below diagram describes XDP buff (xdp_buff), but text says 'XDP frame'.
So XDP frame isn't referring literally to xdp_frame, which I find 
slightly confusing.
It is likely because I think too much about the code and the different 
objects, xdp_frame, xdp_buff, xdp_md (xdp ctx seen be bpf-prog).

I tried to grep in the (recent added) bpf/xdp docs to see if there is a
definition of a XDP "packet" or "frame".  Nothing popped up, except that
Documentation/bpf/map_cpumap.rst talks about raw ``xdp_frame`` objects.

Perhaps we can improve this doc by calling out xdp_buff here, like:

  Within an XDP frame, the metadata layout (accessed via ``xdp_buff``) 
is as follows::

> +
> +  +----------+-----------------+------+
> +  | headroom | custom metadata | data |
> +  +----------+-----------------+------+
> +             ^                 ^
> +             |                 |
> +   xdp_buff->data_meta   xdp_buff->data
> +
> +An 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
> +======
> +
> +:doc:`af_xdp` use-case implies that there is a contract between the BPF
> +program that redirects XDP frames into the ``AF_XDP`` socket (``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. The userspace ``XSK`` consumer computes
> +``xsk_umem__get_data() - METADATA_SIZE`` to locate that metadata.
> +Note, ``xsk_umem__get_data`` is defined in ``libxdp`` and
> +``METADATA_SIZE`` is an application-specific constant.

The main problem with AF_XDP and metadata is that, the AF_XDP descriptor
doesn't contain any info about the length METADATA_SIZE.

The text does says this, but in a very convoluted way.
I think this challenge should be more clearly spelled out.

(p.s. This was something that XDP-hints via BTF have a proposed solution 
for)

> +
> +Here is the ``AF_XDP`` consumer layout (note missing ``data_meta`` pointer)::

The "note" also hint to this issue.

> +
> +  +----------+-----------------+------+
> +  | 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 the ``skb`` out of the ``xdp_buff``
> +contents. Currently, every driver has custom kernel code to parse
> +the descriptors and populate ``skb`` metadata when doing this ``xdp_buff->skb``
> +conversion, 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 an XDP program
> +can override some of the metadata used for building ``skbs``.

Happy this is mentioned as future work.

> +
> +bpf_redirect_map
> +================
> +
> +``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.
> +
> +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``. If such a packet is later redirected into an ``XSK``,
> +that will also only have access to the custom metadata.
> +

Good that this is documented, but I hope we can fix/improve this as
future work.

> +bpf_tail_call
> +=============
> +
> +Adding programs that access metadata kfuncs to the ``BPF_MAP_TYPE_PROG_ARRAY``
> +is currently not supported.
> +
> +Example
> +=======
> +
> +See ``tools/testing/selftests/bpf/progs/xdp_metadata.c`` and
> +``tools/testing/selftests/bpf/prog_tests/xdp_metadata.c`` for an example of
> +BPF program that handles XDP metadata.


--Jesper

Powered by blists - more mailing lists