[dpdk-dev] [PATCH v3 2/2] mbuf: implement generic format for sched field
Dumitrescu, Cristian
cristian.dumitrescu at intel.com
Thu Dec 20 12:19:53 CET 2018
Hi Olivier,
Thanks for your comments.
> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz at 6wind.com]
> Sent: Thursday, December 20, 2018 8:33 AM
> To: Dumitrescu, Cristian <cristian.dumitrescu at intel.com>
> Cc: Pattan, Reshma <reshma.pattan at intel.com>; dev at dpdk.org;
> jerin.jacob at caviumnetworks.com; Rao, Nikhil <nikhil.rao at intel.com>; Singh,
> Jasvinder <jasvinder.singh at 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
More information about the dev
mailing list