Re: [PATCH] [PATCH v3] lib/librte_sched: fix update tc_credits
From: Dumitrescu, Cristian <hidden>
Date: 2017-09-22 15:52:11
quoted hunk
Signed-off-by: Olivier Chirossel <redacted> --- 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.rstb/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_ttc_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(structrte_sched_port *port, struct rte /* Traffic Classes */ dst->tc_period = rte_sched_time_ms_to_bytes(src-quoted
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-quoted
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-quoted
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-quoted
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-quoted
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-quoted
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-quoted
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-quoted
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-quoted
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-quoted
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-quoted
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-quoted
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-quoted
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-quoted
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-quoted
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-quoted
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-quoted
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-quoted
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