[RFC,08/13] mbuf: add fields for mirroring
Checks
Commit Message
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(+)
Comments
> 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.
> */
> --
> 2.47.2
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.
> 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 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.
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.
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Sunday, 13 April 2025 16.32
>
> 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.
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.
@@ -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 */