On Tue, Jul 08, 2025 at 04:15:36PM +0100, Bruce Richardson wrote: > 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. :-( >
So, I started looking into trying to clarify things a bit in the docs, and in the process of doing so found [1] in the mbuf docs. * - If both RTE_MBUF_F_RX_QINQ_STRIPPED and RTE_MBUF_F_RX_VLAN_STRIPPED are * set, the 2 VLANs have been stripped by the hardware and their TCIs are * saved in mbuf->vlan_tci (inner) and mbuf->vlan_tci_outer (outer). * - If RTE_MBUF_F_RX_QINQ_STRIPPED is set and RTE_MBUF_F_RX_VLAN_STRIPPED * is unset, only the outer VLAN is removed from packet data, but both tci * are saved in mbuf->vlan_tci (inner) and mbuf->vlan_tci_outer (outer). This documented behaviour seems to contradict our proposal above. However, is this the current validated behaviour of our NIC drivers? It also seems to make QINQ_STRIP the equivalent of VLAN strip if just one is specified. I wonder if the original intent when QinQ support was added was that vlan strip would strip tags with type 0x8100, and QinQ strip would strip tags with 0x88a8. /Bruce [1] https://doc.dpdk.org/api/rte__mbuf__core_8h.html#af720eb399a97e49e5e21959afa307a0f
