Re: [PATCH v20 25/25] app/pdump: preserve VLAN tags in captured packets

Bruce Richardson
2026-03-25T09:42:49Z

On Wed, Mar 25, 2026 at 10:36:56AM +0100, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Wednesday, 25 March 2026 10.13
> >
> > On Wed, Mar 25, 2026 at 08:41:39AM +0100, Morten Brørup wrote:
> > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > Sent: Tuesday, 24 March 2026 18.12
> > > >
> > > > On Mon, 16 Mar 2026 16:55:29 +0100
> > > > Morten Brørup <mb@smartsharesystems.com> wrote:
> > > >
> > > > > >
> > > > > > This is an example of something I previously flagged. Like with
> > > > real
> > > > > > hardware, I think the PMD should be inserting the VLAN tag into
> > the
> > > > > > packet
> > > > > > as part of the Tx function, not the prepare function.
> > > > >
> > > > > Agree with Bruce on this.
> > > > > For simple stuff like VLAN offload, applications should not be
> > > > required to call tx_prep first.
> > > > >
> > > > > However, the Tx function is supposed to not modify the packets;
> > > > relevant when refcnt > 1.
> > > > >
> > > > > Instead of modifying the packet data to insert/strip the VLAN
> > tag,
> > > > > perhaps the driver can split the write/read operation into
> > multiple
> > > > write/read operations:
> > > > > 1. the Ethernet header
> > > > > 2. the VLAN tag
> > > > > 3. the remaining packet data
> > > > >
> > > > > I haven't really followed the pcap driver, so maybe my suggestion
> > > > doesn't make sense.
> > > >
> > > > The prepare code and VLAN was copied from virtio.
> > > > I assume virtio is widely used already.
> > >
> > > OK, that makes it harder to object to.
> > >
> >
> > Yes, but I also believe that the topic was not previously discussed and
> > that the virtio driver may be wrong in how it behaves.
> >
> > I still think, for consistency with HW drivers, SW drivers should do
> > the
> > tagging in the Tx function.
>
> +1
>
> >
> > I also think that we should provide a DPDK-lib level helper function
> > (be it in
> > ethdev or elsewhere) for doing this sort of thing for all drivers. That
> > way
> > we can put in the necessary copying of packets with refcnt > 1 and have
> > it
> > apply globally.
>
> If an application clones packets instead of copying them, it is probably for performance reasons.
> If the drivers start copying those clones, it may defeat the performance purpose.
>
> <brainstorming>
> Maybe segmentation can be used instead of copying the full packet:
> Make the "copy" packet of two (or more) segments, where the header is copied into a new mbuf (where the VLAN tag is added), and the remaining part of the packet uses an indirect mbuf referring to the "original" packet at the offset after the header.
> </brainstorming>
>
> Furthermore...
> If drivers start copying packets in the Tx function, the Tx queue should have its own mbuf pool to allocate these mbufs from.
> Drivers should not steal mbufs from the pools used by the packets being transmitted.
> E.g. if a segmented packet has a small mbuf for the first few bytes, followed by a large mbuf (from another pool) for the remaining bytes.
> Or if the "original" mbuf comes from a mempool allocated on different CPU socket, the "copy" would too.
>
Yes, agree on using a chain of buffers for Tx in this case. Not bothered
much either way about the separate mempool.

/Bruce