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
