Hi Olivier, Thanks for your comments.
> -----Original Message----- > From: Olivier Matz [mailto:olivier.m...@6wind.com] > Sent: Thursday, December 20, 2018 8:33 AM > To: Dumitrescu, Cristian <cristian.dumitre...@intel.com> > Cc: Pattan, Reshma <reshma.pat...@intel.com>; dev@dpdk.org; > jerin.ja...@caviumnetworks.com; Rao, Nikhil <nikhil....@intel.com>; Singh, > Jasvinder <jasvinder.si...@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
