DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: Olivier Matz <olivier.matz@6wind.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: "johndale@cisco.com" <johndale@cisco.com>,
	"Zhang, Helin" <helin.zhang@intel.com>,
	"adrien.mazarguil@6wind.com" <adrien.mazarguil@6wind.com>,
	"rahul.lakkireddy@chelsio.com" <rahul.lakkireddy@chelsio.com>,
	"alejandro.lucero@netronome.com" <alejandro.lucero@netronome.com>,
	"sony.chacko@qlogic.com" <sony.chacko@qlogic.com>
Subject: Re: [dpdk-dev] [PATCH v2] mbuf: new flag when Vlan is stripped
Date: Mon, 13 Jun 2016 16:31:49 +0000	[thread overview]
Message-ID: <2601191342CEEE43887BDE71AB97725836B703CC@irsmsx105.ger.corp.intel.com> (raw)
In-Reply-To: <575EDA24.3020405@6wind.com>


Hi Olivier,

> 	
> Hi Konstantin,
> 
> On 06/13/2016 04:42 PM, Ananyev, Konstantin wrote:
> >> The behavior of PKT_RX_VLAN_PKT was not very well defined, resulting in
> >> PMDs not advertising the same flags in similar conditions.
> >>
> >> Following discussion in [1], introduce 2 new flags PKT_RX_VLAN_STRIPPED
> >> and PKT_RX_QINQ_STRIPPED that are better defined:
> >>
> >>   PKT_RX_VLAN_STRIPPED: a vlan has been stripped by the hardware and its
> >>   tci is saved in mbuf->vlan_tci. This can only happen if vlan stripping
> >>   is enabled in the RX configuration of the PMD.
> >>
> >> For now, the old flag PKT_RX_VLAN_PKT is kept but marked as deprecated.
> >> It should be removed from applications and PMDs in a future revision.
> >
> > I am not sure it has to be deprecated & removed.
> > ixgbe (and igb as I can read the specs) devices can provide information is that
> > a vlan packet or not even when vlan stripping is disabled.
> > Right now ixgbe PMD do carry thins information to the user,
> > and I suppose igb could be improved to carry it too.
> > So obviously we need a way to pass that information to the upper layer.
> > I remember it was a discussion about introducing new packet_type
> > instead of ol_flag value PKT_RX_VLAN_PKT.
> > But right now it is not there, and again I don't know how easy it would be to replace
> > one with another without performance considering that packet_type is not supported
> > now by ixgbe vRX.
> > If we would be able to replace it, then yes we can deprecate and drop the   PKT_RX_VLAN_PKT.
> > But till then, I think we'd better keep it.
> 
> I think the packet_type feature would be more appropriate than a flag
> for carrying this kind of info.
> 
> Currently the behavior of PKT_RX_VLAN_PKT is not properly defined,
> and it is not the same on all PMDs. So, from an application
> perspective, it's not usable except if it knows that the underlying
> PMD is an ixgbe.

Yes, but it might be apps which do use that ixgbe functionality.

> This is not acceptable for a generic API and that's
> why I think this flag, as it is today, should be deprecated.

I suppose we can't deprecate existing functionality without
providing working alternative.
I agree there is no proper way to know right now which device
supports it, which not, but to me it means we should add such ability,
not deprecate existing and (I believe) useful functionality.

> 
> It won't prevent an application from using the flag right after my
> commit, but it will warn the user that the flag should not be used
> as is. If someone is willing to work on this feature for 16.11, why
> not but again, I think using the packet_type is more appropriate.

I am not against providing that information via packet_type.
What I am saying:
1) right now it is not here.
2) it might not that easy in terms of performance.
 
> The problem is that I don't want to have this flag in this state
> forever, and I also don't want to add in rte_mbuf.h a comment
> saying "this flag does this on ixgbe and that on other drivers".

Then we need either:
- implement it as ptype
- add user ability to query is that flag is supported by the underlying device.

> 
> If we decide to generalize the ixgbe behavior for all PMDs for this
> flag, it will break the applications relying on this flag but with
> other PMDs. So for the same reason we added a new PKT_RX_VLAN_STRIPPED
> we cannot change the behavior of an existing flag.

Ok, then let's make PKT_RX_VLAN_STRIPPED == PKT_RX_VLAN,
and assign new value to the  PKT_RX_VLAN.
Or have PKT_RX_VLAN_STRIPPED == PKT_RX_VLAN and create a new one:
PKT_RX_VLAN_PRESENT or so.
?

Konstantin

  reply	other threads:[~2016-06-13 16:31 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-04-21 23:36 [dpdk-dev] PKT_RX_VLAN_PKT when VLAN stripping is disabled John Daley (johndale)
2016-04-25 12:02 ` Ananyev, Konstantin
2016-04-25 13:50   ` Olivier Matz
2016-04-25 16:17     ` Ananyev, Konstantin
2016-04-26  0:16     ` John Daley (johndale)
2016-04-28 14:43       ` Olivier MATZ
2016-05-10 16:24         ` [dpdk-dev] [RFC] mbuf: new flag when vlan is stripped Olivier Matz
2016-05-12 20:36           ` John Daley (johndale)
2016-05-23  7:59             ` Olivier Matz
2016-05-23  8:46           ` [dpdk-dev] [PATCH] mbuf: new flag when Vlan " Olivier Matz
2016-05-23  8:59             ` Ananyev, Konstantin
2016-05-23  9:12               ` Olivier Matz
2016-05-23  9:23                 ` Ananyev, Konstantin
2016-05-23  9:38                   ` Olivier Matz
2016-05-23  9:20             ` Ananyev, Konstantin
2016-05-23  9:40               ` Olivier Matz
2016-05-27 14:33             ` [dpdk-dev] [PATCH v2] " Olivier Matz
2016-06-13 11:41               ` Olivier Matz
2016-06-13 14:42               ` Ananyev, Konstantin
2016-06-13 16:07                 ` Olivier Matz
2016-06-13 16:31                   ` Ananyev, Konstantin [this message]
2016-06-14  8:32                     ` Olivier MATZ
2016-06-14  9:15                       ` Ananyev, Konstantin
2016-06-14  9:34                         ` Olivier MATZ
2016-06-15 11:48               ` [dpdk-dev] [PATCH v3] " Olivier Matz
2016-06-15 12:33                 ` Ananyev, Konstantin
2016-06-15 15:20                   ` Thomas Monjalon

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=2601191342CEEE43887BDE71AB97725836B703CC@irsmsx105.ger.corp.intel.com \
    --to=konstantin.ananyev@intel.com \
    --cc=adrien.mazarguil@6wind.com \
    --cc=alejandro.lucero@netronome.com \
    --cc=dev@dpdk.org \
    --cc=helin.zhang@intel.com \
    --cc=johndale@cisco.com \
    --cc=olivier.matz@6wind.com \
    --cc=rahul.lakkireddy@chelsio.com \
    --cc=sony.chacko@qlogic.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).