From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Olivier MATZ <olivier.matz@6wind.com>
Cc: dev@dpdk.org, thomas@monjalon.net
Subject: Re: [dpdk-dev] [PATCH] mbuf: rename deprecated VLAN flags
Date: Tue, 24 Oct 2017 17:42:55 -0700 [thread overview]
Message-ID: <a3a453fe-d840-1964-06e7-5ed5774366a9@intel.com> (raw)
In-Reply-To: <20171024160922.fu7qiciondyikgvb@platinum>
On 10/24/2017 9:09 AM, Olivier MATZ wrote:
> Hi Ferruh,
>
> On Mon, Oct 23, 2017 at 07:08:25PM -0700, Ferruh Yigit wrote:
>> On 10/23/2017 5:16 AM, Olivier Matz wrote:
>>> PKT_RX_VLAN_PKT and PKT_RX_QINQ_PKT are deprecated for a while.
>>> As explained in [1], these flags were kept to let the applications and
>>> PMDs move to the new flag. There is also a need to support Rx vlan
>>> offload without vlan strip (at least for the ixgbe driver).
>>>
>>> This patch renames the old flags for this feature, knowing that some
>>> PMDs were using PKT_RX_VLAN_PKT and PKT_RX_QINQ_PKT to indicate that
>>> the vlan tci has been saved in the mbuf structure.
>>
>> So this patch merges two steps,
>> - remove old flags
>> - Add new flag to separate stripped and saved vlan info.
>>
>> Overall looks like a good idea.
>>
>> Just to confirm, in old usage, "PKT_RX_VLAN_PKT | PKT_RX_VLAN_STRIPPED",
>> both were meaning same thing, right. And "PKT_RX_VLAN_PKT" kept for backward
>> compatibility.
>> So previous "PKT_RX_VLAN_PKT" was way to say "stripped and saved", converting it
>> to "PKT_RX_VLAN | PKT_RX_VLAN_STRIPPED" as done in this patch should be safe.
>> Only may be missing cases that only "PKT_RX_VLAN" is valid.
>
> Before this patch:
> - PKT_RX_VLAN_PKT | PKT_RX_VLAN_STRIPPED: stripped and saved
> - PKT_RX_VLAN_STRIPPED: unused, but also means stripped and saved
> - PKT_RX_VLAN_PKT: undefined, but present in several drivers. In some of
> them, I think it means 'not stripped and saved'.
>
> After this patch:
> - PKT_RX_VLAN: not stripped and saved
> - PKT_RX_VLAN | PKT_RX_VLAN_STRIPPED: stripped and saved
> - PKT_RX_VLAN_STRIPPED: same (stripped and saved), PKT_RX_VLAN is implied
> I think we can forbid to have this flag alone.
What I understand is PKT_RX_VLAN_STRIPPED doesn't implies "PKT_RX_VLAN", but
both should be used together, only using "PKT_RX_VLAN_STRIPPED" is not valid,
this is OK too.
>
>> And is "PKT_RX_VLAN_STRIPPED" implies the "PKT_RX_VLAN"? This was mentioned is
>> the initial proposal, but it looks like that is not the case in this patch.
>
> Yes, I can clarify it in the documentation.
>
>> [...]
>>> This patch has no impact on applications that do not
>>> use the deprecated flags.
>>
>> I guess this is only true if "PKT_RX_VLAN_STRIPPED" implies the "PKT_RX_VLAN"
>
> From what I see, currently PKT_RX_VLAN_STRIPPED is always used
> with PKT_RX_VLAN_PKT in PMDs, so I think there will be not impact.
>
>>> For other applications, renaming
>>> the flag would restore the same behavior.
>>
>> Not sure, for an old application, using "PKT_RX_VLAN_PKT", can't just rename to
>> "PKT_RX_VLAN" and use as it is because now it doesn't say if vlan stripped or
>> not. So old applications seems may be effected, if so this may require to be
>> notified in advance.
>
> Old applications using PKT_RX_VLAN_PKT have been notified
> since 16.11 that the flag is deprecated.
>
> If the application is relying on the undocumented fact that
> PKT_RX_VLAN_PKT means "not stripped and saved" on some PMDs, it will
> continue to work as is after renaming.
OK.
>
>>> But, since we are
>>> close to the release, applying it early after the release could
>>> also be considered.
>>
>> Is there any benefit to be in this release, one think I can think of is 17.11
>> being LTS, is there any other?
>>
>> And what do you think doing in two steps,
>> - in this release remove deprecated flags
>> - in the beginning of the next release introduce the new flags
>
> I think one good side effect of having only one patch is that
> it doesn't break applications that were using an undocumented
> behavior of a PMD (it just requires a rename).
Makes sense, overall patch looks good to me, only concern is again making ethdev
layer update in rc2.
If there is no objection, and Thomas is also OK, lets get this into rc2.
Meanwhile patch requires update because of qede PMD update, would you mind
sending a new version based on latest next-net?
Thanks,
ferruh
>
> Olivier
>
next prev parent reply other threads:[~2017-10-25 0:42 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-23 12:16 Olivier Matz
2017-10-24 2:08 ` Ferruh Yigit
2017-10-24 16:09 ` Olivier MATZ
2017-10-25 0:42 ` Ferruh Yigit [this message]
2017-10-25 10:02 ` Olivier MATZ
2017-10-25 15:12 ` [dpdk-dev] [PATCH v2] " Olivier Matz
2017-10-25 17:42 ` Ferruh Yigit
2017-10-25 18:15 ` Ferruh Yigit
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=a3a453fe-d840-1964-06e7-5ed5774366a9@intel.com \
--to=ferruh.yigit@intel.com \
--cc=dev@dpdk.org \
--cc=olivier.matz@6wind.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
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).