Re: [dpdk-dev] [PATCH] mbuf: rename deprecated VLAN flags

[Olivier MATZ <olivier.matz@6wind.com>]

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.

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

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

Olivier