DPDK patches and discussions
 help / color / mirror / Atom feed
From: Olivier MATZ <olivier.matz@6wind.com>
To: "John Daley (johndale)" <johndale@cisco.com>,
	"Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	helin.zhang@intel.com, adrien.mazarguil@6wind.com,
	rahul.lakkireddy@chelsio.com, alejandro.lucero@netronome.com,
	sony.chacko@qlogic.com
Subject: Re: [dpdk-dev] PKT_RX_VLAN_PKT when VLAN stripping is disabled
Date: Thu, 28 Apr 2016 16:43:31 +0200	[thread overview]
Message-ID: <57222193.2040907@6wind.com> (raw)
In-Reply-To: <cbd27918690048faaf8b76c1d1234cab@XCH-RCD-007.cisco.com>

Hi John,

(adding some PMD maintainers in the loop)

> From: Olivier Matz [mailto:olivier.matz@6wind.com]
>> It seems the meaning of the PKT_RX_VLAN_PKT bit depends on the port
>> configuration:
>> - if vlan stripping is configured, it means VLAN is present in vlan_tci
>>    mbuf field
>> - if not configured, it means a VLAN is present in the packet
>>
>> I don't think this is a good behavior since the application has to know the port
>> configuration to properly interpret the meaning of the flag.
>>
>> I suggest to change the meaning of this flag to: "vlan was stripped by
>> hardware, and vlan tag is now located in m->vlan_tci".
>>
>> The same could apply to PKT_RX_QINQ_PKT and m->outer_vlan_tci.
>>
>> We could add a new packet_type to tell if the mbuf is a VLAN/QinQ is
>> detected in the packet but not stripped.
>>
>> Example:
>>
>> - vlan stripping is disabled
>>
>>    - vlan packet recvd: flags=0, ptype=RTE_PTYPE_L2_ETHER_VLAN
>>    - qinq packet recvd: flags=0, ptype=RTE_PTYPE_L2_ETHER_QINQ
>>
>> - vlan stripping is enabled
>>
>>    - vlan packet recvd: flags=PKT_RX_VLAN_PKT,
>> ptype=RTE_PTYPE_L2_ETHER,
>>          m->vlan_tci=id
>>    - qinq packet recvd: flags=PKT_RX_VLAN_PKT|PKT_RX_QINQ_PKT,
>>          ptype=RTE_PTYPE_L2_ETHER, m->vlan_tci=id, m->vlan_tci_outer=id
>>
>>
>> Thoughts?

On 04/26/2016 02:16 AM, John Daley (johndale) wrote:
> I like the new packet types and how they work the same for VLAN and QINQ. Just
> so I understand your suggestion, X710 (as it seems to work today) would not set
> RTE_PTYPE_L2_ETHER_VLAN in dev_supported_ptypes_get() because it does not know
> how to determine that packet type in the receive path if stripping is disabled?
> But if stripping was enabled, the application could still trust m->vlan_tci if
> the flag was set?

Well, this depends on the exact meaning of the supported_ptype flags:
does it mean that the hw/driver *must* recognize and set the
ptype or does it mean that the hw/driver *can* recognize and set the
ptype?

In case a packet with 2 vlans is received, if the hw/driver is able
to strip the first one and recognize the second one, I would say
that RTE_PTYPE_L2_ETHER_VLAN should be set in the mbuf flags.


> Re changing the meaning of the PKT_RX_VLAN_PKT flag- I think it could cause hard
> to find errors and confusion. I would rather see the flag deprecated and a new
> one defined. Perhaps the flag could be called PKT_RX_VLAN_STRIPPED*.

Yes, good idea.


> Maybe another less elegant but more compatible solution would be just keep the
> Niantic behavior and fix other pmd's to match its behavior. For X710, with vlan
> stripping disabled this might mean looking at each packet's Ethernet type and
> set the flag accordingly.  It might not be too expensive since Ethernet type is
> in the 1st cacheline and hopefully prefetched. Thoughts?

I don't think the ixgbe behavior is correct. Today, depending on the
configuration (vlan stripped or not), the meaning of the flag is
different. This is not useable in an application that may use several
ports with different configurations.


> *In the future perhaps another flag could be added called
> PKT_RX_VLAN_TCI_VALID. This may not be the same as PKT_RX_VLAN_STRIPPED- enic
> and maybe some other nics are able to set vlan_tci even when not stripping vlan
> tags and this feature could be exposed with this separate flag.

At first read, I would say it's a bit overkill because it will
complexify the combinatorial dimension of the offload flags. But
if you agree, let's say it's another topic and focus on the fix
of PKT_RX_VLAN_PKT first.


Thanks,
Olivier


PS: please avoid top-posting, it's harder to read in a thread

  reply	other threads:[~2016-04-28 14:43 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-04-21 23:36 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 [this message]
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
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=57222193.2040907@6wind.com \
    --to=olivier.matz@6wind.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=konstantin.ananyev@intel.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).