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.m...@6wind.com] > Sent: Friday, January 11, 2019 8:45 AM > To: Dumitrescu, Cristian <cristian.dumitre...@intel.com> > Cc: Van Haaren, Harry <harry.van.haa...@intel.com>; dev@dpdk.org; Pattan, > Reshma <reshma.pat...@intel.com>; tho...@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.
