> From: Stephen Hemminger [mailto:step...@networkplumber.org] > Sent: Sunday, 13 April 2025 16.32 > > On Sun, 13 Apr 2025 09:00:19 +0200 > Morten Brørup <m...@smartsharesystems.com> wrote: > > > > From: Stephen Hemminger [mailto:step...@networkplumber.org] > > > Sent: Saturday, 12 April 2025 18.57 > > > > > > On Sat, 12 Apr 2025 11:59:10 +0200 > > > Morten Brørup <m...@smartsharesystems.com> wrote: > > > > > > > > From: Stephen Hemminger [mailto:step...@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 <step...@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.
The secondary could provide the dynfield offset to the primary in the control plane when adding the mirror, either as part of the struct rte_eth_mirror, or by some other means before setting dev->data->rx/tx_mirror.
