From mboxrd@z Thu Jan 1 00:00:00 1970 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) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 May 2018 08:29:55 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,359,1520924400"; d="scan'208";a="221370105" Received: from irsmsx110.ger.corp.intel.com ([163.33.3.25]) by orsmga005.jf.intel.com with ESMTP; 03 May 2018 08:29:54 -0700 Received: from irsmsx156.ger.corp.intel.com (10.108.20.68) by irsmsx110.ger.corp.intel.com (163.33.3.25) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 3 May 2018 16:29:53 +0100 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.150]) by IRSMSX156.ger.corp.intel.com ([169.254.3.233]) with mapi id 14.03.0319.002; Thu, 3 May 2018 16:29:53 +0100 From: "Dumitrescu, Cristian" To: "Singh, Jasvinder" , "dev@dpdk.org" Thread-Topic: [PATCH 1/2] librte_sched: add post-init pipe profile api Thread-Index: AQHTt9Y1agFN1gti+0eda+sDTGkBEaQecnsw 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> In-Reply-To: <20180309184114.139136-1-jasvinder.singh@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 1/2] librte_sched: add post-init pipe profile api 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, 03 May 2018 15:29:57 -0000 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