diff mbox series

[v2] mbuf: fix compile by removing struct from function

Message ID 20190110180658.23302-1-harry.van.haaren@intel.com (mailing list archive)
State Superseded, archived
Headers
Series [v2] mbuf: fix compile by removing struct from function |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

Van Haaren, Harry Jan. 10, 2019, 6:06 p.m. UTC 
  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(-)

Comments

Cristian Dumitrescu Jan. 10, 2019, 6:40 p.m. UTC | #1 
> -----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
Gavin Hu Jan. 11, 2019, 3:01 a.m. UTC | #2 
> -----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.
Stephen Hemminger Jan. 11, 2019, 6:03 a.m. UTC | #3 
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.
Olivier Matz Jan. 11, 2019, 8:44 a.m. UTC | #4 
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
Van Haaren, Harry Jan. 11, 2019, 11:20 a.m. UTC | #5 
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.
diff mbox series

Patch

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