[dpdk-dev] [PATCH v2] mbuf: fix compile by removing struct from function
Van Haaren, Harry
harry.van.haaren at intel.com
Fri Jan 11 12:20:30 CET 2019
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 at 6wind.com]
> Sent: Friday, January 11, 2019 8:45 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu at intel.com>
> Cc: Van Haaren, Harry <harry.van.haaren at intel.com>; dev at dpdk.org; Pattan,
> Reshma <reshma.pattan at intel.com>; thomas at 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.
More information about the dev
mailing list