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

Larysa Zaremba
2023-05-22T15:51:57Z

On Mon, May 22, 2023 at 10:37:33AM +0200, Jesper Dangaard Brouer wrote:
>
>
> On 15/05/2023 18.09, Larysa Zaremba wrote:
> > 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.
> >
>
> Naming is hard. My perspective is conveying the meaning without having
> to be knowledgeable about ethtool VLAN commands. My perspective is a
> casual BPF-programmer that reads "bpf_xdp_metadata_rx_stag()".
> Hopefully we can choose a name that says "vlan" somewhere, such that the
> person reading this doesn't have to lookup and find the documentation to
> deduct this code is related to VLANs.
>
> > 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?
> >
>
> One suggestion is (the symmetrical):
> * bpf_xdp_metadata_rx_vlan_inner_tag
> * bpf_xdp_metadata_rx_vlan_outer_tag
>
> As you say above the first "inner" VLAN tag is just the regular 802.1Q
> VLAN tag. The concept of C-tag and S-tag is from 802.1ad that
> introduced the concept of double tagging.
>
> Thus one could argue for shorter names like:
> * bpf_xdp_metadata_rx_vlan_tag
> * bpf_xdp_metadata_rx_vlan_outer_tag
>

AFAIK, outer tag is a broader term, it's pretty often used for stacked 802.1Q
headers. I can't find what exactly is an expected behavior for rxvlan and
rx-vlan-stag-hw-parse in ethtool, but iavf documentation states that rxvlan
"enables outer or single 802.1Q VLAN stripping" and rx-vlan-stag-hw-parse
"enables outer or single 802.1ad VLAN stripping". This is in consistent with how
ice hardware behaves. More credible sources would be welcome.

What about:
* bpf_xdp_metadata_rx_vlan_tag
* bpf_xdp_metadata_rx_vlan_qinq_tag

>
> > >
> > > > + * @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.
> > > > + *
>
> (p.s. Googling I find multiple definitions of what the "S" in S-tag
> means. The most reliable or statistically consistent seems to be
> "Service tag", or "Service provider tag".)
>
> The description for the renamed "bpf_xdp_metadata_rx_vlan_outer_tag"
> should IMHO explain that the outer VLAN tag is often refered to as the S-tag
> (or Service-tag) in Q-in-Q (802.1ad) terminology. Perhaps we can even spell
> out that some hardware support (and must be configured via ethtool) to
> extract this stag.
>
> A dump of the tool rx-vlan related commands:
>
> $ ethtool -k i40e2 | grep rx-vlan
> rx-vlan-offload: on
> rx-vlan-filter: on [fixed]
> rx-vlan-stag-hw-parse: off [fixed]
> rx-vlan-stag-filter: off [fixed]
>
>
>
>
> > > > + * 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;
> > > > +}
> > > > +
> > >
> >
>