Messages in this thread | | | Date | Mon, 15 May 2023 18:09:55 +0200 | From | Larysa Zaremba <> | Subject | Re: [PATCH RESEND bpf-next 09/15] xdp: Add VLAN tag hint |
| |
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; > > +} > > + >
| |