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@xxxxxxxxx> > > --- > [...] > > > 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; > > +} > > + >
