On Tue, Jul 08, 2025 at 05:07:05PM +0200, Morten Brørup wrote: > > From: Bruce Richardson [mailto:bruce.richard...@intel.com] > > Sent: Tuesday, 8 July 2025 12.16 > > > > On Tue, Jul 08, 2025 at 12:00:42AM +0200, Morten Brørup wrote: > > > From: Vladimir Medvedkin [mailto:medvedk...@gmail.com] > > > Sent: Monday, 7 July 2025 22.10 > > > > > > > > > Hi Morten, all, > > > > > > > > > > > > пн, 7 июл. 2025 г. в 19:09, Morten Brørup > > > <[1]m...@smartsharesystems.com>: > > > > > > > From: Bruce Richardson [mailto:[2]bruce.richard...@intel.com] > > > > Sent: Friday, 4 July 2025 13.32 > > > > Hi all, > > > > > > > > this email discussion comes at a bit of a fortunate time for > > me, > > > as I'm > > > > currently looking at our vlan tag/qinq stripping behaviour in > > our > > > Intel > > > > NIC > > > > drivers, and there is some discussion internally as to what our > > > driver > > > > behaviour should be compared to what it has historically been. > > :-) > > > > > > > > The documentation - both in the NIC guide [1] and the testpmd > > > guide [2] > > > > - > > > > is rather short on detail as to what exactly the behaviour > > should > > > be > > > > when > > > > vlan strip or qinq strip is implemented. Therefore, I'd hope > > that > > > those > > > > more familiar with networking than me would be able to help > > > clarify > > > > things > > > > so we can document the correct behaviour precisely - and > > hopefully > > > test > > > > our > > > > drivers against it in future! > > > > > > > > <snipping full discussion for brevity> > > > > So from the discussion, would the following be a good set of guidelines > > to > > document for correct driver behaviour: > > > > * VLAN-strip always strips one VLAN tag if available. If multiple VLAN > > tags > > are present, it strips the outer. > > Yes. > > > * QinQ strip, strips two VLAN tags if present. If only one tag is > > present it > > behaves as VLAN-strip. > > Yes. > > > * Specifying both VLAN-strip and QinQ strip is the same as QinQ strip > > alone (??) > > Yes. > > One more thought about this: > With QinQ stripping enabled, an mbuf for a single VLAN tagged packet will > have the RX_VLAN and RX_VLAN_STRIPPED flags set. > So, considering this output, it would be reasonable requiring that VLAN > stripping is also enabled when QinQ stripping is enabled. > > On the other hand, that might be a new requirement for applications. > So backwards compatibility speaks for making the VLAN stripping configuration > "don't care" when the QinQ stripping configuration is enabled. > > > > > Mbuf reporting behaviour: > > > > Input Traffic VLAN-strip on QinQ strip on > > -------------- ------------- ------------- > > Single VLAN pkts Tag in mbuf->vlan_tci Tag in mbuf->vlan_tci > > > > Double VLAN pkts Outer tag in vlan_tci Outer tag in > > vlan_tci_outer > > Inner tag in > > vlan_tci > > > > > > Does the above seem reasonable and correct? > > Yes. > > BTW: I think that having double tagged packets on a link configured for > single VLAN stripping is weird. > But describing the expected behavior is good! > > > > > My one (minor)concern would be the handling and placement of the single > > tag > > in the QinQ case. Depending on how the hardware treats a single tag in > > that > > mode, the data path may have to pay a penalty if the HW takes a single > > VLAN > > and places it in the "outer" position in the descriptor, since it needs > > to > > go in the "inner" position in the mbuf, necessitating some conditional > > logic. AFAIK (subject to me actually testing for confirmation), this > > will > > be the case for our Intel drivers. > > If that is the case, then maybe someone already thought about this when > designing the NIC HW, and came to a different conclusion than I did. > > Inspired by Vladimir's comments about QinQ packets with an S-TAG (Subscriber > tag) and no C-TAG (Customer tag), maybe we should consider the alternative: > When configured for QinQ stripping, the first tag is always considered the > S-TAG, and thus always goes to the vlan_tci_outer, and the second (inner) tag > is optional, and goes to the vlan_tci if present. > > <feature creep> > When configured for QinQ stripping, we could control the single VLAN tag > placement through the VLAN stripping configuration: > With VLAN stripping also enabled, the link is considered a "super hybrid", > and the VLAN ID of single VLAN tagged packets goes into vlan_tci (being a > normal VLAN tagged packet). > With VLAN stripping disabled, the link is considered a pure QinQ trunk, and > the VLAN ID of single VLAN tagged packets goes into vlan_tci_outer (being the > S-TAG of a QinQ packet with no C-TAG). > </feature creep> > > But again, this only works when VLAN/QinQ stripping is enabled. Into which > fields the various VLAN tags are written when VLAN/QinQ stripping is disabled > will be fixed/hardcoded. > I'm not even sure the vlan_tci and vlan_tci_outer fields are filled when > stripping is disabled. But there are separate mbuf flags for VLAN presence > and VLAN stripped (and QinQ presence and QinQ stripped), so it could be > supported. > > Furthermore, in favor of my original suggestion, consider TX: > TX of an mbuf with a single VLAN tag doesn't know about QinQ with no C-TAG, > and thus looks at vlan_tci when transmitting such packets. > If we like symmetry, RX should behave similarly. > > I'm leaning towards my original suggestion, but I don't have a strong opinion > about this. > There are good arguments for both solutions, either vlan_tci or > vlan_tci_outer for single VLAN tagged packets received on a link configured > for QinQ. > I'd tend toward your original suggestion too, where single vlan always gets put in vlan_tci field if stripped.
I think if we were to go back and change things is would be to not have "vlan_tci_outer" field, but have a "vlan_tci_2" field, where the *second* vlan tag, if present, is put. That would mean that the behaviour would be unambiguous and the field would only ever be under consideration for double-vlan packets with QinQ strip enabled. Sadly, I think that would be too big a change to make to the mbuf at this stage. :-( /Bruce
