Message-ID: <Y7WjKNv10wt0GZ6y@maniforge.lan>
Date:   Wed, 4 Jan 2023 10:02:48 -0600
From:   David Vernet <void@...ifault.com>
To:     Stanislav Fomichev <sdf@...gle.com>
Cc:     bpf@...r.kernel.org, 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>,
        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: [PATCH bpf-next v5 01/17] bpf: Document XDP RX metadata

On Tue, Jan 03, 2023 at 02:23:03PM -0800, Stanislav Fomichev wrote:
> On Wed, Dec 28, 2022 at 9:25 AM David Vernet <void@...ifault.com> wrote:
> >
> > On Tue, Dec 20, 2022 at 02:20:27PM -0800, 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
> > > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > > ---
> > >  Documentation/networking/index.rst           |   1 +
> > >  Documentation/networking/xdp-rx-metadata.rst | 107 +++++++++++++++++++
> > >  2 files changed, 108 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..37e8192d9b60
> > > --- /dev/null
> > > +++ b/Documentation/networking/xdp-rx-metadata.rst
> >
> > Hey Stanislav,
> >
> > This is looking excellent. Left a few more minor comments and
> > suggestions.
> >
> > > @@ -0,0 +1,107 @@
> > > +===============
> > > +XDP RX Metadata
> > > +===============
> > > +
> > > +This document describes how an XDP program can access hardware metadata
> >
> > In similar fashion to LWN articles, can we spell out what XDP means the
> > first time it's used, e.g.:
> >
> > ...describes how an eXpress Data Path (XDP) program...
> >
> > In general this applies to other acronyms unless they're super obvious,
> > like "CPU" (thanks for already having done it for XSK).
> 
> Sure. Hopefully no need to explain RX below? Don't see anything else..
> LMK if I missed something

Yeah, I think we can forego RX.

> 
> > > +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:
> > > +
> > > +- ``bpf_xdp_metadata_rx_timestamp`` returns a packet's RX timestamp
> > > +- ``bpf_xdp_metadata_rx_hash`` returns a packet's RX hash
> >
> > So, I leave this up to you as to whether or not you want to do this, but
> > there is a built-in mechanism in sphinx that converts doxygen comments
> > to rendered documentation for a function, struct, etc, and also
> > automatically links other places in documentation where the function is
> > referenced. See [0] for an example of this in code, and [1] for an
> > example of how it's rendered.
> >
> > [0]: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/Documentation/bpf/kfuncs.rst#n239
> > [1]: https://docs.kernel.org/bpf/kfuncs.html#c.bpf_task_acquire
> >
> > So you would do something like add function headers to the kfuncs, and
> > then do:
> >
> > .. kernel-doc:: net/core/xdp.c
> >    :identifiers: bpf_xdp_metadata_rx_timestamp bpf_xdp_metadata_rx_hash
> >
> > At some point we will need a consistent story for how we document
> > kfuncs. That's not set in stone yet, which is why I'm saying it's up to
> > you whether or not you want to do this or just leave it as teletype with
> > a written description next to it.  Later on when we settle on a
> > documentation story for kfuncs, we can update all of them to be
> > documented in the same way.
> 
> Let me try and see how it looks in the html doc. I like the idea of
> referencing the code directly, hopefully less chance it goes stale.

Sounds good!

[...]

Thanks for making all these changes.

- David

Powered by blists - more mailing lists