[dpdk-dev] [PATCH v2] mbuf: new flag when Vlan is stripped

[Ananyev, Konstantin]

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