[xdp-hints] Re: [PATCH bpf-next v2 09/20] xdp: Add VLAN tag hint

[Jesper Dangaard Brouer <jbrouer@redhat.com>]

On 06/07/2023 16.46, Larysa Zaremba wrote:
> On Tue, Jul 04, 2023 at 04:18:04PM +0200, Jesper Dangaard Brouer wrote:
>>
>>
>> On 04/07/2023 13.02, Larysa Zaremba wrote:
>>> On Tue, Jul 04, 2023 at 12:23:45PM +0200, Jesper Dangaard Brouer wrote:
>>>>
>>>> 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@intel.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):
>>>
>>> If we assume everyone follows the standard, this would be correct.
>>> But apparently, some applications use some ambiguous value as a TPID [0].
>>>
>>> So it is not hard to imagine, some NICs could alllow you to configure your
>>> custom TPID. I am not sure if any in-tree drivers actually do this, but I think
>>> it's nice to provide some flexibility on XDP level, especially considering
>>> network stack stores full vlan_proto.
>>>
>>
>> I'm buying your argument, and agree it makes sense to provide TPID in
>> the call signature.  Given weird hardware exists that allow people to
>> configure custom TPID.
>>
>> Looking through kernel defines (in uapi/linux/if_ether.h) I see evidence
>> that funky QinQ EtherTypes have been used in the past:
>>
>>   #define ETH_P_QINQ1	0x9100		/* deprecated QinQ VLAN [ NOT AN OFFICIALLY
>> REGISTERED ID ] */
>>   #define ETH_P_QINQ2	0x9200		/* deprecated QinQ VLAN [ NOT AN OFFICIALLY
>> REGISTERED ID ] */
>>   #define ETH_P_QINQ3	0x9300		/* deprecated QinQ VLAN [ NOT AN OFFICIALLY
>> REGISTERED ID ] */
>>
>>
>>> [0]
>>> https://techhub.hpe.com/eginfolib/networking/docs/switches/7500/5200-1938a_l2-lan_cg/content/495503472.htm
>>>
>>>>
>>>>    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.
>>>>
>>>
>>> AFAIK, vlan_tag is stored in host byte order, this is how it is in skb.
>>
>> I'm not sure we should follow SKB storage scheme for XDP.
>>
> 
> I think following SKB convention is a good idea in this particular case. As I
> have mentioned below, in ice VLAN TCI in descriptor already comes in LE, so no
> point in converting it into BE, so somebody would use bpf_ntohs() later anyway.
> We are not the only manufacturer that does this.
> 

As long as other NIC hardware does the same this seems okay.


>>> In ice, we receive VLAN tag in descriptor already in LE.
>>> Only protocol is BE (network byte order). So I would replace the last 2
>>> sentences with the following:
>>>
>>> vlan_tag is stored in host byte order, so no byte order conversion is needed.
>>
>> Yikes, that was unexpected.  This needs to be heavily documented in docs.
> 
> You mean the motivation, why it is so and not the other way around?
> 

No, I don't mean the motivation.
I simply mean write it in *bold*.

Look at the description for bpf_xdp_metadata_rx_hash, how it gets
rendered [1] and how the code comments look [2].

  [1] 
https://kernel.org/doc/html/latest/networking/xdp-rx-metadata.html#general-design
  [2] https://elixir.bootlin.com/linux/v6.4/source/net/core/xdp.c#L724

To save you some time compiling htmldocs target:

  make SPHINXDIRS="networking" V=1  htmldocs

>>
>> When parsing packets, it is in network-byte-order, else my code is wrong
>> here[1]:
>>
>>    [1] https://github.com/xdp-project/bpf-examples/blob/master/include/xdp/parsing_helpers.h#L122
>>
>> I'm accessing the skb->vlan_tci here [2], and I notice I don't do any
>> byte-order conversions, so fortunately I didn't make a code mistake.
>>
>>    [2] https://github.com/xdp-project/bpf-examples/blob/master/traffic-pacing-edt/edt_pacer_vlan.c#L215
>>
> 
> In raw packet, VLAN TCI is in network byte order, but skb requires NIC/driver
> to convert it into host byte order before putting it into skb.
>

I'm interested in if *most* NIC hardware will deliver this in LE
(Little-Endian) which is host-byte order on x86 ?


>>> vlan_proto is stored in network byte order, the suggested way to use this value:
>>>
>>> vlan_proto == bpf_htons(ETH_P_8021Q)
>>>
>>>>
>>>>
>>
>> --Jesper
>>
>