[dpdk-dev] [RFC PATCH] mbuf: fix to update documentation of PKT_RX_QINQ_STRIPPED

[dpdk-dev] [RFC PATCH] mbuf: fix to update documentation of PKT_RX_QINQ_STRIPPED

[Somnath Kotur]

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 <somnath.kotur@broadcom.com>
Signed-off-by: JP Lee <jongpil.lee@broadcom.com>
---
 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)
 
-- 
1.8.3.1

Re: [dpdk-dev] [RFC PATCH] mbuf: fix to update documentation of PKT_RX_QINQ_STRIPPED

[Andrew Rybchenko]

On 12/16/19 6:16 AM, Somnath Kotur wrote:
> 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 <somnath.kotur@broadcom.com>
> Signed-off-by: JP Lee <jongpil.lee@broadcom.com>
> ---
>  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?

Re: [dpdk-dev] [RFC PATCH] mbuf: fix to update documentation of PKT_RX_QINQ_STRIPPED

[Somnath Kotur]

On Mon, Dec 16, 2019 at 12:01 PM Andrew Rybchenko
<arybchenko@solarflare.com> wrote:
>
> On 12/16/19 6:16 AM, Somnath Kotur wrote:
> > 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 <somnath.kotur@broadcom.com>
> > Signed-off-by: JP Lee <jongpil.lee@broadcom.com>
> > ---
> >  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?
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 ?

Re: [dpdk-dev] [RFC PATCH] mbuf: fix to update documentation of PKT_RX_QINQ_STRIPPED

[Andrew Rybchenko]

On 12/16/19 11:47 AM, Somnath Kotur wrote:
> On Mon, Dec 16, 2019 at 12:01 PM Andrew Rybchenko
> <arybchenko@solarflare.com> wrote:
>>
>> On 12/16/19 6:16 AM, Somnath Kotur wrote:
>>> 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 <somnath.kotur@broadcom.com>
>>> Signed-off-by: JP Lee <jongpil.lee@broadcom.com>
>>> ---
>>>  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.

> 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.

Re: [dpdk-dev] [RFC PATCH] mbuf: fix to update documentation of PKT_RX_QINQ_STRIPPED

[Somnath Kotur]

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

Thanks

On Mon, Dec 16, 2019 at 2:43 PM Andrew Rybchenko
<arybchenko@solarflare.com> wrote:
>
> On 12/16/19 11:47 AM, Somnath Kotur wrote:
> > On Mon, Dec 16, 2019 at 12:01 PM Andrew Rybchenko
> > <arybchenko@solarflare.com> wrote:
> >>
> >> On 12/16/19 6:16 AM, Somnath Kotur wrote:
> >>> 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 <somnath.kotur@broadcom.com>
> >>> Signed-off-by: JP Lee <jongpil.lee@broadcom.com>
> >>> ---
> >>>  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.
>
> > 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.

Re: [dpdk-dev] [RFC PATCH] mbuf: fix to update documentation of PKT_RX_QINQ_STRIPPED

[Andrew Rybchenko]

On 12/24/19 6:16 AM, Somnath Kotur wrote:
> 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.

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.

> Thanks
> 
> On Mon, Dec 16, 2019 at 2:43 PM Andrew Rybchenko
> <arybchenko@solarflare.com> wrote:
>>
>> On 12/16/19 11:47 AM, Somnath Kotur wrote:
>>> On Mon, Dec 16, 2019 at 12:01 PM Andrew Rybchenko
>>> <arybchenko@solarflare.com> wrote:
>>>>
>>>> On 12/16/19 6:16 AM, Somnath Kotur wrote:
>>>>> 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 <somnath.kotur@broadcom.com>
>>>>> Signed-off-by: JP Lee <jongpil.lee@broadcom.com>
>>>>> ---
>>>>>  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.
>>
>>> 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.

Re: [dpdk-dev] [RFC PATCH] mbuf: fix to update documentation of PKT_RX_QINQ_STRIPPED

[Olivier Matz]

Hi,

On Tue, Dec 24, 2019 at 12:53:21PM +0300, Andrew Rybchenko wrote:
> On 12/24/19 6:16 AM, Somnath Kotur wrote:
> > 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

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.

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.


> 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.
> 
> > Thanks
> > 
> > On Mon, Dec 16, 2019 at 2:43 PM Andrew Rybchenko
> > <arybchenko@solarflare.com> wrote:
> >>
> >> On 12/16/19 11:47 AM, Somnath Kotur wrote:
> >>> On Mon, Dec 16, 2019 at 12:01 PM Andrew Rybchenko
> >>> <arybchenko@solarflare.com> wrote:
> >>>>
> >>>> On 12/16/19 6:16 AM, Somnath Kotur wrote:
> >>>>> 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 <somnath.kotur@broadcom.com>
> >>>>> Signed-off-by: JP Lee <jongpil.lee@broadcom.com>
> >>>>> ---
> >>>>>  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.
> >>
> >>> 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.
> 

Re: [dpdk-dev] [RFC PATCH] mbuf: fix to update documentation of PKT_RX_QINQ_STRIPPED

[Somnath Kotur]

On Fri, Dec 27, 2019 at 8:20 PM Olivier Matz <olivier.matz@6wind.com> wrote:
>
> Hi,
>
> On Tue, Dec 24, 2019 at 12:53:21PM +0300, Andrew Rybchenko wrote:
> > On 12/24/19 6:16 AM, Somnath Kotur wrote:
> > > 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.
>
Agree with your interpretation upto this point...

>    To me, it means:
>
>    inner-vlan is removed from packet data
>    m->vlan_tci_outer=outer-vlan
>    m->vlan_tci=inner-vlan
This is the case my patch wanted to cover ..There could be some HW
which may not be able to
strip and provide both VLANs in the QinQ scenario ...
>
> 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.
>
> 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.
>
>
> > 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.
> >
> > > Thanks
> > >
> > > On Mon, Dec 16, 2019 at 2:43 PM Andrew Rybchenko
> > > <arybchenko@solarflare.com> wrote:
> > >>
> > >> On 12/16/19 11:47 AM, Somnath Kotur wrote:
> > >>> On Mon, Dec 16, 2019 at 12:01 PM Andrew Rybchenko
> > >>> <arybchenko@solarflare.com> wrote:
> > >>>>
> > >>>> On 12/16/19 6:16 AM, Somnath Kotur wrote:
> > >>>>> 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 <somnath.kotur@broadcom.com>
> > >>>>> Signed-off-by: JP Lee <jongpil.lee@broadcom.com>
> > >>>>> ---
> > >>>>>  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.
> > >>
> > >>> 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.
> >

Re: [dpdk-dev] [RFC PATCH] mbuf: fix to update documentation of PKT_RX_QINQ_STRIPPED

[Somnath Kotur]

Andrew,

On Tue, Dec 24, 2019 at 3:23 PM Andrew Rybchenko
<arybchenko@solarflare.com> wrote:
>
> On 12/24/19 6:16 AM, Somnath Kotur wrote:
> > 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.
>
OK, not sure I understood what is the next action here? Will you or someone
from the main tree maintainers be sending out a patch with this clarification?
> It sounds like change of semantics in order to resolve the
> problem, but anyway it is still a small change of semantics.
>
> 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.
>
> > Thanks
> >
> > On Mon, Dec 16, 2019 at 2:43 PM Andrew Rybchenko
> > <arybchenko@solarflare.com> wrote:
> >>
> >> On 12/16/19 11:47 AM, Somnath Kotur wrote:
> >>> On Mon, Dec 16, 2019 at 12:01 PM Andrew Rybchenko
> >>> <arybchenko@solarflare.com> wrote:
> >>>>
> >>>> On 12/16/19 6:16 AM, Somnath Kotur wrote:
> >>>>> 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 <somnath.kotur@broadcom.com>
> >>>>> Signed-off-by: JP Lee <jongpil.lee@broadcom.com>
> >>>>> ---
> >>>>>  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.
> >>
> >>> 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.
>

Re: [dpdk-dev] [RFC PATCH] mbuf: fix to update documentation of PKT_RX_QINQ_STRIPPED

[Andrew Rybchenko]

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:
>> On 12/24/19 6:16 AM, Somnath Kotur wrote:
>>> 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.

>> 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.
>>
>>> Thanks
>>>
>>> On Mon, Dec 16, 2019 at 2:43 PM Andrew Rybchenko
>>> <arybchenko@solarflare.com> wrote:
>>>> On 12/16/19 11:47 AM, Somnath Kotur wrote:
>>>>> On Mon, Dec 16, 2019 at 12:01 PM Andrew Rybchenko
>>>>> <arybchenko@solarflare.com> wrote:
>>>>>> On 12/16/19 6:16 AM, Somnath Kotur wrote:
>>>>>>> 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 <somnath.kotur@broadcom.com>
>>>>>>> Signed-off-by: JP Lee <jongpil.lee@broadcom.com>
>>>>>>> ---
>>>>>>>  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.
>>>>
>>>>> 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.

Re: [dpdk-dev] [RFC PATCH] mbuf: fix to update documentation of PKT_RX_QINQ_STRIPPED

[Andrew Rybchenko]

Somnath,

On 12/31/19 5:15 AM, Somnath Kotur wrote:
> Andrew,
>
> On Tue, Dec 24, 2019 at 3:23 PM Andrew Rybchenko
> <arybchenko@solarflare.com> wrote:
>> On 12/24/19 6:16 AM, Somnath Kotur wrote:
>>> 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.
>>
> OK, not sure I understood what is the next action here? Will you or someone
> from the main tree maintainers be sending out a patch with this clarification?

Please, send non-RCF version of the patch which fixes
PKT_RX_QINQ_STRIPPED and PKT_RX_QINQ description.
PKT_RX_QINQ must not claim that both VLAN headers
have been stripped in the case of PKT_RX_QINQ_STRIPPED.

I think that VLAN should be used instead of "vlan" in description
as well as TCI instead of "tci". Also vlans -> VLANs.

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

May be dropped.

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

Please, don't forget about it as well.

>> Also RFC patch cannot be applied, non-RFC version is required.
>>
>> CC main tree maintainers.
>>
>>> Thanks
>>>
>>> On Mon, Dec 16, 2019 at 2:43 PM Andrew Rybchenko
>>> <arybchenko@solarflare.com> wrote:
>>>> On 12/16/19 11:47 AM, Somnath Kotur wrote:
>>>>> On Mon, Dec 16, 2019 at 12:01 PM Andrew Rybchenko
>>>>> <arybchenko@solarflare.com> wrote:
>>>>>> On 12/16/19 6:16 AM, Somnath Kotur wrote:
>>>>>>> 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 <somnath.kotur@broadcom.com>
>>>>>>> Signed-off-by: JP Lee <jongpil.lee@broadcom.com>
>>>>>>> ---
>>>>>>>  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.
>>>>
>>>>> 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.

Re: [dpdk-dev] [RFC PATCH] mbuf: fix to update documentation of PKT_RX_QINQ_STRIPPED

[Somnath Kotur]

On Thu, Jan 2, 2020 at 6:35 PM Andrew Rybchenko
<arybchenko@solarflare.com> wrote:
>
> Somnath,
>
> On 12/31/19 5:15 AM, Somnath Kotur wrote:
> > Andrew,
> >
> > On Tue, Dec 24, 2019 at 3:23 PM Andrew Rybchenko
> > <arybchenko@solarflare.com> wrote:
> >> On 12/24/19 6:16 AM, Somnath Kotur wrote:
> >>> 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.
> >>
> > OK, not sure I understood what is the next action here? Will you or someone
> > from the main tree maintainers be sending out a patch with this clarification?
>
> Please, send non-RCF version of the patch which fixes
> PKT_RX_QINQ_STRIPPED and PKT_RX_QINQ description.
> PKT_RX_QINQ must not claim that both VLAN headers
> have been stripped in the case of PKT_RX_QINQ_STRIPPED.
>
OK, think i've done the non-RFC patch based on my understanding here

> I think that VLAN should be used instead of "vlan" in description
> as well as TCI instead of "tci". Also vlans -> VLANs.
>
Done
> >> It sounds like change of semantics in order to resolve the
> >> problem, but anyway it is still a small change of semantics.
>
> May be dropped.
>
> >> BTW, it is better to make summary human readable and avoid
> >> PKT_RX_QINQ_STRIPPED (I guess check-git-log.sh yells on it).
>
> Please, don't forget about it as well.
Done

> >> Also RFC patch cannot be applied, non-RFC version is required.
> >>
> >> CC main tree maintainers.
> >>
> >>> Thanks
> >>>
> >>> On Mon, Dec 16, 2019 at 2:43 PM Andrew Rybchenko
> >>> <arybchenko@solarflare.com> wrote:
> >>>> On 12/16/19 11:47 AM, Somnath Kotur wrote:
> >>>>> On Mon, Dec 16, 2019 at 12:01 PM Andrew Rybchenko
> >>>>> <arybchenko@solarflare.com> wrote:
> >>>>>> On 12/16/19 6:16 AM, Somnath Kotur wrote:
> >>>>>>> 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 <somnath.kotur@broadcom.com>
> >>>>>>> Signed-off-by: JP Lee <jongpil.lee@broadcom.com>
> >>>>>>> ---
> >>>>>>>  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.
> >>>>
> >>>>> 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.
>