Re: [RFC 08/13] mbuf: add fields for mirroring

[Stephen Hemminger <stephen@networkplumber.org>]

On Sat, 12 Apr 2025 11:59:10 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Saturday, 12 April 2025 01.45
> > 
> > Add field to union used for sched/event etc, for use when
> > an mbuf is mirrored.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >  lib/mbuf/rte_mbuf_core.h | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h
> > index a0df265b5d..1806dddd67 100644
> > --- a/lib/mbuf/rte_mbuf_core.h
> > +++ b/lib/mbuf/rte_mbuf_core.h
> > @@ -589,6 +589,14 @@ struct __rte_cache_aligned rte_mbuf {
> >  						 * @see
> > rte_event_eth_tx_adapter_txq_set()
> >  						 */
> >  					} txadapter; /**< Eventdev ethdev Tx
> > adapter */
> > +					struct rte_mbuf_mirror {
> > +						uint32_t orig_len;
> > +						uint16_t queue_id;
> > +						uint16_t direction;
> > +						/**< Port mirroring uses this to
> > store origin
> > +						 * @see rte_eth_mirror()
> > +						 */
> > +					} mirror;
> >  					uint32_t usr;
> >  					/**< User defined tags. See
> > rte_distributor_process() */
> >  				} hash;                   /**< hash information  
> 
> Stop overloading the "hash" field!
> 
> We now have dynfields. The mbuf structure's dedicated fields should be limited to absolute core features.
> 
> Long term, the "hash" field should be cleaned up.
> E.g. if we get rid of the Flow Director and make the 8 byte "sched" (Hierarchical Scheduler) a dynfield, the "hash" field can be reduced from 8 byte to 4 byte (RSS hash).
> 
> I acknowledge that some mbuf fields can be overloaded and thus used for multiple purposes - i.e. a value only used for ingress/forwarding (e.g. RSS hash) can share an mbuf field with a value only used for egress (e.g. Scheduler).
> 
> The overloading of the "hash" field is too much already. E.g. can the Hierarchical Scheduler be used together with the Eventdev ethdev Tx adapter, or are they mutually exclusive due to sharing the same mbuf field?
> 
> Going to the extreme, we would completely replace the "hash" field by dynfields.
> 
> In short: Overloading the "hash" field with port mirror information is a step in the wrong direction.

Short answer: Dynamic Fields are hard to work with primary/secondary process model.
The goal was to allow dumpcap to run and just work without modifications to the primary application.
If secondary creates dynamic field, the primary doesn't see it.

The hash field is not going away, flow director is stuck, it has been scheduled for removal
for 3 years and Intel still needs it. Other uses such as storing received hash value are
still needed.

Long answer:
It maybe possible. The patchset went through many revisions during development.
Ended up having to have MP server for start/stop, and if that code was extended
to allow secondary to proxy setting up mirror, then the code in handling mirror() on
primary could also setup the dynamic fields. But accessing dynamic fields is slower,
not that it matters that much if we have to copy mbuf anyway. Other option
would be to pre-pend a pseudo header that capture could then use.