Hi Olivier,

Thanks for your comments.

-----Original Message-----
From: Olivier Matz [mailto:olivier.matz@6wind.com]
Sent: Thursday, December 20, 2018 8:33 AM
To: Dumitrescu, Cristian <redacted>
Cc: Pattan, Reshma <redacted>; dev@dpdk.org;
jerin.jacob@caviumnetworks.com; Rao, Nikhil [off-list ref]; Singh,
Jasvinder [off-list ref]
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:

quoted

quoted

--- 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?

[...]

quoted

quoted

+/**
+ * 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:

quoted

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