Re: [PATCH] net: support VLAN stacking packet type parsing

[Bruce Richardson <bruce.richardson@intel.com>]

On Tue, Jul 08, 2025 at 05:07:05PM +0200, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@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:medvedkinv@gmail.com]
> > >    Sent: Monday, 7 July 2025 22.10
> > >
> > >
> > >    Hi Morten, all,
> > >
> > >
> > >
> > >    пн, 7 июл. 2025 г. в 19:09, Morten Brørup
> > >    <[1]mb@smartsharesystems.com>:
> > >
> > >      > From: Bruce Richardson [mailto:[2]bruce.richardson@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