Hi Olivier,

On 12/27/19 5:50 PM, Olivier Matz wrote:

Hi,

On Tue, Dec 24, 2019 at 12:53:21PM +0300, Andrew Rybchenko wrote:

quoted

On 12/24/19 6:16 AM, Somnath Kotur wrote:

quoted

Given that we haven't heard any objection from anyone in a while on
this ...can we get this in please?

I'm sorry, but have you seen below?
It means that PKT_RX_QINQ_STRIPPED, PKT_RX_QINQ, PKT_RX_VLAN
and PKT_RX_VLAN_STRIPPED must be clarified.

It sounds like change of semantics in order to resolve the
problem, but anyway it is still a small change of semantics.

Let's take this packet:
packet = ether | outer-vlan | inner-vlan | ...

The possible mbufs received from a PMD, depending on configuration, are:

1/ no flag (no offload)

2/ PKT_RX_VLAN
   packet data is unmodified
   m->vlan_tci=outer-vlan

3/ PKT_RX_VLAN | PKT_RX_VLAN_STRIPPED
   outer-vlan is removed from packet data
   m->vlan_tci=outer-vlan

4/ PKT_RX_VLAN | PKT_RX_QINQ
   packet data is unmodified
   m->vlan_tci_outer=outer-vlan
   m->vlan_tci=inner-vlan

5/ PKT_RX_VLAN | PKT_RX_VLAN_STRIPPED | PKT_RX_QINQ

   from PKT_RX_VLAN_STRIPPED:
     A vlan has been stripped by the hardware and its tci is saved in
     mbuf->vlan_tci.
   from PKT_RX_QINQ:
     The RX packet is a double VLAN, and the outer tci has been
     saved in in mbuf->vlan_tci_outer.

   To me, it means:

   inner-vlan is removed from packet data
   m->vlan_tci_outer=outer-vlan
   m->vlan_tci=inner-vlan

Yes, I agree that such behaviour is logical (since PKT_RX_VLAN_STRIPPED is
bound to PKT_RX_VLAN => m->vlan_tci). My understanding was wrong
(see below) since I thought that outer-vlan is saved in vlan_tci in this
case
(since only one is stripped).

6/ PKT_RX_VLAN | PKT_RX_VLAN_STRIPPED | PKT_RX_QINQ | PKT_RX_QINQ_STRIPPED
   both outer-vlan and inner-vlan removed from packet data
   m->vlan_tci_outer=outer-vlan
   m->vlan_tci=inner-vlan

Other flag combinations are not supported.
The proposed patch documents that this new flag combination is now allowed:

7/ PKT_RX_VLAN | PKT_RX_QINQ | PKT_RX_QINQ_STRIPPED
   outer-vlan is removed from packet data
   m->vlan_tci_outer=outer-vlan
   m->vlan_tci=inner-vlan

Except if I missed something, I don't see any semantic change in
the previously supported cases.

Yes, I agree.

I think we should by the way clarify what happens in 5/, probably by
saying somewhere that as soon as PKT_RX_QINQ is set, PKT_RX_VLAN*
refer to inner vlan.

Yes, it would be good, since exactly this cases confuses me.

Many thanks for clarification.

quoted

BTW, it is better to make summary human readable and avoid
PKT_RX_QINQ_STRIPPED (I guess check-git-log.sh yells on it).

Also RFC patch cannot be applied, non-RFC version is required.

CC main tree maintainers.

quoted

Thanks

On Mon, Dec 16, 2019 at 2:43 PM Andrew Rybchenko
[off-list ref] wrote:

quoted

On 12/16/19 11:47 AM, Somnath Kotur wrote:

quoted

On Mon, Dec 16, 2019 at 12:01 PM Andrew Rybchenko
[off-list ref] wrote:

quoted

On 12/16/19 6:16 AM, Somnath Kotur wrote:

quoted

Certain hardware may be able to strip and/or save only the outermost
VLAN instead of both the VLANs in the mbuf in a QinQ scenario.
To handle such cases, we could re-interpret setting of just PKT_RX_QINQ_STRIPPED
to indicate that only the outermost VLAN has been stripped by the hardware and
saved in mbuf->vlan_tci_outer.
Only When both PKT_RX_QINQ_STRIPPED and PKT_RX_VLAN_STRIPPED are set, the 2 vlans
have been stripped by the hardware and their tci are saved in mbuf->vlan_tci (inner)
and mbuf->vlan_tci_outer (outer).

Signed-off-by: Somnath Kotur <redacted>
Signed-off-by: JP Lee <redacted>
---
 lib/librte_mbuf/rte_mbuf_core.h | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf_core.h
index 9a8557d..db1070b 100644
--- a/lib/librte_mbuf/rte_mbuf_core.h
+++ b/lib/librte_mbuf/rte_mbuf_core.h

@@ -124,12 +124,19 @@
 #define PKT_RX_FDIR_FLX      (1ULL << 14)

 /**
- * The 2 vlans have been stripped by the hardware and their tci are
- * saved in mbuf->vlan_tci (inner) and mbuf->vlan_tci_outer (outer).
+ * The outer vlan has been stripped by the hardware and their tci are
+ * saved in mbuf->vlan_tci_outer (outer).
  * This can only happen if vlan stripping is enabled in the RX
  * configuration of the PMD.
- * When PKT_RX_QINQ_STRIPPED is set, the flags (PKT_RX_VLAN |
- * PKT_RX_VLAN_STRIPPED | PKT_RX_QINQ) must also be set.
+ * When PKT_RX_QINQ_STRIPPED is set, the flags (PKT_RX_VLAN |  PKT_RX_QINQ)
+ * must also be set.
+ * When both PKT_RX_QINQ_STRIPPED and PKT_RX_VLAN_STRIPPED are set, the 2 vlans
+ * have been stripped by the hardware and their tci are saved in
+ * mbuf->vlan_tci (inner) and mbuf->vlan_tci_outer (outer).
+ * This can only happen if vlan stripping is enabled in the RX configuration
+ * of the PMD.
+ * When PKT_RX_QINQ_STRIPPED and PKT_RX_VLAN_STRIPPED are set,
+ * (PKT_RX_VLAN | PKT_RX_QINQ) must also be set.
  */
 #define PKT_RX_QINQ_STRIPPED (1ULL << 15)

I always thought that PKT_RX_VLAN_STRIPPED means *one* VLAN
stripped regardless if it is outer (if the packet is double
tagged) or inner (if only one VLAN tag was present).

That's why PKT_RX_QINQ_STRIPPED description says that *two*
VLANs have been stripped.

What is the problem with such approach?

The problem is that RX_VLAN_STRIPPED implies that the stripped VLAN
(outer or inner) is saved in mbuf->vlan_tci, correct?

Yes.

quoted

There is no way to convey that it is in QinQ mode and yet only outer
VLAN has been stripped and saved in mbuf->vlan_tci_outer ?

Ah, it looks like I understand now that the problem is in
PKT_RX_QINQ description which claims that TCI is saved in
mbuf->vlan_tci_outer and PKT_RX_QINQ_STRIPPED means that
both VLAN tags are stripped regardless (PKT_RX_VLAN_STRIPPED).
Moreover PKT_RX_QINQ_STRIPPED requires PKT_RX_VLAN_STRIPPED.

It means that PKT_RX_QINQ_STRIPPED, PKT_RX_QINQ, PKT_RX_VLAN
and PKT_RX_VLAN_STRIPPED must be clarified.

I'm not sure, but it looks like it could affect net/dpaa2,
so I'm including driver maintainers in CC.