[dpdk-dev] [PATCH] mbuf: fix to update documentation of QinQ stripped bit interpretation

[dpdk-dev] [PATCH] mbuf: fix to update documentation of QinQ stripped bit interpretation

[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 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>
---
 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] [PATCH] mbuf: fix to update documentation of QinQ stripped bit interpretation

[Somnath Kotur]

On Mon, Jan 6, 2020 at 2:05 PM Somnath Kotur <somnath.kotur@broadcom.com> 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 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>
> ---
>  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

Seeing as its been a month since it was submitted and no comments on
it yet , are we ok to go ahead and merge this in please?

Thanks
Som

Re: [dpdk-dev] [PATCH] mbuf: fix to update documentation of QinQ stripped bit interpretation

[Olivier Matz]

Hi Somnath,

Sorry for the delay, please find some comments below.

I suggest the following title instead:

  mbuf: extend meaning of QinQ stripped bit

On Mon, Jan 06, 2020 at 02:04:23PM +0530, 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 and saved in mbuf->vlan_tci_outer.

To be sure we're on the same page: we are talking about case 7 of this
link: http://inbox.dpdk.org/dev/20191227145041.GQ22738@platinum/

So, even if the inner vlan_tci is not stripped from packet data, it has
to be saved in m->vlan_tci, because it is implied by PKT_RX_VLAN.

From the same link, the case where the driver only strips+saves the
outer vlan without saving or stripping the inner one is case 3.

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

their tci are -> its tci is

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

ok

> + * 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 is correct, but I'd use a bullet list to add another sentence:

 * - If 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).
 * - If PKT_RX_QINQ_STRIPPED is set and PKT_RX_VLAN_STRIPPED is unset, only the
 *   outer vlan is removed from packet data, but both 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.

The same exact sentence is above, this one can be removed.

> + * When PKT_RX_QINQ_STRIPPED and PKT_RX_VLAN_STRIPPED are set,
> + * (PKT_RX_VLAN | PKT_RX_QINQ) must also be set.

This can be removed too as it is redundant with above sentence:

 * When PKT_RX_QINQ_STRIPPED is set, the flags (PKT_RX_VLAN |  PKT_RX_QINQ)
 * must also be set.


Thanks,
Olivier

Re: [dpdk-dev] [PATCH] mbuf: fix to update documentation of QinQ stripped bit interpretation

[Somnath Kotur]

Olivier,

On Thu, Feb 6, 2020 at 10:55 PM Olivier Matz <olivier.matz@6wind.com> wrote:
>
> Hi Somnath,
>
> Sorry for the delay, please find some comments below.
>
> I suggest the following title instead:
>
>   mbuf: extend meaning of QinQ stripped bit
>
> On Mon, Jan 06, 2020 at 02:04:23PM +0530, 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 and saved in mbuf->vlan_tci_outer.
>
> To be sure we're on the same page: we are talking about case 7 of this
> link: http://inbox.dpdk.org/dev/20191227145041.GQ22738@platinum/

I'm not sure we are on the same page then, please see my response inline below
>
> So, even if the inner vlan_tci is not stripped from packet data, it has
> to be saved in m->vlan_tci, because it is implied by PKT_RX_VLAN.
>
> From the same link, the case where the driver only strips+saves the
> outer vlan without saving or stripping the inner one is case 3.
>
While this is how it works currently, I'm wondering how will the
application know if this was
a double VLAN pkt, correct?
Also when i look at options 5 and 7 I don't really see the difference
in semantics between them ?
Both seem to store the outer-vlan and inner-vlan in m->vlan_tci_outer
and m->vlan_tci_inner respectively

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

I was hoping that with the new interpretation of PKT_RX_QINQ_STRIPPED,
it only meant that outer-vlan is removed from packet data
and m->vlan_tci_outer = outer_vlan, while PKT_RX_QINQ implies it is a
double-vlan pkt and PKT_RX_VLAN implies that pkt has VLAN
associated with it?   Not m->vlan_tci = inner-vlan

Thanks
Som


> > 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>
> > ---
> >  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).
>
> their tci are -> its tci is
>
> >   * 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.
>
> ok
>
> > + * 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 is correct, but I'd use a bullet list to add another sentence:
>
>  * - If 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).
>  * - If PKT_RX_QINQ_STRIPPED is set and PKT_RX_VLAN_STRIPPED is unset, only the
>  *   outer vlan is removed from packet data, but both 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.
>
> The same exact sentence is above, this one can be removed.
>
> > + * When PKT_RX_QINQ_STRIPPED and PKT_RX_VLAN_STRIPPED are set,
> > + * (PKT_RX_VLAN | PKT_RX_QINQ) must also be set.
>
> This can be removed too as it is redundant with above sentence:
>
>  * When PKT_RX_QINQ_STRIPPED is set, the flags (PKT_RX_VLAN |  PKT_RX_QINQ)
>  * must also be set.
>
>
> Thanks,
> Olivier

Re: [dpdk-dev] [PATCH] mbuf: fix to update documentation of QinQ stripped bit interpretation

[Olivier Matz]

Hi Somnath,

On Fri, Feb 07, 2020 at 07:13:04PM +0530, Somnath Kotur wrote:
> Olivier,
> 
> On Thu, Feb 6, 2020 at 10:55 PM Olivier Matz <olivier.matz@6wind.com> wrote:
> >
> > Hi Somnath,
> >
> > Sorry for the delay, please find some comments below.
> >
> > I suggest the following title instead:
> >
> >   mbuf: extend meaning of QinQ stripped bit
> >
> > On Mon, Jan 06, 2020 at 02:04:23PM +0530, 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 and saved in mbuf->vlan_tci_outer.
> >
> > To be sure we're on the same page: we are talking about case 7 of this
> > link: http://inbox.dpdk.org/dev/20191227145041.GQ22738@platinum/
> 
> I'm not sure we are on the same page then, please see my response inline below
> >
> > So, even if the inner vlan_tci is not stripped from packet data, it has
> > to be saved in m->vlan_tci, because it is implied by PKT_RX_VLAN.
> >
> > From the same link, the case where the driver only strips+saves the
> > outer vlan without saving or stripping the inner one is case 3.
> >
> While this is how it works currently, I'm wondering how will the
> application know if this was
> a double VLAN pkt, correct?

If the hardware supports it and configured for it, the flag PKT_RX_QINQ
is set when there are 2 vlans.

FYI, the m->packet_type field can also be useful. It describes what is
really present in the packet data, as described in rte_mbuf_core.h:

	/*
	 * The packet type, which is the combination of outer/inner L2, L3, L4
	 * and tunnel types. The packet_type is about data really present in the
	 * mbuf. Example: if vlan stripping is enabled, a received vlan packet
	 * would have RTE_PTYPE_L2_ETHER and not RTE_PTYPE_L2_VLAN because the
	 * vlan is stripped from the data.
	 */

> Also when i look at options 5 and 7 I don't really see the difference
> in semantics between them ?
> Both seem to store the outer-vlan and inner-vlan in m->vlan_tci_outer
> and m->vlan_tci_inner respectively
> 
> 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

The difference is about what was stripped or not from mbuf data.

> I was hoping that with the new interpretation of PKT_RX_QINQ_STRIPPED,
> it only meant that outer-vlan is removed from packet data
> and m->vlan_tci_outer = outer_vlan, while PKT_RX_QINQ implies it is a
> double-vlan pkt and PKT_RX_VLAN implies that pkt has VLAN
> associated with it?   Not m->vlan_tci = inner-vlan

The meaning of each flag should be as simple as possible, I think we can
summarize them like this:

- PKT_RX_VLAN: the vlan is saved in vlan tci.
- PKT_RX_VLAN_STRIPPED: the vlan hdr is removed from packet data.
- PKT_RX_QINQ: the outer vlan is saved in vlan tci.
- PKT_RX_QINQ_STRIPPED: the inner vlan is stripped from packet data.
- When PKT_RX_QINQ is set, PKT_RX_VLAN* refer to the inner vlan
  of initial packet, else it refers to the first vlan of the packet.

There is a link between vlan flag and vlan_tci field, and qinq flag and
vlan_tci_outer field.

I'm still not sure to understand what you expect. Can you give an
example with flags (which are set), and the expected content of m->vlan_tci
and m->vlan_tci_outer?

By the way, the case 5/ is not very well described too, maybe we should
add something about it.

Thanks,
Olivier


> 
> Thanks
> Som
> 
> 
> > > 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>
> > > ---
> > >  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).
> >
> > their tci are -> its tci is
> >
> > >   * 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.
> >
> > ok
> >
> > > + * 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 is correct, but I'd use a bullet list to add another sentence:
> >
> >  * - If 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).
> >  * - If PKT_RX_QINQ_STRIPPED is set and PKT_RX_VLAN_STRIPPED is unset, only the
> >  *   outer vlan is removed from packet data, but both 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.
> >
> > The same exact sentence is above, this one can be removed.
> >
> > > + * When PKT_RX_QINQ_STRIPPED and PKT_RX_VLAN_STRIPPED are set,
> > > + * (PKT_RX_VLAN | PKT_RX_QINQ) must also be set.
> >
> > This can be removed too as it is redundant with above sentence:
> >
> >  * When PKT_RX_QINQ_STRIPPED is set, the flags (PKT_RX_VLAN |  PKT_RX_QINQ)
> >  * must also be set.
> >
> >
> > Thanks,
> > Olivier

Re: [dpdk-dev] [PATCH] mbuf: fix to update documentation of QinQ stripped bit interpretation

[Stephen Hemminger]

On Fri, 7 Feb 2020 15:29:59 +0100
Olivier Matz <olivier.matz@6wind.com> wrote:

> The meaning of each flag should be as simple as possible, I think we can
> summarize them like this:
> 
> - PKT_RX_VLAN: the vlan is saved in vlan tci.
> - PKT_RX_VLAN_STRIPPED: the vlan hdr is removed from packet data.
> - PKT_RX_QINQ: the outer vlan is saved in vlan tci.
> - PKT_RX_QINQ_STRIPPED: the inner vlan is stripped from packet data.
> - When PKT_RX_QINQ is set, PKT_RX_VLAN* refer to the inner vlan
>   of initial packet, else it refers to the first vlan of the packet.
> 
> There is a link between vlan flag and vlan_tci field, and qinq flag and
> vlan_tci_outer field.
> 
> I'm still not sure to understand what you expect. Can you give an
> example with flags (which are set), and the expected content of m->vlan_tci
> and m->vlan_tci_outer?
> 
> By the way, the case 5/ is not very well described too, maybe we should
> add something about it.
> 
> Thanks,
> Olivier

The patch does help clarify the meaning, and Oliver's summary clarifies
even more. It might be possible for hardware to offload inner vlan but
not outer vlan, though seriously doubt anyone but some conformance test
would do that.
 

Re: [dpdk-dev] [PATCH] mbuf: fix to update documentation of QinQ stripped bit interpretation

[Thomas Monjalon]

Please Somnath, it is waiting for a v2.


26/02/2020 01:55, Stephen Hemminger:
> On Fri, 7 Feb 2020 15:29:59 +0100
> Olivier Matz <olivier.matz@6wind.com> wrote:
> 
> > The meaning of each flag should be as simple as possible, I think we can
> > summarize them like this:
> > 
> > - PKT_RX_VLAN: the vlan is saved in vlan tci.
> > - PKT_RX_VLAN_STRIPPED: the vlan hdr is removed from packet data.
> > - PKT_RX_QINQ: the outer vlan is saved in vlan tci.
> > - PKT_RX_QINQ_STRIPPED: the inner vlan is stripped from packet data.
> > - When PKT_RX_QINQ is set, PKT_RX_VLAN* refer to the inner vlan
> >   of initial packet, else it refers to the first vlan of the packet.
> > 
> > There is a link between vlan flag and vlan_tci field, and qinq flag and
> > vlan_tci_outer field.
> > 
> > I'm still not sure to understand what you expect. Can you give an
> > example with flags (which are set), and the expected content of m->vlan_tci
> > and m->vlan_tci_outer?
> > 
> > By the way, the case 5/ is not very well described too, maybe we should
> > add something about it.
> > 
> > Thanks,
> > Olivier
> 
> The patch does help clarify the meaning, and Oliver's summary clarifies
> even more. It might be possible for hardware to offload inner vlan but
> not outer vlan, though seriously doubt anyone but some conformance test
> would do that.

Re: [dpdk-dev] [PATCH] mbuf: fix to update documentation of QinQ stripped bit interpretation

[Andrew Rybchenko]

I'll review v2 promptly, some minor comments from me below
(taking into account that Olivier's review notes are applied).

On 1/6/20 11:34 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 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>
> ---
>  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

vlan -> VLAN, tci -> TCI

> + * saved in mbuf->vlan_tci_outer (outer).
>   * This can only happen if vlan stripping is enabled in the RX

vlan -> VLAN, RX -> 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

vlans -> VLANs

> + * have been stripped by the hardware and their tci are saved in

tci -> TCI

> + * mbuf->vlan_tci (inner) and mbuf->vlan_tci_outer (outer).
> + * This can only happen if vlan stripping is enabled in the RX configuration

vlan -> VLAN, RX -> Rx

> + * 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 realize that some of my comment above touch not modified
lines, but ~90% of the description is updated and I see no
point to keep remaining 10% untouched.

Re: [dpdk-dev] [PATCH] mbuf: fix to update documentation of QinQ stripped bit interpretation

[Thomas Monjalon]

Somnath, why neither update, nor reply to reviews after February 7?
It is discouraging for reviewers.


24/04/2020 20:24, Thomas Monjalon:
> Please Somnath, it is waiting for a v2.
> 
> 
> 26/02/2020 01:55, Stephen Hemminger:
> > On Fri, 7 Feb 2020 15:29:59 +0100
> > Olivier Matz <olivier.matz@6wind.com> wrote:
> > 
> > > The meaning of each flag should be as simple as possible, I think we can
> > > summarize them like this:
> > > 
> > > - PKT_RX_VLAN: the vlan is saved in vlan tci.
> > > - PKT_RX_VLAN_STRIPPED: the vlan hdr is removed from packet data.
> > > - PKT_RX_QINQ: the outer vlan is saved in vlan tci.
> > > - PKT_RX_QINQ_STRIPPED: the inner vlan is stripped from packet data.
> > > - When PKT_RX_QINQ is set, PKT_RX_VLAN* refer to the inner vlan
> > >   of initial packet, else it refers to the first vlan of the packet.
> > > 
> > > There is a link between vlan flag and vlan_tci field, and qinq flag and
> > > vlan_tci_outer field.
> > > 
> > > I'm still not sure to understand what you expect. Can you give an
> > > example with flags (which are set), and the expected content of m->vlan_tci
> > > and m->vlan_tci_outer?
> > > 
> > > By the way, the case 5/ is not very well described too, maybe we should
> > > add something about it.
> > > 
> > > Thanks,
> > > Olivier
> > 
> > The patch does help clarify the meaning, and Oliver's summary clarifies
> > even more. It might be possible for hardware to offload inner vlan but
> > not outer vlan, though seriously doubt anyone but some conformance test
> > would do that.

Re: [dpdk-dev] [PATCH] mbuf: fix to update documentation of QinQ stripped bit interpretation

[Thomas Monjalon]

Ping

Anybody volunteer to make a v2 please?


24/05/2020 17:34, Thomas Monjalon:
> Somnath, why neither update, nor reply to reviews after February 7?
> It is discouraging for reviewers.
> 
> 
> 24/04/2020 20:24, Thomas Monjalon:
> > Please Somnath, it is waiting for a v2.
> > 
> > 
> > 26/02/2020 01:55, Stephen Hemminger:
> > > On Fri, 7 Feb 2020 15:29:59 +0100
> > > Olivier Matz <olivier.matz@6wind.com> wrote:
> > > 
> > > > The meaning of each flag should be as simple as possible, I think we can
> > > > summarize them like this:
> > > > 
> > > > - PKT_RX_VLAN: the vlan is saved in vlan tci.
> > > > - PKT_RX_VLAN_STRIPPED: the vlan hdr is removed from packet data.
> > > > - PKT_RX_QINQ: the outer vlan is saved in vlan tci.
> > > > - PKT_RX_QINQ_STRIPPED: the inner vlan is stripped from packet data.
> > > > - When PKT_RX_QINQ is set, PKT_RX_VLAN* refer to the inner vlan
> > > >   of initial packet, else it refers to the first vlan of the packet.
> > > > 
> > > > There is a link between vlan flag and vlan_tci field, and qinq flag and
> > > > vlan_tci_outer field.
> > > > 
> > > > I'm still not sure to understand what you expect. Can you give an
> > > > example with flags (which are set), and the expected content of m->vlan_tci
> > > > and m->vlan_tci_outer?
> > > > 
> > > > By the way, the case 5/ is not very well described too, maybe we should
> > > > add something about it.
> > > > 
> > > > Thanks,
> > > > Olivier
> > > 
> > > The patch does help clarify the meaning, and Oliver's summary clarifies
> > > even more. It might be possible for hardware to offload inner vlan but
> > > not outer vlan, though seriously doubt anyone but some conformance test
> > > would do that.

[dpdk-dev] [PATCH v2] mbuf: extend meaning of QinQ stripped bit

[Olivier Matz]

From: Somnath Kotur <somnath.kotur@broadcom.com>

Clarify the documentation of QinQ flags, and extend the meaning of the
flag: if PKT_RX_QINQ_STRIPPED is set and PKT_RX_VLAN_STRIPPED is unset,
only the outer VLAN is removed from packet data, but both 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: Olivier Matz <olivier.matz@6wind.com>
---

Hi,

Here is an update of this old patch from Somnath.

Olivier


v2:
- better reword PKT_RX_QINQ_STRIPPED documentation
- also update PKT_RX_QINQ documentation

 lib/librte_mbuf/rte_mbuf_core.h | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf_core.h
index 8c2c20644d..8f631b84cf 100644
--- a/lib/librte_mbuf/rte_mbuf_core.h
+++ b/lib/librte_mbuf/rte_mbuf_core.h
@@ -126,12 +126,19 @@ extern "C" {
 #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).
- * This can only happen if vlan stripping is enabled in the RX
+ * The outer VLAN has been stripped by the hardware and its TCI is
+ * saved in mbuf->vlan_tci_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 and PKT_RX_QINQ
+ * must also be set.
+ *
+ * - If both PKT_RX_QINQ_STRIPPED and PKT_RX_VLAN_STRIPPED are set, the 2 VLANs
+ *   have been stripped by the hardware and their TCIs are saved in
+ *   mbuf->vlan_tci (inner) and mbuf->vlan_tci_outer (outer).
+ * - If PKT_RX_QINQ_STRIPPED is set and PKT_RX_VLAN_STRIPPED is unset, only the
+ *   outer VLAN is removed from packet data, but both tci are saved in
+ *   mbuf->vlan_tci (inner) and mbuf->vlan_tci_outer (outer).
  */
 #define PKT_RX_QINQ_STRIPPED (1ULL << 15)
 
@@ -159,8 +166,8 @@ extern "C" {
 
 /**
  * The RX packet is a double VLAN, and the outer tci has been
- * saved in in mbuf->vlan_tci_outer. If PKT_RX_QINQ set, PKT_RX_VLAN
- * also should be set and inner tci should be saved to mbuf->vlan_tci.
+ * saved in mbuf->vlan_tci_outer. If this flag is set, PKT_RX_VLAN
+ * must also be set and the inner tci is saved in mbuf->vlan_tci.
  * If the flag PKT_RX_QINQ_STRIPPED is also present, both VLANs
  * headers have been stripped from mbuf data, else they are still
  * present.
-- 
2.25.1

Re: [dpdk-dev] [PATCH v2] mbuf: extend meaning of QinQ stripped bit

[David Marchand]

On Tue, Oct 6, 2020 at 9:23 AM Olivier Matz <olivier.matz@6wind.com> wrote:
>
> From: Somnath Kotur <somnath.kotur@broadcom.com>
>
> Clarify the documentation of QinQ flags, and extend the meaning of the
> flag: if PKT_RX_QINQ_STRIPPED is set and PKT_RX_VLAN_STRIPPED is unset,
> only the outer VLAN is removed from packet data, but both 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: Olivier Matz <olivier.matz@6wind.com>

Is it only a documentation fix and the drivers are already
implementing this behavior?
CC driver maintainers for double check.


Thanks.

-- 
David Marchand

Re: [dpdk-dev] [PATCH v2] mbuf: extend meaning of QinQ stripped bit

[Andrew Rybchenko]

On 10/6/20 10:22 AM, Olivier Matz wrote:
> From: Somnath Kotur <somnath.kotur@broadcom.com>
> 
> Clarify the documentation of QinQ flags, and extend the meaning of the
> flag: if PKT_RX_QINQ_STRIPPED is set and PKT_RX_VLAN_STRIPPED is unset,
> only the outer VLAN is removed from packet data, but both 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: Olivier Matz <olivier.matz@6wind.com>

Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Re: [dpdk-dev] [PATCH v2] mbuf: extend meaning of QinQ stripped bit

[David Marchand]

On Tue, Oct 6, 2020 at 9:23 AM Olivier Matz <olivier.matz@6wind.com> wrote:
>
> From: Somnath Kotur <somnath.kotur@broadcom.com>
>
> Clarify the documentation of QinQ flags, and extend the meaning of the
> flag: if PKT_RX_QINQ_STRIPPED is set and PKT_RX_VLAN_STRIPPED is unset,
> only the outer VLAN is removed from packet data, but both 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: Olivier Matz <olivier.matz@6wind.com>
Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Applied, thanks Olivier.

-- 
David Marchand