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

["Van Haaren, Harry" <harry.van.haaren@intel.com>]

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.