From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 3E1DF1B494 for ; Thu, 20 Dec 2018 09:29:21 +0100 (CET) Received: from rsa59-2-82-233-193-189.fbx.proxad.net ([82.233.193.189] helo=droids-corp.org) by mail.droids-corp.org with esmtpsa (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1gZtjN-000746-RM; Thu, 20 Dec 2018 09:30:59 +0100 Received: by droids-corp.org (sSMTP sendmail emulation); Thu, 20 Dec 2018 09:29:14 +0100 Date: Thu, 20 Dec 2018 09:29:14 +0100 From: Olivier Matz To: Reshma Pattan Cc: dev@dpdk.org, jerin.jacob@caviumnetworks.com, nikhil.rao@intel.com, thomas@monjalon.net, jasvinder.singh@intel.com, cristian.dumitrescu@intel.com Message-ID: <20181220082914.5hluj6nmn6s4rdrj@platinum> References: <20181219153418.52747-1-reshma.pattan@intel.com> <20181219154237.836-1-reshma.pattan@intel.com> <20181219154237.836-2-reshma.pattan@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20181219154237.836-2-reshma.pattan@intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v6 2/2] mbuf: implement generic format for sched field X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 20 Dec 2018 08:29:21 -0000 Hi Reshma, Please find few minor comments below. On Wed, Dec 19, 2018 at 03:42:37PM +0000, Reshma Pattan wrote: > This patch implements the changes proposed in the deprecation > notes [1][2]. > > librte_mbuf changes: > The mbuf::hash::sched field is updated to support generic I think it would be clearer to use mbuf->hash.sched (instead of ::). > definition in line with the ethdev TM and MTR APIs. The new generic > format contains: queue ID, traffic class, color. I wonder if it wouldn't be worth specifying "traffic manager" and "meter" instead of or in addition to TM and MTR. [...] > --- a/lib/librte_mbuf/rte_mbuf.h > +++ b/lib/librte_mbuf/rte_mbuf.h > @@ -468,6 +468,17 @@ __extension__ > typedef uint64_t MARKER64[0]; /**< marker that allows us to overwrite 8 bytes > * with a single assignment */ > > +struct rte_mbuf_sched { > + 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. */ > +}; > + > /** > * 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; I would require small changes, see below. > struct { > - uint32_t lo; > - uint32_t hi; > + uint32_t reserved1; > + uint16_t reserved2; > + uint16_t txq; > /**< The event eth Tx adapter uses this field > * to store Tx queue id. > * @see rte_event_eth_tx_adapter_txq_set() > */ > - } sched; /**< Hierarchical scheduler */ > + } txadapter; /**< Eventdev ethdev Tx adapter */ > /**< User defined tags. See rte_distributor_process() */ > uint32_t usr; > } hash; /**< hash information */ > @@ -2289,6 +2302,106 @@ rte_pktmbuf_linearize(struct rte_mbuf *mbuf) > */ > void rte_pktmbuf_dump(FILE *f, const struct rte_mbuf *m, unsigned dump_len); > > +/** > + * Get the value of mbuf sched queue_id field. > + */ > +static inline uint32_t > +rte_mbuf_sched_queue_get(const struct rte_mbuf *m) > +{ > + return m->hash.sched.queue_id; > +} > + > +/** > + * Get the value of mbuf sched traffic_class field. > + */ > +static inline uint8_t > +rte_mbuf_sched_traffic_class_get(const struct rte_mbuf *m) > +{ > + return m->hash.sched.traffic_class; > +} > + > +/** > + * Get the value of mbuf sched color field. > + */ > +static inline uint8_t > +rte_mbuf_sched_color_get(const struct rte_mbuf *m) > +{ > + return m->hash.sched.color; > +} > + > +/** > + * Get the values of mbuf sched queue_id, traffic_class and color. missing an empty comment line here. > + * @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; > +} > + > +/** > + * Set the mbuf sched queue_id to the defined value. > + */ > +static inline void > +rte_mbuf_sched_queue_set(struct rte_mbuf *m, uint32_t queue_id) > +{ > + m->hash.sched.queue_id = queue_id; > +} > + > +/** > + * Set the mbuf sched traffic_class id to the defined value. > + */ > +static inline void > +rte_mbuf_sched_traffic_class_set(struct rte_mbuf *m, uint8_t traffic_class) > +{ > + m->hash.sched.traffic_class = traffic_class; > +} > + > +/** > + * Set the mbuf sched color id to the defined value. > + */ > +static inline void > +rte_mbuf_sched_color_set(struct rte_mbuf *m, uint8_t color) > +{ > + m->hash.sched.color = color; > +} > + > +/** > + * Set the mbuf sched queue_id, traffic_class and color. empty comment missing > + * @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; Apart from this, the mbuf part looks ok to me. Thanks, Olivier