Date: Tue, 4 Jul 2023 12:23:45 +0200
From: Jesper Dangaard Brouer <jbrouer@...hat.com>
To: Larysa Zaremba <larysa.zaremba@...el.com>,
 John Fastabend <john.fastabend@...il.com>
Cc: brouer@...hat.com, bpf@...r.kernel.org, ast@...nel.org,
 daniel@...earbox.net, andrii@...nel.org, martin.lau@...ux.dev,
 song@...nel.org, yhs@...com, kpsingh@...nel.org, sdf@...gle.com,
 haoluo@...gle.com, jolsa@...nel.org, David Ahern <dsahern@...il.com>,
 Jakub Kicinski <kuba@...nel.org>, Willem de Bruijn <willemb@...gle.com>,
 Anatoly Burakov <anatoly.burakov@...el.com>,
 Alexander Lobakin <alexandr.lobakin@...el.com>,
 Magnus Karlsson <magnus.karlsson@...il.com>,
 Maryam Tahhan <mtahhan@...hat.com>, xdp-hints@...-project.net,
 netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH bpf-next v2 09/20] xdp: Add VLAN tag hint



On 04/07/2023 10.23, Larysa Zaremba wrote:
> On Mon, Jul 03, 2023 at 01:15:34PM -0700, John Fastabend wrote:
>> Larysa Zaremba wrote:
>>> Implement functionality that enables drivers to expose VLAN tag
>>> to XDP code.
>>>
>>> Signed-off-by: Larysa Zaremba <larysa.zaremba@...el.com>
>>> ---
>>>   Documentation/networking/xdp-rx-metadata.rst |  8 +++++++-
>>>   include/linux/netdevice.h                    |  2 ++
>>>   include/net/xdp.h                            |  2 ++
>>>   kernel/bpf/offload.c                         |  2 ++
>>>   net/core/xdp.c                               | 20 ++++++++++++++++++++
>>>   5 files changed, 33 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/net/core/xdp.c b/net/core/xdp.c
>>> index 41e5ca8643ec..f6262c90e45f 100644
>>> --- a/net/core/xdp.c
>>> +++ b/net/core/xdp.c
>>> @@ -738,6 +738,26 @@ __bpf_kfunc int bpf_xdp_metadata_rx_hash(const struct xdp_md *ctx, u32 *hash,
>>>   	return -EOPNOTSUPP;
>>>   }
>>>   
>>> +/**
>>> + * bpf_xdp_metadata_rx_vlan_tag - Get XDP packet outermost VLAN tag with protocol
>>> + * @ctx: XDP context pointer.
>>> + * @vlan_tag: Destination pointer for VLAN tag
>>> + * @vlan_proto: Destination pointer for VLAN protocol identifier in network byte order.
>>> + *
>>> + * In case of success, vlan_tag contains VLAN tag, including 12 least significant bytes
>>> + * containing VLAN ID, vlan_proto contains protocol identifier.
>>
>> Above is a bit confusing to me at least.
>>
>> The vlan tag would be both the 16bit TPID and 16bit TCI. What fields
>> are to be included here? The VlanID or the full 16bit TCI meaning the
>> PCP+DEI+VID?
> 
> It contains PCP+DEI+VID, in patch 16 ("selftests/bpf: Add flags and new hints to
> xdp_hw_metadata") this is more clear, because the tag is parsed.
> 

Do we really care about the "EtherType" proto (in VLAN speak TPID = Tag
Protocol IDentifier)?
I mean, it can basically only have two values[1], and we just wanted to
know if it is a VLAN (that hardware offloaded/removed for us):

  static __always_inline int proto_is_vlan(__u16 h_proto)
  {
	return !!(h_proto == bpf_htons(ETH_P_8021Q) ||
		  h_proto == bpf_htons(ETH_P_8021AD));
  }

[1] 
https://github.com/xdp-project/bpf-examples/blob/master/include/xdp/parsing_helpers.h#L75-L79

Cc. Andrew Lunn, as I notice DSA have a fake VLAN define ETH_P_DSA_8021Q
(in file include/uapi/linux/if_ether.h)
Is this actually in use?
Maybe some hardware can "VLAN" offload this?


> What about rephrasing it this way:
> 
> In case of success, vlan_proto contains VLAN protocol identifier (TPID),
> vlan_tag contains the remaining 16 bits of a 802.1Q tag (PCP+DEI+VID).
> 

Hmm, I think we can improve this further. This text becomes part of the
documentation for end-users (target audience).  Thus, I think it is
worth being more verbose and even mention the existing defines that we
are expecting end-users to take advantage of.

What about:

In case of success. The VLAN EtherType is stored in vlan_proto (usually
either ETH_P_8021Q or ETH_P_8021AD) also known as TPID (Tag Protocol
IDentifier). The VLAN tag is stored in vlan_tag, which is a 16-bit field
containing sub-fields (PCP+DEI+VID). The VLAN ID (VID) is 12-bits
commonly extracted using mask VLAN_VID_MASK (0x0fff).  For the meaning
of the sub-fields Priority Code Point (PCP) and Drop Eligible Indicator
(DEI) (formerly CFI) please reference other documentation. Remember
these 16-bit fields are stored in network-byte. Thus, transformation
with byte-order helper functions like bpf_ntohs() are needed.



>> I think by "including 12 least significant bytes" you
>> mean bits,
> 
> Yes, my bad.
> 
>> but also not clear about those 4 other bits.
>>
>> I can likely figure it out in next patches from implementation but
>> would be nice to clean up docs.
>>
>>> + *
>>> + * Return:
>>> + * * Returns 0 on success or ``-errno`` on error.
>>> + * * ``-EOPNOTSUPP`` : device driver doesn't implement kfunc
>>> + * * ``-ENODATA``    : VLAN tag was not stripped or is not available
>>> + */
>>> +__bpf_kfunc int bpf_xdp_metadata_rx_vlan_tag(const struct xdp_md *ctx, u16 *vlan_tag,
>>> +					     __be16 *vlan_proto)
>>> +{
>>> +	return -EOPNOTSUPP;
>>> +}
>>> +

Powered by blists - more mailing lists