[dpdk-dev,2/7] rte_sched: use reserved field to allow more VLAN's
Commit Message
From: Stephen Hemminger <shemming@brocade.com>
The QoS subport is limited to 8 bits in original code.
But customers demanded ability to support full number of VLAN's (4096)
therefore use reserved field of mbuf for this field instead
of packing inside other classify portions.
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
lib/librte_mbuf/rte_mbuf.h | 2 +-
lib/librte_sched/rte_sched.h | 31 ++++++++++++++++++++-----------
2 files changed, 21 insertions(+), 12 deletions(-)
Comments
Hi Stephen,
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Sunday, February 01, 2015 10:04 AM
> To: dev@dpdk.org
> Cc: Stephen Hemminger
> Subject: [dpdk-dev] [PATCH 2/7] rte_sched: use reserved field to allow more VLAN's
>
> From: Stephen Hemminger <shemming@brocade.com>
>
> The QoS subport is limited to 8 bits in original code.
> But customers demanded ability to support full number of VLAN's (4096)
> therefore use reserved field of mbuf for this field instead
> of packing inside other classify portions.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> lib/librte_mbuf/rte_mbuf.h | 2 +-
> lib/librte_sched/rte_sched.h | 31 ++++++++++++++++++++-----------
> 2 files changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index 16059c6..b6b08f4 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -242,7 +242,7 @@ struct rte_mbuf {
> uint16_t data_len; /**< Amount of data in segment buffer. */
> uint32_t pkt_len; /**< Total pkt len: sum of all segments. */
> uint16_t vlan_tci; /**< VLAN Tag Control Identifier (CPU order) */
> - uint16_t reserved;
> + uint16_t subport; /**< SCHED Subport ID */
As I remember, we keep these reserved 2 bytes for RX 2 double vlan tag offload.
So probably not a good idea to use it for something that is rte_sched specific.
If you really need extra space fo rte_sched fields inside mbuf, can't you move it into second cache line?
Or might be you can use userdata, to either store sched information directly, or as a pointer to some external memory location?
Another possibility - union mbuf.hash is 64bit now, while sched uses only 32bits.
So might be you can rearrange it to make sched 64bits too?
Something like:
union {
uint32_t rss; /**< RSS hash result if RSS enabled */
struct {
union {
struct {
uint16_t hash;
uint16_t id;
};
uint32_t lo;
/**< Second 4 flexible bytes */
};
uint32_t hi;
/**< First 4 flexible bytes or FD ID, dependent on
PKT_RX_FDIR_* flag in ol_flags. */
} fdir; /**< Filter identifier if FDIR enabled */
- uint32_t sched; /**< Hierarchical scheduler */
+ uint64_t sched; /**< Hierarchical scheduler */
uint32_t usr; /**< User defined tags. See @rte_distributor_p
rocess */
} hash; /**< hash information */
Konstantin
> union {
> uint32_t rss; /**< RSS hash result if RSS enabled */
> struct {
> diff --git a/lib/librte_sched/rte_sched.h b/lib/librte_sched/rte_sched.h
> index e6bba22..0688422 100644
> --- a/lib/librte_sched/rte_sched.h
> +++ b/lib/librte_sched/rte_sched.h
> @@ -195,15 +195,18 @@ struct rte_sched_port_params {
> #endif
> };
>
> -/** Path through the scheduler hierarchy used by the scheduler enqueue operation to
> -identify the destination queue for the current packet. Stored in the field hash.sched
> -of struct rte_mbuf of each packet, typically written by the classification stage and read by
> -scheduler enqueue.*/
> +/*
> + * Path through the scheduler hierarchy used by the scheduler enqueue
> + * operation to identify the destination queue for the current
> + * packet. Stored in the field pkt.hash.sched of struct rte_mbuf of
> + * each packet, typically written by the classification stage and read
> + * by scheduler enqueue.
> + */
> struct rte_sched_port_hierarchy {
> uint32_t queue:2; /**< Queue ID (0 .. 3) */
> uint32_t traffic_class:2; /**< Traffic class ID (0 .. 3)*/
> uint32_t pipe:20; /**< Pipe ID */
> - uint32_t subport:6; /**< Subport ID */
> + uint32_t extra:6; /**< currently unused */
> uint32_t color:2; /**< Color */
> };
>
> @@ -350,12 +353,15 @@ rte_sched_queue_read_stats(struct rte_sched_port *port,
> */
> static inline void
> rte_sched_port_pkt_write(struct rte_mbuf *pkt,
> - uint32_t subport, uint32_t pipe, uint32_t traffic_class, uint32_t queue, enum rte_meter_color color)
> + uint32_t subport, uint32_t pipe,
> + uint32_t traffic_class,
> + uint32_t queue, enum rte_meter_color color)
> {
> - struct rte_sched_port_hierarchy *sched = (struct rte_sched_port_hierarchy *) &pkt->hash.sched;
> + struct rte_sched_port_hierarchy *sched
> + = (struct rte_sched_port_hierarchy *) &pkt->hash.sched;
>
> + pkt->subport = subport;
> sched->color = (uint32_t) color;
> - sched->subport = subport;
> sched->pipe = pipe;
> sched->traffic_class = traffic_class;
> sched->queue = queue;
> @@ -379,11 +385,14 @@ rte_sched_port_pkt_write(struct rte_mbuf *pkt,
> *
> */
> static inline void
> -rte_sched_port_pkt_read_tree_path(struct rte_mbuf *pkt, uint32_t *subport, uint32_t *pipe, uint32_t *traffic_class, uint32_t
> *queue)
> +rte_sched_port_pkt_read_tree_path(struct rte_mbuf *pkt, uint32_t *subport,
> + uint32_t *pipe, uint32_t *traffic_class,
> + uint32_t *queue)
> {
> - struct rte_sched_port_hierarchy *sched = (struct rte_sched_port_hierarchy *) &pkt->hash.sched;
> + struct rte_sched_port_hierarchy *sched
> + = (struct rte_sched_port_hierarchy *) &pkt->hash.sched;
>
> - *subport = sched->subport;
> + *subport = pkt->subport;
> *pipe = sched->pipe;
> *traffic_class = sched->traffic_class;
> *queue = sched->queue;
> --
> 2.1.4
On Mon, 2 Feb 2015 14:21:58 +0000
"Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
> Hi Stephen,
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> > Sent: Sunday, February 01, 2015 10:04 AM
> > To: dev@dpdk.org
> > Cc: Stephen Hemminger
> > Subject: [dpdk-dev] [PATCH 2/7] rte_sched: use reserved field to allow more VLAN's
> >
> > From: Stephen Hemminger <shemming@brocade.com>
> >
> > The QoS subport is limited to 8 bits in original code.
> > But customers demanded ability to support full number of VLAN's (4096)
> > therefore use reserved field of mbuf for this field instead
> > of packing inside other classify portions.
> >
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> > lib/librte_mbuf/rte_mbuf.h | 2 +-
> > lib/librte_sched/rte_sched.h | 31 ++++++++++++++++++++-----------
> > 2 files changed, 21 insertions(+), 12 deletions(-)
> >
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index 16059c6..b6b08f4 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -242,7 +242,7 @@ struct rte_mbuf {
> > uint16_t data_len; /**< Amount of data in segment buffer. */
> > uint32_t pkt_len; /**< Total pkt len: sum of all segments. */
> > uint16_t vlan_tci; /**< VLAN Tag Control Identifier (CPU order) */
> > - uint16_t reserved;
> > + uint16_t subport; /**< SCHED Subport ID */
>
> As I remember, we keep these reserved 2 bytes for RX 2 double vlan tag offload.
> So probably not a good idea to use it for something that is rte_sched specific.
> If you really need extra space fo rte_sched fields inside mbuf, can't you move it into second cache line?
> Or might be you can use userdata, to either store sched information directly, or as a pointer to some external memory location?
> Another possibility - union mbuf.hash is 64bit now, while sched uses only 32bits.
> So might be you can rearrange it to make sched 64bits too?
> Something like:
>
> union {
> uint32_t rss; /**< RSS hash result if RSS enabled */
> struct {
> union {
> struct {
> uint16_t hash;
> uint16_t id;
> };
> uint32_t lo;
> /**< Second 4 flexible bytes */
> };
> uint32_t hi;
> /**< First 4 flexible bytes or FD ID, dependent on
> PKT_RX_FDIR_* flag in ol_flags. */
> } fdir; /**< Filter identifier if FDIR enabled */
> - uint32_t sched; /**< Hierarchical scheduler */
> + uint64_t sched; /**< Hierarchical scheduler */
> uint32_t usr; /**< User defined tags. See @rte_distributor_p
> rocess */
> } hash; /**< hash information */
Increasing the size of that union totally breaks other alignment and is a not starter.
The reserved field is not use upstream merged code and therefore is fair game.
First to claim it wins.
> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Monday, February 02, 2015 10:32 PM
> To: Ananyev, Konstantin
> Cc: dev@dpdk.org; Stephen Hemminger
> Subject: Re: [dpdk-dev] [PATCH 2/7] rte_sched: use reserved field to allow more VLAN's
>
> On Mon, 2 Feb 2015 14:21:58 +0000
> "Ananyev, Konstantin" <konstantin.ananyev@intel.com> wrote:
>
> > Hi Stephen,
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> > > Sent: Sunday, February 01, 2015 10:04 AM
> > > To: dev@dpdk.org
> > > Cc: Stephen Hemminger
> > > Subject: [dpdk-dev] [PATCH 2/7] rte_sched: use reserved field to allow more VLAN's
> > >
> > > From: Stephen Hemminger <shemming@brocade.com>
> > >
> > > The QoS subport is limited to 8 bits in original code.
> > > But customers demanded ability to support full number of VLAN's (4096)
> > > therefore use reserved field of mbuf for this field instead
> > > of packing inside other classify portions.
> > >
> > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > > ---
> > > lib/librte_mbuf/rte_mbuf.h | 2 +-
> > > lib/librte_sched/rte_sched.h | 31 ++++++++++++++++++++-----------
> > > 2 files changed, 21 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > > index 16059c6..b6b08f4 100644
> > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > @@ -242,7 +242,7 @@ struct rte_mbuf {
> > > uint16_t data_len; /**< Amount of data in segment buffer. */
> > > uint32_t pkt_len; /**< Total pkt len: sum of all segments. */
> > > uint16_t vlan_tci; /**< VLAN Tag Control Identifier (CPU order) */
> > > - uint16_t reserved;
> > > + uint16_t subport; /**< SCHED Subport ID */
> >
> > As I remember, we keep these reserved 2 bytes for RX 2 double vlan tag offload.
> > So probably not a good idea to use it for something that is rte_sched specific.
> > If you really need extra space fo rte_sched fields inside mbuf, can't you move it into second cache line?
> > Or might be you can use userdata, to either store sched information directly, or as a pointer to some external memory location?
> > Another possibility - union mbuf.hash is 64bit now, while sched uses only 32bits.
> > So might be you can rearrange it to make sched 64bits too?
> > Something like:
> >
> > union {
> > uint32_t rss; /**< RSS hash result if RSS enabled */
> > struct {
> > union {
> > struct {
> > uint16_t hash;
> > uint16_t id;
> > };
> > uint32_t lo;
> > /**< Second 4 flexible bytes */
> > };
> > uint32_t hi;
> > /**< First 4 flexible bytes or FD ID, dependent on
> > PKT_RX_FDIR_* flag in ol_flags. */
> > } fdir; /**< Filter identifier if FDIR enabled */
> > - uint32_t sched; /**< Hierarchical scheduler */
> > + uint64_t sched; /**< Hierarchical scheduler */
> > uint32_t usr; /**< User defined tags. See @rte_distributor_p
> > rocess */
> > } hash; /**< hash information */
>
> Increasing the size of that union totally breaks other alignment and is a not starter.
struct fdir already is 64bit width.
Though yes, we can't use uint64_t directly, as it would break alignment - totally forgot about it.
But nothing prevents you from doing:
struct { uint32_t lo, hi;} sched;
right?
>
> The reserved field is not use upstream merged code and therefore is fair game.
As you can see that reserved field lies inside first 16B from rx_descriptor_fields1;
So hopefully we will be able to load it from RX descriptors in one SSE load/store together with
other RXD fields.
Anyway these 16B are supposed to contain fields that are filled by RXD (as the name suggests).
> First to claim it wins.
Wins what?
Sorry, but you can't pollute mbuf structure with whatever you like.
So NACK for now.
Konstantin
@@ -242,7 +242,7 @@ struct rte_mbuf {
uint16_t data_len; /**< Amount of data in segment buffer. */
uint32_t pkt_len; /**< Total pkt len: sum of all segments. */
uint16_t vlan_tci; /**< VLAN Tag Control Identifier (CPU order) */
- uint16_t reserved;
+ uint16_t subport; /**< SCHED Subport ID */
union {
uint32_t rss; /**< RSS hash result if RSS enabled */
struct {
@@ -195,15 +195,18 @@ struct rte_sched_port_params {
#endif
};
-/** Path through the scheduler hierarchy used by the scheduler enqueue operation to
-identify the destination queue for the current packet. Stored in the field hash.sched
-of struct rte_mbuf of each packet, typically written by the classification stage and read by
-scheduler enqueue.*/
+/*
+ * Path through the scheduler hierarchy used by the scheduler enqueue
+ * operation to identify the destination queue for the current
+ * packet. Stored in the field pkt.hash.sched of struct rte_mbuf of
+ * each packet, typically written by the classification stage and read
+ * by scheduler enqueue.
+ */
struct rte_sched_port_hierarchy {
uint32_t queue:2; /**< Queue ID (0 .. 3) */
uint32_t traffic_class:2; /**< Traffic class ID (0 .. 3)*/
uint32_t pipe:20; /**< Pipe ID */
- uint32_t subport:6; /**< Subport ID */
+ uint32_t extra:6; /**< currently unused */
uint32_t color:2; /**< Color */
};
@@ -350,12 +353,15 @@ rte_sched_queue_read_stats(struct rte_sched_port *port,
*/
static inline void
rte_sched_port_pkt_write(struct rte_mbuf *pkt,
- uint32_t subport, uint32_t pipe, uint32_t traffic_class, uint32_t queue, enum rte_meter_color color)
+ uint32_t subport, uint32_t pipe,
+ uint32_t traffic_class,
+ uint32_t queue, enum rte_meter_color color)
{
- struct rte_sched_port_hierarchy *sched = (struct rte_sched_port_hierarchy *) &pkt->hash.sched;
+ struct rte_sched_port_hierarchy *sched
+ = (struct rte_sched_port_hierarchy *) &pkt->hash.sched;
+ pkt->subport = subport;
sched->color = (uint32_t) color;
- sched->subport = subport;
sched->pipe = pipe;
sched->traffic_class = traffic_class;
sched->queue = queue;
@@ -379,11 +385,14 @@ rte_sched_port_pkt_write(struct rte_mbuf *pkt,
*
*/
static inline void
-rte_sched_port_pkt_read_tree_path(struct rte_mbuf *pkt, uint32_t *subport, uint32_t *pipe, uint32_t *traffic_class, uint32_t *queue)
+rte_sched_port_pkt_read_tree_path(struct rte_mbuf *pkt, uint32_t *subport,
+ uint32_t *pipe, uint32_t *traffic_class,
+ uint32_t *queue)
{
- struct rte_sched_port_hierarchy *sched = (struct rte_sched_port_hierarchy *) &pkt->hash.sched;
+ struct rte_sched_port_hierarchy *sched
+ = (struct rte_sched_port_hierarchy *) &pkt->hash.sched;
- *subport = sched->subport;
+ *subport = pkt->subport;
*pipe = sched->pipe;
*traffic_class = sched->traffic_class;
*queue = sched->queue;