[dpdk-dev] [PATCH v4 1/9] app/procinfo: add usage for new debug

[dpdk-dev] [PATCH v4 1/9] app/procinfo: add usage for new debug

[Vipin Varghese]

Update the file with MACRO for stats border, usage text information
and string comparision.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---

V3:
 - change the usage details - Vipin Varghese

V2:
 - change word dbg to show - Stephen Hemminger
---
 app/proc-info/main.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index c20effa4f..5779f07e7 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -36,6 +36,10 @@
 
 #define MAX_STRING_LEN 256
 
+#define STATS_BDR_FMT "========================================"
+#define STATS_BDR_STR(w, s) printf("%.*s%s%.*s\n", w, \
+	STATS_BDR_FMT, s, w, STATS_BDR_FMT)
+
 /**< mask of enabled ports */
 static uint32_t enabled_port_mask;
 /**< Enable stats. */
@@ -83,7 +87,12 @@ proc_info_usage(const char *prgname)
 		"  --stats-reset: to reset port statistics\n"
 		"  --xstats-reset: to reset port extended statistics\n"
 		"  --collectd-format: to print statistics to STDOUT in expected by collectd format\n"
-		"  --host-id STRING: host id used to identify the system process is running on\n",
+		"  --host-id STRING: host id used to identify the system process is running on\n"
+		"  --show-port to display ports information\n"
+		"  --show-tm to display traffic manager information for ports\n"
+		"  --show-crypto to display crypto information\n"
+		"  --show-ring[=name] to display ring information\n"
+		"  --show-mempool[=name] to display mempool information\n",
 		prgname);
 }
 
@@ -190,6 +199,11 @@ proc_info_parse_args(int argc, char **argv)
 		{"collectd-format", 0, NULL, 0},
 		{"xstats-ids", 1, NULL, 1},
 		{"host-id", 0, NULL, 0},
+		{"show-port", 0, NULL, 0},
+		{"show-tm", 0, NULL, 0},
+		{"show-crypto", 0, NULL, 0},
+		{"show-ring", optional_argument, NULL, 0},
+		{"show-mempool", optional_argument, NULL, 0},
 		{NULL, 0, 0, 0}
 	};
 
-- 
2.17.1

[dpdk-dev] [PATCH v4 2/9] app/procinfo: add compare for new options

[Vipin Varghese]

Add code for new debug options to compare usage strings and set
enable flag.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---

V3:
 - variables from debug to show - Vipin Varghese

V2:
 - compare string from dbg to show - Stephen Hemminger
---
 app/proc-info/main.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index 5779f07e7..76266d5cb 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -63,6 +63,18 @@ static uint32_t mem_info;
 /**< Enable displaying xstat name. */
 static uint32_t enable_xstats_name;
 static char *xstats_name;
+/**< Enable show port. */
+static uint32_t enable_shw_port;
+/**< Enable show tm. */
+static uint32_t enable_shw_tm;
+/**< Enable show crypto. */
+static uint32_t enable_shw_crypto;
+/**< Enable show ring. */
+static uint32_t enable_shw_ring;
+static char *ring_name;
+/**< Enable show mempool. */
+static uint32_t enable_shw_mempool;
+static char *mempool_name;
 
 /**< Enable xstats by ids. */
 #define MAX_NB_XSTATS_IDS 1024
@@ -247,6 +259,24 @@ proc_info_parse_args(int argc, char **argv)
 			else if (!strncmp(long_option[option_index].name, "xstats-reset",
 					MAX_LONG_OPT_SZ))
 				reset_xstats = 1;
+			else if (!strncmp(long_option[option_index].name,
+					"show-port", MAX_LONG_OPT_SZ))
+				enable_shw_port = 1;
+			else if (!strncmp(long_option[option_index].name,
+					"show-tm", MAX_LONG_OPT_SZ))
+				enable_shw_tm = 1;
+			else if (!strncmp(long_option[option_index].name,
+					"show-crypto", MAX_LONG_OPT_SZ))
+				enable_shw_crypto = 1;
+			else if (!strncmp(long_option[option_index].name,
+					"show-ring", MAX_LONG_OPT_SZ)) {
+				enable_shw_ring = 1;
+				ring_name = optarg;
+			} else if (!strncmp(long_option[option_index].name,
+					"show-mempool", MAX_LONG_OPT_SZ)) {
+				enable_shw_mempool = 1;
+				mempool_name = optarg;
+			}
 			break;
 		case 1:
 			/* Print xstat single value given by name*/
-- 
2.17.1

[dpdk-dev] [PATCH v4 3/9] app/procinfo: add prototype for debug instances

[Vipin Varghese]

Add prototype function calls for the show functions.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---

V3:
 - update function names from debug to show - Vipin Varghese

V2:
 - removed if else ladder - Vipin Varghese
---
 app/proc-info/main.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index 76266d5cb..8b9cf629d 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -628,6 +628,36 @@ metrics_display(int port_id)
 	rte_free(names);
 }
 
+static void
+show_port(void)
+{
+	printf(" port\n");
+}
+
+static void
+show_tm(void)
+{
+	printf(" tm\n");
+}
+
+static void
+show_crypto(void)
+{
+	printf(" crypto\n");
+}
+
+static void
+show_ring(char *name)
+{
+	printf(" rings Name (%s)\n", name);
+}
+
+static void
+show_mempool(char *name)
+{
+	printf(" mempools Name (%s)\n", name);
+}
+
 int
 main(int argc, char **argv)
 {
@@ -708,6 +738,18 @@ main(int argc, char **argv)
 	if (enable_metrics)
 		metrics_display(RTE_METRICS_GLOBAL);
 
+	/* show information for PMD */
+	if (enable_shw_port)
+		show_port();
+	if (enable_shw_tm)
+		show_tm();
+	if (enable_shw_crypto)
+		show_crypto();
+	if (enable_shw_ring)
+		show_ring(ring_name);
+	if (enable_shw_mempool)
+		show_mempool(mempool_name);
+
 	ret = rte_eal_cleanup();
 	if (ret)
 		printf("Error from rte_eal_cleanup(), %d\n", ret);
-- 
2.17.1

[dpdk-dev] [PATCH v4 4/9] app/procinfo: add support for show port

[Vipin Varghese]

Function show_port is used for displaying the port PMD information under
under primary process. The information shows basic, per queue and security.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---

V3:
 - fix meson build - Reshma Pattan
 - change 100 to MAX_STRING_LEN - Reshma Pattan
 - memset to struct elements - Reshma Pattan
 - printf tab space - Reshma Pattan
 - remove 'drop packet information' - Vipin Varghese

V2:
 - redefine code format - Vipin Varghese
---
 app/proc-info/main.c      | 120 +++++++++++++++++++++++++++++++++++++-
 app/proc-info/meson.build |   2 +-
 2 files changed, 120 insertions(+), 2 deletions(-)

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index 8b9cf629d..48477097f 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -29,6 +29,9 @@
 #include <rte_branch_prediction.h>
 #include <rte_string_fns.h>
 #include <rte_metrics.h>
+#include <rte_cycles.h>
+#include <rte_security.h>
+#include <rte_cryptodev.h>
 
 /* Maximum long option length for option parsing. */
 #define MAX_LONG_OPT_SZ 64
@@ -81,6 +84,9 @@ static char *mempool_name;
 static uint32_t nb_xstats_ids;
 static uint64_t xstats_ids[MAX_NB_XSTATS_IDS];
 
+/* border variable to hold for show */
+char bdr_str[MAX_STRING_LEN];
+
 /**< display usage */
 static void
 proc_info_usage(const char *prgname)
@@ -631,7 +637,119 @@ metrics_display(int port_id)
 static void
 show_port(void)
 {
-	printf(" port\n");
+	uint16_t i = 0;
+	int ret = 0, j, k;
+
+	snprintf(bdr_str, MAX_STRING_LEN, " show - Port PMD %"PRIu64,
+			rte_get_tsc_hz());
+	STATS_BDR_STR(10, bdr_str);
+
+	RTE_ETH_FOREACH_DEV(i) {
+		uint16_t mtu = 0;
+		struct rte_eth_link link;
+		struct rte_eth_dev_info dev_info;
+		struct rte_eth_rxq_info queue_info;
+		struct rte_eth_stats stats;
+		struct rte_eth_rss_conf rss_conf;
+
+		memset(&link, 0, sizeof(link));
+		memset(&dev_info, 0, sizeof(dev_info));
+		memset(&queue_info, 0, sizeof(queue_info));
+		memset(&stats, 0, sizeof(stats));
+		memset(&rss_conf, 0, sizeof(rss_conf));
+
+		snprintf(bdr_str, 100, " Port (%u)", i);
+		STATS_BDR_STR(5, bdr_str);
+		printf("  - generic config\n");
+
+		printf("\t  -- Socket %d\n", rte_eth_dev_socket_id(i));
+		rte_eth_link_get(i, &link);
+		printf("\t  -- link speed %d duplex %d,"
+			" auto neg %d status %d\n",
+			link.link_speed,
+			link.link_duplex,
+			link.link_autoneg,
+			link.link_status);
+		printf("\t  -- promiscuous (%d)\n",
+			rte_eth_promiscuous_get(i));
+		ret = rte_eth_dev_get_mtu(i, &mtu);
+		if (ret == 0)
+			printf("\t  -- mtu (%d)\n", mtu);
+
+		printf("  - queue\n");
+
+		rte_eth_dev_info_get(i, &dev_info);
+
+		for (j = 0; j < dev_info.nb_rx_queues; j++) {
+			ret = rte_eth_rx_queue_info_get(i, j, &queue_info);
+			if (ret == 0) {
+				printf("\t  -- queue %d rx scatter %d"
+					" descriptors %d offloads 0x%"PRIx64
+					" mempool socket %d\n",
+					j,
+					queue_info.scattered_rx,
+					queue_info.nb_desc,
+					queue_info.conf.offloads,
+					queue_info.mp->socket_id);
+
+				ret = rte_eth_stats_get(i, &stats);
+				if (ret == 0) {
+					printf("\t  -- packet input %"PRIu64
+						" output %"PRIu64""
+						" error %"PRIu64"\n",
+						stats.q_ipackets[j],
+						stats.q_opackets[j],
+						stats.q_errors[j]);
+				}
+			}
+
+			ret = rte_eth_dev_rss_hash_conf_get(i, &rss_conf);
+			if ((ret) || (rss_conf.rss_key == NULL))
+				continue;
+
+			printf("\t  -- RSS len %u key (hex):",
+				rss_conf.rss_key_len);
+			for (k = 0; k < rss_conf.rss_key_len; k++)
+				printf(" %x", rss_conf.rss_key[k]);
+			printf("\t  -- hf 0x%"PRIx64"\n",
+				rss_conf.rss_hf);
+		}
+
+		ret = rte_eth_stats_get(i, &stats);
+		if (ret == 0) {
+			printf("\t  -- packet input %"PRIu64
+				" output %"PRIu64"\n",
+				stats.ipackets,
+				stats.opackets);
+			printf("\t  -- packet error input %"PRIu64
+				" output %"PRIu64"\n",
+				stats.ierrors,
+				stats.oerrors);
+			printf("\t  -- RX no mbuf %"PRIu64"\n",
+				stats.rx_nombuf);
+		}
+
+		printf("  - cyrpto context\n");
+		void *ptr_ctx = rte_eth_dev_get_sec_ctx(i);
+		printf("\t  -- security context - %p\n", ptr_ctx);
+
+		if (ptr_ctx) {
+			printf("\t  -- size %u\n",
+				rte_security_session_get_size(ptr_ctx));
+			const struct rte_security_capability *ptr_sec_cap =
+				rte_security_capabilities_get(ptr_ctx);
+			if (ptr_sec_cap) {
+				printf("\t  -- action (0x%x), protocol (0x%x),"
+					" offload flags (0x%x)\n",
+					ptr_sec_cap->action,
+					ptr_sec_cap->protocol,
+					ptr_sec_cap->ol_flags);
+				printf("\t  -- capabilities - oper type %x\n",
+					ptr_sec_cap->crypto_capabilities->op);
+			}
+		}
+	}
+	STATS_BDR_STR(50, "");
 }
 
 static void
diff --git a/app/proc-info/meson.build b/app/proc-info/meson.build
index a52b2ee4a..866b390d6 100644
--- a/app/proc-info/meson.build
+++ b/app/proc-info/meson.build
@@ -3,4 +3,4 @@
 
 sources = files('main.c')
 allow_experimental_apis = true
-deps += ['ethdev', 'metrics']
+deps += ['ethdev', 'metrics', 'security']
-- 
2.17.1

[dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm

[Vipin Varghese]

Function show_tm is used for displaying the tm PMD under the
primary process. This covers basic and per node|level details with
stats.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---

V3:
 - memset for struct elements - Vipin Varghese
 - code cleanup for TM - Vipin Varghese
 - fetch for leaf nodes if node exist - Jasvinder Singh
 - display MARCO to function - Reshma Pathan & Stephen Hemminger

V2:
 - MACRO for display node|level - cap - Vipin Varghese
---
 app/proc-info/main.c | 276 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 275 insertions(+), 1 deletion(-)

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index 48477097f..8ec2e9474 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -32,6 +32,7 @@
 #include <rte_cycles.h>
 #include <rte_security.h>
 #include <rte_cryptodev.h>
+#include <rte_tm.h>
 
 /* Maximum long option length for option parsing. */
 #define MAX_LONG_OPT_SZ 64
@@ -752,10 +753,283 @@ show_port(void)
 	STATS_BDR_STR(50, "");
 }
 
+static void
+display_nodecap_info(int is_leaf, struct rte_tm_node_capabilities *cap)
+{
+	if (cap == NULL)
+		return;
+
+	if (!is_leaf) {
+		printf("\t  -- nonleaf sched max:\n"
+			"\t\t  + children (%u)\n"
+			"\t\t  + sp priorities (%u)\n"
+			"\t\t  + wfq children per group (%u)\n"
+			"\t\t  + wfq groups (%u)\n"
+			"\t\t  + wfq weight (%u)\n",
+			cap->nonleaf.sched_n_children_max,
+			cap->nonleaf.sched_sp_n_priorities_max,
+			cap->nonleaf.sched_wfq_n_children_per_group_max,
+			cap->nonleaf.sched_wfq_n_groups_max,
+			cap->nonleaf.sched_wfq_weight_max);
+	} else {
+		printf("\t  -- leaf cman support:\n"
+			"\t\t  + wred pkt mode (%d)\n"
+			"\t\t  + wred byte mode (%d)\n"
+			"\t\t  + head drop (%d)\n"
+			"\t\t  + wred context private (%d)\n"
+			"\t\t  + wred context shared (%u)\n",
+			cap->leaf.cman_wred_packet_mode_supported,
+			cap->leaf.cman_wred_byte_mode_supported,
+			cap->leaf.cman_head_drop_supported,
+			cap->leaf.cman_wred_context_private_supported,
+			cap->leaf.cman_wred_context_shared_n_max);
+	}
+}
+
+static void
+display_levelcap_info(int is_leaf, struct rte_tm_level_capabilities *cap)
+{
+	if (cap == NULL)
+		return;
+
+	if (!is_leaf) {
+		printf("\t  -- shaper private: (%d) dual rate (%d)\n",
+			cap->nonleaf.shaper_private_supported,
+			cap->nonleaf.shaper_private_dual_rate_supported);
+		printf("\t  -- shaper share: (%u)\n",
+			cap->nonleaf.shaper_shared_n_max);
+		printf("\t  -- non leaf sched MAX:\n"
+			"\t\t  + children (%u)\n"
+			"\t\t  + sp (%u)\n"
+			"\t\t  + wfq children per group (%u)\n"
+			"\t\t  + wfq groups (%u)\n"
+			"\t\t  + wfq weight (%u)\n",
+			cap->nonleaf.sched_n_children_max,
+			cap->nonleaf.sched_sp_n_priorities_max,
+			cap->nonleaf.sched_wfq_n_children_per_group_max,
+			cap->nonleaf.sched_wfq_n_groups_max,
+			cap->nonleaf.sched_wfq_weight_max);
+	} else {
+		printf("\t  -- shaper private: (%d) dual rate (%d)\n",
+			cap->leaf.shaper_private_supported,
+			cap->leaf.shaper_private_dual_rate_supported);
+		printf("\t  -- shaper share: (%u)\n",
+			cap->leaf.shaper_shared_n_max);
+		printf("  -- leaf cman support:\n"
+			"\t\t  + wred pkt mode (%d)\n"
+			"\t\t  + wred byte mode (%d)\n"
+			"\t\t  + head drop (%d)\n"
+			"\t\t  + wred context private (%d)\n"
+			"\t\t  + wred context shared (%u)\n",
+			cap->leaf.cman_wred_packet_mode_supported,
+			cap->leaf.cman_wred_byte_mode_supported,
+			cap->leaf.cman_head_drop_supported,
+			cap->leaf.cman_wred_context_private_supported,
+			cap->leaf.cman_wred_context_shared_n_max);
+	}
+}
+
 static void
 show_tm(void)
 {
-	printf(" tm\n");
+	int ret = 0, check_for_leaf = 0, is_leaf = 0;
+	unsigned int j, k;
+	uint16_t i = 0;
+
+	snprintf(bdr_str, MAX_STRING_LEN, " show - TM PMD %"PRIu64,
+			rte_get_tsc_hz());
+	STATS_BDR_STR(10, bdr_str);
+
+	RTE_ETH_FOREACH_DEV(i) {
+		struct rte_eth_dev_info dev_info;
+		struct rte_tm_capabilities cap;
+		struct rte_tm_error error;
+		struct rte_tm_node_capabilities capnode;
+		struct rte_tm_level_capabilities caplevel;
+		uint32_t n_leaf_nodes = 0;
+
+		memset(&dev_info, 0, sizeof(dev_info));
+		memset(&cap, 0, sizeof(cap));
+		memset(&error, 0, sizeof(error));
+
+		rte_eth_dev_info_get(i, &dev_info);
+		printf("  - Generic for port (%u)\n"
+			"\t  -- driver name %s\n"
+			"\t  -- max vf (%u)\n"
+			"\t  -- max tx queues (%u)\n"
+			"\t  -- number of tx queues (%u)\n",
+			i,
+			dev_info.driver_name,
+			dev_info.max_vfs,
+			dev_info.max_tx_queues,
+			dev_info.nb_tx_queues);
+
+		ret = rte_tm_capabilities_get(i, &cap, &error);
+		if (ret)
+			continue;
+
+		printf("  - MAX: nodes (%u) levels (%u) children (%u)\n",
+			cap.n_nodes_max,
+			cap.n_levels_max,
+			cap.sched_n_children_max);
+
+		printf("  - identical nodes: non leaf (%d) leaf (%d)\n",
+			cap.non_leaf_nodes_identical,
+			cap.leaf_nodes_identical);
+
+		printf("  - Shaper MAX:\n"
+			"\t  -- total (%u)\n"
+			"\t  -- private (%u) private dual (%d)\n"
+			"\t  -- shared (%u) shared dual (%u)\n",
+			cap.shaper_n_max,
+			cap.shaper_private_n_max,
+			cap.shaper_private_dual_rate_n_max,
+			cap.shaper_shared_n_max,
+			cap.shaper_shared_dual_rate_n_max);
+
+		printf("  - mark support:\n");
+		printf("\t  -- vlan dei: GREEN (%d) YELLOW (%d) RED (%d)\n",
+			cap.mark_vlan_dei_supported[RTE_TM_GREEN],
+			cap.mark_vlan_dei_supported[RTE_TM_YELLOW],
+			cap.mark_vlan_dei_supported[RTE_TM_RED]);
+		printf("\t  -- ip ecn tcp: GREEN (%d) YELLOW (%d) RED (%d)\n",
+			cap.mark_ip_ecn_tcp_supported[RTE_TM_GREEN],
+			cap.mark_ip_ecn_tcp_supported[RTE_TM_YELLOW],
+			cap.mark_ip_ecn_tcp_supported[RTE_TM_RED]);
+		printf("\t  -- ip ecn sctp: GREEN (%d) YELLOW (%d) RED (%d)\n",
+			cap.mark_ip_ecn_sctp_supported[RTE_TM_GREEN],
+			cap.mark_ip_ecn_sctp_supported[RTE_TM_YELLOW],
+			cap.mark_ip_ecn_sctp_supported[RTE_TM_RED]);
+		printf("\t  -- ip dscp: GREEN (%d) YELLOW (%d) RED (%d)\n",
+			cap.mark_ip_dscp_supported[RTE_TM_GREEN],
+			cap.mark_ip_dscp_supported[RTE_TM_YELLOW],
+			cap.mark_ip_dscp_supported[RTE_TM_RED]);
+
+		printf("  - mask stats (0x%"PRIx64")"
+			" dynamic update (0x%"PRIx64")\n",
+			cap.stats_mask,
+			cap.dynamic_update_mask);
+
+		printf("  - sched MAX:\n"
+			"\t  -- total (%u)\n"
+			"\t  -- sp levels (%u)\n"
+			"\t  -- wfq children per group (%u)\n"
+			"\t  -- wfq groups (%u)\n"
+			"\t  -- wfq weight (%u)\n",
+			cap.sched_sp_n_priorities_max,
+			cap.sched_sp_n_priorities_max,
+			cap.sched_wfq_n_children_per_group_max,
+			cap.sched_wfq_n_groups_max,
+			cap.sched_wfq_weight_max);
+
+		printf("  - CMAN support:\n"
+			"\t  -- WRED mode: pkt (%d) byte (%d)\n"
+			"\t  -- head drop (%d)\n",
+			cap.cman_wred_packet_mode_supported,
+			cap.cman_wred_byte_mode_supported,
+			cap.cman_head_drop_supported);
+		printf("\t  -- MAX WRED CONTEXT:"
+			" total (%u) private (%u) shared (%u)\n",
+			cap.cman_wred_context_n_max,
+			cap.cman_wred_context_private_n_max,
+			cap.cman_wred_context_shared_n_max);
+
+		for (j = 0; j < cap.n_nodes_max; j++) {
+			memset(&capnode, 0, sizeof(capnode));
+			ret = rte_tm_node_capabilities_get(i, j,
+				&capnode, &error);
+			if (ret)
+				continue;
+
+			check_for_leaf = 1;
+
+			printf("  NODE %u\n", j);
+			printf("\t  - shaper private: (%d) dual rate (%d)\n",
+				capnode.shaper_private_supported,
+				capnode.shaper_private_dual_rate_supported);
+			printf("\t  - shaper shared max: (%u)\n",
+				capnode.shaper_shared_n_max);
+			printf("\t  - stats mask %"PRIx64"\n",
+				capnode.stats_mask);
+
+			ret = rte_tm_node_type_get(i, j, &is_leaf, &error);
+			if (ret)
+				continue;
+
+			display_nodecap_info(is_leaf, &capnode);
+		}
+
+		for (j = 0; j < cap.n_levels_max; j++) {
+			memset(&caplevel, 0, sizeof(caplevel));
+			ret = rte_tm_level_capabilities_get(i, j,
+				&caplevel, &error);
+			if (ret)
+				continue;
+
+			printf("  - Level %u\n", j);
+			printf("\t  -- node MAX: %u non leaf %u leaf %u\n",
+				caplevel.n_nodes_max,
+				caplevel.n_nodes_nonleaf_max,
+				caplevel.n_nodes_leaf_max);
+			printf("\t  -- indetical: non leaf %u leaf %u\n",
+				caplevel.non_leaf_nodes_identical,
+				caplevel.leaf_nodes_identical);
+
+			for (k = 0; k < caplevel.n_nodes_max; k++) {
+				ret = rte_tm_node_type_get(i, k,
+					&is_leaf, &error);
+				if (ret)
+					continue;
+
+				display_levelcap_info(is_leaf, &caplevel);
+			}
+		}
+
+		if (check_for_leaf) {
+			ret = rte_tm_get_number_of_leaf_nodes(i,
+					&n_leaf_nodes, &error);
+			if (ret == 0)
+				printf("  - leaf nodes (%u)\n", n_leaf_nodes);
+		}
+
+		for (j = 0; j < n_leaf_nodes; j++) {
+			struct rte_tm_node_stats stats;
+			memset(&stats, 0, sizeof(stats));
+
+			ret = rte_tm_node_stats_read(i, j,
+				&stats, &cap.stats_mask, 0, &error);
+			if (ret)
+				continue;
+
+			printf("  - STATS for node (%u)\n", j);
+			printf("  -- pkts (%"PRIu64") bytes (%"PRIu64")\n",
+				stats.n_pkts, stats.n_bytes);
+
+			ret = rte_tm_node_type_get(i, j, &is_leaf, &error);
+			if ((ret) | (!is_leaf))
+				continue;
+
+			printf("  -- leaf queued:"
+				" pkts (%"PRIu64") bytes (%"PRIu64")\n",
+				stats.leaf.n_pkts_queued,
+				stats.leaf.n_bytes_queued);
+			printf("  - dropped:\n"
+				"\t  -- GREEN:"
+				" pkts (%"PRIu64") bytes (%"PRIu64")\n"
+				"\t  -- YELLOW:"
+				" pkts (%"PRIu64") bytes (%"PRIu64")\n"
+				"\t  -- RED:"
+				" pkts (%"PRIu64") bytes (%"PRIu64")\n",
+				stats.leaf.n_pkts_dropped[RTE_TM_GREEN],
+				stats.leaf.n_bytes_dropped[RTE_TM_GREEN],
+				stats.leaf.n_pkts_dropped[RTE_TM_YELLOW],
+				stats.leaf.n_bytes_dropped[RTE_TM_YELLOW],
+				stats.leaf.n_pkts_dropped[RTE_TM_RED],
+				stats.leaf.n_bytes_dropped[RTE_TM_RED]);
+		}
+	}
+
+	STATS_BDR_STR(50, "");
 }
 
 static void
-- 
2.17.1

[dpdk-dev] [PATCH v4 6/9] app/procinfo: add support for show crypto

[Vipin Varghese]

Function show_crypto is used for displaying the crypto PMD under
the primary process.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---

v4:
 - add space to compare - Vipin Varghese

V3:
 - replace MACRO to function - Reshma Pathan & Stephen Hemminger
 - add memset for struct elements - Reshma Pathan
 - change display formating of flags - Vipin Varghese
 - use MACRO for string - Vipin Varghese
---
 app/proc-info/main.c | 80 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 79 insertions(+), 1 deletion(-)

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index 8ec2e9474..7213b43fe 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -1032,10 +1032,88 @@ show_tm(void)
 	STATS_BDR_STR(50, "");
 }
 
+static void
+display_crypto_feature_info(uint64_t x)
+{
+	if (x == 0)
+		return;
+
+	printf("\t  -- feature flags\n");
+	printf("\t\t  + symmetric (%c), asymmetric (%c)\n"
+		"\t\t  + symmetric operation chaining (%c)\n",
+		(x & RTE_CRYPTODEV_FF_SYMMETRIC_CRYPTO) ? 'y' : 'n',
+		(x & RTE_CRYPTODEV_FF_ASYMMETRIC_CRYPTO) ? 'y' : 'n',
+		(x & RTE_CRYPTODEV_FF_SYM_OPERATION_CHAINING) ? 'y' : 'n');
+	printf("\t\t  + CPU: SSE (%c), AVX (%c), AVX2 (%c), AVX512 (%c)\n",
+		(x & RTE_CRYPTODEV_FF_CPU_SSE) ? 'y' : 'n',
+		(x & RTE_CRYPTODEV_FF_CPU_AVX) ? 'y' : 'n',
+		(x & RTE_CRYPTODEV_FF_CPU_AVX2) ? 'y' : 'n',
+		(x & RTE_CRYPTODEV_FF_CPU_AVX512) ? 'y' : 'n');
+	printf("\t\t  + AESNI: CPU (%c), HW (%c)\n",
+		(x & RTE_CRYPTODEV_FF_CPU_AESNI) ? 'y' : 'n',
+		(x & RTE_CRYPTODEV_FF_HW_ACCELERATED) ? 'y' : 'n');
+	printf("\t\t  + INLINE (%c)\n",
+		(x & RTE_CRYPTODEV_FF_SECURITY) ? 'y' : 'n');
+	printf("\t\t  + ARM: NEON (%c), CE (%c)\n",
+		(x & RTE_CRYPTODEV_FF_CPU_NEON) ? 'y' : 'n',
+		(x & RTE_CRYPTODEV_FF_CPU_ARM_CE) ? 'y' : 'n');
+	printf("\t  -- buffer offload\n");
+	printf("\t\t  + IN_PLACE_SGL (%c)\n",
+		(x & RTE_CRYPTODEV_FF_IN_PLACE_SGL) ? 'y' : 'n');
+	printf("\t\t  + OOP_SGL_IN_SGL_OUT (%c)\n",
+		(x & RTE_CRYPTODEV_FF_OOP_SGL_IN_SGL_OUT) ? 'y' : 'n');
+	printf("\t\t  + OOP_SGL_IN_LB_OUT (%c)\n",
+		(x & RTE_CRYPTODEV_FF_OOP_SGL_IN_LB_OUT) ? 'y' : 'n');
+	printf("\t\t  + OOP_LB_IN_SGL_OUT (%c)\n",
+		(x & RTE_CRYPTODEV_FF_OOP_LB_IN_SGL_OUT) ? 'y' : 'n');
+	printf("\t\t  + OOP_LB_IN_LB_OUT (%c)\n",
+		(x & RTE_CRYPTODEV_FF_OOP_LB_IN_LB_OUT) ? 'y' : 'n');
+}
+
 static void
 show_crypto(void)
 {
-	printf(" crypto\n");
+	uint8_t crypto_dev_count = rte_cryptodev_count(), i;
+
+	snprintf(bdr_str, MAX_STRING_LEN, " show - CRYPTO PMD %"PRIu64,
+		rte_get_tsc_hz());
+	STATS_BDR_STR(10, bdr_str);
+
+	for (i = 0; i < crypto_dev_count; i++) {
+		struct rte_cryptodev_info dev_info;
+		struct rte_cryptodev_stats stats;
+
+		memset(&dev_info, 0, sizeof(dev_info));
+		rte_cryptodev_info_get(i, &dev_info);
+
+		printf("  - device (%u)\n", i);
+		printf("\t  -- name (%s)\n"
+			"\t  -- driver (%s)\n"
+			"\t  -- id (%u) on socket (%d)\n"
+			"\t  -- queue pairs (%d)\n",
+			rte_cryptodev_name_get(i),
+			dev_info.driver_name,
+			dev_info.driver_id,
+			dev_info.device->numa_node,
+			rte_cryptodev_queue_pair_count(i));
+
+		display_crypto_feature_info(dev_info.feature_flags);
+
+		printf("\t  -- stats\n");
+		memset(&stats, 0, sizeof(0));
+		if (rte_cryptodev_stats_get(i, &stats) == 0) {
+			printf("\t\t  + enqueue count (%"PRIu64")"
+				" error (%"PRIu64")\n",
+				stats.enqueued_count,
+				stats.enqueue_err_count);
+			printf("\t\t  + dequeue count (%"PRIu64")"
+				" error (%"PRIu64")\n",
+				stats.dequeued_count,
+				stats.dequeue_err_count);
+		}
+	}
+
+	STATS_BDR_STR(50, "");
 }
 
 static void
-- 
2.17.1

[dpdk-dev] [PATCH v4 7/9] app/procinfo: add support for debug ring

[Vipin Varghese]

Function show_ring is used for displaying the RING of the
primary process.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---

V3:
 - replace space to tab in printf - Reshma Pathan
 - change ring display information - Vipin Varghese
---
 app/proc-info/main.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index 7213b43fe..b97668d7f 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -1119,7 +1119,39 @@ show_crypto(void)
 static void
 show_ring(char *name)
 {
-	printf(" rings Name (%s)\n", name);
+	snprintf(bdr_str, MAX_STRING_LEN, " show - RING %"PRIu64,
+		rte_get_tsc_hz());
+	STATS_BDR_STR(10, bdr_str);
+
+	if (name != NULL) {
+		struct rte_ring *ptr = rte_ring_lookup(name);
+		if (ptr != NULL) {
+			printf("  - Name (%s) on socket (%d)\n"
+				"  - flags:\n"
+				"\t  -- Single Producer Enqueue (%u)\n"
+				"\t  -- Single Consmer Dequeue (%u)\n",
+				ptr->name,
+				ptr->memzone->socket_id,
+				ptr->flags & RING_F_SP_ENQ,
+				ptr->flags & RING_F_SC_DEQ);
+			printf("  - size (%u) mask (0x%x) capacity (%u)\n",
+				ptr->size,
+				ptr->mask,
+				ptr->capacity);
+			printf("  - count (%u) free count (%u)\n",
+				rte_ring_count(ptr),
+				rte_ring_free_count(ptr));
+			printf("  - full (%d) empty (%d)\n",
+				rte_ring_full(ptr),
+				rte_ring_empty(ptr));
+
+			STATS_BDR_STR(50, "");
+			return;
+		}
+	}
+
+	rte_ring_list_dump(stdout);
+	STATS_BDR_STR(50, "");
 }
 
 static void
-- 
2.17.1

[dpdk-dev] [PATCH v4 8/9] app/procinfo: add support for show mempool

[Vipin Varghese]

Function show_mempool is used for displaying the MEMPOOL of the
primary process. For valid mempool elements are iterated for max
of 256 bytes.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---

v4:
 - add spacing for flag compare - Vipin Varghese

V3:
 - add avail and in use - Vipin Varghese
 - add flag split - Vipin Varghese
---
 app/proc-info/main.c | 67 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 66 insertions(+), 1 deletion(-)

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index b97668d7f..bd28556ed 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -33,6 +33,7 @@
 #include <rte_security.h>
 #include <rte_cryptodev.h>
 #include <rte_tm.h>
+#include <rte_hexdump.h>
 
 /* Maximum long option length for option parsing. */
 #define MAX_LONG_OPT_SZ 64
@@ -1154,10 +1155,74 @@ show_ring(char *name)
 	STATS_BDR_STR(50, "");
 }
 
+static void
+mempool_itr_obj(struct rte_mempool *mp,
+			void *opaque, void *obj,
+			unsigned int obj_idx)
+{
+	printf("  - obj_idx %u opaque %p obj %p\n",
+		obj_idx, opaque, obj);
+
+	if (obj)
+		rte_hexdump(stdout, " Obj Content",
+			obj, (mp->elt_size > 256)?256:mp->elt_size);
+}
+
 static void
 show_mempool(char *name)
 {
-	printf(" mempools Name (%s)\n", name);
+	uint64_t flags = 0;
+
+	snprintf(bdr_str, MAX_STRING_LEN, " show - MEMPOOL %"PRIu64,
+		rte_get_tsc_hz());
+	STATS_BDR_STR(10, bdr_str);
+
+	if (name != NULL) {
+		struct rte_mempool *ptr = rte_mempool_lookup(name);
+		if (ptr != NULL) {
+			flags = ptr->flags;
+			printf("  - Name: %s on socket %d\n"
+				"  - flags:\n"
+				"\t  -- No spread (%c)\n"
+				"\t  -- No cache align (%c)\n"
+				"\t  -- SP put (%c), SC get (%c)\n"
+				"\t  -- Pool created (%c)\n"
+				"\t  -- No IOVA config (%c)\n",
+				ptr->name,
+				ptr->socket_id,
+				(flags & MEMPOOL_F_NO_SPREAD) ? 'y' : 'n',
+				(flags & MEMPOOL_F_NO_CACHE_ALIGN) ? 'y' : 'n',
+				(flags & MEMPOOL_F_SP_PUT) ? 'y' : 'n',
+				(flags & MEMPOOL_F_SC_GET) ? 'y' : 'n',
+				(flags & MEMPOOL_F_POOL_CREATED) ? 'y' : 'n',
+				(flags & MEMPOOL_F_NO_IOVA_CONTIG) ? 'y' : 'n');
+			printf("  - Size %u Cache %u element %u\n"
+				"  - header %u trailer %u\n"
+				"  - private data size %u\n",
+				ptr->size,
+				ptr->cache_size,
+				ptr->elt_size,
+				ptr->header_size,
+				ptr->trailer_size,
+				ptr->private_data_size);
+			printf("  - memezone - socket %d\n",
+				ptr->mz->socket_id);
+			printf("  - Count: avail (%u), in use (%u)\n",
+				rte_mempool_avail_count(ptr),
+				rte_mempool_in_use_count(ptr));
+
+			/* iterate each object */
+			int ret = rte_mempool_obj_iter(ptr,
+				mempool_itr_obj, NULL);
+			printf("  - iterated %u objects\n", ret);
+
+			STATS_BDR_STR(50, "");
+			return;
+		}
+	}
+
+	rte_mempool_list_dump(stdout);
+	STATS_BDR_STR(50, "");
 }
 
 int
-- 
2.17.1

[dpdk-dev] [PATCH v4 9/9] doc/procinfo: add information for debug options

[Vipin Varghese]

Document update for debug options and information for PMD instances
like port, traffic manager, crypto, mempool and ring instances.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---

V3:
 - update document from dbg to show - Vipin Varghese

V2:
 - update word style for content - Vipin Varghese
---
 doc/guides/tools/proc_info.rst | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/doc/guides/tools/proc_info.rst b/doc/guides/tools/proc_info.rst
index d5b5ed6a6..5c2ce762b 100644
--- a/doc/guides/tools/proc_info.rst
+++ b/doc/guides/tools/proc_info.rst
@@ -6,9 +6,9 @@ dpdk-procinfo Application
 
 The dpdk-procinfo application is a Data Plane Development Kit (DPDK) application
 that runs as a DPDK secondary process and is capable of retrieving port
-statistics, resetting port statistics and printing DPDK memory information.
-This application extends the original functionality that was supported by
-dump_cfg.
+statistics, resetting port statistics and printing DPDK memory information and
+debug information for port|tm|crypto|ring|mempool. This application extends the
+original functionality that was supported by dump_cfg.
 
 Running the Application
 -----------------------
@@ -17,7 +17,8 @@ The application has a number of command line options:
 .. code-block:: console
 
    ./$(RTE_TARGET)/app/dpdk-procinfo -- -m | [-p PORTMASK] [--stats | --xstats |
-   --stats-reset | --xstats-reset]
+   --stats-reset | --xstats-reset | --show-port | --show-tm | --show-crypto |
+   --show-mempool[=name] | --show-ring[=name]]
 
 Parameters
 ~~~~~~~~~~
@@ -41,6 +42,28 @@ If no port mask is specified xstats are reset for all DPDK ports.
 
 **-m**: Print DPDK memory information.
 
+**--show-port**
+The show-port parameter displays port level various configuration and
+mbuf pool information associated to RX queues.
+
+**--show-tm**
+The show-tm parameter displays per port traffic manager settings and
+current configuration. It also display statistics too.
+
+**--show-crypto**
+The show-crypto parameter displays available cryptodev configurations,
+settings and stats per node.
+
+**--show-mempool[=name]**
+The show-mempool parameter display current allocation of all mempool
+with debug information. Specifying the name allows display details for
+specific mempool. For invalid|no mempool name, whole list is dump.
+
+**--show-ring[=name]**
+The show-ring pararmeter display current allocation of all ring with
+debug information. Specifying the name allows to display details for specific
+ring. For invalid|no ring name, whole list is dump.
+
 Limitations
 -----------
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH v4 4/9] app/procinfo: add support for show port

[Pattan, Reshma]

> -----Original Message-----
> From: Varghese, Vipin
> Sent: Tuesday, November 6, 2018 12:49 PM
> To: dev@dpdk.org; thomas@monjalon.net; Pattan, Reshma
> <reshma.pattan@intel.com>; stephen@networkplumber.org; Mcnamara, John
> <john.mcnamara@intel.com>
> Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>; Varghese,
> Vipin <vipin.varghese@intel.com>
> Subject: [PATCH v4 4/9] app/procinfo: add support for show port


> +		snprintf(bdr_str, 100, " Port (%u)", i);

%s/100/MAX_STRING_LEN ?

> +			ret = rte_eth_dev_rss_hash_conf_get(i, &rss_conf);
> +			if ((ret) || (rss_conf.rss_key == NULL))
> +				continue;
> +
> +			printf("\t  -- RSS len %u key (hex):",
> +				rss_conf.rss_key_len);
> +			for (k = 0; k < rss_conf.rss_key_len; k++)
> +				printf(" %x", rss_conf.rss_key[k]);
> +			printf("\t  -- hf 0x%"PRIx64"\n",
> +				rss_conf.rss_hf);
> +		}
> +

Should this be out of queues for loop? 

Thanks,
Reshma

Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm

[Pattan, Reshma]

> -----Original Message-----
> From: Varghese, Vipin
> Sent: Tuesday, November 6, 2018 12:49 PM
> To: dev@dpdk.org; thomas@monjalon.net; Pattan, Reshma
> <reshma.pattan@intel.com>; stephen@networkplumber.org; Mcnamara, John
> <john.mcnamara@intel.com>
> Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>; Varghese,
> Vipin <vipin.varghese@intel.com>
> Subject: [PATCH v4 5/9] app/procinfo: add support for show tm


> +			if ((ret) | (!is_leaf))
> +

Is the operator here should be || ?

Re: [dpdk-dev] [PATCH v4 8/9] app/procinfo: add support for show mempool

[Pattan, Reshma]

> -----Original Message-----
> From: Varghese, Vipin
> Sent: Tuesday, November 6, 2018 12:49 PM
> To: dev@dpdk.org; thomas@monjalon.net; Pattan, Reshma
> <reshma.pattan@intel.com>; stephen@networkplumber.org; Mcnamara, John
> <john.mcnamara@intel.com>
> Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>; Varghese,
> Vipin <vipin.varghese@intel.com>
> +			/* iterate each object */
> +			int ret = rte_mempool_obj_iter(ptr,
> +				mempool_itr_obj, NULL);

rte_mempool_obj_iter returns uint32_t,  so good to use  "ret" type to be uint32_t. So it will be in synch with below printf too.

> +			printf("  - iterated %u objects\n", ret);

Thanks,
Reshma

Re: [dpdk-dev] [PATCH v4 4/9] app/procinfo: add support for show port

[Stephen Hemminger]

On Tue,  6 Nov 2018 18:19:07 +0530
Vipin Varghese <vipin.varghese@intel.com> wrote:

Minor observations which are things that checkpatch etc won't see
but make the code easier to read/maintain.
 
> +/* border variable to hold for show */
> +char bdr_str[MAX_STRING_LEN];

Does this have to be global, could it just be static?

> +		memset(&link, 0, sizeof(link));
> +		memset(&dev_info, 0, sizeof(dev_info));
> +		memset(&queue_info, 0, sizeof(queue_info));
> +		memset(&stats, 0, sizeof(stats));
> +		memset(&rss_conf, 0, sizeof(rss_conf));

These memset's should be unnecessary. For example, dev_info
is always cleared already inside rte_eth_dev_info_get().

> +			if ((ret) || (rss_conf.rss_key == NULL))
> +				continue;

Unnecessary parenthesis hurt readability in this if statement.

Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm

[Varghese, Vipin]

Hi Reshma,

<snipped>

> 
> > +			if ((ret) | (!is_leaf))
> > +
> 
> Is the operator here should be || ?
> 
> 

Check is done for 'if either ret is not 0 or if it ret is 0 but not leaf' we skip leaf details print. If 'ret is 0 and is leaf' we skip continue to print leaf details.

Re: [dpdk-dev] [PATCH v4 4/9] app/procinfo: add support for show port

[Varghese, Vipin]

> > +		snprintf(bdr_str, 100, " Port (%u)", i);
> 
> %s/100/MAX_STRING_LEN ?	

Done for v5

> 
> > +			ret = rte_eth_dev_rss_hash_conf_get(i, &rss_conf);
> > +			if ((ret) || (rss_conf.rss_key == NULL))
> > +				continue;
> > +
> > +			printf("\t  -- RSS len %u key (hex):",
> > +				rss_conf.rss_key_len);
> > +			for (k = 0; k < rss_conf.rss_key_len; k++)
> > +				printf(" %x", rss_conf.rss_key[k]);
> > +			printf("\t  -- hf 0x%"PRIx64"\n",
> > +				rss_conf.rss_hf);
> > +		}
> > +
> 
> Should this be out of queues for loop?
> 

Done for v5

Re: [dpdk-dev] [PATCH v4 4/9] app/procinfo: add support for show port

[Varghese, Vipin]

> 
> Minor observations which are things that checkpatch etc won't see but make
> the code easier to read/maintain.
> 
> > +/* border variable to hold for show */ char bdr_str[MAX_STRING_LEN];

Done for v5

> 
> Does this have to be global, could it just be static?
> 
> > +		memset(&link, 0, sizeof(link));
> > +		memset(&dev_info, 0, sizeof(dev_info));
> > +		memset(&queue_info, 0, sizeof(queue_info));
> > +		memset(&stats, 0, sizeof(stats));
> > +		memset(&rss_conf, 0, sizeof(rss_conf));
> 
> These memset's should be unnecessary. For example, dev_info is always
> cleared already inside rte_eth_dev_info_get().

Done for v5 (link, dev_info, queue_info, stats)

> 
> > +			if ((ret) || (rss_conf.rss_key == NULL))
> > +				continue;
> 
> Unnecessary parenthesis hurt readability in this if statement.

done

Re: [dpdk-dev] [PATCH v4 8/9] app/procinfo: add support for show mempool

[Varghese, Vipin]

> > +			/* iterate each object */
> > +			int ret = rte_mempool_obj_iter(ptr,
> > +				mempool_itr_obj, NULL);
> 
> rte_mempool_obj_iter returns uint32_t,  so good to use  "ret" type to be
> uint32_t. So it will be in synch with below printf too.
> 
> > +			printf("  - iterated %u objects\n", ret);

Done for v5

Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm

[Pattan, Reshma]

> -----Original Message-----
> From: Varghese, Vipin
> Sent: Thursday, November 22, 2018 1:28 PM
> To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> <john.mcnamara@intel.com>
> Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> 
> Hi Reshma,
> 
> <snipped>
> 
> >
> > > +			if ((ret) | (!is_leaf))
> > > +
> >
> > Is the operator here should be || ?
> >
> >
> 
> Check is done for 'if either ret is not 0 or if it ret is 0 but not leaf' we skip leaf
> details print. If 'ret is 0 and is leaf' we skip continue to print leaf details.

IMO, using logical operator over bitwise operator is good here in if statement  . Like below.?

If (ret || (is_leaf == 0 )) 

Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm

[Varghese, Vipin]

> -----Original Message-----
> From: Pattan, Reshma
> Sent: Friday, November 23, 2018 5:21 PM
> To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org;
> thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> <john.mcnamara@intel.com>
> Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> 
> 
> 
> > -----Original Message-----
> > From: Varghese, Vipin
> > Sent: Thursday, November 22, 2018 1:28 PM
> > To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> > thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> > <john.mcnamara@intel.com>
> > Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> > <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> >
> > Hi Reshma,
> >
> > <snipped>
> >
> > >
> > > > +			if ((ret) | (!is_leaf))
> > > > +
> > >
> > > Is the operator here should be || ?
> > >
> > >
> >
> > Check is done for 'if either ret is not 0 or if it ret is 0 but not
> > leaf' we skip leaf details print. If 'ret is 0 and is leaf' we skip continue to print
> leaf details.
> 
> IMO, using logical operator over bitwise operator is good here in if statement  .
> Like below.?
> 
> If (ret || (is_leaf == 0 ))

Thanks for the information, if the logic is correct do I need to change for v6
	

Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm

[Pattan, Reshma]

> -----Original Message-----
> From: Varghese, Vipin
> Sent: Friday, November 23, 2018 1:29 PM
> To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> <john.mcnamara@intel.com>
> Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> 
> 
> > -----Original Message-----
> > From: Pattan, Reshma
> > Sent: Friday, November 23, 2018 5:21 PM
> > To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org;
> > thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> > <john.mcnamara@intel.com>
> > Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> > <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> >
> >
> >
> > > -----Original Message-----
> > > From: Varghese, Vipin
> > > Sent: Thursday, November 22, 2018 1:28 PM
> > > To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> > > thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> > > <john.mcnamara@intel.com>
> > > Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> > > <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > > Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> > >
> > > Hi Reshma,
> > >
> > > <snipped>
> > >
> > > >
> > > > > +			if ((ret) | (!is_leaf))
> > > > > +
> > > >
> > > > Is the operator here should be || ?
> > > >
> > > >
> > >
> > > Check is done for 'if either ret is not 0 or if it ret is 0 but not
> > > leaf' we skip leaf details print. If 'ret is 0 and is leaf' we skip
> > > continue to print
> > leaf details.
> >
> > IMO, using logical operator over bitwise operator is good here in if statement
> .
> > Like below.?
> >
> > If (ret || (is_leaf == 0 ))
> 
> Thanks for the information, if the logic is correct do I need to change for v6
> 

OK in v6, but you can wait to hear more comments from others if any before sending v6 .

Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm

[Varghese, Vipin]

> -----Original Message-----
> From: Pattan, Reshma
> Sent: Friday, November 23, 2018 7:04 PM
> To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org;
> thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> <john.mcnamara@intel.com>
> Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> 
> 
> 
> > -----Original Message-----
> > From: Varghese, Vipin
> > Sent: Friday, November 23, 2018 1:29 PM
> > To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> > thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> > <john.mcnamara@intel.com>
> > Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> > <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> >
> >
> > > -----Original Message-----
> > > From: Pattan, Reshma
> > > Sent: Friday, November 23, 2018 5:21 PM
> > > To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org;
> > > thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> > > <john.mcnamara@intel.com>
> > > Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> > > <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > > Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Varghese, Vipin
> > > > Sent: Thursday, November 22, 2018 1:28 PM
> > > > To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> > > > thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> > > > <john.mcnamara@intel.com>
> > > > Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> > > > <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > > > Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> > > >
> > > > Hi Reshma,
> > > >
> > > > <snipped>
> > > >
> > > > >
> > > > > > +			if ((ret) | (!is_leaf))
> > > > > > +
> > > > >
> > > > > Is the operator here should be || ?
> > > > >
> > > > >
> > > >
> > > > Check is done for 'if either ret is not 0 or if it ret is 0 but
> > > > not leaf' we skip leaf details print. If 'ret is 0 and is leaf' we
> > > > skip continue to print
> > > leaf details.
> > >
> > > IMO, using logical operator over bitwise operator is good here in if
> > > statement
> > .
> > > Like below.?
> > >
> > > If (ret || (is_leaf == 0 ))
> >
> > Thanks for the information, if the logic is correct do I need to
> > change for v6
> >
> 
> OK in v6, but you can wait to hear more comments from others if any before
> sending v6 .

Ok thanks Reshma, but can you tell me how the earlier logic fails and runs slow compared to logical or?

Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm

[Pattan, Reshma]

> -----Original Message-----
> From: Varghese, Vipin
> Sent: Friday, November 23, 2018 1:56 PM
> To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> <john.mcnamara@intel.com>
> Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> 
> 
> 
> > -----Original Message-----
> > From: Pattan, Reshma
> > Sent: Friday, November 23, 2018 7:04 PM
> > To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org;
> > thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> > <john.mcnamara@intel.com>
> > Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> > <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> >
> >
> >
> > > -----Original Message-----
> > > From: Varghese, Vipin
> > > Sent: Friday, November 23, 2018 1:29 PM
> > > To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> > > thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> > > <john.mcnamara@intel.com>
> > > Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> > > <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > > Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> > >
> > >
> > > > -----Original Message-----
> > > > From: Pattan, Reshma
> > > > Sent: Friday, November 23, 2018 5:21 PM
> > > > To: Varghese, Vipin <vipin.varghese@intel.com>; dev@dpdk.org;
> > > > thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> > > > <john.mcnamara@intel.com>
> > > > Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> > > > <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > > > Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show tm
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Varghese, Vipin
> > > > > Sent: Thursday, November 22, 2018 1:28 PM
> > > > > To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org;
> > > > > thomas@monjalon.net; stephen@networkplumber.org; Mcnamara, John
> > > > > <john.mcnamara@intel.com>
> > > > > Cc: Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J
> > > > > <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > > > > Subject: RE: [PATCH v4 5/9] app/procinfo: add support for show
> > > > > tm
> > > > >
> > > > > Hi Reshma,
> > > > >
> > > > > <snipped>
> > > > >
> > > > > >
> > > > > > > +			if ((ret) | (!is_leaf))
> > > > > > > +
> > > > > >
> > > > > > Is the operator here should be || ?
> > > > > >
> > > > > >
> > > > >
> > > > > Check is done for 'if either ret is not 0 or if it ret is 0 but
> > > > > not leaf' we skip leaf details print. If 'ret is 0 and is leaf'
> > > > > we skip continue to print
> > > > leaf details.
> > > >
> > > > IMO, using logical operator over bitwise operator is good here in
> > > > if statement
> > > .
> > > > Like below.?
> > > >
> > > > If (ret || (is_leaf == 0 ))
> > >
> > > Thanks for the information, if the logic is correct do I need to
> > > change for v6
> > >
> >
> > OK in v6, but you can wait to hear more comments from others if any
> > before sending v6 .
> 
> Ok thanks Reshma, but can you tell me how the earlier logic fails and runs slow
> compared to logical or?

Not about faster or slower. 

Logical operators are commonly used in decision making in C programming. 
Bitwise operators are used in C programming to perform bit-level operations.

Since , above if condition is for decision making here logical || operator will fit , so I am suggesting to use that. 

We  don't need to do any bitwise manipulation in if condition to make the decision, so bitwise | operator is not needed

Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm

[Varghese, Vipin]

> > > > > >
> > > > > > >
> > > > > > > > +			if ((ret) | (!is_leaf))
> > > > > > > > +
> > > > > > >
> > > > > > > Is the operator here should be || ?
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > Check is done for 'if either ret is not 0 or if it ret is 0
> > > > > > but not leaf' we skip leaf details print. If 'ret is 0 and is leaf'
> > > > > > we skip continue to print
> > > > > leaf details.
> > > > >
> > > > > IMO, using logical operator over bitwise operator is good here
> > > > > in if statement
> > > > .
> > > > > Like below.?
> > > > >
> > > > > If (ret || (is_leaf == 0 ))
> > > >
> > > > Thanks for the information, if the logic is correct do I need to
> > > > change for v6
> > > >
> > >
> > > OK in v6, but you can wait to hear more comments from others if any
> > > before sending v6 .
> >
> > Ok thanks Reshma, but can you tell me how the earlier logic fails and
> > runs slow compared to logical or?
> 
> Not about faster or slower.

Now I see, I was wondering the suggestion was for improvement for performance.

> 
> Logical operators are commonly used in decision making in C programming.
> Bitwise operators are used in C programming to perform bit-level operations.
> 

Agreed

> Since , above if condition is for decision making here logical || operator will fit
> , so I am suggesting to use that.
> 

But bitwise OR is not wrong right?

> We  don't need to do any bitwise manipulation in if condition to make the
> decision, so bitwise | operator is not needed

We can correct this in next patch set not v6 if this is only change for 'show tm'

Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm

[Stephen Hemminger]

On Fri, 23 Nov 2018 15:05:07 +0000
"Varghese, Vipin" <vipin.varghese@intel.com> wrote:

> > > > > > >  
> > > > > > > >  
> > > > > > > > > +			if ((ret) | (!is_leaf))
> > > > > > > > > +  
> > > > > > > >
> > > > > > > > Is the operator here should be || ?
> > > > > > > >
> > > > > > > >  
> > > > > > >
> > > > > > > Check is done for 'if either ret is not 0 or if it ret is 0
> > > > > > > but not leaf' we skip leaf details print. If 'ret is 0 and is leaf'
> > > > > > > we skip continue to print  
> > > > > > leaf details.
> > > > > >
> > > > > > IMO, using logical operator over bitwise operator is good here
> > > > > > in if statement  
> > > > > .  
> > > > > > Like below.?
> > > > > >
> > > > > > If (ret || (is_leaf == 0 ))  
> > > > >
> > > > > Thanks for the information, if the logic is correct do I need to
> > > > > change for v6
> > > > >  
> > > >
> > > > OK in v6, but you can wait to hear more comments from others if any
> > > > before sending v6 .  
> > >
> > > Ok thanks Reshma, but can you tell me how the earlier logic fails and
> > > runs slow compared to logical or?  
> > 
> > Not about faster or slower.  
> 
> Now I see, I was wondering the suggestion was for improvement for performance.
> 
> > 
> > Logical operators are commonly used in decision making in C programming.
> > Bitwise operators are used in C programming to perform bit-level operations.
> >   
> 
> Agreed
> 
> > Since , above if condition is for decision making here logical || operator will fit
> > , so I am suggesting to use that.
> >   
> 
> But bitwise OR is not wrong right?
> 
> > We  don't need to do any bitwise manipulation in if condition to make the
> > decision, so bitwise | operator is not needed  
> 
> We can correct this in next patch set not v6 if this is only change for 'show tm'

It could be that compiler might optimize logical into bitwise operation
to avoid cost of conditional branch (if there are no side effects).

Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm

[Varghese, Vipin]

Hi Stephen,

snipped

> 
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > +			if ((ret) | (!is_leaf))
> > > > > > > > > > +
> > > > > > > > >
> > > > > > > > > Is the operator here should be || ?
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > > > Check is done for 'if either ret is not 0 or if it ret is
> > > > > > > > 0 but not leaf' we skip leaf details print. If 'ret is 0 and is leaf'
> > > > > > > > we skip continue to print
> > > > > > > leaf details.
> > > > > > >
> > > > > > > IMO, using logical operator over bitwise operator is good
> > > > > > > here in if statement
> > > > > > .
> > > > > > > Like below.?
> > > > > > >
> > > > > > > If (ret || (is_leaf == 0 ))
> > > > > >
> > > > > > Thanks for the information, if the logic is correct do I need
> > > > > > to change for v6
> > > > > >
> > > > >
> > > > > OK in v6, but you can wait to hear more comments from others if
> > > > > any before sending v6 .
> > > >
> > > > Ok thanks Reshma, but can you tell me how the earlier logic fails
> > > > and runs slow compared to logical or?
> > >
> > > Not about faster or slower.
> >
> > Now I see, I was wondering the suggestion was for improvement for
> performance.
> >
> > >
> > > Logical operators are commonly used in decision making in C programming.
> > > Bitwise operators are used in C programming to perform bit-level operations.
> > >
> >
> > Agreed
> >
> > > Since , above if condition is for decision making here logical ||
> > > operator will fit , so I am suggesting to use that.
> > >
> >
> > But bitwise OR is not wrong right?
> >
> > > We  don't need to do any bitwise manipulation in if condition to
> > > make the decision, so bitwise | operator is not needed
> >
> > We can correct this in next patch set not v6 if this is only change for 'show tm'
> 
> It could be that compiler might optimize logical into bitwise operation to avoid
> cost of conditional branch (if there are no side effects).


Checking with 'EXTRA_CFLAGS=-g' we get the code generated as 

"""
                        if ((ret) | (!is_leaf))
   9a512:       8b 85 28 fd ff ff       mov    eax,DWORD PTR [rbp-0x2d8]
   9a518:       85 c0                   test   eax,eax
   9a51a:       0f 94 c0                sete   al
   9a51d:       0f b6 c0                movzx  eax,al
   9a520:       0b 85 3c fd ff ff       or     eax,DWORD PTR [rbp-0x2c4]
   9a526:       85 c0                   test   eax,eax
   9a528:       75 74                   jne    9a59e <show_tm+0x73e>
                                continue;
"""

Looks like the is_leaf is picked assembly 'test' is done then byte is set to 1 based on flags. This is then or with 'ret'. I think your observation is correct 'compiler is remapping to or'

Thanks
Vipin Varghese

Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Varghese, Vipin
> Sent: Monday, November 26, 2018 4:15 AM
> To: Stephen Hemminger <stephen@networkplumber.org>
> Cc: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org; thomas@monjalon.net; Mcnamara, John <john.mcnamara@intel.com>;
> Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm
> 
> Hi Stephen,
> 
> snipped
> 
> >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > +			if ((ret) | (!is_leaf))
> > > > > > > > > > > +
> > > > > > > > > >
> > > > > > > > > > Is the operator here should be || ?
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Check is done for 'if either ret is not 0 or if it ret is
> > > > > > > > > 0 but not leaf' we skip leaf details print. If 'ret is 0 and is leaf'
> > > > > > > > > we skip continue to print
> > > > > > > > leaf details.
> > > > > > > >
> > > > > > > > IMO, using logical operator over bitwise operator is good
> > > > > > > > here in if statement
> > > > > > > .
> > > > > > > > Like below.?
> > > > > > > >
> > > > > > > > If (ret || (is_leaf == 0 ))
> > > > > > >
> > > > > > > Thanks for the information, if the logic is correct do I need
> > > > > > > to change for v6
> > > > > > >
> > > > > >
> > > > > > OK in v6, but you can wait to hear more comments from others if
> > > > > > any before sending v6 .
> > > > >
> > > > > Ok thanks Reshma, but can you tell me how the earlier logic fails
> > > > > and runs slow compared to logical or?
> > > >
> > > > Not about faster or slower.
> > >
> > > Now I see, I was wondering the suggestion was for improvement for
> > performance.
> > >
> > > >
> > > > Logical operators are commonly used in decision making in C programming.
> > > > Bitwise operators are used in C programming to perform bit-level operations.
> > > >
> > >
> > > Agreed
> > >
> > > > Since , above if condition is for decision making here logical ||
> > > > operator will fit , so I am suggesting to use that.
> > > >
> > >
> > > But bitwise OR is not wrong right?
> > >
> > > > We  don't need to do any bitwise manipulation in if condition to
> > > > make the decision, so bitwise | operator is not needed
> > >
> > > We can correct this in next patch set not v6 if this is only change for 'show tm'
> >
> > It could be that compiler might optimize logical into bitwise operation to avoid
> > cost of conditional branch (if there are no side effects).
> 
> 
> Checking with 'EXTRA_CFLAGS=-g' we get the code generated as
> 
> """
>                         if ((ret) | (!is_leaf))
>    9a512:       8b 85 28 fd ff ff       mov    eax,DWORD PTR [rbp-0x2d8]
>    9a518:       85 c0                   test   eax,eax
>    9a51a:       0f 94 c0                sete   al
>    9a51d:       0f b6 c0                movzx  eax,al
>    9a520:       0b 85 3c fd ff ff       or     eax,DWORD PTR [rbp-0x2c4]
>    9a526:       85 c0                   test   eax,eax
>    9a528:       75 74                   jne    9a59e <show_tm+0x73e>
>                                 continue;
> """
> 
> Looks like the is_leaf is picked assembly 'test' is done then byte is set to 1 based on flags. This is then or with 'ret'. I think your observation is
> correct 'compiler is remapping to or'

If the operator is '|' what else except of 'OR' you expect it to be remapped?
Konstantin

Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev, Konstantin
> Sent: Monday, November 26, 2018 9:28 AM
> To: Varghese, Vipin <vipin.varghese@intel.com>; Stephen Hemminger <stephen@networkplumber.org>
> Cc: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org; thomas@monjalon.net; Mcnamara, John <john.mcnamara@intel.com>;
> Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Varghese, Vipin
> > Sent: Monday, November 26, 2018 4:15 AM
> > To: Stephen Hemminger <stephen@networkplumber.org>
> > Cc: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org; thomas@monjalon.net; Mcnamara, John
> <john.mcnamara@intel.com>;
> > Byrne, Stephen1 <stephen1.byrne@intel.com>; Glynn, Michael J <michael.j.glynn@intel.com>; Patel, Amol <amol.patel@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm
> >
> > Hi Stephen,
> >
> > snipped
> >
> > >
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > +			if ((ret) | (!is_leaf))
> > > > > > > > > > > > +
> > > > > > > > > > >
> > > > > > > > > > > Is the operator here should be || ?
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Check is done for 'if either ret is not 0 or if it ret is
> > > > > > > > > > 0 but not leaf' we skip leaf details print. If 'ret is 0 and is leaf'
> > > > > > > > > > we skip continue to print
> > > > > > > > > leaf details.
> > > > > > > > >
> > > > > > > > > IMO, using logical operator over bitwise operator is good
> > > > > > > > > here in if statement
> > > > > > > > .
> > > > > > > > > Like below.?
> > > > > > > > >
> > > > > > > > > If (ret || (is_leaf == 0 ))
> > > > > > > >
> > > > > > > > Thanks for the information, if the logic is correct do I need
> > > > > > > > to change for v6
> > > > > > > >
> > > > > > >
> > > > > > > OK in v6, but you can wait to hear more comments from others if
> > > > > > > any before sending v6 .
> > > > > >
> > > > > > Ok thanks Reshma, but can you tell me how the earlier logic fails
> > > > > > and runs slow compared to logical or?
> > > > >
> > > > > Not about faster or slower.
> > > >
> > > > Now I see, I was wondering the suggestion was for improvement for
> > > performance.
> > > >
> > > > >
> > > > > Logical operators are commonly used in decision making in C programming.
> > > > > Bitwise operators are used in C programming to perform bit-level operations.
> > > > >
> > > >
> > > > Agreed
> > > >
> > > > > Since , above if condition is for decision making here logical ||
> > > > > operator will fit , so I am suggesting to use that.
> > > > >
> > > >
> > > > But bitwise OR is not wrong right?
> > > >
> > > > > We  don't need to do any bitwise manipulation in if condition to
> > > > > make the decision, so bitwise | operator is not needed
> > > >
> > > > We can correct this in next patch set not v6 if this is only change for 'show tm'
> > >
> > > It could be that compiler might optimize logical into bitwise operation to avoid
> > > cost of conditional branch (if there are no side effects).
> >
> >
> > Checking with 'EXTRA_CFLAGS=-g' we get the code generated as
> >
> > """
> >                         if ((ret) | (!is_leaf))
> >    9a512:       8b 85 28 fd ff ff       mov    eax,DWORD PTR [rbp-0x2d8]
> >    9a518:       85 c0                   test   eax,eax
> >    9a51a:       0f 94 c0                sete   al
> >    9a51d:       0f b6 c0                movzx  eax,al
> >    9a520:       0b 85 3c fd ff ff       or     eax,DWORD PTR [rbp-0x2c4]
> >    9a526:       85 c0                   test   eax,eax
> >    9a528:       75 74                   jne    9a59e <show_tm+0x73e>
> >                                 continue;
> > """
> >
> > Looks like the is_leaf is picked assembly 'test' is done then byte is set to 1 based on flags. This is then or with 'ret'. I think your observation
> is
> > correct 'compiler is remapping to or'
> 
> If the operator is '|' what else except of 'OR' you expect it to be remapped?
BTW, I am agree with Reshma it has to be '||' here.

Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm

[Varghese, Vipin]

Hi Konstantin,

> >
> > If the operator is '|' what else except of 'OR' you expect it to be remapped?

Thanks for your correction. I have rerun with logical or 

                        if ((ret) || (!is_leaf))
   9a512:       83 bd 3c fd ff ff 00    cmp    DWORD PTR [rbp-0x2c4],0x0
   9a519:       75 7e                   jne    9a599 <show_tm+0x739>
   9a51b:       8b 85 28 fd ff ff       mov    eax,DWORD PTR [rbp-0x2d8]
   9a521:       85 c0                   test   eax,eax
   9a523:       74 74                   je     9a599 <show_tm+0x739>
                                continue;

There is no conversion to 'Or' operation here. Thanks for 

> BTW, I am agree with Reshma it has to be '||' here.
Thanks for your opinion, can you share review comments for show_tm for debug?

Thanks
Vipin Varghese

Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm

[Ananyev, Konstantin]

Hi Vipin,

> 
> Hi Konstantin,
> 
> > >
> > > If the operator is '|' what else except of 'OR' you expect it to be remapped?
> 
> Thanks for your correction. I have rerun with logical or
> 
>                         if ((ret) || (!is_leaf))
>    9a512:       83 bd 3c fd ff ff 00    cmp    DWORD PTR [rbp-0x2c4],0x0
>    9a519:       75 7e                   jne    9a599 <show_tm+0x739>
>    9a51b:       8b 85 28 fd ff ff       mov    eax,DWORD PTR [rbp-0x2d8]
>    9a521:       85 c0                   test   eax,eax
>    9a523:       74 74                   je     9a599 <show_tm+0x739>
>                                 continue;
> 
> There is no conversion to 'Or' operation here. Thanks for
> 
> > BTW, I am agree with Reshma it has to be '||' here.
> Thanks for your opinion, can you share review comments for show_tm for debug?

Not sure I understand your last sentence...
Konstantin

Re: [dpdk-dev] [PATCH v4 5/9] app/procinfo: add support for show tm

[Varghese, Vipin]

Hi,

> Hi Vipin,
> 
> >
> > Hi Konstantin,
> >
> > > >
> > > > If the operator is '|' what else except of 'OR' you expect it to be remapped?
> >
> > Thanks for your correction. I have rerun with logical or
> >
> >                         if ((ret) || (!is_leaf))
> >    9a512:       83 bd 3c fd ff ff 00    cmp    DWORD PTR [rbp-0x2c4],0x0
> >    9a519:       75 7e                   jne    9a599 <show_tm+0x739>
> >    9a51b:       8b 85 28 fd ff ff       mov    eax,DWORD PTR [rbp-0x2d8]
> >    9a521:       85 c0                   test   eax,eax
> >    9a523:       74 74                   je     9a599 <show_tm+0x739>
> >                                 continue;
> >
> > There is no conversion to 'Or' operation here. Thanks for
> >
> > > BTW, I am agree with Reshma it has to be '||' here.
> > Thanks for your opinion, can you share review comments for show_tm for
> debug?
> 
> Not sure I understand your last sentence...
> Konstantin

As mentioned earlier email I am ready to spin v6 with 'logical or'. But if there any other comments for show_tm it will be nice to accommodate to v6 rather than v7. So, can you please share if there any other comments for show_tm?

Thanks
Vipin Varghese

[dpdk-dev] [PATCH v4 6/9] app/procinfo: add support for show crypto

[Vipin Varghese]

Function show_crypto is used for displaying the crypto PMD under
the primary process.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---

v4:
 - add space to compare - Vipin Varghese

V3:
 - replace MACRO to function - Reshma Pathan & Stephen Hemminger
 - add memset for struct elements - Reshma Pathan
 - change display formating of flags - Vipin Varghese
 - use MACRO for string - Vipin Varghese
---
 app/proc-info/main.c | 80 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 79 insertions(+), 1 deletion(-)

diff --git a/app/proc-info/main.c b/app/proc-info/main.c
index 8ec2e9474..7213b43fe 100644
--- a/app/proc-info/main.c
+++ b/app/proc-info/main.c
@@ -1032,10 +1032,88 @@ show_tm(void)
 	STATS_BDR_STR(50, "");
 }
 
+static void
+display_crypto_feature_info(uint64_t x)
+{
+	if (x == 0)
+		return;
+
+	printf("\t  -- feature flags\n");
+	printf("\t\t  + symmetric (%c), asymmetric (%c)\n"
+		"\t\t  + symmetric operation chaining (%c)\n",
+		(x & RTE_CRYPTODEV_FF_SYMMETRIC_CRYPTO) ? 'y' : 'n',
+		(x & RTE_CRYPTODEV_FF_ASYMMETRIC_CRYPTO) ? 'y' : 'n',
+		(x & RTE_CRYPTODEV_FF_SYM_OPERATION_CHAINING) ? 'y' : 'n');
+	printf("\t\t  + CPU: SSE (%c), AVX (%c), AVX2 (%c), AVX512 (%c)\n",
+		(x & RTE_CRYPTODEV_FF_CPU_SSE) ? 'y' : 'n',
+		(x & RTE_CRYPTODEV_FF_CPU_AVX) ? 'y' : 'n',
+		(x & RTE_CRYPTODEV_FF_CPU_AVX2) ? 'y' : 'n',
+		(x & RTE_CRYPTODEV_FF_CPU_AVX512) ? 'y' : 'n');
+	printf("\t\t  + AESNI: CPU (%c), HW (%c)\n",
+		(x & RTE_CRYPTODEV_FF_CPU_AESNI) ? 'y' : 'n',
+		(x & RTE_CRYPTODEV_FF_HW_ACCELERATED) ? 'y' : 'n');
+	printf("\t\t  + INLINE (%c)\n",
+		(x & RTE_CRYPTODEV_FF_SECURITY) ? 'y' : 'n');
+	printf("\t\t  + ARM: NEON (%c), CE (%c)\n",
+		(x & RTE_CRYPTODEV_FF_CPU_NEON) ? 'y' : 'n',
+		(x & RTE_CRYPTODEV_FF_CPU_ARM_CE) ? 'y' : 'n');
+	printf("\t  -- buffer offload\n");
+	printf("\t\t  + IN_PLACE_SGL (%c)\n",
+		(x & RTE_CRYPTODEV_FF_IN_PLACE_SGL) ? 'y' : 'n');
+	printf("\t\t  + OOP_SGL_IN_SGL_OUT (%c)\n",
+		(x & RTE_CRYPTODEV_FF_OOP_SGL_IN_SGL_OUT) ? 'y' : 'n');
+	printf("\t\t  + OOP_SGL_IN_LB_OUT (%c)\n",
+		(x & RTE_CRYPTODEV_FF_OOP_SGL_IN_LB_OUT) ? 'y' : 'n');
+	printf("\t\t  + OOP_LB_IN_SGL_OUT (%c)\n",
+		(x & RTE_CRYPTODEV_FF_OOP_LB_IN_SGL_OUT) ? 'y' : 'n');
+	printf("\t\t  + OOP_LB_IN_LB_OUT (%c)\n",
+		(x & RTE_CRYPTODEV_FF_OOP_LB_IN_LB_OUT) ? 'y' : 'n');
+}
+
 static void
 show_crypto(void)
 {
-	printf(" crypto\n");
+	uint8_t crypto_dev_count = rte_cryptodev_count(), i;
+
+	snprintf(bdr_str, MAX_STRING_LEN, " show - CRYPTO PMD %"PRIu64,
+		rte_get_tsc_hz());
+	STATS_BDR_STR(10, bdr_str);
+
+	for (i = 0; i < crypto_dev_count; i++) {
+		struct rte_cryptodev_info dev_info;
+		struct rte_cryptodev_stats stats;
+
+		memset(&dev_info, 0, sizeof(dev_info));
+		rte_cryptodev_info_get(i, &dev_info);
+
+		printf("  - device (%u)\n", i);
+		printf("\t  -- name (%s)\n"
+			"\t  -- driver (%s)\n"
+			"\t  -- id (%u) on socket (%d)\n"
+			"\t  -- queue pairs (%d)\n",
+			rte_cryptodev_name_get(i),
+			dev_info.driver_name,
+			dev_info.driver_id,
+			dev_info.device->numa_node,
+			rte_cryptodev_queue_pair_count(i));
+
+		display_crypto_feature_info(dev_info.feature_flags);
+
+		printf("\t  -- stats\n");
+		memset(&stats, 0, sizeof(0));
+		if (rte_cryptodev_stats_get(i, &stats) == 0) {
+			printf("\t\t  + enqueue count (%"PRIu64")"
+				" error (%"PRIu64")\n",
+				stats.enqueued_count,
+				stats.enqueue_err_count);
+			printf("\t\t  + dequeue count (%"PRIu64")"
+				" error (%"PRIu64")\n",
+				stats.dequeued_count,
+				stats.dequeue_err_count);
+		}
+	}
+
+	STATS_BDR_STR(50, "");
 }
 
 static void
-- 
2.17.1