Messages in this thread | | | From | Jesper Dangaard Brouer <> | Date | Mon, 15 May 2023 17:36:12 +0200 | Subject | Re: [PATCH RESEND bpf-next 09/15] xdp: Add VLAN tag hint |
| |
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?
> + * @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).
> + * 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.
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; > +} > +
| |