From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>
Subject: Re: [PATCH v6 2/2] mbuf: implement generic format for
	sched field
Date: Thu, 20 Dec 2018 11:28:01 +0000
Message-ID: <3EB4FA525960D640B5BDFFD6A3D891268E817B4F@IRSMSX108.ger.corp.intel.com>
References: <20181219153418.52747-1-reshma.pattan@intel.com>
 <20181219154237.836-1-reshma.pattan@intel.com>
 <20181219154237.836-2-reshma.pattan@intel.com>
 <20181220082914.5hluj6nmn6s4rdrj@platinum>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Cc: "dev@dpdk.org" <dev@dpdk.org>, "jerin.jacob@caviumnetworks.com"
 <jerin.jacob@caviumnetworks.com>, "Rao, Nikhil" <nikhil.rao@intel.com>,
 "thomas@monjalon.net" <thomas@monjalon.net>, "Singh, Jasvinder"
 <jasvinder.singh@intel.com>
To: Olivier Matz <olivier.matz@6wind.com>, "Pattan, Reshma"
 <reshma.pattan@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mga14.intel.com (mga14.intel.com [192.55.52.115])
 by dpdk.org (Postfix) with ESMTP id 98C8F1BA57
 for <dev@dpdk.org>; Thu, 20 Dec 2018 12:28:04 +0100 (CET)
In-Reply-To: <20181220082914.5hluj6nmn6s4rdrj@platinum>
Content-Language: en-US
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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 */
>=20
>=20
> 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.
>=20
> I mean something like this:
>=20
> 			struct rte_mbuf_sched {
> 				uint32_t queue_id;      /**< Queue ID. */
> 				uint8_t traffic_class;
> 				/**< Traffic class ID (0 =3D 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 wi=
thin its parent struct rte_mbuf, then it would also fit my needs and I am m=
ore 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 =3D m->hash.sched;
> > +
> > +	*queue_id =3D sched.queue_id;
> > +	*traffic_class =3D sched.traffic_class;
> > +	*color =3D sched.color;
>=20
> I don't think there is a need to have an additional local copy.
>=20
> *queue_id =3D m->hash.sched.queue_id;
> *traffic_class =3D m->hash.sched.traffic_class;
> *color =3D m->hash.sched.color;
>=20

With local copy, compiler typically generates a single 8-byte read instruct=
ion. Without the local copy, compiler typically generates 3x read instructi=
ons.

The set/get functions are used in some performance critical actions, so thi=
s 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 =3D (struct rte_mbuf_sched){
> > +				.queue_id =3D queue_id,
> > +				.traffic_class =3D traffic_class,
> > +				.color =3D color,
> > +			};
>=20
> Why not this?
>=20
> m->hash.sched.queue_id =3D queue_id;
> m->hash.sched.traffic_class =3D traffic_class;
> m->hash.sched.color =3D color;
>=20

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

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

Thanks,
Cristian