From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Dumitrescu, Cristian" Subject: Re: [PATCH 1/2] librte_sched: add post-init pipe profile api Date: Thu, 3 May 2018 15:29:52 +0000 Message-ID: <3EB4FA525960D640B5BDFFD6A3D891267BB654B4@IRSMSX108.ger.corp.intel.com> References: <20180309184114.139136-1-jasvinder.singh@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable To: "Singh, Jasvinder" , "dev@dpdk.org" Return-path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id B2FAEB62 for ; Thu, 3 May 2018 17:29:56 +0200 (CEST) In-Reply-To: <20180309184114.139136-1-jasvinder.singh@intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Jasvinder, Looks good, a few things to fix below: > -----Original Message----- > From: Singh, Jasvinder > Sent: Friday, March 9, 2018 6:41 PM > To: dev@dpdk.org > Cc: Dumitrescu, Cristian > Subject: [PATCH 1/2] librte_sched: add post-init pipe profile api >=20 > Add new API function to add more pipe configuration profiles > post initialization to the set of exisitng profiles specified during > the creation of scheduler port. >=20 > This API removes the current limitation that forces the user > to define the full set of pipe profiles as the part of port parameters > while port is being created. >=20 > Signed-off-by: Jasvinder Singh > --- > lib/librte_sched/rte_sched.c | 141 > +++++++++++++++++++++++++++++++++ > lib/librte_sched/rte_sched.h | 17 ++++ > lib/librte_sched/rte_sched_version.map | 6 ++ > 3 files changed, 164 insertions(+) >=20 > diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c > index 634486c..43728ec 100644 > --- a/lib/librte_sched/rte_sched.c > +++ b/lib/librte_sched/rte_sched.c > @@ -932,6 +932,147 @@ rte_sched_pipe_config(struct rte_sched_port > *port, > return 0; > } >=20 > +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]; > + } > +} I suggest a slightly different name: convert instead of get; I encourage us= ing dst and src as the names for the args, as it makes the code easier to f= ollow. Please call this function as part of rte_sched_port_config_pipe_profile_tab= le() 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; > + } > + Please put this checks into a new pipe_profile_check() function and call it= from rte_sched_port_check_params() to avoid code duplication. > + /* 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)); There is no need to use a temporary local copy, you can work straight on to= p of the real &port->pipe_profiles[port->n_pipe_profiles], as long as you o= nly increment port->n_pipe_profiles when you are completely sure everything= is OK. > + 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)); Based on the above comment of pp temp not needed, this memcpy can be elimin= ated. > + > + 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; Maybe move this port->n_pipe_profiles++ earlier, just after we performed th= e "pipe profile exists" check, easier to read the code. I would also put a = comment similar to "pipe profile commit" for this. > + > + 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); >=20 > /** > + * 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); Why is pipe_profile_id of type int32_t instead of uint32_t? The port-> n_pi= pe_profiles is uint32_t. > + > +/** > * 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; >=20 > } DPDK_2.0; > + > +DPDK_18.05 { > + global: > + > + rte_sched_pipe_profile_add; > +} DPDK_2.1; > -- We probably need to increment LIBABIVER in the Makefile and bump the .so nu= mber in release notes. > 2.9.3 Thanks very much! Regards, Cristian