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: Tue, 14 Jun 2016 09:15:25 +0000 [thread overview]
Message-ID: <2601191342CEEE43887BDE71AB97725836B707E8@irsmsx105.ger.corp.intel.com> (raw)
In-Reply-To: <575FC133.3090205@6wind.com>
Hi Olivier
>
> Hi Konstantin,
>
> On 06/13/2016 06:31 PM, Ananyev, Konstantin wrote:
> >
> > 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.
> > ?
> >
>
> I think adding this new flag/packet_type is a new feature,
> because only ixgbe was behaving like this, and this was not
> documented. To me, marking the old flag as deprecated is
> a good compromise to keep the application relying on this
> working. If you feel the term "deprecated" is not adapted,
> we could reword it to something weaker.
Yes, that would do I think.
Basically my only concern that we will mark it as deprecated,
and then will remove it (as it is deprecated), without providing
anything new to replace it.
>
> We should try to not stay in that state too long,
Agree.
> and anybody willing to implement this feature would be welcome. For my
> part, this is not something I plan to do yet.
>
Ok, we'll see what we can do for 16.11.
But no hard promises right now either :)
Thanks
Konstantin
> Regards,
> Olivier
next prev parent reply other threads:[~2016-06-14 9:16 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
2016-06-14 8:32 ` Olivier MATZ
2016-06-14 9:15 ` Ananyev, Konstantin [this message]
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=2601191342CEEE43887BDE71AB97725836B707E8@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).