From: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>
To: "Singh, Jasvinder" <jasvinder.singh@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH 1/2] librte_sched: add post-init pipe profile api
Date: Thu, 3 May 2018 15:29:52 +0000 [thread overview]
Message-ID: <3EB4FA525960D640B5BDFFD6A3D891267BB654B4@IRSMSX108.ger.corp.intel.com> (raw)
In-Reply-To: <20180309184114.139136-1-jasvinder.singh@intel.com>
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 <cristian.dumitrescu@intel.com>
> Subject: [PATCH 1/2] librte_sched: add post-init pipe profile api
>
> 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.
>
> 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.
>
> Signed-off-by: Jasvinder Singh <jasvinder.singh@intel.com>
> ---
> 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(+)
>
> 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;
> }
>
> +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 == port->rate) {
> + p->tb_credits_per_period = 1;
> + p->tb_period = 1;
> + } else {
> + double tb_rate = (double) params->tb_rate
> + / (double) port->rate;
> + double d = RTE_SCHED_TB_RATE_CONFIG_ERR;
> +
> + rte_approx(tb_rate, d,
> + &p->tb_credits_per_period, &p-
> >tb_period);
> + }
> +
> + p->tb_size = params->tb_size;
> +
> + /* Traffic Classes */
> + p->tc_period = rte_sched_time_ms_to_bytes(params->tc_period,
> + port->rate);
> +
> + for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++)
> + p->tc_credits_per_period[i]
> + = rte_sched_time_ms_to_bytes(params->tc_period,
> + params->tc_rate[i]);
> +
> +#ifdef RTE_SCHED_SUBPORT_TC_OV
> + p->tc_ov_weight = params->tc_ov_weight;
> +#endif
> +
> + /* WRR */
> + for (i = 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 = i * RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS;
> +
> + wrr_cost[0] = params->wrr_weights[qindex];
> + wrr_cost[1] = params->wrr_weights[qindex + 1];
> + wrr_cost[2] = params->wrr_weights[qindex + 2];
> + wrr_cost[3] = params->wrr_weights[qindex + 3];
> +
> + lcd1 = rte_get_lcd(wrr_cost[0], wrr_cost[1]);
> + lcd2 = rte_get_lcd(wrr_cost[2], wrr_cost[3]);
> + lcd = rte_get_lcd(lcd1, lcd2);
> +
> + wrr_cost[0] = lcd / wrr_cost[0];
> + wrr_cost[1] = lcd / wrr_cost[1];
> + wrr_cost[2] = lcd / wrr_cost[2];
> + wrr_cost[3] = lcd / wrr_cost[3];
> +
> + p->wrr_cost[qindex] = (uint8_t) wrr_cost[0];
> + p->wrr_cost[qindex + 1] = (uint8_t) wrr_cost[1];
> + p->wrr_cost[qindex + 2] = (uint8_t) wrr_cost[2];
> + p->wrr_cost[qindex + 3] = (uint8_t) wrr_cost[3];
> + }
> +}
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.
Please call this function as part of rte_sched_port_config_pipe_profile_table() 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 == NULL)
> + return -1;
> +
> + /* Pipe parameters */
> + if (params == NULL)
> + return -2;
> +
> + /* TB rate: non-zero, not greater than port rate */
> + if (params->tb_rate == 0 ||
> + params->tb_rate > port->rate)
> + return -3;
> +
> + /* TB size: non-zero */
> + if (params->tb_size == 0)
> + return -4;
> +
> + /* TC rate: non-zero, less than pipe rate */
> + for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++) {
> + if (params->tc_rate[i] == 0 ||
> + params->tc_rate[i] > params->tb_rate)
> + return -5;
> + }
> +
> + /* TC period: non-zero */
> + if (params->tc_period == 0)
> + return -6;
> +
> +#ifdef RTE_SCHED_SUBPORT_TC_OV
> + /* TC3 oversubscription weight: non-zero */
> + if (params->tc_ov_weight == 0)
> + return -7;
> +#endif
> +
> + /* Queue WRR weights: non-zero */
> + for (i = 0; i < RTE_SCHED_QUEUES_PER_PIPE; i++) {
> + if (params->wrr_weights[i] == 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 >= 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 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 is OK.
> + rte_sched_pipe_profile_get(port, params, &pp);
> +
> + /* Pipe profile not exists */
> + for (i = 0; i < port->n_pipe_profiles; i++) {
> + if (memcmp(port->pipe_profiles + i, &pp, sizeof(pp)) == 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 eliminated.
> +
> + uint32_t pipe_tc3_rate = params->tc_rate[3];
> +
> + if (port->pipe_tc3_rate_max < pipe_tc3_rate)
> + port->pipe_tc3_rate_max = pipe_tc3_rate;
> +
> + *profile_id = port->n_pipe_profiles;
> + port->n_pipe_profiles += 1;
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.
> +
> + 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);
Why is pipe_profile_id of type int32_t instead of uint32_t? The port-> n_pipe_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;
>
> } 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 number in release notes.
> 2.9.3
Thanks very much!
Regards,
Cristian
next prev parent reply other threads:[~2018-05-03 15:29 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-09 18:41 Jasvinder Singh
2018-03-09 18:41 ` [dpdk-dev] [PATCH 2/2] test/sched: add test for pipe profile add api Jasvinder Singh
2018-05-03 15:29 ` Dumitrescu, Cristian [this message]
2018-05-04 8:41 ` [dpdk-dev] [PATCH 1/2] librte_sched: add post-init pipe profile api Singh, Jasvinder
2018-05-04 14:10 ` [dpdk-dev] [PATCH v2] " Jasvinder Singh
2018-05-04 14:29 ` Dumitrescu, Cristian
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3EB4FA525960D640B5BDFFD6A3D891267BB654B4@IRSMSX108.ger.corp.intel.com \
--to=cristian.dumitrescu@intel.com \
--cc=dev@dpdk.org \
--cc=jasvinder.singh@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).