DPDK patches and discussions
 help / color / mirror / Atom feed
From: Somnath Kotur <somnath.kotur@broadcom.com>
To: Olivier Matz <olivier.matz@6wind.com>
Cc: Andrew Rybchenko <arybchenko@solarflare.com>, dev <dev@dpdk.org>,
	 Ferruh Yigit <ferruh.yigit@intel.com>,
	Hemant Agrawal <hemant.agrawal@nxp.com>,
	Sachin Saxena <sachin.saxena@nxp.com>,
	Thomas Monjalon <thomas@monjalon.net>,
	 David Marchand <david.marchand@6wind.com>,
	JP Lee <jongpil.lee@broadcom.com>
Subject: Re: [dpdk-dev] [RFC PATCH] mbuf: fix to update documentation of PKT_RX_QINQ_STRIPPED
Date: Tue, 31 Dec 2019 07:43:33 +0530
Message-ID: <CAOBf=mt24mahv+S_4ZvGpb-F9oYrGbrONsVZ1pknssN6_pUQgQ@mail.gmail.com> (raw)
In-Reply-To: <20191227145041.GQ22738@platinum>

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

  reply	other threads:[~2019-12-31  2:13 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-16  3:16 Somnath Kotur
2019-12-16  6:31 ` Andrew Rybchenko
2019-12-16  8:47   ` Somnath Kotur
2019-12-16  9:13     ` Andrew Rybchenko
2019-12-24  3:16       ` Somnath Kotur
2019-12-24  9:53         ` Andrew Rybchenko
2019-12-27 14:50           ` Olivier Matz
2019-12-31  2:13             ` Somnath Kotur [this message]
2020-01-02 12:57             ` Andrew Rybchenko
2019-12-31  2:15           ` Somnath Kotur
2020-01-02 13:04             ` Andrew Rybchenko
2020-01-06  8:36               ` Somnath Kotur

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAOBf=mt24mahv+S_4ZvGpb-F9oYrGbrONsVZ1pknssN6_pUQgQ@mail.gmail.com' \
    --to=somnath.kotur@broadcom.com \
    --cc=arybchenko@solarflare.com \
    --cc=david.marchand@6wind.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=hemant.agrawal@nxp.com \
    --cc=jongpil.lee@broadcom.com \
    --cc=olivier.matz@6wind.com \
    --cc=sachin.saxena@nxp.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git