Re: [PATCH RESEND bpf-next 09/15] xdp: Add VLAN tag hint

Larysa Zaremba
2023-05-15T16:12:54Z

On Mon, May 15, 2023 at 05:36:12PM +0200, Jesper Dangaard Brouer wrote:
>
>
> On 12/05/2023 17.26, Larysa Zaremba wrote:
> > Implement functionality that enables drivers to expose VLAN tag
> > to XDP code.
> >
> > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> > ---
> [...]
>
> > diff --git a/net/core/xdp.c b/net/core/xdp.c
> > index 41e5ca8643ec..eff21501609f 100644
> > --- a/net/core/xdp.c
> > +++ b/net/core/xdp.c
> > @@ -738,6 +738,30 @@ __bpf_kfunc int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash,
> > return -EOPNOTSUPP;
> > }
>
> Remember below becomes part of main documentation on HW metadata hints:
> - https://kernel.org/doc/html/latest/networking/xdp-rx-metadata.html
>
> Hint compiling locally I use:
> make SPHINXDIRS="networking" htmldocs
>
> > +/**
> > + * bpf_xdp_metadata_rx_ctag - Read XDP packet inner vlan tag.
>
> Is bpf_xdp_metadata_rx_ctag a good function name for the inner vlan tag?
> Like wise below "stag".
>
> I cannot remember if the C-tag or S-tag is the inner or outer vlan tag.
>
> When reading BPF code that use these function names, then I would have
> to ask Google for help, or find-and-read this doc.
>
> Can we come-up with a more intuitive name, that e.g. helps when reading
> the BPF-prog code?

Well, my reasoning for such naming is that if someone can configure s-tag
stripping in ethtool with 'rx-vlan-stag-hw-parse', they shouldn't have any
problem with understanding those function names.

One possible improvement that comes to mind is maybe (similarly ethtool) calling
c-tag just 'tag' and letting s-tag stay 'stag'. Because c-tag is this default
802.1q tag, which is supported by various hardware, while s-tag is significantly
less widespread.

But there are many options, really.

What are your suggestions?

>
> > + * @ctx: XDP context pointer.
> > + * @vlan_tag: Return value pointer.
> > + *
>
> IMHO right here, there should be a description.
>
> E.g. for what a VLAN "tag" means. I assume a "tag" isn't the VLAN id,
> but the raw VLAN tag that also contains the prio numbers etc.
>
> It this VLAN tag expected to be in network-byte-order ?
> IMHO this doc should define what is expected (and driver devel must
> follow this).

Will specify that.

>
> > + * Returns 0 on success or ``-errno`` on error.
> > + */
> > +__bpf_kfunc int bpf_xdp_metadata_rx_ctag(const struct xdp_md *ctx, u16 *vlan_tag)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +/**
> > + * bpf_xdp_metadata_rx_stag - Read XDP packet outer vlan tag.
> > + * @ctx: XDP context pointer.
> > + * @vlan_tag: Return value pointer.
> > + *
> > + * Returns 0 on success or ``-errno`` on error.
>
> IMHO we should provide more guidance to expected return codes, and what
> they mean. IMHO driver developers must only return codes that are
> described here, and if they invent a new, add it as part of their patch.

That's a good suggestion, I will expand the comment to describe error codes used
so far.

>
> See, formatting in bpf_xdp_metadata_rx_hash and check how this gets
> compiled into HTML.
>
>
> > + */
> > +__bpf_kfunc int bpf_xdp_metadata_rx_stag(const struct xdp_md *ctx, u16 *vlan_tag)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +
>