From: Larysa Zaremba <larysa.zaremba@intel.com>
To: Jesper Dangaard Brouer <jbrouer@redhat.com>
Cc: brouer@redhat.com, bpf@vger.kernel.org,
Stanislav Fomichev <sdf@google.com>,
Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Andrii Nakryiko <andrii@kernel.org>,
Jakub Kicinski <kuba@kernel.org>,
Martin KaFai Lau <martin.lau@linux.dev>,
Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
John Fastabend <john.fastabend@gmail.com>,
KP Singh <kpsingh@kernel.org>, Jiri Olsa <jolsa@kernel.org>,
Jesse Brandeburg <jesse.brandeburg@intel.com>,
Tony Nguyen <anthony.l.nguyen@intel.com>,
Anatoly Burakov <anatoly.burakov@intel.com>,
Alexander Lobakin <alexandr.lobakin@intel.com>,
Magnus Karlsson <magnus.karlsson@gmail.com>,
Maryam Tahhan <mtahhan@redhat.com>,
xdp-hints@xdp-project.net, netdev@vger.kernel.org,
intel-wired-lan@lists.osuosl.org, linux-kernel@vger.kernel.org
Subject: [xdp-hints] Re: [PATCH RESEND bpf-next 09/15] xdp: Add VLAN tag hint
Date: Tue, 23 May 2023 19:35:17 +0200 [thread overview]
Message-ID: <ZGz5VWan/nROHxhc@lincoln> (raw)
In-Reply-To: <1693e3e3-c486-80c8-aec0-cca0c9080c34@redhat.com>
On Tue, May 23, 2023 at 12:16:46PM +0200, Jesper Dangaard Brouer wrote:
>
>
> On 22/05/2023 17.48, Larysa Zaremba wrote:
> > 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.
> >
>
> It would be good to figure out how other hardware behaves.
>
> The iavf doc sounds like very similar behavior from both functions, just
> 802.1Q vs 802.1ad.
> Sounds like both will just pop/strip the outer vlan tag.
> I have seen Ethertype 802.1Q being used (in practice) for double tagged
> packets, even-though 802.1ad should have been used to comply with the
> standard.
>
> > What about:
> > * bpf_xdp_metadata_rx_vlan_tag
> > * bpf_xdp_metadata_rx_vlan_qinq_tag
> >
>
> This sounds good to me.
>
> I do wonder if we really need two functions for this?
> Would one function be enough?
>
> Given the (iavf) description, the functions basically does the same.
> Looking at your ice driver implementation, they could be merged into one
> function, as it is the same location in the descriptor.
>
This design was very debatable in the first place.
I looked at different in-tree driver implementations of NETIF_F_HW_VLAN_STAG_RX
feature once more. Among those I could comprehend, seems like none has c-tag and
s-tag stored separately. Actually, there are 2 situations:
1. (ex. mlx4) HW always strips outer or single VLAN tag, without distinction
between 802.1Q and 802.1ad. TPID in such case is deduced from descriptor.
NETIF_F_HW_VLAN_STAG_RX and NETIF_F_HW_VLAN_CTAG_RX must be enabled together.
2. (ex. ice) HW strips outer or single VLAN tag with a configured TPID. In such
case descriptor doesn't carry info about TPID, because it's the same for all
stripped tags. C-tag and s-tag stripping are mutually exclusive.
Example:
- 802.1Q double VLAN, with s-tag stripping enabled, packet arrives
untouched, with c-tag stripping outermost tag gets stripped.
- 802.1ad+802.1Q, with s-tag stripping enabled, 802.1ad header gets stripped,
with c-tag stripping, packet arrives untouched.
Obviously, I can be sure only about our hardware.
Long story short, probably re-inventing the wheel wasn't a good idea on my part.
Now I am much more inclined to just copy the logic from skb, so function would
look like this:
bpf_xdp_metadata_rx_vlan_tag(const struct xdp_md *ctx, __u16 *vlan_tag,
__u16 *tpid);
Maybe some applications would make use of just:
bpf_xdp_metadata_rx_vlan_tag(const struct xdp_md *ctx, __u16 *vlan_tag);
Both of the above functions would return information about outermost tag, if was
stripped. Would have to think about the naming.
Comments are welcome!
> > >
> > > > >
> > > > > > + * @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]
> > >
> [...]
>
next prev parent reply other threads:[~2023-05-23 17:38 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-12 15:25 [xdp-hints] [PATCH RESEND bpf-next 00/15] new kfunc XDP hints and ice implementation Larysa Zaremba
2023-05-12 15:25 ` [xdp-hints] [PATCH RESEND bpf-next 01/15] ice: make RX hash reading code more reusable Larysa Zaremba
2023-05-19 16:46 ` [xdp-hints] " Alexander Lobakin
2023-05-22 15:03 ` Larysa Zaremba
2023-05-22 15:36 ` Alexander Lobakin
2023-05-12 15:25 ` [xdp-hints] [PATCH RESEND bpf-next 02/15] ice: make RX HW timestamp " Larysa Zaremba
2023-05-19 16:52 ` [xdp-hints] " Alexander Lobakin
2023-05-22 15:07 ` Larysa Zaremba
2023-05-12 15:25 ` [xdp-hints] [PATCH RESEND bpf-next 03/15] ice: make RX checksum checking " Larysa Zaremba
2023-05-22 15:51 ` [xdp-hints] " Alexander Lobakin
2023-05-22 16:05 ` Larysa Zaremba
2023-05-12 15:25 ` [xdp-hints] [PATCH RESEND bpf-next 04/15] ice: Make ptype internal to descriptor info processing Larysa Zaremba
2023-05-12 15:25 ` [xdp-hints] [PATCH RESEND bpf-next 05/15] ice: Introduce ice_xdp_buff Larysa Zaremba
2023-05-22 16:46 ` [xdp-hints] " Alexander Lobakin
2023-05-23 8:02 ` Larysa Zaremba
2023-05-25 11:02 ` Alexander Lobakin
2023-05-12 15:25 ` [xdp-hints] [PATCH RESEND bpf-next 06/15] ice: Support HW timestamp hint Larysa Zaremba
2023-05-12 18:19 ` [xdp-hints] " Stanislav Fomichev
2023-05-16 16:17 ` Jesper Dangaard Brouer
2023-05-12 15:25 ` [xdp-hints] [PATCH RESEND bpf-next 07/15] ice: Support RX hash XDP hint Larysa Zaremba
2023-05-12 18:22 ` [xdp-hints] " Stanislav Fomichev
2023-05-15 13:46 ` Larysa Zaremba
2023-05-12 15:26 ` [xdp-hints] [PATCH RESEND bpf-next 08/15] ice: Support XDP hints in AF_XDP ZC mode Larysa Zaremba
2023-05-12 15:26 ` [xdp-hints] [PATCH RESEND bpf-next 09/15] xdp: Add VLAN tag hint Larysa Zaremba
2023-05-12 18:28 ` [xdp-hints] " Stanislav Fomichev
2023-05-15 15:36 ` Jesper Dangaard Brouer
2023-05-15 16:09 ` Larysa Zaremba
2023-05-22 8:37 ` Jesper Dangaard Brouer
2023-05-22 15:48 ` Larysa Zaremba
2023-05-23 10:16 ` Jesper Dangaard Brouer
2023-05-23 17:35 ` Larysa Zaremba [this message]
2023-05-12 15:26 ` [xdp-hints] [PATCH RESEND bpf-next 10/15] ice: Implement " Larysa Zaremba
2023-05-12 18:31 ` [xdp-hints] " Stanislav Fomichev
2023-05-15 13:41 ` Larysa Zaremba
2023-05-15 15:07 ` Jesper Dangaard Brouer
2023-05-15 15:45 ` Larysa Zaremba
2023-05-12 15:26 ` [xdp-hints] [PATCH RESEND bpf-next 11/15] xdp: Add checksum level hint Larysa Zaremba
2023-05-12 18:34 ` [xdp-hints] " Stanislav Fomichev
2023-05-15 13:49 ` Larysa Zaremba
2023-05-12 15:26 ` [xdp-hints] [PATCH RESEND bpf-next 12/15] ice: Implement " Larysa Zaremba
2023-05-12 15:26 ` [xdp-hints] [PATCH RESEND bpf-next 13/15] selftests/bpf: Allow VLAN packets in xdp_hw_metadata Larysa Zaremba
2023-05-12 18:33 ` [xdp-hints] " Stanislav Fomichev
2023-05-15 14:05 ` Larysa Zaremba
2023-05-12 15:26 ` [xdp-hints] [PATCH RESEND bpf-next 14/15] net, xdp: allow metadata > 32 Larysa Zaremba
2023-05-15 16:17 ` [xdp-hints] " Jesper Dangaard Brouer
2023-05-15 17:08 ` Larysa Zaremba
2023-05-16 12:37 ` Alexander Lobakin
2023-05-16 15:35 ` Jesper Dangaard Brouer
2023-05-19 16:35 ` Alexander Lobakin
2023-05-22 11:41 ` Jesper Dangaard Brouer
2023-05-22 15:28 ` Alexander Lobakin
2023-05-22 15:55 ` Daniel Borkmann
2023-05-12 15:26 ` [xdp-hints] [PATCH RESEND bpf-next 15/15] selftests/bpf: Add flags and new hints to xdp_hw_metadata Larysa Zaremba
2023-05-12 18:37 ` [xdp-hints] " Stanislav Fomichev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://lists.xdp-project.net/postorius/lists/xdp-hints.xdp-project.net/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZGz5VWan/nROHxhc@lincoln \
--to=larysa.zaremba@intel.com \
--cc=alexandr.lobakin@intel.com \
--cc=anatoly.burakov@intel.com \
--cc=andrii@kernel.org \
--cc=anthony.l.nguyen@intel.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=brouer@redhat.com \
--cc=daniel@iogearbox.net \
--cc=intel-wired-lan@lists.osuosl.org \
--cc=jbrouer@redhat.com \
--cc=jesse.brandeburg@intel.com \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=magnus.karlsson@gmail.com \
--cc=martin.lau@linux.dev \
--cc=mtahhan@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=sdf@google.com \
--cc=song@kernel.org \
--cc=xdp-hints@xdp-project.net \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox