[dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read

[dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read

[Stephen Hemminger]

From: Stephen Hemminger <shemming@brocade.com>

Make rte_sched statistics API work like the ethernet statistics API.
Don't auto-clear statistics.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_sched/rte_sched.c | 30 ++++++++++++++++++++++++++++++
 lib/librte_sched/rte_sched.h | 29 +++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
index 8cb8bf1..d891e50 100644
--- a/lib/librte_sched/rte_sched.c
+++ b/lib/librte_sched/rte_sched.c
@@ -935,6 +935,21 @@ rte_sched_subport_read_stats(struct rte_sched_port *port,
 }
 
 int
+rte_sched_subport_stats_reset(struct rte_sched_port *port,
+			      uint32_t subport_id)
+{
+	struct rte_sched_subport *s;
+
+	/* Check user parameters */
+	if (port == NULL || subport_id >= port->n_subports_per_port)
+		return -1;
+
+	s = port->subport + subport_id;
+	memset(&s->stats, 0, sizeof(struct rte_sched_subport_stats));
+	return 0;
+}
+
+int
 rte_sched_queue_read_stats(struct rte_sched_port *port,
 	uint32_t queue_id,
 	struct rte_sched_queue_stats *stats,
@@ -963,6 +978,21 @@ rte_sched_queue_read_stats(struct rte_sched_port *port,
 	return 0;
 }
 
+int
+rte_sched_queue_stats_reset(struct rte_sched_port *port,
+			    uint32_t queue_id)
+{
+	struct rte_sched_queue_extra *qe;
+
+	/* Check user parameters */
+	if (port == NULL || queue_id >= rte_sched_port_queues_per_port(port))
+		return -1;
+
+	qe = port->queue_extra + queue_id;
+	memset(&qe->stats, 0, sizeof(struct rte_sched_queue_stats));
+	return 0;
+}
+
 static inline uint32_t
 rte_sched_port_qindex(struct rte_sched_port *port, uint32_t subport, uint32_t pipe, uint32_t traffic_class, uint32_t queue)
 {
diff --git a/lib/librte_sched/rte_sched.h b/lib/librte_sched/rte_sched.h
index e9bf18a..3d007e4 100644
--- a/lib/librte_sched/rte_sched.h
+++ b/lib/librte_sched/rte_sched.h
@@ -317,6 +317,21 @@ rte_sched_subport_read_stats(struct rte_sched_port *port,
 	struct rte_sched_subport_stats *stats,
 	uint32_t *tc_ov);
 
+
+/**
+ * Hierarchical scheduler subport statistics reset
+ *
+ * @param port
+ *   Handle to port scheduler instance
+ * @param subport_id
+ *   Subport ID
+ * @return
+ *   0 upon success, error code otherwise
+ */
+int
+rte_sched_subport_stats_reset(struct rte_sched_port *port,
+			      uint32_t subport_id);
+
 /**
  * Hierarchical scheduler queue statistics read
  *
@@ -338,6 +353,20 @@ rte_sched_queue_read_stats(struct rte_sched_port *port,
 	struct rte_sched_queue_stats *stats,
 	uint16_t *qlen);
 
+/**
+ * Hierarchical scheduler queue statistics reset
+ *
+ * @param port
+ *   Handle to port scheduler instance
+ * @param queue_id
+ *   Queue ID within port scheduler
+ * @return
+ *   0 upon success, error code otherwise
+ */
+int
+rte_sched_queue_stats_reset(struct rte_sched_port *port,
+			    uint32_t queue_id);
+
 /*
  * Run-time
  *
-- 
2.1.4

[dpdk-dev] [PATCH v2 5/7] rte_sched: don't put tabs in log messages

[Stephen Hemminger]

From: Stephen Hemminger <shemming@brocade.com>

syslog does not like tabs in log messages; tab gets translated to #011

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_sched/rte_sched.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
index d891e50..55fbc14 100644
--- a/lib/librte_sched/rte_sched.c
+++ b/lib/librte_sched/rte_sched.c
@@ -495,10 +495,10 @@ rte_sched_port_log_pipe_profile(struct rte_sched_port *port, uint32_t i)
 	struct rte_sched_pipe_profile *p = port->pipe_profiles + i;
 
 	RTE_LOG(INFO, SCHED, "Low level config for pipe profile %u:\n"
-		"\tToken bucket: period = %u, credits per period = %u, size = %u\n"
-		"\tTraffic classes: period = %u, credits per period = [%u, %u, %u, %u]\n"
-		"\tTraffic class 3 oversubscription: weight = %hhu\n"
-		"\tWRR cost: [%hhu, %hhu, %hhu, %hhu], [%hhu, %hhu, %hhu, %hhu], [%hhu, %hhu, %hhu, %hhu], [%hhu, %hhu, %hhu, %hhu]\n",
+		"    Token bucket: period = %u, credits per period = %u, size = %u\n"
+		"    Traffic classes: period = %u, credits per period = [%u, %u, %u, %u]\n"
+		"    Traffic class 3 oversubscription: weight = %hhu\n"
+		"    WRR cost: [%hhu, %hhu, %hhu, %hhu], [%hhu, %hhu, %hhu, %hhu], [%hhu, %hhu, %hhu, %hhu], [%hhu, %hhu, %hhu, %hhu]\n",
 		i,
 
 		/* Token bucket */
@@ -716,9 +716,9 @@ rte_sched_port_log_subport_config(struct rte_sched_port *port, uint32_t i)
 	struct rte_sched_subport *s = port->subport + i;
 
 	RTE_LOG(INFO, SCHED, "Low level config for subport %u:\n"
-		"\tToken bucket: period = %u, credits per period = %u, size = %u\n"
-		"\tTraffic classes: period = %u, credits per period = [%u, %u, %u, %u]\n"
-		"\tTraffic class 3 oversubscription: wm min = %u, wm max = %u\n",
+		"    Token bucket: period = %u, credits per period = %u, size = %u\n"
+		"    Traffic classes: period = %u, credits per period = [%u, %u, %u, %u]\n"
+		"    Traffic class 3 oversubscription: wm min = %u, wm max = %u\n",
 		i,
 
 		/* Token bucket */
-- 
2.1.4

Re: [dpdk-dev] [PATCH v2 5/7] rte_sched: don't put tabs in log messages

[Dumitrescu, Cristian]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen
> Hemminger
> Sent: Thursday, February 5, 2015 6:14 AM
> To: dev@dpdk.org
> Cc: Stephen Hemminger
> Subject: [dpdk-dev] [PATCH v2 5/7] rte_sched: don't put tabs in log
> messages
> 
> From: Stephen Hemminger <shemming@brocade.com>
> 
> syslog does not like tabs in log messages; tab gets translated to #011
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_sched/rte_sched.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
> index d891e50..55fbc14 100644
> --- a/lib/librte_sched/rte_sched.c
> +++ b/lib/librte_sched/rte_sched.c
> @@ -495,10 +495,10 @@ rte_sched_port_log_pipe_profile(struct
> rte_sched_port *port, uint32_t i)
>  	struct rte_sched_pipe_profile *p = port->pipe_profiles + i;
> 
>  	RTE_LOG(INFO, SCHED, "Low level config for pipe profile %u:\n"
> -		"\tToken bucket: period = %u, credits per period = %u, size =
> %u\n"
> -		"\tTraffic classes: period = %u, credits per period = [%u, %u,
> %u, %u]\n"
> -		"\tTraffic class 3 oversubscription: weight = %hhu\n"
> -		"\tWRR cost: [%hhu, %hhu, %hhu, %hhu], [%hhu, %hhu,
> %hhu, %hhu], [%hhu, %hhu, %hhu, %hhu], [%hhu, %hhu, %hhu, %hhu]\n",
> +		"    Token bucket: period = %u, credits per period = %u, size =
> %u\n"
> +		"    Traffic classes: period = %u, credits per period = [%u, %u,
> %u, %u]\n"
> +		"    Traffic class 3 oversubscription: weight = %hhu\n"
> +		"    WRR cost: [%hhu, %hhu, %hhu, %hhu], [%hhu, %hhu,
> %hhu, %hhu], [%hhu, %hhu, %hhu, %hhu], [%hhu, %hhu, %hhu, %hhu]\n",
>  		i,
> 
>  		/* Token bucket */
> @@ -716,9 +716,9 @@ rte_sched_port_log_subport_config(struct
> rte_sched_port *port, uint32_t i)
>  	struct rte_sched_subport *s = port->subport + i;
> 
>  	RTE_LOG(INFO, SCHED, "Low level config for subport %u:\n"
> -		"\tToken bucket: period = %u, credits per period = %u, size =
> %u\n"
> -		"\tTraffic classes: period = %u, credits per period = [%u, %u,
> %u, %u]\n"
> -		"\tTraffic class 3 oversubscription: wm min = %u, wm max =
> %u\n",
> +		"    Token bucket: period = %u, credits per period = %u, size =
> %u\n"
> +		"    Traffic classes: period = %u, credits per period = [%u, %u,
> %u, %u]\n"
> +		"    Traffic class 3 oversubscription: wm min = %u, wm max =
> %u\n",
>  		i,
> 
>  		/* Token bucket */
> --
> 2.1.4

Acked by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.

[dpdk-dev] [PATCH v2 6/7] rte_sched: eliminate floating point in calculating byte clock

[Stephen Hemminger]

From: Stephen Hemminger <shemming@brocade.com>

The old code was doing a floating point divide for each rte_dequeue()
which is very expensive. Change to using fixed point scaled math instead.
This improved performance from 5Gbit/sec to 10 Gbit/sec

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_sched/rte_sched.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
index 55fbc14..3023457 100644
--- a/lib/librte_sched/rte_sched.c
+++ b/lib/librte_sched/rte_sched.c
@@ -102,6 +102,9 @@
 
 #define RTE_SCHED_BMP_POS_INVALID             UINT32_MAX
 
+/* For cycles_per_byte calculation */
+#define RTE_SCHED_TIME_SHIFT		      20
+
 struct rte_sched_subport {
 	/* Token bucket (TB) */
 	uint64_t tb_time; /* time of last update */
@@ -239,7 +242,7 @@ struct rte_sched_port {
 	uint64_t time_cpu_cycles;     /* Current CPU time measured in CPU cyles */
 	uint64_t time_cpu_bytes;      /* Current CPU time measured in bytes */
 	uint64_t time;                /* Current NIC TX time measured in bytes */
-	double cycles_per_byte;       /* CPU cycles per byte */
+	uint32_t cycles_per_byte;       /* CPU cycles per byte (scaled) */
 
 	/* Scheduling loop detection */
 	uint32_t pipe_loop;
@@ -657,7 +660,9 @@ rte_sched_port_config(struct rte_sched_port_params *params)
 	port->time_cpu_cycles = rte_get_tsc_cycles();
 	port->time_cpu_bytes = 0;
 	port->time = 0;
-	port->cycles_per_byte = ((double) rte_get_tsc_hz()) / ((double) params->rate);
+
+	port->cycles_per_byte = (rte_get_tsc_hz() << RTE_SCHED_TIME_SHIFT)
+		/ params->rate;
 
 	/* Scheduling loop detection */
 	port->pipe_loop = RTE_SCHED_PIPE_INVALID;
@@ -2156,11 +2161,12 @@ rte_sched_port_time_resync(struct rte_sched_port *port)
 {
 	uint64_t cycles = rte_get_tsc_cycles();
 	uint64_t cycles_diff = cycles - port->time_cpu_cycles;
-	double bytes_diff = ((double) cycles_diff) / port->cycles_per_byte;
+	uint64_t bytes_diff = (cycles_diff << RTE_SCHED_TIME_SHIFT)
+		/ port->cycles_per_byte;
 
 	/* Advance port time */
 	port->time_cpu_cycles = cycles;
-	port->time_cpu_bytes += (uint64_t) bytes_diff;
+	port->time_cpu_bytes += bytes_diff;
 	if (port->time < port->time_cpu_bytes) {
 		port->time = port->time_cpu_bytes;
 	}
-- 
2.1.4

Re: [dpdk-dev] [PATCH v2 6/7] rte_sched: eliminate floating point in calculating byte clock

[Dumitrescu, Cristian]

Hi Stephen,

Sorry, NACK.

1. Overflow issue
As you declare cycles_per_byte as uint32_t, for a CPU frequency of 2-3 GHz, the line of code below results in overflow:
	port->cycles_per_byte = (rte_get_tsc_hz() << RTE_SCHED_TIME_SHIFT) / params->rate;
Therefore, there is most likely a significant accuracy loss, which might result in more packets allowed to go out than it should.

2. Integer division has a higher cost than floating point division
My understanding is we are considering a performance improvement by replacing the double precision floating point division in:
	double bytes_diff = ((double) cycles_diff) / port->cycles_per_byte;
with an integer division:
	uint64_t bytes_diff = (cycles_diff << RTE_SCHED_TIME_SHIFT) / port->cycles_per_byte;
I don't think this is going to have the claimed benefit, as acording to "Intel 64 and IA-32 Architectures Optimization  Reference Manual" (Appendix C), the latency of the integer division instruction is significantly bigger than the latency of integer division:
	Instruction FDIV double precision: latency = 38-40 cycles
	Instruction IDIV: latency = 56 - 80 cycles

3. Alternative
I hear though your suggestion about replacing the floating point division with a more performant construction. One suggestion would be to replace it with an integer multiplication followed by a shift right, probably by using a uint64_t bytes_per_cycle_scaled_up (the inverse of cycles_per_bytes). I need to prototype this code myself. Would you be OK to look into providing an alternative implementation?

Thanks,
Cristian


-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
Sent: Thursday, February 5, 2015 6:14 AM
To: dev@dpdk.org
Cc: Stephen Hemminger
Subject: [dpdk-dev] [PATCH v2 6/7] rte_sched: eliminate floating point in calculating byte clock

From: Stephen Hemminger <shemming@brocade.com>

The old code was doing a floating point divide for each rte_dequeue()
which is very expensive. Change to using fixed point scaled math instead.
This improved performance from 5Gbit/sec to 10 Gbit/sec

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_sched/rte_sched.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
index 55fbc14..3023457 100644
--- a/lib/librte_sched/rte_sched.c
+++ b/lib/librte_sched/rte_sched.c
@@ -102,6 +102,9 @@
 
 #define RTE_SCHED_BMP_POS_INVALID             UINT32_MAX
 
+/* For cycles_per_byte calculation */
+#define RTE_SCHED_TIME_SHIFT		      20
+
 struct rte_sched_subport {
 	/* Token bucket (TB) */
 	uint64_t tb_time; /* time of last update */
@@ -239,7 +242,7 @@ struct rte_sched_port {
 	uint64_t time_cpu_cycles;     /* Current CPU time measured in CPU cyles */
 	uint64_t time_cpu_bytes;      /* Current CPU time measured in bytes */
 	uint64_t time;                /* Current NIC TX time measured in bytes */
-	double cycles_per_byte;       /* CPU cycles per byte */
+	uint32_t cycles_per_byte;       /* CPU cycles per byte (scaled) */
 
 	/* Scheduling loop detection */
 	uint32_t pipe_loop;
@@ -657,7 +660,9 @@ rte_sched_port_config(struct rte_sched_port_params *params)
 	port->time_cpu_cycles = rte_get_tsc_cycles();
 	port->time_cpu_bytes = 0;
 	port->time = 0;
-	port->cycles_per_byte = ((double) rte_get_tsc_hz()) / ((double) params->rate);
+
+	port->cycles_per_byte = (rte_get_tsc_hz() << RTE_SCHED_TIME_SHIFT)
+		/ params->rate;
 
 	/* Scheduling loop detection */
 	port->pipe_loop = RTE_SCHED_PIPE_INVALID;
@@ -2156,11 +2161,12 @@ rte_sched_port_time_resync(struct rte_sched_port *port)
 {
 	uint64_t cycles = rte_get_tsc_cycles();
 	uint64_t cycles_diff = cycles - port->time_cpu_cycles;
-	double bytes_diff = ((double) cycles_diff) / port->cycles_per_byte;
+	uint64_t bytes_diff = (cycles_diff << RTE_SCHED_TIME_SHIFT)
+		/ port->cycles_per_byte;
 
 	/* Advance port time */
 	port->time_cpu_cycles = cycles;
-	port->time_cpu_bytes += (uint64_t) bytes_diff;
+	port->time_cpu_bytes += bytes_diff;
 	if (port->time < port->time_cpu_bytes) {
 		port->time = port->time_cpu_bytes;
 	}
-- 
2.1.4

--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.

Re: [dpdk-dev] [PATCH v2 6/7] rte_sched: eliminate floating point in calculating byte clock

[Stephen Hemminger]

On Mon, 16 Feb 2015 22:44:31 +0000
"Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:

> Hi Stephen,
> 
> Sorry, NACK.
> 
> 1. Overflow issue
> As you declare cycles_per_byte as uint32_t, for a CPU frequency of 2-3 GHz, the line of code below results in overflow:
> 	port->cycles_per_byte = (rte_get_tsc_hz() << RTE_SCHED_TIME_SHIFT) / params->rate;
> Therefore, there is most likely a significant accuracy loss, which might result in more packets allowed to go out than it should.

The tsc shifted is still 64 bits.
and rate is 32 bits bytes/sec.

I chose scale such that
if clock = 3 Ghz
then min rate = 715 bytes/sec =  5722 bits/sec

> 2. Integer division has a higher cost than floating point division
> My understanding is we are considering a performance improvement by replacing the double precision floating point division in:
> 	double bytes_diff = ((double) cycles_diff) / port->cycles_per_byte;
> with an integer division:
> 	uint64_t bytes_diff = (cycles_diff << RTE_SCHED_TIME_SHIFT) / port->cycles_per_byte;
> I don't think this is going to have the claimed benefit, as acording to "Intel 64 and IA-32 Architectures Optimization  Reference Manual" (Appendix C), the latency of the integer division instruction is significantly bigger than the latency of integer division:
> 	Instruction FDIV double precision: latency = 38-40 cycles
> 	Instruction IDIV: latency = 56 - 80 cycles

I observed that performance when from 5Gbit/sec to 10Gbit/sec.
Mostly because the floating point engages more instruction units and does not
pipeline. Cycle count is not everything.  This was on Ivy Bridge processor.


> 3. Alternative
> I hear though your suggestion about replacing the floating point division with a more performant construction. One suggestion would be to replace it with an integer multiplication followed by a shift right, probably by using a uint64_t bytes_per_cycle_scaled_up (the inverse of cycles_per_bytes). I need to prototype this code myself. Would you be OK to look into providing an alternative implementation?
>

I looked into multiplative integer method, and will do it in future. But it has
more scaling issues since it would require that the values both be 32 bits.

[dpdk-dev] [PATCH v2 7/7] rte_sched: rearrange data structures

[Stephen Hemminger]

From: Stephen Hemminger <shemming@brocade.com>

Rearrange internal data structures to eliminate holes.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_sched/rte_sched.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
index 3023457..7de1395 100644
--- a/lib/librte_sched/rte_sched.c
+++ b/lib/librte_sched/rte_sched.c
@@ -198,6 +198,8 @@ struct rte_sched_grinder {
 	enum grinder_state state;
 	uint32_t productive;
 	uint32_t pindex;
+	uint32_t qpos;
+
 	struct rte_sched_subport *subport;
 	struct rte_sched_pipe *pipe;
 	struct rte_sched_pipe_profile *pipe_params;
@@ -212,11 +214,10 @@ struct rte_sched_grinder {
 	uint32_t tc_index;
 	struct rte_sched_queue *queue[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
 	struct rte_mbuf **qbase[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
+	struct rte_mbuf *pkt;
 	uint32_t qindex[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
-	uint16_t qsize;
 	uint32_t qmask;
-	uint32_t qpos;
-	struct rte_mbuf *pkt;
+	uint16_t qsize;
 
 	/* WRR */
 	uint16_t wrr_tokens[RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS];
@@ -234,9 +235,7 @@ struct rte_sched_port {
 	uint16_t qsize[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
 	uint32_t n_pipe_profiles;
 	uint32_t pipe_tc3_rate_max;
-#ifdef RTE_SCHED_RED
-	struct rte_red_config red_config[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE][e_RTE_METER_COLORS];
-#endif
+	uint32_t busy_grinders;
 
 	/* Timing */
 	uint64_t time_cpu_cycles;     /* Current CPU time measured in CPU cyles */
@@ -247,16 +246,15 @@ struct rte_sched_port {
 	/* Scheduling loop detection */
 	uint32_t pipe_loop;
 	uint32_t pipe_exhaustion;
+	uint32_t n_pkts_out;
 
 	/* Bitmap */
 	struct rte_bitmap *bmp;
+	struct rte_mbuf **pkts_out;
 	uint32_t grinder_base_bmp_pos[RTE_SCHED_PORT_N_GRINDERS] __rte_aligned_16;
 
 	/* Grinders */
 	struct rte_sched_grinder grinder[RTE_SCHED_PORT_N_GRINDERS];
-	uint32_t busy_grinders;
-	struct rte_mbuf **pkts_out;
-	uint32_t n_pkts_out;
 
 	/* Queue base calculation */
 	uint32_t qsize_add[RTE_SCHED_QUEUES_PER_PIPE];
@@ -270,6 +268,9 @@ struct rte_sched_port {
 	struct rte_sched_pipe_profile *pipe_profiles;
 	uint8_t *bmp_array;
 	struct rte_mbuf **queue_array;
+#ifdef RTE_SCHED_RED
+	struct rte_red_config red_config[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE][e_RTE_METER_COLORS];
+#endif
 	uint8_t memory[0] __rte_cache_aligned;
 } __rte_cache_aligned;
 
-- 
2.1.4

Re: [dpdk-dev] [PATCH v2 7/7] rte_sched: rearrange data structures

[Dumitrescu, Cristian]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen
> Hemminger
> Sent: Thursday, February 5, 2015 6:14 AM
> To: dev@dpdk.org
> Cc: Stephen Hemminger
> Subject: [dpdk-dev] [PATCH v2 7/7] rte_sched: rearrange data structures
> 
> From: Stephen Hemminger <shemming@brocade.com>
> 
> Rearrange internal data structures to eliminate holes.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_sched/rte_sched.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
> index 3023457..7de1395 100644
> --- a/lib/librte_sched/rte_sched.c
> +++ b/lib/librte_sched/rte_sched.c
> @@ -198,6 +198,8 @@ struct rte_sched_grinder {
>  	enum grinder_state state;
>  	uint32_t productive;
>  	uint32_t pindex;
> +	uint32_t qpos;
> +
>  	struct rte_sched_subport *subport;
>  	struct rte_sched_pipe *pipe;
>  	struct rte_sched_pipe_profile *pipe_params;
> @@ -212,11 +214,10 @@ struct rte_sched_grinder {
>  	uint32_t tc_index;
>  	struct rte_sched_queue
> *queue[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
>  	struct rte_mbuf **qbase[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> +	struct rte_mbuf *pkt;
>  	uint32_t qindex[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
> -	uint16_t qsize;
>  	uint32_t qmask;
> -	uint32_t qpos;
> -	struct rte_mbuf *pkt;
> +	uint16_t qsize;
> 
>  	/* WRR */
>  	uint16_t wrr_tokens[RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS];
> @@ -234,9 +235,7 @@ struct rte_sched_port {
>  	uint16_t qsize[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE];
>  	uint32_t n_pipe_profiles;
>  	uint32_t pipe_tc3_rate_max;
> -#ifdef RTE_SCHED_RED
> -	struct rte_red_config
> red_config[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE][e_RTE_METER_COLO
> RS];
> -#endif
> +	uint32_t busy_grinders;
> 
>  	/* Timing */
>  	uint64_t time_cpu_cycles;     /* Current CPU time measured in CPU
> cyles */
> @@ -247,16 +246,15 @@ struct rte_sched_port {
>  	/* Scheduling loop detection */
>  	uint32_t pipe_loop;
>  	uint32_t pipe_exhaustion;
> +	uint32_t n_pkts_out;
> 
>  	/* Bitmap */
>  	struct rte_bitmap *bmp;
> +	struct rte_mbuf **pkts_out;
>  	uint32_t grinder_base_bmp_pos[RTE_SCHED_PORT_N_GRINDERS]
> __rte_aligned_16;
> 
>  	/* Grinders */
>  	struct rte_sched_grinder grinder[RTE_SCHED_PORT_N_GRINDERS];
> -	uint32_t busy_grinders;
> -	struct rte_mbuf **pkts_out;
> -	uint32_t n_pkts_out;
> 
>  	/* Queue base calculation */
>  	uint32_t qsize_add[RTE_SCHED_QUEUES_PER_PIPE];
> @@ -270,6 +268,9 @@ struct rte_sched_port {
>  	struct rte_sched_pipe_profile *pipe_profiles;
>  	uint8_t *bmp_array;
>  	struct rte_mbuf **queue_array;
> +#ifdef RTE_SCHED_RED
> +	struct rte_red_config
> red_config[RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE][e_RTE_METER_COLO
> RS];
> +#endif
>  	uint8_t memory[0] __rte_cache_aligned;
>  } __rte_cache_aligned;
> 
> --
> 2.1.4

The fields of the grinder structure (and other structures, as well) are organized into groups (based on the low level functionality they're used for) for readability purpose.

I agree this sometimes requires the compiler to use padding. I don't think we getting into using an additional cache line due to padding.

Did you see any performance improvement due to this change? I don't think there is any, and I would favour readability over performance in this case?

Thanks,
Cristian

--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.

Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read

[Neil Horman]

On Wed, Feb 04, 2015 at 10:13:58PM -0800, Stephen Hemminger wrote:
> From: Stephen Hemminger <shemming@brocade.com>
> 
> Make rte_sched statistics API work like the ethernet statistics API.
> Don't auto-clear statistics.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_sched/rte_sched.c | 30 ++++++++++++++++++++++++++++++
>  lib/librte_sched/rte_sched.h | 29 +++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
> index 8cb8bf1..d891e50 100644
> --- a/lib/librte_sched/rte_sched.c
> +++ b/lib/librte_sched/rte_sched.c
> @@ -935,6 +935,21 @@ rte_sched_subport_read_stats(struct rte_sched_port *port,
>  }
>  
>  int
> +rte_sched_subport_stats_reset(struct rte_sched_port *port,
> +			      uint32_t subport_id)
> +{
> +	struct rte_sched_subport *s;
> +
> +	/* Check user parameters */
> +	if (port == NULL || subport_id >= port->n_subports_per_port)
> +		return -1;
> +
> +	s = port->subport + subport_id;
> +	memset(&s->stats, 0, sizeof(struct rte_sched_subport_stats));
> +	return 0;
> +}
> +
> +int
>  rte_sched_queue_read_stats(struct rte_sched_port *port,
>  	uint32_t queue_id,
>  	struct rte_sched_queue_stats *stats,
> @@ -963,6 +978,21 @@ rte_sched_queue_read_stats(struct rte_sched_port *port,
>  	return 0;
>  }
>  
> +int
> +rte_sched_queue_stats_reset(struct rte_sched_port *port,
> +			    uint32_t queue_id)
> +{
> +	struct rte_sched_queue_extra *qe;
> +
> +	/* Check user parameters */
> +	if (port == NULL || queue_id >= rte_sched_port_queues_per_port(port))
> +		return -1;
> +
> +	qe = port->queue_extra + queue_id;
> +	memset(&qe->stats, 0, sizeof(struct rte_sched_queue_stats));
> +	return 0;
> +}
> +
>  static inline uint32_t
>  rte_sched_port_qindex(struct rte_sched_port *port, uint32_t subport, uint32_t pipe, uint32_t traffic_class, uint32_t queue)
>  {
> diff --git a/lib/librte_sched/rte_sched.h b/lib/librte_sched/rte_sched.h
> index e9bf18a..3d007e4 100644
> --- a/lib/librte_sched/rte_sched.h
> +++ b/lib/librte_sched/rte_sched.h
> @@ -317,6 +317,21 @@ rte_sched_subport_read_stats(struct rte_sched_port *port,
>  	struct rte_sched_subport_stats *stats,
>  	uint32_t *tc_ov);
>  
> +
> +/**
> + * Hierarchical scheduler subport statistics reset
> + *
> + * @param port
> + *   Handle to port scheduler instance
> + * @param subport_id
> + *   Subport ID
> + * @return
> + *   0 upon success, error code otherwise
> + */
> +int
> +rte_sched_subport_stats_reset(struct rte_sched_port *port,
> +			      uint32_t subport_id);
> +
>  /**
>   * Hierarchical scheduler queue statistics read
>   *
> @@ -338,6 +353,20 @@ rte_sched_queue_read_stats(struct rte_sched_port *port,
>  	struct rte_sched_queue_stats *stats,
>  	uint16_t *qlen);
>  
> +/**
> + * Hierarchical scheduler queue statistics reset
> + *
> + * @param port
> + *   Handle to port scheduler instance
> + * @param queue_id
> + *   Queue ID within port scheduler
> + * @return
> + *   0 upon success, error code otherwise
> + */
> +int
> +rte_sched_queue_stats_reset(struct rte_sched_port *port,
> +			    uint32_t queue_id);
> +
Both need to be added to the version map to expose them properly.
Neil

Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read

[Thomas Monjalon]

2015-02-05 07:43, Neil Horman:
> On Wed, Feb 04, 2015 at 10:13:58PM -0800, Stephen Hemminger wrote:
> > +
> > +/**
> > + * Hierarchical scheduler subport statistics reset
> > + *
> > + * @param port
> > + *   Handle to port scheduler instance
> > + * @param subport_id
> > + *   Subport ID
> > + * @return
> > + *   0 upon success, error code otherwise
> > + */
> > +int
> > +rte_sched_subport_stats_reset(struct rte_sched_port *port,
> > +			      uint32_t subport_id);
> > +
> >  /**
> >   * Hierarchical scheduler queue statistics read
> >   *
> > @@ -338,6 +353,20 @@ rte_sched_queue_read_stats(struct rte_sched_port *port,
> >  	struct rte_sched_queue_stats *stats,
> >  	uint16_t *qlen);
> >  
> > +/**
> > + * Hierarchical scheduler queue statistics reset
> > + *
> > + * @param port
> > + *   Handle to port scheduler instance
> > + * @param queue_id
> > + *   Queue ID within port scheduler
> > + * @return
> > + *   0 upon success, error code otherwise
> > + */
> > +int
> > +rte_sched_queue_stats_reset(struct rte_sched_port *port,
> > +			    uint32_t queue_id);
> > +
> Both need to be added to the version map to expose them properly.
> Neil

Stephen, this patchset is partially acked and could enter in 2.0.0-rc1.
May you send a v3 addressing comments? Or should I break the serie by
applying only some of them? Or postpone the serie to 2.1?

Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read

[Stephen Hemminger]

On Mon, 23 Feb 2015 23:51:31 +0000
Thomas Monjalon <thomas.monjalon@6wind.com> wrote:

> 2015-02-05 07:43, Neil Horman:
> > On Wed, Feb 04, 2015 at 10:13:58PM -0800, Stephen Hemminger wrote:
> > > +
> > > +/**
> > > + * Hierarchical scheduler subport statistics reset
> > > + *
> > > + * @param port
> > > + *   Handle to port scheduler instance
> > > + * @param subport_id
> > > + *   Subport ID
> > > + * @return
> > > + *   0 upon success, error code otherwise
> > > + */
> > > +int
> > > +rte_sched_subport_stats_reset(struct rte_sched_port *port,
> > > +			      uint32_t subport_id);
> > > +
> > >  /**
> > >   * Hierarchical scheduler queue statistics read
> > >   *
> > > @@ -338,6 +353,20 @@ rte_sched_queue_read_stats(struct rte_sched_port *port,
> > >  	struct rte_sched_queue_stats *stats,
> > >  	uint16_t *qlen);
> > >  
> > > +/**
> > > + * Hierarchical scheduler queue statistics reset
> > > + *
> > > + * @param port
> > > + *   Handle to port scheduler instance
> > > + * @param queue_id
> > > + *   Queue ID within port scheduler
> > > + * @return
> > > + *   0 upon success, error code otherwise
> > > + */
> > > +int
> > > +rte_sched_queue_stats_reset(struct rte_sched_port *port,
> > > +			    uint32_t queue_id);
> > > +
> > Both need to be added to the version map to expose them properly.
> > Neil
> 
> Stephen, this patchset is partially acked and could enter in 2.0.0-rc1.
> May you send a v3 addressing comments? Or should I break the serie by
> applying only some of them? Or postpone the serie to 2.1?

I can resend v3. Wasn't clear that a conclusion was reached.
IMHO read should not clear.

Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read

[Thomas Monjalon]

2015-02-24 11:18, Stephen Hemminger:
> On Mon, 23 Feb 2015 23:51:31 +0000
> Thomas Monjalon <thomas.monjalon@6wind.com> wrote:
> 
> > 2015-02-05 07:43, Neil Horman:
> > > On Wed, Feb 04, 2015 at 10:13:58PM -0800, Stephen Hemminger wrote:
> > > > +
> > > > +/**
> > > > + * Hierarchical scheduler subport statistics reset
> > > > + *
> > > > + * @param port
> > > > + *   Handle to port scheduler instance
> > > > + * @param subport_id
> > > > + *   Subport ID
> > > > + * @return
> > > > + *   0 upon success, error code otherwise
> > > > + */
> > > > +int
> > > > +rte_sched_subport_stats_reset(struct rte_sched_port *port,
> > > > +			      uint32_t subport_id);
> > > > +
> > > >  /**
> > > >   * Hierarchical scheduler queue statistics read
> > > >   *
> > > > @@ -338,6 +353,20 @@ rte_sched_queue_read_stats(struct rte_sched_port *port,
> > > >  	struct rte_sched_queue_stats *stats,
> > > >  	uint16_t *qlen);
> > > >  
> > > > +/**
> > > > + * Hierarchical scheduler queue statistics reset
> > > > + *
> > > > + * @param port
> > > > + *   Handle to port scheduler instance
> > > > + * @param queue_id
> > > > + *   Queue ID within port scheduler
> > > > + * @return
> > > > + *   0 upon success, error code otherwise
> > > > + */
> > > > +int
> > > > +rte_sched_queue_stats_reset(struct rte_sched_port *port,
> > > > +			    uint32_t queue_id);
> > > > +
> > > Both need to be added to the version map to expose them properly.
> > > Neil
> > 
> > Stephen, this patchset is partially acked and could enter in 2.0.0-rc1.
> > May you send a v3 addressing comments? Or should I break the serie by
> > applying only some of them? Or postpone the serie to 2.1?
> 
> I can resend v3. Wasn't clear that a conclusion was reached.
> IMHO read should not clear.

Me too. I'm just saying that I cannot apply anything.
So you have to decide the strategy to adopt for your patches.

Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read

[Dumitrescu, Cristian]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Tuesday, February 24, 2015 8:07 PM
> To: Stephen Hemminger
> Cc: dev@dpdk.org; Stephen Hemminger
> Subject: Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when
> read
> 
> 2015-02-24 11:18, Stephen Hemminger:
> > On Mon, 23 Feb 2015 23:51:31 +0000
> > Thomas Monjalon <thomas.monjalon@6wind.com> wrote:
> >
> > > 2015-02-05 07:43, Neil Horman:
> > > > On Wed, Feb 04, 2015 at 10:13:58PM -0800, Stephen Hemminger wrote:
> > > > > +
> > > > > +/**
> > > > > + * Hierarchical scheduler subport statistics reset
> > > > > + *
> > > > > + * @param port
> > > > > + *   Handle to port scheduler instance
> > > > > + * @param subport_id
> > > > > + *   Subport ID
> > > > > + * @return
> > > > > + *   0 upon success, error code otherwise
> > > > > + */
> > > > > +int
> > > > > +rte_sched_subport_stats_reset(struct rte_sched_port *port,
> > > > > +			      uint32_t subport_id);
> > > > > +
> > > > >  /**
> > > > >   * Hierarchical scheduler queue statistics read
> > > > >   *
> > > > > @@ -338,6 +353,20 @@ rte_sched_queue_read_stats(struct
> rte_sched_port *port,
> > > > >  	struct rte_sched_queue_stats *stats,
> > > > >  	uint16_t *qlen);
> > > > >
> > > > > +/**
> > > > > + * Hierarchical scheduler queue statistics reset
> > > > > + *
> > > > > + * @param port
> > > > > + *   Handle to port scheduler instance
> > > > > + * @param queue_id
> > > > > + *   Queue ID within port scheduler
> > > > > + * @return
> > > > > + *   0 upon success, error code otherwise
> > > > > + */
> > > > > +int
> > > > > +rte_sched_queue_stats_reset(struct rte_sched_port *port,
> > > > > +			    uint32_t queue_id);
> > > > > +
> > > > Both need to be added to the version map to expose them properly.
> > > > Neil
> > >
> > > Stephen, this patchset is partially acked and could enter in 2.0.0-rc1.
> > > May you send a v3 addressing comments? Or should I break the serie by
> > > applying only some of them? Or postpone the serie to 2.1?
> >
> > I can resend v3. Wasn't clear that a conclusion was reached.
> > IMHO read should not clear.
> 
> Me too. I'm just saying that I cannot apply anything.
> So you have to decide the strategy to adopt for your patches.

How about my latest proposal to have the stats read functions either reset the counters or not, based on init-time user configuration? I did not see any reply on this.

Maybe you guys missed my reply, I am pasting it below:

"Personally, I think we should avoid proliferating the number of stats functions, I would keep a single set of stats read functions, which can clear the stats or not, depending on behaviour configured per rte_sched object at creation time. Basically, based on the value of configuration parameter struct rte_sched_params::clear_stats_on_reset, the stats read functions do clear the counters or not. In my opinion, this allows a clean init-time selection of the required behaviour, and it also provides backward compatibility. Any issues with this approach?"

Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read

[Thomas Monjalon]

2015-02-24 21:06, Thomas Monjalon:
> 2015-02-24 11:18, Stephen Hemminger:
> > On Mon, 23 Feb 2015 23:51:31 +0000
> > Thomas Monjalon <thomas.monjalon@6wind.com> wrote:
> > > Stephen, this patchset is partially acked and could enter in 2.0.0-rc1.
> > > May you send a v3 addressing comments? Or should I break the serie by
> > > applying only some of them? Or postpone the serie to 2.1?
> > 
> > I can resend v3. Wasn't clear that a conclusion was reached.
> > IMHO read should not clear.
> 
> Me too. I'm just saying that I cannot apply anything.
> So you have to decide the strategy to adopt for your patches.

Please re-send accepted patches with a correct threading.
Decision will be taken in 2.1 for discussed patches.

Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read

[Dumitrescu, Cristian]

Hi Stephen,

What is the reason not to clear statistics on read? Do you have a use-case / justification for it?

(BTW, I see you added the reset functions, but was it also your intention to remove the memset to 0 from the stats read functions? :) )

Regards,
Cristian

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
Sent: Thursday, February 5, 2015 6:14 AM
To: dev@dpdk.org
Cc: Stephen Hemminger
Subject: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read

From: Stephen Hemminger <shemming@brocade.com>

Make rte_sched statistics API work like the ethernet statistics API.
Don't auto-clear statistics.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_sched/rte_sched.c | 30 ++++++++++++++++++++++++++++++
 lib/librte_sched/rte_sched.h | 29 +++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
index 8cb8bf1..d891e50 100644
--- a/lib/librte_sched/rte_sched.c
+++ b/lib/librte_sched/rte_sched.c
@@ -935,6 +935,21 @@ rte_sched_subport_read_stats(struct rte_sched_port *port,
 }
 
 int
+rte_sched_subport_stats_reset(struct rte_sched_port *port,
+			      uint32_t subport_id)
+{
+	struct rte_sched_subport *s;
+
+	/* Check user parameters */
+	if (port == NULL || subport_id >= port->n_subports_per_port)
+		return -1;
+
+	s = port->subport + subport_id;
+	memset(&s->stats, 0, sizeof(struct rte_sched_subport_stats));
+	return 0;
+}
+
+int
 rte_sched_queue_read_stats(struct rte_sched_port *port,
 	uint32_t queue_id,
 	struct rte_sched_queue_stats *stats,
@@ -963,6 +978,21 @@ rte_sched_queue_read_stats(struct rte_sched_port *port,
 	return 0;
 }
 
+int
+rte_sched_queue_stats_reset(struct rte_sched_port *port,
+			    uint32_t queue_id)
+{
+	struct rte_sched_queue_extra *qe;
+
+	/* Check user parameters */
+	if (port == NULL || queue_id >= rte_sched_port_queues_per_port(port))
+		return -1;
+
+	qe = port->queue_extra + queue_id;
+	memset(&qe->stats, 0, sizeof(struct rte_sched_queue_stats));
+	return 0;
+}
+
 static inline uint32_t
 rte_sched_port_qindex(struct rte_sched_port *port, uint32_t subport, uint32_t pipe, uint32_t traffic_class, uint32_t queue)
 {
diff --git a/lib/librte_sched/rte_sched.h b/lib/librte_sched/rte_sched.h
index e9bf18a..3d007e4 100644
--- a/lib/librte_sched/rte_sched.h
+++ b/lib/librte_sched/rte_sched.h
@@ -317,6 +317,21 @@ rte_sched_subport_read_stats(struct rte_sched_port *port,
 	struct rte_sched_subport_stats *stats,
 	uint32_t *tc_ov);
 
+
+/**
+ * Hierarchical scheduler subport statistics reset
+ *
+ * @param port
+ *   Handle to port scheduler instance
+ * @param subport_id
+ *   Subport ID
+ * @return
+ *   0 upon success, error code otherwise
+ */
+int
+rte_sched_subport_stats_reset(struct rte_sched_port *port,
+			      uint32_t subport_id);
+
 /**
  * Hierarchical scheduler queue statistics read
  *
@@ -338,6 +353,20 @@ rte_sched_queue_read_stats(struct rte_sched_port *port,
 	struct rte_sched_queue_stats *stats,
 	uint16_t *qlen);
 
+/**
+ * Hierarchical scheduler queue statistics reset
+ *
+ * @param port
+ *   Handle to port scheduler instance
+ * @param queue_id
+ *   Queue ID within port scheduler
+ * @return
+ *   0 upon success, error code otherwise
+ */
+int
+rte_sched_queue_stats_reset(struct rte_sched_port *port,
+			    uint32_t queue_id);
+
 /*
  * Run-time
  *
-- 
2.1.4

--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.

Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read

[Stephen Hemminger]

On Mon, 9 Feb 2015 22:48:36 +0000
"Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:

> Hi Stephen,
> 
> What is the reason not to clear statistics on read? Do you have a use-case / justification for it?
> 
> (BTW, I see you added the reset functions, but was it also your intention to remove the memset to 0 from the stats read functions? :) )
> 
> Regards,
> Cristian

Read and clear is a non-standard model. Interface statistics are not read/clear.
We have lots of scripts that read statistics. Users don't like it if when 
stastics disappear.

Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Monday, February 9, 2015 10:55 PM
> To: Dumitrescu, Cristian
> Cc: dev@dpdk.org; Stephen Hemminger
> Subject: Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when
> read
> 
> On Mon, 9 Feb 2015 22:48:36 +0000
> "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:
> 
> > Hi Stephen,
> >
> > What is the reason not to clear statistics on read? Do you have a use-case /
> justification for it?
> >
> > (BTW, I see you added the reset functions, but was it also your intention to
> remove the memset to 0 from the stats read functions? :) )
> >
> > Regards,
> > Cristian
> 
> Read and clear is a non-standard model. Interface statistics are not
> read/clear.
> We have lots of scripts that read statistics. Users don't like it if when
> stastics disappear.

Stephen, I suggest adding a new build-time configuration option for the librte_sched library in config/common_* files: CONFIG_RTE_SCHED_STATS_CLEAR_ON_READ.
- When set to YES, we clear the stats counters on read. When set to NO, we do not clear stats counters on read;
- The default value for this configuration option should be YES, in order to preserve the backward compatibility with the current behavior;
- The stats reset functions introduced by this patch should always be enabled/compiled into the code, regardless of whether this new option is set to YES or NO.

What do you think?

Regards,
Cristian


--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.

Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read

[Stephen Hemminger]

On Fri, 20 Feb 2015 18:32:03 +0000
"Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:

> Stephen, I suggest adding a new build-time configuration option for the librte_sched library in config/common_* files: CONFIG_RTE_SCHED_STATS_CLEAR_ON_READ.

Build time config options do not work for distributions.

Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Friday, February 20, 2015 7:53 PM
> To: Dumitrescu, Cristian
> Cc: dev@dpdk.org; Stephen Hemminger
> Subject: Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when
> read
> 
> On Fri, 20 Feb 2015 18:32:03 +0000
> "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:
> 
> > Stephen, I suggest adding a new build-time configuration option for the
> librte_sched library in config/common_* files:
> CONFIG_RTE_SCHED_STATS_CLEAR_ON_READ.
> 
> Build time config options do not work for distributions.

Why?

This does not affect the API, as the new API functions are always compiled in, and the prototypes are not changed, and no data structures are affected.
This only changes the behavior of certain functions, so that user can select which mode it needs.
It also preserves backward compatibility.

We have so many compilation options in config file, why is this one different?

--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.

Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read

[Thomas Monjalon]

2015-02-20 20:23, Dumitrescu, Cristian:
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > On Fri, 20 Feb 2015 18:32:03 +0000
> > "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:
> > 
> > > Stephen, I suggest adding a new build-time configuration option for the
> > > librte_sched library in config/common_* files:
> > > CONFIG_RTE_SCHED_STATS_CLEAR_ON_READ.
> > 
> > Build time config options do not work for distributions.
> 
> Why?
> 
> This does not affect the API, as the new API functions are always compiled
> in, and the prototypes are not changed, and no data structures are affected.

Behaviour is an important part of the API. Think comments as part of the API.

> This only changes the behavior of certain functions, so that user can
> select which mode it needs.

When user doesn't or cannot rebuild, he has no choice.

> It also preserves backward compatibility.
> 
> We have so many compilation options in config file, why is this one different?

We must remove and avoid build-time options.
The only ones which might be acceptable are the ones which allow more
performance by disabling some features.

> This e-mail and any attachments may contain confidential material for the
> sole use of the intended recipient(s). Any review or distribution by others
> is strictly prohibited. If you are not the intended recipient, please contact
> the sender and delete all copies.

Please ask to your administrator to remove this disclaimer.

Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, February 20, 2015 9:01 PM
> To: Dumitrescu, Cristian
> Cc: dev@dpdk.org; Stephen Hemminger
> Subject: Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when
> read
> 
> 2015-02-20 20:23, Dumitrescu, Cristian:
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > On Fri, 20 Feb 2015 18:32:03 +0000
> > > "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:
> > >
> > > > Stephen, I suggest adding a new build-time configuration option for the
> > > > librte_sched library in config/common_* files:
> > > > CONFIG_RTE_SCHED_STATS_CLEAR_ON_READ.
> > >
> > > Build time config options do not work for distributions.
> >
> > Why?
> >
> > This does not affect the API, as the new API functions are always compiled
> > in, and the prototypes are not changed, and no data structures are
> affected.
> 
> Behaviour is an important part of the API. Think comments as part of the API.

Makes sense.

> 
> > This only changes the behavior of certain functions, so that user can
> > select which mode it needs.
> 
> When user doesn't or cannot rebuild, he has no choice.
> 
> > It also preserves backward compatibility.
> >
> > We have so many compilation options in config file, why is this one
> different?
> 
> We must remove and avoid build-time options.
> The only ones which might be acceptable are the ones which allow more
> performance by disabling some features.

Agree.
Stephen, how about a run-time solution (I agree it would be much better, why did I not consider this in the first place?) of adding a new bool parameter in struct rte_sched_port_params: clear_stats_on_reset?
Both stats read function get the port handle (struct rte_sched_port *) as parameter, so there should be no ripple effect to propagate this flag.

> 
> > This e-mail and any attachments may contain confidential material for the
> > sole use of the intended recipient(s). Any review or distribution by others
> > is strictly prohibited. If you are not the intended recipient, please contact
> > the sender and delete all copies.
> 
> Please ask to your administrator to remove this disclaimer.

I did earlier, this is still work in progress unfortunately.

--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.

Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read

[Stephen Hemminger]

On Fri, 20 Feb 2015 21:28:55 +0000
"Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:

> Agree.
> Stephen, how about a run-time solution (I agree it would be much better, why did I not consider this in the first place?) of adding a new bool parameter in struct rte_sched_port_params: clear_stats_on_reset?
> Both stats read function get the port handle (struct rte_sched_port *) as parameter, so there should be no ripple effect to propagate this flag.

Why not read_and_clear function if absolutely necessary.

Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read

[Dumitrescu, Cristian]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Saturday, February 21, 2015 1:53 AM
> To: Dumitrescu, Cristian
> Cc: Thomas Monjalon; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when
> read
> 
> On Fri, 20 Feb 2015 21:28:55 +0000
> "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:
> 
> > Agree.
> > Stephen, how about a run-time solution (I agree it would be much better,
> why did I not consider this in the first place?) of adding a new bool parameter
> in struct rte_sched_port_params: clear_stats_on_reset?
> > Both stats read function get the port handle (struct rte_sched_port *) as
> parameter, so there should be no ripple effect to propagate this flag.
> 
> Why not read_and_clear function if absolutely necessary.

I am not sure I understand what you mean. Are you suggesting different set of stats read functions, one that is read and clear, while the other one is read without clear?

Personally, I think we should avoid proliferating the number of stats functions, I would keep a single set of stats read functions, which can clear the stats or not, depending on behaviour configured per rte_sched object at creation time. Basically, based on the value of configuration parameter struct rte_sched_params::clear_stats_on_reset, the stats read functions do clear the counters or not. In my opinion, this allows a clean init-time selection of the required behaviour, and it also provides backward compatibility. Any issues with this approach?

Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read

[Neil Horman]

On Mon, Feb 09, 2015 at 10:48:36PM +0000, Dumitrescu, Cristian wrote:
> Hi Stephen,
> 
> What is the reason not to clear statistics on read? Do you have a use-case / justification for it?
> 
> (BTW, I see you added the reset functions, but was it also your intention to remove the memset to 0 from the stats read functions? :) )
> 
> Regards,
> Cristian
> 
Its the difference between a hardware and a software interface.  Hardware stats
are often read-clear, but software hides that, making stats continuous.
Exposing it is atypical for a software stack.
Neil

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Thursday, February 5, 2015 6:14 AM
> To: dev@dpdk.org
> Cc: Stephen Hemminger
> Subject: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read
> 
> From: Stephen Hemminger <shemming@brocade.com>
> 
> Make rte_sched statistics API work like the ethernet statistics API.
> Don't auto-clear statistics.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_sched/rte_sched.c | 30 ++++++++++++++++++++++++++++++
>  lib/librte_sched/rte_sched.h | 29 +++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
> index 8cb8bf1..d891e50 100644
> --- a/lib/librte_sched/rte_sched.c
> +++ b/lib/librte_sched/rte_sched.c
> @@ -935,6 +935,21 @@ rte_sched_subport_read_stats(struct rte_sched_port *port,
>  }
>  
>  int
> +rte_sched_subport_stats_reset(struct rte_sched_port *port,
> +			      uint32_t subport_id)
> +{
> +	struct rte_sched_subport *s;
> +
> +	/* Check user parameters */
> +	if (port == NULL || subport_id >= port->n_subports_per_port)
> +		return -1;
> +
> +	s = port->subport + subport_id;
> +	memset(&s->stats, 0, sizeof(struct rte_sched_subport_stats));
> +	return 0;
> +}
> +
> +int
>  rte_sched_queue_read_stats(struct rte_sched_port *port,
>  	uint32_t queue_id,
>  	struct rte_sched_queue_stats *stats,
> @@ -963,6 +978,21 @@ rte_sched_queue_read_stats(struct rte_sched_port *port,
>  	return 0;
>  }
>  
> +int
> +rte_sched_queue_stats_reset(struct rte_sched_port *port,
> +			    uint32_t queue_id)
> +{
> +	struct rte_sched_queue_extra *qe;
> +
> +	/* Check user parameters */
> +	if (port == NULL || queue_id >= rte_sched_port_queues_per_port(port))
> +		return -1;
> +
> +	qe = port->queue_extra + queue_id;
> +	memset(&qe->stats, 0, sizeof(struct rte_sched_queue_stats));
> +	return 0;
> +}
> +
>  static inline uint32_t
>  rte_sched_port_qindex(struct rte_sched_port *port, uint32_t subport, uint32_t pipe, uint32_t traffic_class, uint32_t queue)
>  {
> diff --git a/lib/librte_sched/rte_sched.h b/lib/librte_sched/rte_sched.h
> index e9bf18a..3d007e4 100644
> --- a/lib/librte_sched/rte_sched.h
> +++ b/lib/librte_sched/rte_sched.h
> @@ -317,6 +317,21 @@ rte_sched_subport_read_stats(struct rte_sched_port *port,
>  	struct rte_sched_subport_stats *stats,
>  	uint32_t *tc_ov);
>  
> +
> +/**
> + * Hierarchical scheduler subport statistics reset
> + *
> + * @param port
> + *   Handle to port scheduler instance
> + * @param subport_id
> + *   Subport ID
> + * @return
> + *   0 upon success, error code otherwise
> + */
> +int
> +rte_sched_subport_stats_reset(struct rte_sched_port *port,
> +			      uint32_t subport_id);
> +
>  /**
>   * Hierarchical scheduler queue statistics read
>   *
> @@ -338,6 +353,20 @@ rte_sched_queue_read_stats(struct rte_sched_port *port,
>  	struct rte_sched_queue_stats *stats,
>  	uint16_t *qlen);
>  
> +/**
> + * Hierarchical scheduler queue statistics reset
> + *
> + * @param port
> + *   Handle to port scheduler instance
> + * @param queue_id
> + *   Queue ID within port scheduler
> + * @return
> + *   0 upon success, error code otherwise
> + */
> +int
> +rte_sched_queue_stats_reset(struct rte_sched_port *port,
> +			    uint32_t queue_id);
> +
>  /*
>   * Run-time
>   *
> -- 
> 2.1.4
> 
> --------------------------------------------------------------
> Intel Shannon Limited
> Registered in Ireland
> Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
> Registered Number: 308263
> Business address: Dromore House, East Park, Shannon, Co. Clare
> 
> This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
> 
> 
>