DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline ports and tables
@ 2015-05-26 13:39 Maciej Gajdzica
  2015-05-26 13:51 ` Dumitrescu, Cristian
  2015-05-26 14:57 ` Stephen Hemminger
  0 siblings, 2 replies; 7+ messages in thread
From: Maciej Gajdzica @ 2015-05-26 13:39 UTC (permalink / raw)
  To: dev

This patch adds statistics collection for librte_pipeline.
Those statistics ale disabled by default during build time.

Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
---
 lib/librte_pipeline/rte_pipeline.c |  185 +++++++++++++++++++++++++++++++++---
 lib/librte_pipeline/rte_pipeline.h |   98 +++++++++++++++++++
 2 files changed, 272 insertions(+), 11 deletions(-)

diff --git a/lib/librte_pipeline/rte_pipeline.c b/lib/librte_pipeline/rte_pipeline.c
index 36d92c9..3ef36ad 100644
--- a/lib/librte_pipeline/rte_pipeline.c
+++ b/lib/librte_pipeline/rte_pipeline.c
@@ -48,6 +48,17 @@
 
 #define RTE_TABLE_INVALID                                 UINT32_MAX
 
+#if RTE_LOG_LEVEL == RTE_LOG_DEBUG
+#define RTE_PIPELINE_STATS_ADD(counter, val) \
+	({ (counter) += (val); })
+
+#define RTE_PIPELINE_STATS_ADD_M(counter, mask) \
+	({ (counter) += __builtin_popcountll(mask); })
+#else
+#define RTE_PIPELINE_STATS_ADD(counter, val)
+#define RTE_PIPELINE_STATS_ADD_M(counter, mask)
+#endif
+
 struct rte_port_in {
 	/* Input parameters */
 	struct rte_port_in_ops ops;
@@ -63,6 +74,8 @@ struct rte_port_in {
 
 	/* List of enabled ports */
 	struct rte_port_in *next;
+
+	uint64_t n_pkts_dropped_by_ah;
 };
 
 struct rte_port_out {
@@ -74,6 +87,8 @@ struct rte_port_out {
 
 	/* Handle to low-level port */
 	void *h_port;
+
+	uint64_t n_pkts_dropped_by_ah;
 };
 
 struct rte_table {
@@ -90,6 +105,12 @@ struct rte_table {
 
 	/* Handle to the low-level table object */
 	void *h_table;
+
+	/* Stats for this table. */
+	uint64_t n_pkts_dropped_by_lkp_hit_ah;
+	uint64_t n_pkts_dropped_by_lkp_miss_ah;
+	uint64_t n_pkts_dropped_lkp_hit;
+	uint64_t n_pkts_dropped_lkp_miss;
 };
 
 #define RTE_PIPELINE_MAX_NAME_SZ                           124
@@ -1040,6 +1061,8 @@ rte_pipeline_action_handler_port_bulk(struct rte_pipeline *p,
 
 		port_out->f_action_bulk(p->pkts, &pkts_mask, port_out->arg_ah);
 		p->action_mask0[RTE_PIPELINE_ACTION_DROP] |= pkts_mask ^  mask;
+		RTE_PIPELINE_STATS_ADD_M(port_out->n_pkts_dropped_by_ah,
+				pkts_mask ^  mask);
 	}
 
 	/* Output port TX */
@@ -1071,6 +1094,9 @@ rte_pipeline_action_handler_port(struct rte_pipeline *p, uint64_t pkts_mask)
 				p->action_mask0[RTE_PIPELINE_ACTION_DROP] |=
 					(pkt_mask ^ 1LLU) << i;
 
+				RTE_PIPELINE_STATS_ADD(port_out->n_pkts_dropped_by_ah,
+						pkt_mask ^ 1LLU);
+
 				/* Output port TX */
 				if (pkt_mask != 0)
 					port_out->ops.f_tx(port_out->h_port,
@@ -1104,6 +1130,9 @@ rte_pipeline_action_handler_port(struct rte_pipeline *p, uint64_t pkts_mask)
 				p->action_mask0[RTE_PIPELINE_ACTION_DROP] |=
 					(pkt_mask ^ 1LLU) << i;
 
+				RTE_PIPELINE_STATS_ADD(port_out->n_pkts_dropped_by_ah,
+						pkt_mask ^ 1LLU);
+
 				/* Output port TX */
 				if (pkt_mask != 0)
 					port_out->ops.f_tx(port_out->h_port,
@@ -1140,6 +1169,9 @@ rte_pipeline_action_handler_port_meta(struct rte_pipeline *p,
 				p->action_mask0[RTE_PIPELINE_ACTION_DROP] |=
 					(pkt_mask ^ 1LLU) << i;
 
+				RTE_PIPELINE_STATS_ADD(port_out->n_pkts_dropped_by_ah,
+						pkt_mask ^ 1ULL);
+
 				/* Output port TX */
 				if (pkt_mask != 0)
 					port_out->ops.f_tx(port_out->h_port,
@@ -1174,6 +1206,9 @@ rte_pipeline_action_handler_port_meta(struct rte_pipeline *p,
 				p->action_mask0[RTE_PIPELINE_ACTION_DROP] |=
 					(pkt_mask ^ 1LLU) << i;
 
+				RTE_PIPELINE_STATS_ADD(port_out->n_pkts_dropped_by_ah,
+						pkt_mask ^ 1ULL);
+
 				/* Output port TX */
 				if (pkt_mask != 0)
 					port_out->ops.f_tx(port_out->h_port,
@@ -1232,10 +1267,10 @@ rte_pipeline_run(struct rte_pipeline *p)
 		if (port_in->f_action != NULL) {
 			uint64_t mask = pkts_mask;
 
-			port_in->f_action(p->pkts, n_pkts, &pkts_mask,
-				port_in->arg_ah);
-			p->action_mask0[RTE_PIPELINE_ACTION_DROP] |=
-				pkts_mask ^ mask;
+			port_in->f_action(p->pkts, n_pkts, &pkts_mask, port_in->arg_ah);
+			mask ^= pkts_mask;
+			p->action_mask0[RTE_PIPELINE_ACTION_DROP] |= mask;
+			RTE_PIPELINE_STATS_ADD_M(port_in->n_pkts_dropped_by_ah, mask);
 		}
 
 		/* Table */
@@ -1261,9 +1296,10 @@ rte_pipeline_run(struct rte_pipeline *p)
 					table->f_action_miss(p->pkts,
 						&lookup_miss_mask,
 						default_entry, table->arg_ah);
-					p->action_mask0[
-						RTE_PIPELINE_ACTION_DROP] |=
-						lookup_miss_mask ^ mask;
+					mask ^= lookup_miss_mask;
+					p->action_mask0[RTE_PIPELINE_ACTION_DROP] |= mask;
+					RTE_PIPELINE_STATS_ADD_M(
+						table->n_pkts_dropped_by_lkp_miss_ah, mask);
 				}
 
 				/* Table reserved actions */
@@ -1277,6 +1313,10 @@ rte_pipeline_run(struct rte_pipeline *p)
 					uint32_t pos = default_entry->action;
 
 					p->action_mask0[pos] = lookup_miss_mask;
+					if (pos == RTE_PIPELINE_ACTION_DROP) {
+						RTE_PIPELINE_STATS_ADD_M(table->n_pkts_dropped_lkp_miss,
+							lookup_miss_mask);
+					}
 				}
 			}
 
@@ -1289,9 +1329,10 @@ rte_pipeline_run(struct rte_pipeline *p)
 					table->f_action_hit(p->pkts,
 						&lookup_hit_mask,
 						p->entries, table->arg_ah);
-					p->action_mask0[
-						RTE_PIPELINE_ACTION_DROP] |=
-						lookup_hit_mask ^ mask;
+					mask ^= lookup_hit_mask;
+					p->action_mask0[RTE_PIPELINE_ACTION_DROP] |= mask;
+					RTE_PIPELINE_STATS_ADD_M(
+						table->n_pkts_dropped_by_lkp_hit_ah, mask);
 				}
 
 				/* Table reserved actions */
@@ -1308,6 +1349,9 @@ rte_pipeline_run(struct rte_pipeline *p)
 				p->action_mask0[RTE_PIPELINE_ACTION_TABLE] |=
 					p->action_mask1[
 						RTE_PIPELINE_ACTION_TABLE];
+
+				RTE_PIPELINE_STATS_ADD_M(table->n_pkts_dropped_lkp_hit,
+						p->action_mask1[RTE_PIPELINE_ACTION_DROP]);
 			}
 
 			/* Prepare for next iteration */
@@ -1370,9 +1414,128 @@ rte_pipeline_port_out_packet_insert(struct rte_pipeline *p,
 
 		if (pkt_mask != 0) /* Output port TX */
 			port_out->ops.f_tx(port_out->h_port, pkt);
-		else
+		else {
 			rte_pktmbuf_free(pkt);
+			RTE_PIPELINE_STATS_ADD(port_out->n_pkts_dropped_by_ah, 1);
+		}
+	}
+
+	return 0;
+}
+
+int rte_pipeline_port_in_stats_read(struct rte_pipeline *p, uint32_t port_id,
+	struct rte_pipeline_port_in_stats *stats, int clear)
+{
+	struct rte_port_in *port;
+	int retval;
+
+	if (p == NULL) {
+		RTE_LOG(ERR, PIPELINE, "%s: pipeline parameter NULL\n",
+			__func__);
+		return -EINVAL;
+	}
+
+	if (port_id >= p->num_ports_in) {
+		RTE_LOG(ERR, PIPELINE,
+			"%s: port IN ID %u is out of range\n",
+			__func__, port_id);
+		return -EINVAL;
+	}
+
+	port = &p->ports_in[port_id];
+
+	if (port->ops.f_stats != NULL) {
+		retval = port->ops.f_stats(port->h_port, &stats->stats, clear);
+		if (retval)
+			return retval;
+	} else if (stats != NULL)
+		memset(&stats->stats, 0, sizeof(stats->stats));
+
+	if (stats != NULL)
+		stats->n_pkts_dropped_by_ah = port->n_pkts_dropped_by_ah;
+
+	if (clear != 0)
+		port->n_pkts_dropped_by_ah = 0;
+
+	return 0;
+}
+
+int rte_pipeline_port_out_stats_read(struct rte_pipeline *p, uint32_t port_id,
+	struct rte_pipeline_port_out_stats *stats, int clear)
+{
+	struct rte_port_out *port;
+	int retval;
+
+	if (p == NULL) {
+		RTE_LOG(ERR, PIPELINE, "%s: pipeline parameter NULL\n", __func__);
+		return -EINVAL;
+	}
+
+	if (port_id >= p->num_ports_out) {
+		RTE_LOG(ERR, PIPELINE,
+			"%s: port OUT ID %u is out of range\n", __func__, port_id);
+		return -EINVAL;
+	}
+
+	port = &p->ports_out[port_id];
+	if (port->ops.f_stats != NULL) {
+		retval = port->ops.f_stats(port->h_port, &stats->stats, clear);
+		if (retval != 0)
+			return retval;
+	} else if (stats != NULL)
+		memset(&stats->stats, 0, sizeof(stats->stats));
+
+	if (stats != NULL)
+		stats->n_pkts_dropped_by_ah = port->n_pkts_dropped_by_ah;
+
+	if (clear != 0)
+		port->n_pkts_dropped_by_ah = 0;
+
+	return 0;
+}
+
+int rte_pipeline_table_stats_read(struct rte_pipeline *p, uint32_t table_id,
+	struct rte_pipeline_table_stats *stats, int clear)
+{
+	struct rte_table *table;
+	int retval;
+
+	if (p == NULL) {
+		RTE_LOG(ERR, PIPELINE, "%s: pipeline parameter NULL\n",
+			__func__);
+		return -EINVAL;
+	}
+
+	if (table_id >= p->num_tables) {
+		RTE_LOG(ERR, PIPELINE,
+				"%s: table %u is out of range\n", __func__, table_id);
+		return -EINVAL;
+	}
+
+	table = &p->tables[table_id];
+	if (table->ops.f_stats != NULL) {
+		retval = table->ops.f_stats(table->h_table, &stats->stats, clear);
+		if (retval != 0)
+			return retval;
+	} else if (stats != NULL)
+		memset(&stats->stats, 0, sizeof(stats->stats));
+
+	if (stats != NULL) {
+		stats->n_pkts_dropped_by_lkp_hit_ah =
+			table->n_pkts_dropped_by_lkp_hit_ah;
+		stats->n_pkts_dropped_by_lkp_miss_ah =
+			table->n_pkts_dropped_by_lkp_miss_ah;
+		stats->n_pkts_dropped_lkp_hit = table->n_pkts_dropped_lkp_hit;
+		stats->n_pkts_dropped_lkp_miss = table->n_pkts_dropped_lkp_miss;
+	}
+
+	if (clear != 0) {
+		table->n_pkts_dropped_by_lkp_hit_ah = 0;
+		table->n_pkts_dropped_by_lkp_miss_ah = 0;
+		table->n_pkts_dropped_lkp_hit = 0;
+		table->n_pkts_dropped_lkp_miss = 0;
 	}
 
 	return 0;
 }
+
diff --git a/lib/librte_pipeline/rte_pipeline.h b/lib/librte_pipeline/rte_pipeline.h
index 145fc06..59e0710 100644
--- a/lib/librte_pipeline/rte_pipeline.h
+++ b/lib/librte_pipeline/rte_pipeline.h
@@ -112,6 +112,45 @@ struct rte_pipeline_params {
 	uint32_t offset_port_id;
 };
 
+/** Pipeline port in stats. */
+struct rte_pipeline_port_in_stats {
+	/** Port in stats. */
+	struct rte_port_in_stats stats;
+
+	/** Number of packets dropped by action handler. */
+	uint64_t n_pkts_dropped_by_ah;
+
+};
+
+/** Pipeline port out stats. */
+struct rte_pipeline_port_out_stats {
+	/** Port out stats. */
+	struct rte_port_out_stats stats;
+
+	/** Number of packets dropped by action handler. */
+	uint64_t n_pkts_dropped_by_ah;
+};
+
+/** Pipeline table stats. */
+struct rte_pipeline_table_stats {
+	/** Table stats. */
+	struct rte_table_stats stats;
+
+	/** Number of packets dropped by lookup hit action handler. */
+	uint64_t n_pkts_dropped_by_lkp_hit_ah;
+
+	/** Number of packets dropped by lookup miss action handler. */
+	uint64_t n_pkts_dropped_by_lkp_miss_ah;
+
+	/** Number of packets dropped by pipeline in behalf of this table based on
+	 * on action specified in table entry. */
+	uint64_t n_pkts_dropped_lkp_hit;
+
+	/** Number of packets dropped by pipeline in behalf of this table based on
+	 * on action specified in table entry. */
+	uint64_t n_pkts_dropped_lkp_miss;
+};
+
 /**
  * Pipeline create
  *
@@ -426,6 +465,26 @@ int rte_pipeline_table_entry_delete(struct rte_pipeline *p,
 	int *key_found,
 	struct rte_pipeline_table_entry *entry);
 
+/**
+ * Read pipeline table stats.
+ *
+ * This function reads table statistics identified by *table_id* of given
+ * pipeline *p*.
+ *
+ * @param p
+ *   Handle to pipeline instance.
+ * @param table_id
+ *   Port ID what stats will be returned.
+ * @param stats
+ *   Statistics buffer.
+ * @param clear
+ *   If not 0 clear stats after reading.
+ * @return
+ *   0 on success, error code otherwise
+ */
+int rte_pipeline_table_stats_read(struct rte_pipeline *p, uint32_t table_id,
+	struct rte_pipeline_table_stats *stats, int clear);
+
 /*
  * Port IN
  *
@@ -540,6 +599,26 @@ int rte_pipeline_port_in_enable(struct rte_pipeline *p,
 int rte_pipeline_port_in_disable(struct rte_pipeline *p,
 	uint32_t port_id);
 
+/**
+ * Read pipeline port in stats.
+ *
+ * This function reads port in statistics identified by *port_id* of given
+ * pipeline *p*.
+ *
+ * @param p
+ *   Handle to pipeline instance.
+ * @param port_id
+ *   Port ID what stats will be returned.
+ * @param stats
+ *   Statistics buffer.
+ * @param clear
+ *   If not 0 clear stats after reading.
+ * @return
+ *   0 on success, error code otherwise
+ */
+int rte_pipeline_port_in_stats_read(struct rte_pipeline *p, uint32_t port_id,
+	struct rte_pipeline_port_in_stats *stats, int clear);
+
 /*
  * Port OUT
  *
@@ -658,6 +737,25 @@ int rte_pipeline_port_out_packet_insert(struct rte_pipeline *p,
 	uint32_t port_id,
 	struct rte_mbuf *pkt);
 
+/**
+ * Read pipeline port out stats.
+ *
+ * This function reads port out statistics identified by *port_id* of given
+ * pipeline *p*.
+ *
+ * @param p
+ *   Handle to pipeline instance.
+ * @param port_id
+ *   Port ID what stats will be returned.
+ * @param stats
+ *   Statistics buffer.
+ * @param clear
+ *   If not 0 clear stats after reading.
+ * @return
+ *   0 on success, error code otherwise
+ */
+int rte_pipeline_port_out_stats_read(struct rte_pipeline *p, uint32_t port_id,
+	struct rte_pipeline_port_out_stats *stats, int clear);
 #ifdef __cplusplus
 }
 #endif
-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline ports and tables
  2015-05-26 13:39 [dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline ports and tables Maciej Gajdzica
@ 2015-05-26 13:51 ` Dumitrescu, Cristian
  2015-05-26 14:57 ` Stephen Hemminger
  1 sibling, 0 replies; 7+ messages in thread
From: Dumitrescu, Cristian @ 2015-05-26 13:51 UTC (permalink / raw)
  To: Gajdzica, MaciejX T, dev



> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maciej Gajdzica
> Sent: Tuesday, May 26, 2015 2:39 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline
> ports and tables
> 
> This patch adds statistics collection for librte_pipeline.
> Those statistics ale disabled by default during build time.
> 
> Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
> ---
>  lib/librte_pipeline/rte_pipeline.c |  185
> +++++++++++++++++++++++++++++++++---
>  lib/librte_pipeline/rte_pipeline.h |   98 +++++++++++++++++++
>  2 files changed, 272 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/librte_pipeline/rte_pipeline.c
> b/lib/librte_pipeline/rte_pipeline.c
> index 36d92c9..3ef36ad 100644
> --- a/lib/librte_pipeline/rte_pipeline.c
> +++ b/lib/librte_pipeline/rte_pipeline.c
> @@ -48,6 +48,17 @@
> 
>  #define RTE_TABLE_INVALID                                 UINT32_MAX
> 
> +#if RTE_LOG_LEVEL == RTE_LOG_DEBUG
> +#define RTE_PIPELINE_STATS_ADD(counter, val) \
> +	({ (counter) += (val); })
> +
> +#define RTE_PIPELINE_STATS_ADD_M(counter, mask) \
> +	({ (counter) += __builtin_popcountll(mask); })
> +#else
> +#define RTE_PIPELINE_STATS_ADD(counter, val)
> +#define RTE_PIPELINE_STATS_ADD_M(counter, mask)
> +#endif
> +
>  struct rte_port_in {
>  	/* Input parameters */
>  	struct rte_port_in_ops ops;
> @@ -63,6 +74,8 @@ struct rte_port_in {
> 
>  	/* List of enabled ports */
>  	struct rte_port_in *next;
> +
> +	uint64_t n_pkts_dropped_by_ah;
>  };
> 
>  struct rte_port_out {
> @@ -74,6 +87,8 @@ struct rte_port_out {
> 
>  	/* Handle to low-level port */
>  	void *h_port;
> +
> +	uint64_t n_pkts_dropped_by_ah;
>  };
> 
>  struct rte_table {
> @@ -90,6 +105,12 @@ struct rte_table {
> 
>  	/* Handle to the low-level table object */
>  	void *h_table;
> +
> +	/* Stats for this table. */
> +	uint64_t n_pkts_dropped_by_lkp_hit_ah;
> +	uint64_t n_pkts_dropped_by_lkp_miss_ah;
> +	uint64_t n_pkts_dropped_lkp_hit;
> +	uint64_t n_pkts_dropped_lkp_miss;
>  };
> 
>  #define RTE_PIPELINE_MAX_NAME_SZ                           124
> @@ -1040,6 +1061,8 @@ rte_pipeline_action_handler_port_bulk(struct
> rte_pipeline *p,
> 
>  		port_out->f_action_bulk(p->pkts, &pkts_mask, port_out-
> >arg_ah);
>  		p->action_mask0[RTE_PIPELINE_ACTION_DROP] |=
> pkts_mask ^  mask;
> +		RTE_PIPELINE_STATS_ADD_M(port_out-
> >n_pkts_dropped_by_ah,
> +				pkts_mask ^  mask);
>  	}
> 
>  	/* Output port TX */
> @@ -1071,6 +1094,9 @@ rte_pipeline_action_handler_port(struct
> rte_pipeline *p, uint64_t pkts_mask)
>  				p-
> >action_mask0[RTE_PIPELINE_ACTION_DROP] |=
>  					(pkt_mask ^ 1LLU) << i;
> 
> +				RTE_PIPELINE_STATS_ADD(port_out-
> >n_pkts_dropped_by_ah,
> +						pkt_mask ^ 1LLU);
> +
>  				/* Output port TX */
>  				if (pkt_mask != 0)
>  					port_out->ops.f_tx(port_out-
> >h_port,
> @@ -1104,6 +1130,9 @@ rte_pipeline_action_handler_port(struct
> rte_pipeline *p, uint64_t pkts_mask)
>  				p-
> >action_mask0[RTE_PIPELINE_ACTION_DROP] |=
>  					(pkt_mask ^ 1LLU) << i;
> 
> +				RTE_PIPELINE_STATS_ADD(port_out-
> >n_pkts_dropped_by_ah,
> +						pkt_mask ^ 1LLU);
> +
>  				/* Output port TX */
>  				if (pkt_mask != 0)
>  					port_out->ops.f_tx(port_out-
> >h_port,
> @@ -1140,6 +1169,9 @@ rte_pipeline_action_handler_port_meta(struct
> rte_pipeline *p,
>  				p-
> >action_mask0[RTE_PIPELINE_ACTION_DROP] |=
>  					(pkt_mask ^ 1LLU) << i;
> 
> +				RTE_PIPELINE_STATS_ADD(port_out-
> >n_pkts_dropped_by_ah,
> +						pkt_mask ^ 1ULL);
> +
>  				/* Output port TX */
>  				if (pkt_mask != 0)
>  					port_out->ops.f_tx(port_out-
> >h_port,
> @@ -1174,6 +1206,9 @@ rte_pipeline_action_handler_port_meta(struct
> rte_pipeline *p,
>  				p-
> >action_mask0[RTE_PIPELINE_ACTION_DROP] |=
>  					(pkt_mask ^ 1LLU) << i;
> 
> +				RTE_PIPELINE_STATS_ADD(port_out-
> >n_pkts_dropped_by_ah,
> +						pkt_mask ^ 1ULL);
> +
>  				/* Output port TX */
>  				if (pkt_mask != 0)
>  					port_out->ops.f_tx(port_out-
> >h_port,
> @@ -1232,10 +1267,10 @@ rte_pipeline_run(struct rte_pipeline *p)
>  		if (port_in->f_action != NULL) {
>  			uint64_t mask = pkts_mask;
> 
> -			port_in->f_action(p->pkts, n_pkts, &pkts_mask,
> -				port_in->arg_ah);
> -			p->action_mask0[RTE_PIPELINE_ACTION_DROP] |=
> -				pkts_mask ^ mask;
> +			port_in->f_action(p->pkts, n_pkts, &pkts_mask,
> port_in->arg_ah);
> +			mask ^= pkts_mask;
> +			p->action_mask0[RTE_PIPELINE_ACTION_DROP] |=
> mask;
> +			RTE_PIPELINE_STATS_ADD_M(port_in-
> >n_pkts_dropped_by_ah, mask);
>  		}
> 
>  		/* Table */
> @@ -1261,9 +1296,10 @@ rte_pipeline_run(struct rte_pipeline *p)
>  					table->f_action_miss(p->pkts,
>  						&lookup_miss_mask,
>  						default_entry, table-
> >arg_ah);
> -					p->action_mask0[
> -
> 	RTE_PIPELINE_ACTION_DROP] |=
> -						lookup_miss_mask ^ mask;
> +					mask ^= lookup_miss_mask;
> +					p-
> >action_mask0[RTE_PIPELINE_ACTION_DROP] |= mask;
> +					RTE_PIPELINE_STATS_ADD_M(
> +						table-
> >n_pkts_dropped_by_lkp_miss_ah, mask);
>  				}
> 
>  				/* Table reserved actions */
> @@ -1277,6 +1313,10 @@ rte_pipeline_run(struct rte_pipeline *p)
>  					uint32_t pos = default_entry->action;
> 
>  					p->action_mask0[pos] =
> lookup_miss_mask;
> +					if (pos ==
> RTE_PIPELINE_ACTION_DROP) {
> +
> 	RTE_PIPELINE_STATS_ADD_M(table->n_pkts_dropped_lkp_miss,
> +							lookup_miss_mask);
> +					}
>  				}
>  			}
> 
> @@ -1289,9 +1329,10 @@ rte_pipeline_run(struct rte_pipeline *p)
>  					table->f_action_hit(p->pkts,
>  						&lookup_hit_mask,
>  						p->entries, table->arg_ah);
> -					p->action_mask0[
> -
> 	RTE_PIPELINE_ACTION_DROP] |=
> -						lookup_hit_mask ^ mask;
> +					mask ^= lookup_hit_mask;
> +					p-
> >action_mask0[RTE_PIPELINE_ACTION_DROP] |= mask;
> +					RTE_PIPELINE_STATS_ADD_M(
> +						table-
> >n_pkts_dropped_by_lkp_hit_ah, mask);
>  				}
> 
>  				/* Table reserved actions */
> @@ -1308,6 +1349,9 @@ rte_pipeline_run(struct rte_pipeline *p)
>  				p-
> >action_mask0[RTE_PIPELINE_ACTION_TABLE] |=
>  					p->action_mask1[
> 
> 	RTE_PIPELINE_ACTION_TABLE];
> +
> +				RTE_PIPELINE_STATS_ADD_M(table-
> >n_pkts_dropped_lkp_hit,
> +						p-
> >action_mask1[RTE_PIPELINE_ACTION_DROP]);
>  			}
> 
>  			/* Prepare for next iteration */
> @@ -1370,9 +1414,128 @@ rte_pipeline_port_out_packet_insert(struct
> rte_pipeline *p,
> 
>  		if (pkt_mask != 0) /* Output port TX */
>  			port_out->ops.f_tx(port_out->h_port, pkt);
> -		else
> +		else {
>  			rte_pktmbuf_free(pkt);
> +			RTE_PIPELINE_STATS_ADD(port_out-
> >n_pkts_dropped_by_ah, 1);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +int rte_pipeline_port_in_stats_read(struct rte_pipeline *p, uint32_t
> port_id,
> +	struct rte_pipeline_port_in_stats *stats, int clear)
> +{
> +	struct rte_port_in *port;
> +	int retval;
> +
> +	if (p == NULL) {
> +		RTE_LOG(ERR, PIPELINE, "%s: pipeline parameter NULL\n",
> +			__func__);
> +		return -EINVAL;
> +	}
> +
> +	if (port_id >= p->num_ports_in) {
> +		RTE_LOG(ERR, PIPELINE,
> +			"%s: port IN ID %u is out of range\n",
> +			__func__, port_id);
> +		return -EINVAL;
> +	}
> +
> +	port = &p->ports_in[port_id];
> +
> +	if (port->ops.f_stats != NULL) {
> +		retval = port->ops.f_stats(port->h_port, &stats->stats,
> clear);
> +		if (retval)
> +			return retval;
> +	} else if (stats != NULL)
> +		memset(&stats->stats, 0, sizeof(stats->stats));
> +
> +	if (stats != NULL)
> +		stats->n_pkts_dropped_by_ah = port-
> >n_pkts_dropped_by_ah;
> +
> +	if (clear != 0)
> +		port->n_pkts_dropped_by_ah = 0;
> +
> +	return 0;
> +}
> +
> +int rte_pipeline_port_out_stats_read(struct rte_pipeline *p, uint32_t
> port_id,
> +	struct rte_pipeline_port_out_stats *stats, int clear)
> +{
> +	struct rte_port_out *port;
> +	int retval;
> +
> +	if (p == NULL) {
> +		RTE_LOG(ERR, PIPELINE, "%s: pipeline parameter NULL\n",
> __func__);
> +		return -EINVAL;
> +	}
> +
> +	if (port_id >= p->num_ports_out) {
> +		RTE_LOG(ERR, PIPELINE,
> +			"%s: port OUT ID %u is out of range\n", __func__,
> port_id);
> +		return -EINVAL;
> +	}
> +
> +	port = &p->ports_out[port_id];
> +	if (port->ops.f_stats != NULL) {
> +		retval = port->ops.f_stats(port->h_port, &stats->stats,
> clear);
> +		if (retval != 0)
> +			return retval;
> +	} else if (stats != NULL)
> +		memset(&stats->stats, 0, sizeof(stats->stats));
> +
> +	if (stats != NULL)
> +		stats->n_pkts_dropped_by_ah = port-
> >n_pkts_dropped_by_ah;
> +
> +	if (clear != 0)
> +		port->n_pkts_dropped_by_ah = 0;
> +
> +	return 0;
> +}
> +
> +int rte_pipeline_table_stats_read(struct rte_pipeline *p, uint32_t table_id,
> +	struct rte_pipeline_table_stats *stats, int clear)
> +{
> +	struct rte_table *table;
> +	int retval;
> +
> +	if (p == NULL) {
> +		RTE_LOG(ERR, PIPELINE, "%s: pipeline parameter NULL\n",
> +			__func__);
> +		return -EINVAL;
> +	}
> +
> +	if (table_id >= p->num_tables) {
> +		RTE_LOG(ERR, PIPELINE,
> +				"%s: table %u is out of range\n", __func__,
> table_id);
> +		return -EINVAL;
> +	}
> +
> +	table = &p->tables[table_id];
> +	if (table->ops.f_stats != NULL) {
> +		retval = table->ops.f_stats(table->h_table, &stats->stats,
> clear);
> +		if (retval != 0)
> +			return retval;
> +	} else if (stats != NULL)
> +		memset(&stats->stats, 0, sizeof(stats->stats));
> +
> +	if (stats != NULL) {
> +		stats->n_pkts_dropped_by_lkp_hit_ah =
> +			table->n_pkts_dropped_by_lkp_hit_ah;
> +		stats->n_pkts_dropped_by_lkp_miss_ah =
> +			table->n_pkts_dropped_by_lkp_miss_ah;
> +		stats->n_pkts_dropped_lkp_hit = table-
> >n_pkts_dropped_lkp_hit;
> +		stats->n_pkts_dropped_lkp_miss = table-
> >n_pkts_dropped_lkp_miss;
> +	}
> +
> +	if (clear != 0) {
> +		table->n_pkts_dropped_by_lkp_hit_ah = 0;
> +		table->n_pkts_dropped_by_lkp_miss_ah = 0;
> +		table->n_pkts_dropped_lkp_hit = 0;
> +		table->n_pkts_dropped_lkp_miss = 0;
>  	}
> 
>  	return 0;
>  }
> +
> diff --git a/lib/librte_pipeline/rte_pipeline.h
> b/lib/librte_pipeline/rte_pipeline.h
> index 145fc06..59e0710 100644
> --- a/lib/librte_pipeline/rte_pipeline.h
> +++ b/lib/librte_pipeline/rte_pipeline.h
> @@ -112,6 +112,45 @@ struct rte_pipeline_params {
>  	uint32_t offset_port_id;
>  };
> 
> +/** Pipeline port in stats. */
> +struct rte_pipeline_port_in_stats {
> +	/** Port in stats. */
> +	struct rte_port_in_stats stats;
> +
> +	/** Number of packets dropped by action handler. */
> +	uint64_t n_pkts_dropped_by_ah;
> +
> +};
> +
> +/** Pipeline port out stats. */
> +struct rte_pipeline_port_out_stats {
> +	/** Port out stats. */
> +	struct rte_port_out_stats stats;
> +
> +	/** Number of packets dropped by action handler. */
> +	uint64_t n_pkts_dropped_by_ah;
> +};
> +
> +/** Pipeline table stats. */
> +struct rte_pipeline_table_stats {
> +	/** Table stats. */
> +	struct rte_table_stats stats;
> +
> +	/** Number of packets dropped by lookup hit action handler. */
> +	uint64_t n_pkts_dropped_by_lkp_hit_ah;
> +
> +	/** Number of packets dropped by lookup miss action handler. */
> +	uint64_t n_pkts_dropped_by_lkp_miss_ah;
> +
> +	/** Number of packets dropped by pipeline in behalf of this table
> based on
> +	 * on action specified in table entry. */
> +	uint64_t n_pkts_dropped_lkp_hit;
> +
> +	/** Number of packets dropped by pipeline in behalf of this table
> based on
> +	 * on action specified in table entry. */
> +	uint64_t n_pkts_dropped_lkp_miss;
> +};
> +
>  /**
>   * Pipeline create
>   *
> @@ -426,6 +465,26 @@ int rte_pipeline_table_entry_delete(struct
> rte_pipeline *p,
>  	int *key_found,
>  	struct rte_pipeline_table_entry *entry);
> 
> +/**
> + * Read pipeline table stats.
> + *
> + * This function reads table statistics identified by *table_id* of given
> + * pipeline *p*.
> + *
> + * @param p
> + *   Handle to pipeline instance.
> + * @param table_id
> + *   Port ID what stats will be returned.
> + * @param stats
> + *   Statistics buffer.
> + * @param clear
> + *   If not 0 clear stats after reading.
> + * @return
> + *   0 on success, error code otherwise
> + */
> +int rte_pipeline_table_stats_read(struct rte_pipeline *p, uint32_t table_id,
> +	struct rte_pipeline_table_stats *stats, int clear);
> +
>  /*
>   * Port IN
>   *
> @@ -540,6 +599,26 @@ int rte_pipeline_port_in_enable(struct rte_pipeline
> *p,
>  int rte_pipeline_port_in_disable(struct rte_pipeline *p,
>  	uint32_t port_id);
> 
> +/**
> + * Read pipeline port in stats.
> + *
> + * This function reads port in statistics identified by *port_id* of given
> + * pipeline *p*.
> + *
> + * @param p
> + *   Handle to pipeline instance.
> + * @param port_id
> + *   Port ID what stats will be returned.
> + * @param stats
> + *   Statistics buffer.
> + * @param clear
> + *   If not 0 clear stats after reading.
> + * @return
> + *   0 on success, error code otherwise
> + */
> +int rte_pipeline_port_in_stats_read(struct rte_pipeline *p, uint32_t
> port_id,
> +	struct rte_pipeline_port_in_stats *stats, int clear);
> +
>  /*
>   * Port OUT
>   *
> @@ -658,6 +737,25 @@ int rte_pipeline_port_out_packet_insert(struct
> rte_pipeline *p,
>  	uint32_t port_id,
>  	struct rte_mbuf *pkt);
> 
> +/**
> + * Read pipeline port out stats.
> + *
> + * This function reads port out statistics identified by *port_id* of given
> + * pipeline *p*.
> + *
> + * @param p
> + *   Handle to pipeline instance.
> + * @param port_id
> + *   Port ID what stats will be returned.
> + * @param stats
> + *   Statistics buffer.
> + * @param clear
> + *   If not 0 clear stats after reading.
> + * @return
> + *   0 on success, error code otherwise
> + */
> +int rte_pipeline_port_out_stats_read(struct rte_pipeline *p, uint32_t
> port_id,
> +	struct rte_pipeline_port_out_stats *stats, int clear);
>  #ifdef __cplusplus
>  }
>  #endif
> --
> 1.7.9.5

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline ports and tables
  2015-05-26 13:39 [dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline ports and tables Maciej Gajdzica
  2015-05-26 13:51 ` Dumitrescu, Cristian
@ 2015-05-26 14:57 ` Stephen Hemminger
  2015-05-26 21:35   ` Dumitrescu, Cristian
  2015-05-27  4:30   ` Thomas Monjalon
  1 sibling, 2 replies; 7+ messages in thread
From: Stephen Hemminger @ 2015-05-26 14:57 UTC (permalink / raw)
  To: Maciej Gajdzica; +Cc: dev

On Tue, 26 May 2015 15:39:18 +0200
Maciej Gajdzica <maciejx.t.gajdzica@intel.com> wrote:

> +#if RTE_LOG_LEVEL == RTE_LOG_DEBUG
> +#define RTE_PIPELINE_STATS_ADD(counter, val) \
> +	({ (counter) += (val); })
> +
> +#define RTE_PIPELINE_STATS_ADD_M(counter, mask) \
> +	({ (counter) += __builtin_popcountll(mask); })
> +#else
> +#define RTE_PIPELINE_STATS_ADD(counter, val)
> +#define RTE_PIPELINE_STATS_ADD_M(counter, mask)
> +#endif

This is worse idea than the previous one.
I want statistics done on a per lcore basis, and always available
because real users on production system want statistics (but they
don't want log spam).

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline ports and tables
  2015-05-26 14:57 ` Stephen Hemminger
@ 2015-05-26 21:35   ` Dumitrescu, Cristian
  2015-05-26 21:47     ` Stephen Hemminger
  2015-05-27  4:30   ` Thomas Monjalon
  1 sibling, 1 reply; 7+ messages in thread
From: Dumitrescu, Cristian @ 2015-05-26 21:35 UTC (permalink / raw)
  To: Stephen Hemminger, Gajdzica, MaciejX T; +Cc: dev



> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen
> Hemminger
> Sent: Tuesday, May 26, 2015 3:57 PM
> To: Gajdzica, MaciejX T
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline
> ports and tables
> 
> On Tue, 26 May 2015 15:39:18 +0200
> Maciej Gajdzica <maciejx.t.gajdzica@intel.com> wrote:
> 
> > +#if RTE_LOG_LEVEL == RTE_LOG_DEBUG
> > +#define RTE_PIPELINE_STATS_ADD(counter, val) \
> > +	({ (counter) += (val); })
> > +
> > +#define RTE_PIPELINE_STATS_ADD_M(counter, mask) \
> > +	({ (counter) += __builtin_popcountll(mask); })
> > +#else
> > +#define RTE_PIPELINE_STATS_ADD(counter, val)
> > +#define RTE_PIPELINE_STATS_ADD_M(counter, mask)
> > +#endif
> 
> This is worse idea than the previous one.
> I want statistics done on a per lcore basis, and always available
> because real users on production system want statistics (but they
> don't want log spam).

Stephen,

Thomas and myself converged towards this solution, Thomas asked if anybody objects, you did not (http://www.dpdk.org/ml/archives/dev/2015-May/018099.html) . You say this idea is bad, but what exactly is your objection? Do you have an alternative proposal?

You already mentioned in the previous thread you would like to have per lcore statistics. I was kindly asking you to describe your idea, but you did not do this yet (http://www.dpdk.org/ml/archives/dev/2015-May/017956.html ). Can you please describe it? Each port instance has its own statistics counters. Each lcore can run one or more pipeline instances, therefore each lcore typically runs several port instances of the same or different type (each port instance with its own statistics), so how is "per lcore stats" requirement applicable here?

You also reiterate that you would like to have the stats always enabled. You can definitely do this, it is one of the available choices, but why not also accommodate the users that want to pick the opposite choice? Why force apps to spend cycles on stats if the app either does not want these counters (library counters not relevant for that app, maybe the app is only interested in maintaining some other stats that it implements itself) or do not want them anymore (maybe they only needed them during debug phase), etc? Jay asked this question, and I did my best in my reply to describe our motivation (http://www.dpdk.org/ml/archives/dev/2015-May/017992.html). Maybe you missed that post, it would be good to get your reply on this one too.

Thanks,
Cristian

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline ports and tables
  2015-05-26 21:35   ` Dumitrescu, Cristian
@ 2015-05-26 21:47     ` Stephen Hemminger
  2015-05-26 22:30       ` Dumitrescu, Cristian
  0 siblings, 1 reply; 7+ messages in thread
From: Stephen Hemminger @ 2015-05-26 21:47 UTC (permalink / raw)
  To: Dumitrescu, Cristian; +Cc: dev

On Tue, 26 May 2015 21:35:22 +0000
"Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:

> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen
> > Hemminger
> > Sent: Tuesday, May 26, 2015 3:57 PM
> > To: Gajdzica, MaciejX T
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline
> > ports and tables
> > 
> > On Tue, 26 May 2015 15:39:18 +0200
> > Maciej Gajdzica <maciejx.t.gajdzica@intel.com> wrote:
> > 
> > > +#if RTE_LOG_LEVEL == RTE_LOG_DEBUG
> > > +#define RTE_PIPELINE_STATS_ADD(counter, val) \
> > > +	({ (counter) += (val); })
> > > +
> > > +#define RTE_PIPELINE_STATS_ADD_M(counter, mask) \
> > > +	({ (counter) += __builtin_popcountll(mask); })
> > > +#else
> > > +#define RTE_PIPELINE_STATS_ADD(counter, val)
> > > +#define RTE_PIPELINE_STATS_ADD_M(counter, mask)
> > > +#endif
> > 
> > This is worse idea than the previous one.
> > I want statistics done on a per lcore basis, and always available
> > because real users on production system want statistics (but they
> > don't want log spam).
> 
> Stephen,
> 
> Thomas and myself converged towards this solution, Thomas asked if anybody objects, you did not (http://www.dpdk.org/ml/archives/dev/2015-May/018099.html) . You say this idea is bad, but what exactly is your objection? Do you have an alternative proposal?

Yes. Always keep statistics.

We use functions like this internally.

struct xxx_stats {
	uint64_t mib[XXX_MIB_MAX];
};
extern struct xxx_stats xxx_stats[RTE_MAX_LCORE];

#define XXXSTAT_INC(type)	xxxstats[rte_lcore_id()].mibs[type]++


> You already mentioned in the previous thread you would like to have per lcore statistics. I was kindly asking you to describe your idea, but you did not do this yet (http://www.dpdk.org/ml/archives/dev/2015-May/017956.html ). Can you please describe it? Each port instance has its own statistics counters. Each lcore can run one or more pipeline instances, therefore each lcore typically runs several port instances of the same or different type (each port instance with its own statistics), so how is "per lcore stats" requirement applicable here?

I thought you were familiar with technique of having per-cpu structures and variables
widely used in Linux kernel to prevent cache thrashing. Although you call it false sharing,
it isn't false., it happens when same counter ping-pongs between multiple threads for
no added benefit.


> You also reiterate that you would like to have the stats always enabled. You can definitely do this, it is one of the available choices, but why not also accommodate the users that want to pick the opposite choice? Why force apps to spend cycles on stats if the app either does not want these counters (library counters not relevant for that app, maybe the app is only interested in maintaining some other stats that it implements itself) or do not want them anymore (maybe they only needed them during debug phase), etc? Jay asked this question, and I did my best in my reply to describe our motivation (http://www.dpdk.org/ml/archives/dev/2015-May/017992.html). Maybe you missed that post, it would be good to get your reply on this one too.

I want to see DPDK get out of the config madness.
This is real code, not an Intel benchmark special.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline ports and tables
  2015-05-26 21:47     ` Stephen Hemminger
@ 2015-05-26 22:30       ` Dumitrescu, Cristian
  0 siblings, 0 replies; 7+ messages in thread
From: Dumitrescu, Cristian @ 2015-05-26 22:30 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev



> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, May 26, 2015 10:48 PM
> To: Dumitrescu, Cristian
> Cc: Gajdzica, MaciejX T; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline
> ports and tables
> 
> On Tue, 26 May 2015 21:35:22 +0000
> "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:
> 
> >
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen
> > > Hemminger
> > > Sent: Tuesday, May 26, 2015 3:57 PM
> > > To: Gajdzica, MaciejX T
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v3] pipeline: add statistics for
> librte_pipeline
> > > ports and tables
> > >
> > > On Tue, 26 May 2015 15:39:18 +0200
> > > Maciej Gajdzica <maciejx.t.gajdzica@intel.com> wrote:
> > >
> > > > +#if RTE_LOG_LEVEL == RTE_LOG_DEBUG
> > > > +#define RTE_PIPELINE_STATS_ADD(counter, val) \
> > > > +	({ (counter) += (val); })
> > > > +
> > > > +#define RTE_PIPELINE_STATS_ADD_M(counter, mask) \
> > > > +	({ (counter) += __builtin_popcountll(mask); })
> > > > +#else
> > > > +#define RTE_PIPELINE_STATS_ADD(counter, val)
> > > > +#define RTE_PIPELINE_STATS_ADD_M(counter, mask)
> > > > +#endif
> > >
> > > This is worse idea than the previous one.
> > > I want statistics done on a per lcore basis, and always available
> > > because real users on production system want statistics (but they
> > > don't want log spam).
> >
> > Stephen,
> >
> > Thomas and myself converged towards this solution, Thomas asked if
> anybody objects, you did not (http://www.dpdk.org/ml/archives/dev/2015-
> May/018099.html) . You say this idea is bad, but what exactly is your
> objection? Do you have an alternative proposal?
> 
> Yes. Always keep statistics.
> 
> We use functions like this internally.
> 
> struct xxx_stats {
> 	uint64_t mib[XXX_MIB_MAX];
> };
> extern struct xxx_stats xxx_stats[RTE_MAX_LCORE];
> 
> #define XXXSTAT_INC(type)	xxxstats[rte_lcore_id()].mibs[type]++
> 
> 

This can only be applied if stats are always enabled. I know this is your preferred choice, but other people have the requirement to be able to have the stats collection configurable, and we should also accommodate the needs of those people. Your code snippet does not provide a solution for this problem.

> > You already mentioned in the previous thread you would like to have per
> lcore statistics. I was kindly asking you to describe your idea, but you did not
> do this yet (http://www.dpdk.org/ml/archives/dev/2015-May/017956.html ).
> Can you please describe it? Each port instance has its own statistics counters.
> Each lcore can run one or more pipeline instances, therefore each lcore
> typically runs several port instances of the same or different type (each port
> instance with its own statistics), so how is "per lcore stats" requirement
> applicable here?
> 
> I thought you were familiar with technique of having per-cpu structures and
> variables
> widely used in Linux kernel to prevent cache thrashing. Although you call it
> false sharing,
> it isn't false., it happens when same counter ping-pongs between multiple
> threads for
> no added benefit.

The "per lcore stats" is applicable to stats structures that are accessed by more than one lcore. In our case, each port instance is handled by a single lcore, so it is never the case that two lcores would access the stats of the same port instance. So, we can safely say that port stats are "per lcore" already.

> 
> 
> > You also reiterate that you would like to have the stats always enabled. You
> can definitely do this, it is one of the available choices, but why not also
> accommodate the users that want to pick the opposite choice? Why force
> apps to spend cycles on stats if the app either does not want these counters
> (library counters not relevant for that app, maybe the app is only interested
> in maintaining some other stats that it implements itself) or do not want
> them anymore (maybe they only needed them during debug phase), etc?
> Jay asked this question, and I did my best in my reply to describe our
> motivation (http://www.dpdk.org/ml/archives/dev/2015-May/017992.html).
> Maybe you missed that post, it would be good to get your reply on this one
> too.
> 
> I want to see DPDK get out of the config madness.
> This is real code, not an Intel benchmark special.

You seem to make this assumption that our real reason for having the stats collection configurable is to get good benchmark results by disabling the stats. This cannot be a real reason, as a complete benchmark reports always specifies if stats were enabled or not. Easy to check.

By stating this, you implicitly acknowledge that stats collection consumes CPU cycles that might be significant for the overall app performance, right? So why waste CPU cycles in the case when the app does not need those counters?

The message I am trying to get across to you is that there are multiple _real_ reasons why an app would not want to collect the stats counters of library X, considering that stats counters do not come for free, but at the expense of CPU cycles:

1. For app A, the stats counters of library X might be totally irrelevant. Maybe the app does not care how many packets got through/got dropped by port Y of pipeline instance Z. Maybe the app has some other stats that it cares about, e.g. number of online users, mean connection time, etc that are collected by the app, not the library, therefore counters collected by library X are meaningless and they should not be collected;

2. For app B, the library stats are only relevant during debugging phase; once debugging completed, stats are no longer required.

Why do you want to have the library forcing the app to collect some counters that the app might not want to collect? Let's not force people to do what we think is right, our job here is to provide options for them to pick.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline ports and tables
  2015-05-26 14:57 ` Stephen Hemminger
  2015-05-26 21:35   ` Dumitrescu, Cristian
@ 2015-05-27  4:30   ` Thomas Monjalon
  1 sibling, 0 replies; 7+ messages in thread
From: Thomas Monjalon @ 2015-05-27  4:30 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: dev

2015-05-26 07:57, Stephen Hemminger:
> On Tue, 26 May 2015 15:39:18 +0200
> Maciej Gajdzica <maciejx.t.gajdzica@intel.com> wrote:
> 
> > +#if RTE_LOG_LEVEL == RTE_LOG_DEBUG
> > +#define RTE_PIPELINE_STATS_ADD(counter, val) \
> > +	({ (counter) += (val); })
> > +
> > +#define RTE_PIPELINE_STATS_ADD_M(counter, mask) \
> > +	({ (counter) += __builtin_popcountll(mask); })
> > +#else
> > +#define RTE_PIPELINE_STATS_ADD(counter, val)
> > +#define RTE_PIPELINE_STATS_ADD_M(counter, mask)
> > +#endif
> 
> This is worse idea than the previous one.
> I want statistics done on a per lcore basis, and always available
> because real users on production system want statistics (but they
> don't want log spam).

If they don't want log spam, they will increase log level with
the option --log-level.
By the way, maybe that RTE_LOG_INFO would be more appropriate for
these statistics in order to build them without having debug checks
in drivers.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2015-05-27  4:32 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-26 13:39 [dpdk-dev] [PATCH v3] pipeline: add statistics for librte_pipeline ports and tables Maciej Gajdzica
2015-05-26 13:51 ` Dumitrescu, Cristian
2015-05-26 14:57 ` Stephen Hemminger
2015-05-26 21:35   ` Dumitrescu, Cristian
2015-05-26 21:47     ` Stephen Hemminger
2015-05-26 22:30       ` Dumitrescu, Cristian
2015-05-27  4:30   ` Thomas Monjalon

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).