Re: [dpdk-dev] [PATCH v6 2/2] mbuf: implement generic format for sched field

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

Hi Olivier,

Thanks for your reply!

<snip>

> >   * The generic rte_mbuf, containing a packet mbuf.
> >   */
> > @@ -574,14 +585,16 @@ struct rte_mbuf {
> >  				 * on PKT_RX_FDIR_* flag in ol_flags.
> >  				 */
> >  			} fdir;	/**< Filter identifier if FDIR enabled */
> > +			struct rte_mbuf_sched sched; /**< Hierarchical
> scheduler */
> 
> 
> What about directly embedding the structure like the others? Since mbuf
> is a very packed structure, I think it helps to show that rte_mbuf_sched
> does not exceed the size of the union.
> 
> I mean something like this:
> 
> 			struct rte_mbuf_sched {
> 				uint32_t queue_id;      /**< Queue ID. */
> 				uint8_t traffic_class;
> 				/**< Traffic class ID (0 = highest priority). */
> 				uint8_t color;
> 				/**< Color. @see enum rte_color. */
> 				uint16_t reserved;      /**< Reserved. */
> 			} sched;

If this syntax does not limit the scope of struct rte_mbuf_sched to just within its parent struct rte_mbuf, then it would also fit my needs and I am more than happy to use it.

All I need is a name for this rte_mbuf_sched structure, so I can use it to get a decent implementation of set/get functions.

<snip>

> > + * @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,
> > +			uint8_t *color)
> > +{
> > +	struct rte_mbuf_sched sched = m->hash.sched;
> > +
> > +	*queue_id = sched.queue_id;
> > +	*traffic_class = sched.traffic_class;
> > +	*color = sched.color;
> 
> I don't think there is a need to have an additional local copy.
> 
> *queue_id = m->hash.sched.queue_id;
> *traffic_class = m->hash.sched.traffic_class;
> *color = m->hash.sched.color;
> 

With local copy, compiler typically generates a single 8-byte read instruction. Without the local copy, compiler typically generates 3x read instructions.

The set/get functions are used in some performance critical actions, so this is the reason to make sure we get them right.

<snip>

> > + * @param m
> > + *   Mbuf to set
> > + * @param queue_id
> > + *  Queue id value to be set
> > + * @param traffic_class
> > + *  Traffic class id value to be set
> > + * @param color
> > + *  Color id to be set
> > + */
> > +static inline void
> > +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,
> > +			};
> 
> Why not this?
> 
> m->hash.sched.queue_id = queue_id;
> m->hash.sched.traffic_class = traffic_class;
> m->hash.sched.color = color;
> 

Same here, we need the compiler to generate a single 8-byte write instruction rather than 3x read-modify-write operations. Makes sense?

> 
> Apart from this, the mbuf part looks ok to me.
> 
> Thanks,
> Olivier

Thanks,
Cristian