[dpdk-dev] [PATCH] mbuf: fix compile by making sched struct visible

[dpdk-dev] [PATCH] mbuf: fix compile by making sched struct visible

[Harry van Haaren]

Although C compilation works with the struct rte_mbuf_sched
declared inside the struct rte_mbuf namespace, C++ fails to
compile. This fix moves the rte_mbuf_sched struct up to the
global namespace, instead of declaring it inside the struct
mbuf namespace.

The struct rte_mbuf_sched is being used on the stack in
rte_mbuf_sched_get() and as a cast in _set(). For this
reason, it must be exposed as an available type.

Fixes: 5d3f72100904 ("mbuf: implement generic format for sched field")

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>

---

Cc: reshma.pattan@intel.com
Cc: cristian.dumitrescu@intel.com
Cc: thomas@monjalon.net

Hey folks,

Currently the mbuf header will fail to compile with a C++ compiler,
this patch is one possible solution. I'm not particularly happy with
this as a fix as it reduces mbuf struct readability, however it does
resolve the issue.

Regards, -Harry

---
 lib/librte_mbuf/rte_mbuf.h | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index bc562dc8a..a9df0cd00 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -468,6 +468,17 @@ __extension__
 typedef uint64_t MARKER64[0]; /**< marker that allows us to overwrite 8 bytes
                                * with a single assignment */
 
+struct rte_mbuf_sched {
+	uint32_t queue_id;   /**< Queue ID. */
+	uint8_t traffic_class;
+	/**< Traffic class ID. Traffic class 0
+	 * is the highest priority traffic class.
+	 */
+	uint8_t color;
+	/**< Color. @see enum rte_color.*/
+	uint16_t reserved;   /**< Reserved. */
+}; /**< Hierarchical scheduler */
+
 /**
  * The generic rte_mbuf, containing a packet mbuf.
  */
@@ -574,16 +585,7 @@ struct rte_mbuf {
 				 * on PKT_RX_FDIR_* flag in ol_flags.
 				 */
 			} fdir;	/**< Filter identifier if FDIR enabled */
-			struct rte_mbuf_sched {
-				uint32_t queue_id;   /**< Queue ID. */
-				uint8_t traffic_class;
-				/**< Traffic class ID. Traffic class 0
-				 * is the highest priority traffic class.
-				 */
-				uint8_t color;
-				/**< Color. @see enum rte_color.*/
-				uint16_t reserved;   /**< Reserved. */
-			} sched; /**< Hierarchical scheduler */
+			struct rte_mbuf_sched sched; /**< Hierarchical scheduler */
 			struct {
 				uint32_t reserved1;
 				uint16_t reserved2;
-- 
2.17.1

Re: [dpdk-dev] [PATCH] mbuf: fix compile by making sched struct visible

[Thomas Monjalon]

10/01/2019 17:50, Harry van Haaren:
> Although C compilation works with the struct rte_mbuf_sched
> declared inside the struct rte_mbuf namespace, C++ fails to
> compile. This fix moves the rte_mbuf_sched struct up to the
> global namespace, instead of declaring it inside the struct
> mbuf namespace.
> 
> The struct rte_mbuf_sched is being used on the stack in
> rte_mbuf_sched_get() and as a cast in _set(). For this
> reason, it must be exposed as an available type.
> 
> Fixes: 5d3f72100904 ("mbuf: implement generic format for sched field")
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> 
> ---
> 
> Cc: reshma.pattan@intel.com
> Cc: cristian.dumitrescu@intel.com
> Cc: thomas@monjalon.net
> 
> Hey folks,
> 
> Currently the mbuf header will fail to compile with a C++ compiler,
> this patch is one possible solution. I'm not particularly happy with
> this as a fix as it reduces mbuf struct readability, however it does
> resolve the issue.

What are the other possible solutions?

Re: [dpdk-dev] [PATCH] mbuf: fix compile by making sched struct visible

[Van Haaren, Harry]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, January 10, 2019 5:35 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>; Dumitrescu,
> Cristian <cristian.dumitrescu@intel.com>; olivier.matz@6wind.com
> Subject: Re: [PATCH] mbuf: fix compile by making sched struct visible
> 
> 10/01/2019 17:50, Harry van Haaren:
> > Although C compilation works with the struct rte_mbuf_sched
> > declared inside the struct rte_mbuf namespace, C++ fails to
> > compile. This fix moves the rte_mbuf_sched struct up to the
> > global namespace, instead of declaring it inside the struct
> > mbuf namespace.
> >
> > The struct rte_mbuf_sched is being used on the stack in
> > rte_mbuf_sched_get() and as a cast in _set(). For this
> > reason, it must be exposed as an available type.
> >
> > Fixes: 5d3f72100904 ("mbuf: implement generic format for sched field")
> >
> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> >
> > ---
> >
> > Cc: reshma.pattan@intel.com
> > Cc: cristian.dumitrescu@intel.com
> > Cc: thomas@monjalon.net
> >
> > Hey folks,
> >
> > Currently the mbuf header will fail to compile with a C++ compiler,
> > this patch is one possible solution. I'm not particularly happy with
> > this as a fix as it reduces mbuf struct readability, however it does
> > resolve the issue.
> 
> What are the other possible solutions?


I guess we could avoid using the struct in inline functions, by reading
or writing from the mbuf struct directly (without pulling out a temporary
rte_mbuf_sched field):

get()
*queue_id = m->hash.sched.queue_id;
...

set()
m->hash.sched.queue_id = queue_id;
...


I believe this was discussed when the patch was being reviewed;
http://patches.dpdk.org/patch/49126/

I have a mild preference to keep all mbuf structs visible inside the unions,
it makes it easier to understand the layout of mbuf, hence I prefer the
above pseudo code suggestion over the v1 patch sent before.

If others have a strong opinion for another solution, I'm ok with it.

[dpdk-dev] [PATCH v2] mbuf: fix compile by removing struct from function

[Harry van Haaren]

Although C compilation works with the struct rte_mbuf_sched
declared inside the struct rte_mbuf namespace, C++ fails to
compile.

This fix removes the temporary struct rte_mbuf_sched, instead
reading from the mbuf directly for each struct member. As the
struct is now not used directly, the C++ compiler doesn't need
to know about the struct, resolving the issue.

Fixes: 5d3f72100904 ("mbuf: implement generic format for sched field")

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>

---

Cc: reshma.pattan@intel.com
Cc: cristian.dumitrescu@intel.com
Cc: thomas@monjalon.net
Cc: olivier.matz@6wind.com

See mailing list for v1 discussion, perhaps this solution is more
readable due to leaving sched struct in-line in the mbuf struct.
---
 lib/librte_mbuf/rte_mbuf.h | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index bc562dc8a..1b260efd5 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -2344,11 +2344,9 @@ rte_mbuf_sched_get(const struct rte_mbuf *m, uint32_t *queue_id,
 			uint8_t *traffic_class,
 			uint8_t *color)
 {
-	struct rte_mbuf_sched sched = m->hash.sched;
-
-	*queue_id = sched.queue_id;
-	*traffic_class = sched.traffic_class;
-	*color = sched.color;
+	*queue_id = m->hash.sched.queue_id;
+	*traffic_class = m->hash.sched.traffic_class;
+	*color = m->hash.sched.color;
 }
 
 /**
@@ -2395,11 +2393,9 @@ rte_mbuf_sched_set(struct rte_mbuf *m, uint32_t queue_id,
 			uint8_t traffic_class,
 			uint8_t color)
 {
-	m->hash.sched = (struct rte_mbuf_sched){
-				.queue_id = queue_id,
-				.traffic_class = traffic_class,
-				.color = color,
-			};
+	m->hash.sched.queue_id = queue_id;
+	m->hash.sched.traffic_class = traffic_class;
+	m->hash.sched.color = color;
 }
 
 #ifdef __cplusplus
-- 
2.17.1

Re: [dpdk-dev] [PATCH v2] mbuf: fix compile by removing struct from function

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Van Haaren, Harry
> Sent: Thursday, January 10, 2019 6:07 PM
> To: dev@dpdk.org
> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Pattan, Reshma
> <reshma.pattan@intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>; thomas@monjalon.net;
> olivier.matz@6wind.com
> Subject: [PATCH v2] mbuf: fix compile by removing struct from function
> 
> Although C compilation works with the struct rte_mbuf_sched
> declared inside the struct rte_mbuf namespace, C++ fails to
> compile.
> 
> This fix removes the temporary struct rte_mbuf_sched, instead
> reading from the mbuf directly for each struct member. As the
> struct is now not used directly, the C++ compiler doesn't need
> to know about the struct, resolving the issue.
> 
> Fixes: 5d3f72100904 ("mbuf: implement generic format for sched field")
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> 
> ---
> 
> Cc: reshma.pattan@intel.com
> Cc: cristian.dumitrescu@intel.com
> Cc: thomas@monjalon.net
> Cc: olivier.matz@6wind.com
> 
> See mailing list for v1 discussion, perhaps this solution is more
> readable due to leaving sched struct in-line in the mbuf struct.
> ---
>  lib/librte_mbuf/rte_mbuf.h | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index bc562dc8a..1b260efd5 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -2344,11 +2344,9 @@ rte_mbuf_sched_get(const struct rte_mbuf *m,
> uint32_t *queue_id,
>  			uint8_t *traffic_class,
>  			uint8_t *color)
>  {
> -	struct rte_mbuf_sched sched = m->hash.sched;
> -
> -	*queue_id = sched.queue_id;
> -	*traffic_class = sched.traffic_class;
> -	*color = sched.color;
> +	*queue_id = m->hash.sched.queue_id;
> +	*traffic_class = m->hash.sched.traffic_class;
> +	*color = m->hash.sched.color;
>  }
> 
>  /**
> @@ -2395,11 +2393,9 @@ rte_mbuf_sched_set(struct rte_mbuf *m,
> uint32_t queue_id,
>  			uint8_t traffic_class,
>  			uint8_t color)
>  {
> -	m->hash.sched = (struct rte_mbuf_sched){
> -				.queue_id = queue_id,
> -				.traffic_class = traffic_class,
> -				.color = color,
> -			};
> +	m->hash.sched.queue_id = queue_id;
> +	m->hash.sched.traffic_class = traffic_class;
> +	m->hash.sched.color = color;
>  }
> 
>  #ifdef __cplusplus
> --
> 2.17.1

NAK.

I am fine with V1, but against this V2 due to the reasons previously discussed and agreed by Olivier [1] regarding performance. We should not sacrifice performance for the sake of cosmetics criteria that can met some other way.

In order to meet readability requirements from Olivier, I suggest we go back to V1 and we explicitly mention the size of the mbuf->sched field inslide the mbuf as 8 bytes:

struct rte_mbuf {
	...
	struct rte_mbuf_sched sched; /**< Hierarchical scheduler: 8 bytes */
	...
}

Olivier, is this a good compromise?

Regards,
Cristian

[1] https://mails.dpdk.org/archives/dev/2018-December/121806.html

Re: [dpdk-dev] [PATCH v2] mbuf: fix compile by removing struct from function

[Gavin Hu (Arm Technology China)]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Dumitrescu, Cristian
> Sent: Friday, January 11, 2019 2:40 AM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org
> Cc: Pattan, Reshma <reshma.pattan@intel.com>; thomas@monjalon.net;
> olivier.matz@6wind.com
> Subject: Re: [dpdk-dev] [PATCH v2] mbuf: fix compile by removing struct
> from function
>
>
>
> > -----Original Message-----
> > From: Van Haaren, Harry
> > Sent: Thursday, January 10, 2019 6:07 PM
> > To: dev@dpdk.org
> > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Pattan, Reshma
> > <reshma.pattan@intel.com>; Dumitrescu, Cristian
> > <cristian.dumitrescu@intel.com>; thomas@monjalon.net;
> > olivier.matz@6wind.com
> > Subject: [PATCH v2] mbuf: fix compile by removing struct from function
> >
> > Although C compilation works with the struct rte_mbuf_sched
> > declared inside the struct rte_mbuf namespace, C++ fails to
> > compile.
> >
> > This fix removes the temporary struct rte_mbuf_sched, instead
> > reading from the mbuf directly for each struct member. As the
> > struct is now not used directly, the C++ compiler doesn't need
> > to know about the struct, resolving the issue.
> >
> > Fixes: 5d3f72100904 ("mbuf: implement generic format for sched field")
> >
> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> >
> > ---
> >
> > Cc: reshma.pattan@intel.com
> > Cc: cristian.dumitrescu@intel.com
> > Cc: thomas@monjalon.net
> > Cc: olivier.matz@6wind.com
> >
> > See mailing list for v1 discussion, perhaps this solution is more
> > readable due to leaving sched struct in-line in the mbuf struct.
> > ---
> >  lib/librte_mbuf/rte_mbuf.h | 16 ++++++----------
> >  1 file changed, 6 insertions(+), 10 deletions(-)
> >
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index bc562dc8a..1b260efd5 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -2344,11 +2344,9 @@ rte_mbuf_sched_get(const struct rte_mbuf
> *m,
> > uint32_t *queue_id,
> >  uint8_t *traffic_class,
> >  uint8_t *color)
> >  {
> > -struct rte_mbuf_sched sched = m->hash.sched;

Did C++ complain *sched? *sched is better with less stack mem footprint.
If the pointer works, the code looks better.

> > -
> > -*queue_id = sched.queue_id;
> > -*traffic_class = sched.traffic_class;
> > -*color = sched.color;
> > +*queue_id = m->hash.sched.queue_id;
> > +*traffic_class = m->hash.sched.traffic_class;
> > +*color = m->hash.sched.color;
> >  }
> >
> >  /**
> > @@ -2395,11 +2393,9 @@ rte_mbuf_sched_set(struct rte_mbuf *m,
> > uint32_t queue_id,
> >  uint8_t traffic_class,
> >  uint8_t color)
> >  {
> > -m->hash.sched = (struct rte_mbuf_sched){
> > -.queue_id = queue_id,
> > -.traffic_class = traffic_class,
> > -.color = color,
> > -};
> > +m->hash.sched.queue_id = queue_id;
> > +m->hash.sched.traffic_class = traffic_class;
> > +m->hash.sched.color = color;
> >  }
> >
> >  #ifdef __cplusplus
> > --
> > 2.17.1
>
> NAK.
>
> I am fine with V1, but against this V2 due to the reasons previously discussed
> and agreed by Olivier [1] regarding performance. We should not sacrifice
> performance for the sake of cosmetics criteria that can met some other way.
>
> In order to meet readability requirements from Olivier, I suggest we go back
> to V1 and we explicitly mention the size of the mbuf->sched field inslide the
> mbuf as 8 bytes:
>
> struct rte_mbuf {
> ...
> struct rte_mbuf_sched sched; /**< Hierarchical scheduler: 8 bytes
> */
> ...
> }
>
> Olivier, is this a good compromise?
>
> Regards,
> Cristian
>
> [1] https://mails.dpdk.org/archives/dev/2018-December/121806.html

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [dpdk-dev] [PATCH v2] mbuf: fix compile by removing struct from function

[Stephen Hemminger]

On Fri, 11 Jan 2019 03:01:43 +0000
"Gavin Hu (Arm Technology China)" <Gavin.Hu@arm.com> wrote:

> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Dumitrescu, Cristian
> > Sent: Friday, January 11, 2019 2:40 AM
> > To: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org
> > Cc: Pattan, Reshma <reshma.pattan@intel.com>; thomas@monjalon.net;
> > olivier.matz@6wind.com
> > Subject: Re: [dpdk-dev] [PATCH v2] mbuf: fix compile by removing struct
> > from function
> >
> >
> >  
> > > -----Original Message-----
> > > From: Van Haaren, Harry
> > > Sent: Thursday, January 10, 2019 6:07 PM
> > > To: dev@dpdk.org
> > > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Pattan, Reshma
> > > <reshma.pattan@intel.com>; Dumitrescu, Cristian
> > > <cristian.dumitrescu@intel.com>; thomas@monjalon.net;
> > > olivier.matz@6wind.com
> > > Subject: [PATCH v2] mbuf: fix compile by removing struct from function
> > >
> > > Although C compilation works with the struct rte_mbuf_sched
> > > declared inside the struct rte_mbuf namespace, C++ fails to
> > > compile.
> > >
> > > This fix removes the temporary struct rte_mbuf_sched, instead
> > > reading from the mbuf directly for each struct member. As the
> > > struct is now not used directly, the C++ compiler doesn't need
> > > to know about the struct, resolving the issue.
> > >
> > > Fixes: 5d3f72100904 ("mbuf: implement generic format for sched field")
> > >
> > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> > >
> > > ---
> > >
> > > Cc: reshma.pattan@intel.com
> > > Cc: cristian.dumitrescu@intel.com
> > > Cc: thomas@monjalon.net
> > > Cc: olivier.matz@6wind.com
> > >
> > > See mailing list for v1 discussion, perhaps this solution is more
> > > readable due to leaving sched struct in-line in the mbuf struct.
> > > ---
> > >  lib/librte_mbuf/rte_mbuf.h | 16 ++++++----------
> > >  1 file changed, 6 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > > index bc562dc8a..1b260efd5 100644
> > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > @@ -2344,11 +2344,9 @@ rte_mbuf_sched_get(const struct rte_mbuf  
> > *m,  
> > > uint32_t *queue_id,
> > >  uint8_t *traffic_class,
> > >  uint8_t *color)
> > >  {
> > > -struct rte_mbuf_sched sched = m->hash.sched;  
> 
> Did C++ complain *sched? *sched is better with less stack mem footprint.
> If the pointer works, the code looks better.

Using *sched will cause compiler to generate multiple references which
is the real performance issue here.

Re: [dpdk-dev] [PATCH v2] mbuf: fix compile by removing struct from function

[Olivier Matz]

Hi,

On Thu, Jan 10, 2019 at 06:40:06PM +0000, Dumitrescu, Cristian wrote:
> 
> 
> > -----Original Message-----
> > From: Van Haaren, Harry
> > Sent: Thursday, January 10, 2019 6:07 PM
> > To: dev@dpdk.org
> > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Pattan, Reshma
> > <reshma.pattan@intel.com>; Dumitrescu, Cristian
> > <cristian.dumitrescu@intel.com>; thomas@monjalon.net;
> > olivier.matz@6wind.com
> > Subject: [PATCH v2] mbuf: fix compile by removing struct from function
> > 
> > Although C compilation works with the struct rte_mbuf_sched
> > declared inside the struct rte_mbuf namespace, C++ fails to
> > compile.
> > 
> > This fix removes the temporary struct rte_mbuf_sched, instead
> > reading from the mbuf directly for each struct member. As the
> > struct is now not used directly, the C++ compiler doesn't need
> > to know about the struct, resolving the issue.
> > 
> > Fixes: 5d3f72100904 ("mbuf: implement generic format for sched field")
> > 
> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> > 
> > ---
> > 
> > Cc: reshma.pattan@intel.com
> > Cc: cristian.dumitrescu@intel.com
> > Cc: thomas@monjalon.net
> > Cc: olivier.matz@6wind.com
> > 
> > See mailing list for v1 discussion, perhaps this solution is more
> > readable due to leaving sched struct in-line in the mbuf struct.
> > ---
> >  lib/librte_mbuf/rte_mbuf.h | 16 ++++++----------
> >  1 file changed, 6 insertions(+), 10 deletions(-)
> > 
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index bc562dc8a..1b260efd5 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -2344,11 +2344,9 @@ rte_mbuf_sched_get(const struct rte_mbuf *m,
> > uint32_t *queue_id,
> >  			uint8_t *traffic_class,
> >  			uint8_t *color)
> >  {
> > -	struct rte_mbuf_sched sched = m->hash.sched;
> > -
> > -	*queue_id = sched.queue_id;
> > -	*traffic_class = sched.traffic_class;
> > -	*color = sched.color;
> > +	*queue_id = m->hash.sched.queue_id;
> > +	*traffic_class = m->hash.sched.traffic_class;
> > +	*color = m->hash.sched.color;
> >  }
> > 
> >  /**
> > @@ -2395,11 +2393,9 @@ rte_mbuf_sched_set(struct rte_mbuf *m,
> > uint32_t queue_id,
> >  			uint8_t traffic_class,
> >  			uint8_t color)
> >  {
> > -	m->hash.sched = (struct rte_mbuf_sched){
> > -				.queue_id = queue_id,
> > -				.traffic_class = traffic_class,
> > -				.color = color,
> > -			};
> > +	m->hash.sched.queue_id = queue_id;
> > +	m->hash.sched.traffic_class = traffic_class;
> > +	m->hash.sched.color = color;
> >  }
> > 
> >  #ifdef __cplusplus
> > --
> > 2.17.1
> 
> NAK.
> 
> I am fine with V1, but against this V2 due to the reasons previously discussed and agreed by Olivier [1] regarding performance. We should not sacrifice performance for the sake of cosmetics criteria that can met some other way.
> 
> In order to meet readability requirements from Olivier, I suggest we go back to V1 and we explicitly mention the size of the mbuf->sched field inslide the mbuf as 8 bytes:
> 
> struct rte_mbuf {
> 	...
> 	struct rte_mbuf_sched sched; /**< Hierarchical scheduler: 8 bytes */
> 	...
> }
> 
> Olivier, is this a good compromise?

Looks good to me, yes.


Thanks

Re: [dpdk-dev] [PATCH v2] mbuf: fix compile by removing struct from function

[Van Haaren, Harry]

Converging discussion, +Stephen Hemminger wrote:

> I believe this was done so that the compiler doesn't generate
> bad code.
>
> If you reference the mbuf to get the fields then each operation becomes
> a load shift and mask operation to get to the bitfield.  But if they
> are local then this is all done on a single register value.
>
> Check the generated code.

I did :) I see no difference in generated assembly here, system has
GCC 7.3.0 with Meson release build and -g for debug symbols.


Anyway, as per below the consensus seems to be to just lift the struct
as per V1 with a note that the sched field is 8 bytes. I'll send v3 with
the comment updated so we can close this ASAP.



> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Friday, January 11, 2019 8:45 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@dpdk.org; Pattan,
> Reshma <reshma.pattan@intel.com>; thomas@monjalon.net
> Subject: Re: [PATCH v2] mbuf: fix compile by removing struct from function

<snip>

> > >  /**
> > > @@ -2395,11 +2393,9 @@ rte_mbuf_sched_set(struct rte_mbuf *m,
> > > uint32_t queue_id,
> > >  			uint8_t traffic_class,
> > >  			uint8_t color)
> > >  {
> > > -	m->hash.sched = (struct rte_mbuf_sched){
> > > -				.queue_id = queue_id,
> > > -				.traffic_class = traffic_class,
> > > -				.color = color,
> > > -			};
> > > +	m->hash.sched.queue_id = queue_id;
> > > +	m->hash.sched.traffic_class = traffic_class;
> > > +	m->hash.sched.color = color;
> > >  }
> > >
> > >  #ifdef __cplusplus
> > > --
> > > 2.17.1
> >
> > NAK.
> >
> > I am fine with V1, but against this V2 due to the reasons previously
> discussed and agreed by Olivier [1] regarding performance. We should not
> sacrifice performance for the sake of cosmetics criteria that can met some
> other way.
> >
> > In order to meet readability requirements from Olivier, I suggest we go
> back to V1 and we explicitly mention the size of the mbuf->sched field
> inslide the mbuf as 8 bytes:
> >
> > struct rte_mbuf {
> > 	...
> > 	struct rte_mbuf_sched sched; /**< Hierarchical scheduler: 8 bytes */
> > 	...
> > }
> >
> > Olivier, is this a good compromise?
> 
> Looks good to me, yes.
> 
> 
> Thanks

See above, I'll send v3 with updated comment.

[dpdk-dev] [PATCH v3] mbuf: fix compile by making sched struct visible

[Harry van Haaren]

Although C compilation works with the struct rte_mbuf_sched
declared inside the struct rte_mbuf namespace, C++ fails to
compile. This fix moves the rte_mbuf_sched struct up to the
global namespace, instead of declaring it inside the struct
mbuf namespace.

The struct rte_mbuf_sched is being used on the stack in
rte_mbuf_sched_get() and as a cast in _set(). For this
reason, it must be exposed as an available type.

Fixes: 5d3f72100904 ("mbuf: implement generic format for sched field")

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>

---

v3:
 - Update comment in mbuf to state size of struct sched (Crisitian)

v2:
 - Different solution, not applicable, v3 based on v1 (ML discussion)

Cc: reshma.pattan@intel.com
Cc: cristian.dumitrescu@intel.com
Cc: olivier.matz@6wind.com
Cc: thomas@monjalon.net
Cc: stephen@networkplumber.org
Cc: gavin.hu@arm.com
Cc: honnappa.nagarahalli@arm.com
---
 lib/librte_mbuf/rte_mbuf.h | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index bc562dc8a..d53b68253 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -468,6 +468,17 @@ __extension__
 typedef uint64_t MARKER64[0]; /**< marker that allows us to overwrite 8 bytes
                                * with a single assignment */
 
+struct rte_mbuf_sched {
+	uint32_t queue_id;   /**< Queue ID. */
+	uint8_t traffic_class;
+	/**< Traffic class ID. Traffic class 0
+	 * is the highest priority traffic class.
+	 */
+	uint8_t color;
+	/**< Color. @see enum rte_color.*/
+	uint16_t reserved;   /**< Reserved. */
+}; /**< Hierarchical scheduler */
+
 /**
  * The generic rte_mbuf, containing a packet mbuf.
  */
@@ -574,16 +585,8 @@ struct rte_mbuf {
 				 * on PKT_RX_FDIR_* flag in ol_flags.
 				 */
 			} fdir;	/**< Filter identifier if FDIR enabled */
-			struct rte_mbuf_sched {
-				uint32_t queue_id;   /**< Queue ID. */
-				uint8_t traffic_class;
-				/**< Traffic class ID. Traffic class 0
-				 * is the highest priority traffic class.
-				 */
-				uint8_t color;
-				/**< Color. @see enum rte_color.*/
-				uint16_t reserved;   /**< Reserved. */
-			} sched; /**< Hierarchical scheduler */
+			struct rte_mbuf_sched sched;
+			/**< Hierarchical scheduler : 8 bytes */
 			struct {
 				uint32_t reserved1;
 				uint16_t reserved2;
-- 
2.17.1

Re: [dpdk-dev] [PATCH v3] mbuf: fix compile by making sched struct visible

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Van Haaren, Harry
> Sent: Friday, January 11, 2019 11:33 AM
> To: dev@dpdk.org
> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Pattan, Reshma
> <reshma.pattan@intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>; olivier.matz@6wind.com;
> thomas@monjalon.net; stephen@networkplumber.org;
> gavin.hu@arm.com; honnappa.nagarahalli@arm.com
> Subject: [PATCH v3] mbuf: fix compile by making sched struct visible
> 
> Although C compilation works with the struct rte_mbuf_sched
> declared inside the struct rte_mbuf namespace, C++ fails to
> compile. This fix moves the rte_mbuf_sched struct up to the
> global namespace, instead of declaring it inside the struct
> mbuf namespace.
> 
> The struct rte_mbuf_sched is being used on the stack in
> rte_mbuf_sched_get() and as a cast in _set(). For this
> reason, it must be exposed as an available type.
> 
> Fixes: 5d3f72100904 ("mbuf: implement generic format for sched field")
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> 
> ---
> 
> v3:
>  - Update comment in mbuf to state size of struct sched (Crisitian)
> 
> v2:
>  - Different solution, not applicable, v3 based on v1 (ML discussion)
> 
> Cc: reshma.pattan@intel.com
> Cc: cristian.dumitrescu@intel.com
> Cc: olivier.matz@6wind.com
> Cc: thomas@monjalon.net
> Cc: stephen@networkplumber.org
> Cc: gavin.hu@arm.com
> Cc: honnappa.nagarahalli@arm.com
> ---
>  lib/librte_mbuf/rte_mbuf.h | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index bc562dc8a..d53b68253 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -468,6 +468,17 @@ __extension__
>  typedef uint64_t MARKER64[0]; /**< marker that allows us to overwrite 8
> bytes
>                                 * with a single assignment */
> 
> +struct rte_mbuf_sched {
> +	uint32_t queue_id;   /**< Queue ID. */
> +	uint8_t traffic_class;
> +	/**< Traffic class ID. Traffic class 0
> +	 * is the highest priority traffic class.
> +	 */
> +	uint8_t color;
> +	/**< Color. @see enum rte_color.*/
> +	uint16_t reserved;   /**< Reserved. */
> +}; /**< Hierarchical scheduler */
> +
>  /**
>   * The generic rte_mbuf, containing a packet mbuf.
>   */
> @@ -574,16 +585,8 @@ struct rte_mbuf {
>  				 * on PKT_RX_FDIR_* flag in ol_flags.
>  				 */
>  			} fdir;	/**< Filter identifier if FDIR enabled */
> -			struct rte_mbuf_sched {
> -				uint32_t queue_id;   /**< Queue ID. */
> -				uint8_t traffic_class;
> -				/**< Traffic class ID. Traffic class 0
> -				 * is the highest priority traffic class.
> -				 */
> -				uint8_t color;
> -				/**< Color. @see enum rte_color.*/
> -				uint16_t reserved;   /**< Reserved. */
> -			} sched; /**< Hierarchical scheduler */
> +			struct rte_mbuf_sched sched;
> +			/**< Hierarchical scheduler : 8 bytes */
>  			struct {
>  				uint32_t reserved1;
>  				uint16_t reserved2;
> --
> 2.17.1

Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

Of course, final say belongs to Olivier, so Olivier please let un know your vote.

Re: [dpdk-dev] [PATCH v3] mbuf: fix compile by making sched struct visible

[Olivier Matz]

On Fri, Jan 11, 2019 at 02:33:16PM +0000, Dumitrescu, Cristian wrote:
> 
> 
> > -----Original Message-----
> > From: Van Haaren, Harry
> > Sent: Friday, January 11, 2019 11:33 AM
> > To: dev@dpdk.org
> > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Pattan, Reshma
> > <reshma.pattan@intel.com>; Dumitrescu, Cristian
> > <cristian.dumitrescu@intel.com>; olivier.matz@6wind.com;
> > thomas@monjalon.net; stephen@networkplumber.org;
> > gavin.hu@arm.com; honnappa.nagarahalli@arm.com
> > Subject: [PATCH v3] mbuf: fix compile by making sched struct visible
> > 
> > Although C compilation works with the struct rte_mbuf_sched
> > declared inside the struct rte_mbuf namespace, C++ fails to
> > compile. This fix moves the rte_mbuf_sched struct up to the
> > global namespace, instead of declaring it inside the struct
> > mbuf namespace.
> > 
> > The struct rte_mbuf_sched is being used on the stack in
> > rte_mbuf_sched_get() and as a cast in _set(). For this
> > reason, it must be exposed as an available type.
> > 
> > Fixes: 5d3f72100904 ("mbuf: implement generic format for sched field")
> > 
> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> > 
> > ---
> > 
> > v3:
> >  - Update comment in mbuf to state size of struct sched (Crisitian)
> > 
> > v2:
> >  - Different solution, not applicable, v3 based on v1 (ML discussion)
> > 
> > Cc: reshma.pattan@intel.com
> > Cc: cristian.dumitrescu@intel.com
> > Cc: olivier.matz@6wind.com
> > Cc: thomas@monjalon.net
> > Cc: stephen@networkplumber.org
> > Cc: gavin.hu@arm.com
> > Cc: honnappa.nagarahalli@arm.com
> > ---
> >  lib/librte_mbuf/rte_mbuf.h | 23 +++++++++++++----------
> >  1 file changed, 13 insertions(+), 10 deletions(-)
> > 
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index bc562dc8a..d53b68253 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -468,6 +468,17 @@ __extension__
> >  typedef uint64_t MARKER64[0]; /**< marker that allows us to overwrite 8
> > bytes
> >                                 * with a single assignment */
> > 
> > +struct rte_mbuf_sched {
> > +	uint32_t queue_id;   /**< Queue ID. */
> > +	uint8_t traffic_class;
> > +	/**< Traffic class ID. Traffic class 0
> > +	 * is the highest priority traffic class.
> > +	 */
> > +	uint8_t color;
> > +	/**< Color. @see enum rte_color.*/
> > +	uint16_t reserved;   /**< Reserved. */
> > +}; /**< Hierarchical scheduler */
> > +
> >  /**
> >   * The generic rte_mbuf, containing a packet mbuf.
> >   */
> > @@ -574,16 +585,8 @@ struct rte_mbuf {
> >  				 * on PKT_RX_FDIR_* flag in ol_flags.
> >  				 */
> >  			} fdir;	/**< Filter identifier if FDIR enabled */
> > -			struct rte_mbuf_sched {
> > -				uint32_t queue_id;   /**< Queue ID. */
> > -				uint8_t traffic_class;
> > -				/**< Traffic class ID. Traffic class 0
> > -				 * is the highest priority traffic class.
> > -				 */
> > -				uint8_t color;
> > -				/**< Color. @see enum rte_color.*/
> > -				uint16_t reserved;   /**< Reserved. */
> > -			} sched; /**< Hierarchical scheduler */
> > +			struct rte_mbuf_sched sched;
> > +			/**< Hierarchical scheduler : 8 bytes */
> >  			struct {
> >  				uint32_t reserved1;
> >  				uint16_t reserved2;
> > --
> > 2.17.1
> 
> Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> 
> Of course, final say belongs to Olivier, so Olivier please let un know your vote.

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Re: [dpdk-dev] [PATCH v3] mbuf: fix compile by making sched struct visible

[Thomas Monjalon]

14/01/2019 15:58, Olivier Matz:
> On Fri, Jan 11, 2019 at 02:33:16PM +0000, Dumitrescu, Cristian wrote:
> > From: Van Haaren, Harry
> > > 
> > > Although C compilation works with the struct rte_mbuf_sched
> > > declared inside the struct rte_mbuf namespace, C++ fails to
> > > compile. This fix moves the rte_mbuf_sched struct up to the
> > > global namespace, instead of declaring it inside the struct
> > > mbuf namespace.
> > > 
> > > The struct rte_mbuf_sched is being used on the stack in
> > > rte_mbuf_sched_get() and as a cast in _set(). For this
> > > reason, it must be exposed as an available type.
> > > 
> > > Fixes: 5d3f72100904 ("mbuf: implement generic format for sched field")
> > > 
> > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> > > 
> > > ---
> > > 
> > > v3:
> > >  - Update comment in mbuf to state size of struct sched (Crisitian)
> > > 
> > > v2:
> > >  - Different solution, not applicable, v3 based on v1 (ML discussion)
> > 
> > Acked-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> > 
> > Of course, final say belongs to Olivier, so Olivier please let un know your vote.
> 
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Applied, thanks

Re: [dpdk-dev] [PATCH] mbuf: fix compile by making sched struct visible

[Stephen Hemminger]

On Thu, 10 Jan 2019 16:50:51 +0000
Harry van Haaren <harry.van.haaren@intel.com> wrote:

> Although C compilation works with the struct rte_mbuf_sched
> declared inside the struct rte_mbuf namespace, C++ fails to
> compile. This fix moves the rte_mbuf_sched struct up to the
> global namespace, instead of declaring it inside the struct
> mbuf namespace.
> 
> The struct rte_mbuf_sched is being used on the stack in
> rte_mbuf_sched_get() and as a cast in _set(). For this
> reason, it must be exposed as an available type.
> 
> Fixes: 5d3f72100904 ("mbuf: implement generic format for sched field")
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> 
I believe this was done so that the compiler doesn't generate
bad code.

If you reference the mbuf to get the fields then each operation becomes
a load shift and mask operation to get to the bitfield.  But if they
are local then this is all done on a single register value.

Check the generated code.

One solution would be to move this into a private header file
where C++ won't find it.