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

[Stephen Hemminger <stephen@networkplumber.org>]

On Sun, 13 Apr 2025 09:00:19 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Saturday, 12 April 2025 18.57
> > 
> > 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.  
> 
> I skimmed the mbuf dynfield source code, and it looks like it is designed for primary/secondary process model.
> If the primary process doesn't see a dynfield created in a secondary process, it is a bug in the mbuf dynfield library. I couldn't find such a bug in Bugzilla.
> I would be much better to fix the bug than overloading the "hash" field.

The problem is that if secondary makes a new field, the primary still has to lookup the offset.
And don't want to do that in the packet path. Need to invoke a control path argument in the primary.
If primary always makes the dynamic field, there really is not much point in it being dynamic.