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
