Re: [PATCH v3 2/2] mbuf: implement generic format for sched field

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

Hi Olivier,

Thanks for your comments.

> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Thursday, December 20, 2018 8:33 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> jerin.jacob@caviumnetworks.com; Rao, Nikhil <nikhil.rao@intel.com>; Singh,
> Jasvinder <jasvinder.singh@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 2/2] mbuf: implement generic format for
> sched field
> 
> Hi Cristian,
> 
> On Fri, Dec 14, 2018 at 10:52:18PM +0000, Dumitrescu, Cristian wrote:
> > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > @@ -34,6 +34,7 @@
> > >  #include <stdint.h>
> > >  #include <rte_compat.h>
> > >  #include <rte_common.h>
> > > +#include <rte_color.h>
> > >  #include <rte_config.h>
> > >  #include <rte_mempool.h>
> > >  #include <rte_memory.h>
> > > @@ -575,13 +576,24 @@ struct rte_mbuf {
> > >  				 */
> > >  			} fdir;	/**< Filter identifier if FDIR enabled */
> > >  			struct {
> > > -				uint32_t lo;
> > > -				uint32_t hi;
> > > +				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 */
> >
> > New idea: let's make this an explicit struct rte_mbuf_sched that we
> instantiate here: struct rte_mbuf_sched sched;
> 
> Sorry, I don't really agree here. I think having it inside the mbuf
> struct helps to estimate the size of the union here, and it would be
> less consistent with other fields.
> 

All I need is a name for this structure that I can use in some other parts of the code, i.e. for the set/get functions below.

I am not sure if we can declare and also instantiate this structure within the mbuf structure to fit bot my need and your preference. Basically, I am not sure if syntax like this is legal in C language; if it is, it would fit both purposes:

struct rte_mbuf {
	...
	struct rte_mbuf_sched {
		...
	} sched;
	...
};

Would this syntax limit the scope of struct rte_mbuf_sched just to within the struct rte_mbuf?

> 
> [...]
> 
> > > +/**
> > > + * Get the values of mbuf sched queue_id, traffic_class and color.
> > > + * @param m
> > > + *   Mbuf to read
> > > + * @param queue_id
> > > + *  Returns the queue id
> > > + * @param traffic_class
> > > + *  Returns the traffic class id
> > > + * @param color
> > > + *  Returns the colour id
> > > + */
> > > +static inline void
> > > +rte_mbuf_sched_get(const struct rte_mbuf *m, uint32_t *queue_id,
> > > +			uint8_t *traffic_class,
> > > +			enum rte_color *color)
> > > +{
> > > +	*queue_id = m->hash.sched.queue_id;
> > > +	*traffic_class = m->hash.sched.traffic_class;
> > > +	*color = (enum rte_color)m->hash.sched.color;
> >
> > For performance reasons, let's ask the compiler to read all sched fields in a
> single operation as opposed to 3:
> >
> > struct rte_mbuf_sched sched = m->hash.sched;
> > *queue_id = sched.queue_id;
> > *traffic_class = sched.traffic_class;
> > *color = (enum rte_colo)sched.color;
> 
> Are you sure it would me more efficient?

Yes, this is one of the reasons: this structure is 8-byte in size and this function is used in performance critical actions, so we need to read this structure in a single 8-byte read operation (my proposal) as opposed to compiler generating 3 separate read operations. Makes sense?

Same for the rte_mbuf_sched_set() function. 

> 
> 
> Thanks,
> Olivier

Regards,
Cristian