From: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>
To: Olivier Chirossel <olivier.chirossel@gmail.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] [PATCH v3] lib/librte_sched: fix update tc_credits
Date: Fri, 22 Sep 2017 15:52:07 +0000 [thread overview]
Message-ID: <3EB4FA525960D640B5BDFFD6A3D891267BABFB4E@IRSMSX108.ger.corp.intel.com> (raw)
In-Reply-To: <71309e54-c9b1-2a47-0114-a018c9047a21@intel.com>
>
> Signed-off-by: Olivier Chirossel <olivier.chirossel@gmail.com>
> ---
> doc/guides/prog_guide/qos_framework.rst | 10 ++-
> lib/librte_sched/rte_sched.c | 124
> ++++++++++++++++++++++++++------
> 2 files changed, 112 insertions(+), 22 deletions(-)
>
> diff --git a/doc/guides/prog_guide/qos_framework.rst
> b/doc/guides/prog_guide/qos_framework.rst
> index f3f60b8..e566ff9 100644
> --- a/doc/guides/prog_guide/qos_framework.rst
> +++ b/doc/guides/prog_guide/qos_framework.rst
> @@ -799,6 +799,9 @@ as described in :numref:`table_qos_10` and
> :numref:`table_qos_11`.
> | 4 | tc_credits | Bytes | Current upper limit for the
> number of credits that can be consumed by |
> | | | | the current traffic class for
> the remainder of the current |
> | | | | enforcement period.
> |
> + | | | | when The credits is update
> (every tc_period) the |
> + | | | | tc_credits_per_period is added
> to the value (tc_credits) if the new |
> + | | | | value is lower than tc_rate.
> else the value is set to tc_rate. |
> | | | |
> |
>
> +---+-----------------------+-------+------------------------------------------------------
> -----------------+
> @@ -819,8 +822,11 @@ as described in :numref:`table_qos_10` and
> :numref:`table_qos_11`.
> | | |
> |
> | | | if (time >= tc_time) {
> |
> | | |
> |
> - | | | tc_credits = tc_credits_per_period;
> |
> - | | |
> |
> + | | | if (tc_credits +
> tc_credits_per_period < tc_rate) { |
> + | | | tc_credits +=
> tc_credits_per_period |
> + | | | else {
> |
> + | | | tc_credits = tc_rate
> |
> + | | | }
> |
> | | | tc_time = time + tc_period;
> |
> | | |
> |
> | | | }
> |
> diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
> index b7cba11..732d5ef 100644
> --- a/lib/librte_sched/rte_sched.c
> +++ b/lib/librte_sched/rte_sched.c
> @@ -85,6 +85,7 @@ struct rte_sched_subport {
> /* Traffic classes (TCs) */
> uint64_t tc_time; /* time of next update */
> + uint32_t tc_rate[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> uint32_t
> tc_credits_per_period[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> uint32_t tc_credits[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> uint32_t tc_period;
> @@ -109,6 +110,7 @@ struct rte_sched_pipe_profile {
> uint32_t tb_size;
> /* Pipe traffic classes */
> + uint32_t tc_rate[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> uint32_t tc_period;
> uint32_t
> tc_credits_per_period[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> uint8_t tc_ov_weight;
> @@ -568,11 +570,14 @@ rte_sched_port_config_pipe_profile_table(struct
> rte_sched_port *port, struct rte
> /* Traffic Classes */
> dst->tc_period = rte_sched_time_ms_to_bytes(src-
> >tc_period,
> params->rate);
> -
> - for (j = 0; j < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; j++)
> +
> + for (j = 0; j < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; j++) {
> + dst->tc_rate[j] = src->tc_rate[j];
> dst->tc_credits_per_period[j]
> = rte_sched_time_ms_to_bytes(src-
> >tc_period,
> src->tc_rate[j]);
> + }
> +
> #ifdef RTE_SCHED_SUBPORT_TC_OV
> dst->tc_ov_weight = src->tc_ov_weight;
> @@ -838,6 +843,7 @@ rte_sched_subport_config(struct rte_sched_port
> *port,
> /* Traffic Classes (TCs) */
> s->tc_period = rte_sched_time_ms_to_bytes(params->tc_period,
> port->rate);
> for (i = 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE; i++) {
> + s->tc_rate[i] = params->tc_rate[i];
> s->tc_credits_per_period[i]
> = rte_sched_time_ms_to_bytes(params->tc_period,
> params->tc_rate[i]);
> @@ -1495,19 +1501,59 @@ grinder_credits_update(struct rte_sched_port
> *port, uint32_t pos)
> /* Subport TCs */
> if (unlikely(port->time >= subport->tc_time)) {
> - subport->tc_credits[0] = subport->tc_credits_per_period[0];
> - subport->tc_credits[1] = subport->tc_credits_per_period[1];
> - subport->tc_credits[2] = subport->tc_credits_per_period[2];
> - subport->tc_credits[3] = subport->tc_credits_per_period[3];
> + if (likely((subport->tc_credits[0] +
> subport->tc_credits_per_period[0]) < subport->tc_rate[0])) {
> + subport->tc_credits[0] += subport-
> >tc_credits_per_period[0];
> + }
> + else {
> + subport->tc_credits[0] = subport->tc_rate[0];
> + }
> + if (likely((subport->tc_credits[1] +
> subport->tc_credits_per_period[1]) < subport->tc_rate[1])) {
> + subport->tc_credits[1] += subport-
> >tc_credits_per_period[1];
> + }
> + else {
> + subport->tc_credits[1] = subport->tc_rate[1];
> + }
> + if (likely((subport->tc_credits[2] +
> subport->tc_credits_per_period[2]) < subport->tc_rate[2])) {
> + subport->tc_credits[2] += subport-
> >tc_credits_per_period[2];
> + }
> + else {
> + subport->tc_credits[2] = subport->tc_rate[2];
> + }
> + if (likely((subport->tc_credits[3] +
> subport->tc_credits_per_period[3]) < subport->tc_rate[3])) {
> + subport->tc_credits[3] += subport-
> >tc_credits_per_period[3];
> + }
> + else {
> + subport->tc_credits[3] = subport->tc_rate[3];
> + }
> subport->tc_time = port->time + subport->tc_period;
> }
> /* Pipe TCs */
> if (unlikely(port->time >= pipe->tc_time)) {
> - pipe->tc_credits[0] = params->tc_credits_per_period[0];
> - pipe->tc_credits[1] = params->tc_credits_per_period[1];
> - pipe->tc_credits[2] = params->tc_credits_per_period[2];
> - pipe->tc_credits[3] = params->tc_credits_per_period[3];
> + if (likely((pipe->tc_credits[0] +
> params->tc_credits_per_period[0]) < params->tc_rate[0])) {
> + pipe->tc_credits[0] += params-
> >tc_credits_per_period[0];
> + }
> + else {
> + pipe->tc_credits[0] = params->tc_rate[0];
> + }
> + if (likely((pipe->tc_credits[1] +
> params->tc_credits_per_period[1]) < params->tc_rate[1])) {
> + pipe->tc_credits[1] += params-
> >tc_credits_per_period[1];
> + }
> + else {
> + pipe->tc_credits[1] = params->tc_rate[1];
> + }
> + if (likely((pipe->tc_credits[2] +
> params->tc_credits_per_period[2]) < params->tc_rate[2])) {
> + pipe->tc_credits[2] += params-
> >tc_credits_per_period[2];
> + }
> + else {
> + pipe->tc_credits[2] = params->tc_rate[2];
> + }
> + if (likely((pipe->tc_credits[3] +
> params->tc_credits_per_period[3]) < params->tc_rate[3])) {
> + pipe->tc_credits[3] += params-
> >tc_credits_per_period[3];
> + }
> + else {
> + pipe->tc_credits[3] = params->tc_rate[3];
> + }
> pipe->tc_time = port->time + params->tc_period;
> }
> }
> @@ -1573,22 +1619,60 @@ grinder_credits_update(struct rte_sched_port
> *port, uint32_t pos)
> /* Subport TCs */
> if (unlikely(port->time >= subport->tc_time)) {
> subport->tc_ov_wm = grinder_tc_ov_credits_update(port,
> pos);
> -
> - subport->tc_credits[0] = subport->tc_credits_per_period[0];
> - subport->tc_credits[1] = subport->tc_credits_per_period[1];
> - subport->tc_credits[2] = subport->tc_credits_per_period[2];
> - subport->tc_credits[3] = subport->tc_credits_per_period[3];
> -
> + if (likely((subport->tc_credits[0] +
> subport->tc_credits_per_period[0]) < subport->tc_rate[0])) {
> + subport->tc_credits[0] += subport-
> >tc_credits_per_period[0];
> + }
> + else {
> + subport->tc_credits[0] = subport->tc_rate[0];
> + }
> + if (likely((subport->tc_credits[1] +
> subport->tc_credits_per_period[1]) < subport->tc_rate[1])) {
> + subport->tc_credits[1] += subport-
> >tc_credits_per_period[1];
> + }
> + else {
> + subport->tc_credits[1] = subport->tc_rate[1];
> + }
> + if (likely((subport->tc_credits[2] +
> subport->tc_credits_per_period[2]) < subport->tc_rate[2])) {
> + subport->tc_credits[2] += subport-
> >tc_credits_per_period[2];
> + }
> + else {
> + subport->tc_credits[2] = subport->tc_rate[2];
> + }
> + if (likely((subport->tc_credits[3] +
> subport->tc_credits_per_period[3]) < subport->tc_rate[3])) {
> + subport->tc_credits[3] += subport-
> >tc_credits_per_period[3];
> + }
> + else {
> + subport->tc_credits[3] = subport->tc_rate[3];
> + }
> subport->tc_time = port->time + subport->tc_period;
> subport->tc_ov_period_id++;
> }
> /* Pipe TCs */
> if (unlikely(port->time >= pipe->tc_time)) {
> - pipe->tc_credits[0] = params->tc_credits_per_period[0];
> - pipe->tc_credits[1] = params->tc_credits_per_period[1];
> - pipe->tc_credits[2] = params->tc_credits_per_period[2];
> - pipe->tc_credits[3] = params->tc_credits_per_period[3];
> + if (likely((pipe->tc_credits[0] +
> params->tc_credits_per_period[0]) < params->tc_rate[0])) {
> + pipe->tc_credits[0] += params-
> >tc_credits_per_period[0];
> + }
> + else {
> + pipe->tc_credits[0] = params->tc_rate[0];
> + }
> + if (likely((pipe->tc_credits[1] +
> params->tc_credits_per_period[1]) < params->tc_rate[1])) {
> + pipe->tc_credits[1] += params-
> >tc_credits_per_period[1];
> + }
> + else {
> + pipe->tc_credits[1] = params->tc_rate[1];
> + }
> + if (likely((pipe->tc_credits[2] +
> params->tc_credits_per_period[2]) < params->tc_rate[2])) {
> + pipe->tc_credits[2] += params-
> >tc_credits_per_period[2];
> + }
> + else {
> + pipe->tc_credits[2] = params->tc_rate[2];
> + }
> + if (likely((pipe->tc_credits[3] +
> params->tc_credits_per_period[3]) < params->tc_rate[3])) {
> + pipe->tc_credits[3] += params-
> >tc_credits_per_period[3];
> + }
> + else {
> + pipe->tc_credits[3] = params->tc_rate[3];
> + }
> pipe->tc_time = port->time + params->tc_period;
> }
> -- 2.7.4
>
Hi Olivier,
Thanks for your patch, I have reviewed it in detail, but I have to NAK it.
This patch might help for some particular case related to low rate pipe configuration, but it breaks everything else.
Right now, to avoid the MTU problem that you are describing, the solution for low rate pipe is to pick the tc_period in such a way that tc_credits_per_period is bigger than MTU.
Examples: tc_period = 40 ms, MTU = 1500 bytes
a) pipe rate = tc rate = 1 MB/s (high) => tc_credits_per_period = 40 kB > MTU (OK)
b) pipe rate = tc rate = 1 kB/s => tc_credits_per_period = 40 B < MTU (not OK)
c) pipe rate = tc rate = 40 kB/s => tc_credits_per_period = 1600 B ~= MTU (this is the threshold for minimum pipe rate for given tc_period)
I agree this could be improved, but your proposed fix does not achieve this; it only disables the tc rate enforcement mechanism by setting tc credits to a very high rate (i.e. tc_rate) that is not correlated with tc_period in any way.
So, NAK for now.
Regards,
Cristian
prev parent reply other threads:[~2017-09-22 15:52 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-06 13:44 [dpdk-dev] " Olivier Chirossel
2017-07-06 13:44 ` [dpdk-dev] [PATCH] " Olivier Chirossel
[not found] ` <71309e54-c9b1-2a47-0114-a018c9047a21@intel.com>
2017-09-22 15:52 ` Dumitrescu, Cristian [this message]
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=3EB4FA525960D640B5BDFFD6A3D891267BABFB4E@IRSMSX108.ger.corp.intel.com \
--to=cristian.dumitrescu@intel.com \
--cc=dev@dpdk.org \
--cc=olivier.chirossel@gmail.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).