From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Singh, Jasvinder" <jasvinder.singh@intel.com>
Subject: Re: [PATCH 1/2] librte_sched: add post-init pipe profile
	api
Date: Fri, 4 May 2018 08:41:42 +0000
Message-ID: <54CBAA185211B4429112C315DA58FF6D33452C06@IRSMSX103.ger.corp.intel.com>
References: <20180309184114.139136-1-jasvinder.singh@intel.com>
 <3EB4FA525960D640B5BDFFD6A3D891267BB654B4@IRSMSX108.ger.corp.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
To: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>, "dev@dpdk.org"
 <dev@dpdk.org>
Return-path: <dev-bounces@dpdk.org>
Received: from mga09.intel.com (mga09.intel.com [134.134.136.24])
 by dpdk.org (Postfix) with ESMTP id 92475DD2
 for <dev@dpdk.org>; Fri,  4 May 2018 10:41:46 +0200 (CEST)
In-Reply-To: <3EB4FA525960D640B5BDFFD6A3D891267BB654B4@IRSMSX108.ger.corp.intel.com>
Content-Language: en-US
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>


<snip>

> > +static void
> > +rte_sched_pipe_profile_get(struct rte_sched_port *port,
> > +	struct rte_sched_pipe_params *params,
> > +	struct rte_sched_pipe_profile *p)
> > +{
> > +	uint32_t i;
> > +
> > +	/* Token Bucket */
> > +	if (params->tb_rate =3D=3D port->rate) {
> > +		p->tb_credits_per_period =3D 1;
> > +		p->tb_period =3D 1;
> > +	} else {
> > +		double tb_rate =3D (double) params->tb_rate
> > +				/ (double) port->rate;
> > +		double d =3D RTE_SCHED_TB_RATE_CONFIG_ERR;
> > +
> > +		rte_approx(tb_rate, d,
> > +				   &p->tb_credits_per_period, &p-
> > >tb_period);
> > +	}
> > +
> > +	p->tb_size =3D params->tb_size;
> > +
> > +	/* Traffic Classes */
> > +	p->tc_period =3D rte_sched_time_ms_to_bytes(params->tc_period,
> > +		port->rate);
> > +
> > +	for (i =3D 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++)
> > +		p->tc_credits_per_period[i]
> > +			=3D rte_sched_time_ms_to_bytes(params->tc_period,
> > +				params->tc_rate[i]);
> > +
> > +#ifdef RTE_SCHED_SUBPORT_TC_OV
> > +	p->tc_ov_weight =3D params->tc_ov_weight; #endif
> > +
> > +	/* WRR */
> > +	for (i =3D 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++) {
> > +		uint32_t
> > wrr_cost[RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS];
> > +		uint32_t lcd, lcd1, lcd2;
> > +		uint32_t qindex;
> > +
> > +		qindex =3D i * RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS;
> > +
> > +		wrr_cost[0] =3D params->wrr_weights[qindex];
> > +		wrr_cost[1] =3D params->wrr_weights[qindex + 1];
> > +		wrr_cost[2] =3D params->wrr_weights[qindex + 2];
> > +		wrr_cost[3] =3D params->wrr_weights[qindex + 3];
> > +
> > +		lcd1 =3D rte_get_lcd(wrr_cost[0], wrr_cost[1]);
> > +		lcd2 =3D rte_get_lcd(wrr_cost[2], wrr_cost[3]);
> > +		lcd =3D rte_get_lcd(lcd1, lcd2);
> > +
> > +		wrr_cost[0] =3D lcd / wrr_cost[0];
> > +		wrr_cost[1] =3D lcd / wrr_cost[1];
> > +		wrr_cost[2] =3D lcd / wrr_cost[2];
> > +		wrr_cost[3] =3D lcd / wrr_cost[3];
> > +
> > +		p->wrr_cost[qindex] =3D (uint8_t) wrr_cost[0];
> > +		p->wrr_cost[qindex + 1] =3D (uint8_t) wrr_cost[1];
> > +		p->wrr_cost[qindex + 2] =3D (uint8_t) wrr_cost[2];
> > +		p->wrr_cost[qindex + 3] =3D (uint8_t) wrr_cost[3];
> > +	}
> > +}
>=20
> I suggest a slightly different name: convert instead of get; I encourage =
using dst
> and src as the names for the args, as it makes the code easier to follow.
>=20
> Please call this function as part of rte_sched_port_config_pipe_profile_t=
able()
> to avoid duplicating this code.


> > +
> > +int
> > +rte_sched_pipe_profile_add(struct rte_sched_port *port,
> > +	struct rte_sched_pipe_params *params,
> > +	int32_t *profile_id)
> > +{
> > +	struct rte_sched_pipe_profile pp;
> > +	uint32_t i;
> > +
> > +	/* Port */
> > +	if (port =3D=3D NULL)
> > +		return -1;
> > +
> > +	/* Pipe parameters */
> > +	if (params =3D=3D NULL)
> > +		return -2;
> > +
> > +	/* TB rate: non-zero, not greater than port rate */
> > +	if (params->tb_rate =3D=3D 0 ||
> > +		params->tb_rate > port->rate)
> > +		return -3;
> > +
> > +	/* TB size: non-zero */
> > +	if (params->tb_size =3D=3D 0)
> > +		return -4;
> > +
> > +	/* TC rate: non-zero, less than pipe rate */
> > +	for (i =3D 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++) {
> > +		if (params->tc_rate[i] =3D=3D 0 ||
> > +			params->tc_rate[i] > params->tb_rate)
> > +			return -5;
> > +	}
> > +
> > +	/* TC period: non-zero */
> > +	if (params->tc_period =3D=3D 0)
> > +		return -6;
> > +
> > +#ifdef RTE_SCHED_SUBPORT_TC_OV
> > +	/* TC3 oversubscription weight: non-zero */
> > +	if (params->tc_ov_weight =3D=3D 0)
> > +		return -7;
> > +#endif
> > +
> > +	/* Queue WRR weights: non-zero */
> > +	for (i =3D 0; i < RTE_SCHED_QUEUES_PER_PIPE; i++) {
> > +		if (params->wrr_weights[i] =3D=3D 0)
> > +			return -8;
> > +	}
> > +
>=20
> Please put this checks into a new pipe_profile_check() function and call =
it from
> rte_sched_port_check_params() to avoid code duplication.
>=20
> > +	/* Pipe profiles not exceeds the max limit */
> > +	if (port->n_pipe_profiles >=3D RTE_SCHED_PIPE_PROFILES_PER_PORT)
> > +		return -9;
> > +
> > +	memset(&pp, 0, sizeof(struct rte_sched_pipe_profile));
>=20
> There is no need to use a temporary local copy, you can work straight on =
top of
> the real &port->pipe_profiles[port->n_pipe_profiles], as long as you only
> increment port->n_pipe_profiles when you are completely sure everything i=
s
> OK.
>=20
> > +	rte_sched_pipe_profile_get(port, params, &pp);
> > +
> > +	/* Pipe profile not exists */
> > +	for (i =3D 0; i < port->n_pipe_profiles; i++) {
> > +		if (memcmp(port->pipe_profiles + i, &pp, sizeof(pp)) =3D=3D 0)
> > +			return -10;
> > +	}
> > +
> > +	/* Set port params */
> > +	memcpy(port->pipe_profiles + port->n_pipe_profiles, &pp,
> > sizeof(pp));
>=20
> Based on the above comment of pp temp not needed, this memcpy can be
> eliminated.
>=20
> > +
> > +	uint32_t pipe_tc3_rate =3D params->tc_rate[3];
> > +
> > +	if (port->pipe_tc3_rate_max < pipe_tc3_rate)
> > +		port->pipe_tc3_rate_max =3D pipe_tc3_rate;
> > +
> > +	*profile_id =3D port->n_pipe_profiles;
> > +	port->n_pipe_profiles +=3D 1;
>=20
> Maybe move this port->n_pipe_profiles++ earlier, just after we performed =
the
> "pipe profile exists" check, easier to read the code. I would also put a =
comment
> similar to "pipe profile commit" for this.
>=20
> > +
> > +	rte_sched_port_log_pipe_profile(port, *profile_id);
> > +
> > +	return 0;
> > +}
> > +
> >  void
> >  rte_sched_port_pkt_write(struct rte_mbuf *pkt,
> >  			 uint32_t subport, uint32_t pipe, uint32_t traffic_class,
> diff
> > --git a/lib/librte_sched/rte_sched.h b/lib/librte_sched/rte_sched.h
> > index 5d2a688..7edccbe 100644
> > --- a/lib/librte_sched/rte_sched.h
> > +++ b/lib/librte_sched/rte_sched.h
> > @@ -271,6 +271,23 @@ rte_sched_pipe_config(struct rte_sched_port
> > *port,
> >  	int32_t pipe_profile);
> >
> >  /**
> > + * Hierarchical scheduler pipe profile add
> > + *
> > + * @param port
> > + *   Handle to port scheduler instance
> > + * @param params
> > + *   Pipe configuration parameters
> > + * @param pipe_profile_id
> > + *   Set to valid profile id when profile is added successfully.
> > + * @return
> > + *   0 upon success, error code otherwise
> > + */
> > +int
> > +rte_sched_pipe_profile_add(struct rte_sched_port *port,
> > +	struct rte_sched_pipe_params *params,
> > +	int32_t *pipe_profile_id);
>=20
> Why is pipe_profile_id of type int32_t instead of uint32_t? The port->
> n_pipe_profiles is uint32_t.
>=20
> > +
> > +/**
> >   * Hierarchical scheduler memory footprint size per port
> >   *
> >   * @param params
> > diff --git a/lib/librte_sched/rte_sched_version.map
> > b/lib/librte_sched/rte_sched_version.map
> > index 3aa159a..b709cf8 100644
> > --- a/lib/librte_sched/rte_sched_version.map
> > +++ b/lib/librte_sched/rte_sched_version.map
> > @@ -29,3 +29,9 @@ DPDK_2.1 {
> >  	rte_sched_port_pkt_read_color;
> >
> >  } DPDK_2.0;
> > +
> > +DPDK_18.05 {
> > +	global:
> > +
> > +	rte_sched_pipe_profile_add;
> > +} DPDK_2.1;
> > --
>=20
> We probably need to increment LIBABIVER in the Makefile and bump the .so
> number in release notes.
>=20

I will revise the patch and send v2 with suggested changes. Thank you.

Jasvinder