[xdp-hints] Re: [RFC bpf-next 09/23] xdp: Add VLAN tag hint

[Larysa Zaremba <larysa.zaremba@intel.com>]

On Thu, Sep 14, 2023 at 06:18:40PM +0200, Alexander Lobakin wrote:
> From: Larysa Zaremba <larysa.zaremba@intel.com>
> Date: Thu, 24 Aug 2023 21:26:48 +0200
> 
> > Implement functionality that enables drivers to expose VLAN tag
> > to XDP code.
> 
> I'd leave a couple more words here. Mention that it exports both tag and
> protocol, for example. That TCI is host-Endian and proto is BE (just
> like how skb stores them and it's fine).
>

OK

> > 
> > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> > ---
> >  Documentation/networking/xdp-rx-metadata.rst |  8 ++++-
> >  include/net/xdp.h                            |  4 +++
> >  kernel/bpf/offload.c                         |  2 ++
> >  net/core/xdp.c                               | 34 ++++++++++++++++++++
> >  4 files changed, 47 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/networking/xdp-rx-metadata.rst b/Documentation/networking/xdp-rx-metadata.rst
> > index 25ce72af81c2..ea6dd79a21d3 100644
> > --- a/Documentation/networking/xdp-rx-metadata.rst
> > +++ b/Documentation/networking/xdp-rx-metadata.rst
> > @@ -18,7 +18,13 @@ 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
> > +   :identifiers: bpf_xdp_metadata_rx_timestamp
> > +
> > +.. kernel-doc:: net/core/xdp.c
> > +   :identifiers: bpf_xdp_metadata_rx_hash
> > +
> > +.. kernel-doc:: net/core/xdp.c
> > +   :identifiers: bpf_xdp_metadata_rx_vlan_tag
> >  
> >  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
> > diff --git a/include/net/xdp.h b/include/net/xdp.h
> > index 1e9870d5f025..8bb64fc76498 100644
> > --- a/include/net/xdp.h
> > +++ b/include/net/xdp.h
> > @@ -388,6 +388,8 @@ void xdp_attachment_setup(struct xdp_attachment_info *info,
> >  			   bpf_xdp_metadata_rx_timestamp) \
> >  	XDP_METADATA_KFUNC(XDP_METADATA_KFUNC_RX_HASH, \
> >  			   bpf_xdp_metadata_rx_hash) \
> > +	XDP_METADATA_KFUNC(XDP_METADATA_KFUNC_RX_VLAN_TAG, \
> > +			   bpf_xdp_metadata_rx_vlan_tag) \
> >  
> >  enum {
> >  #define XDP_METADATA_KFUNC(name, _) name,
> > @@ -449,6 +451,8 @@ struct xdp_metadata_ops {
> >  	int	(*xmo_rx_timestamp)(const struct xdp_md *ctx, u64 *timestamp);
> >  	int	(*xmo_rx_hash)(const struct xdp_md *ctx, u32 *hash,
> >  			       enum xdp_rss_hash_type *rss_type);
> > +	int	(*xmo_rx_vlan_tag)(const struct xdp_md *ctx, u16 *vlan_tci,
> > +				   __be16 *vlan_proto);
> 
> Was "TCI first, proto second" aligned with something or I can ask "why
> not proto first, TCI second"?

No particular reason. Now I have looked it up and this is the other way in all 
places >_<. I do probably need to switch this. Time to put my regular expressions 
skills to the test.

> 
> >  };
> >  
> >  #ifdef CONFIG_NET
> 
> [...]
> 
> Thanks,
> Olek