DPDK-dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 11/24] net/i40e: add flow validate function
From: Beilei Xing @ 2016-12-02 11:53 UTC (permalink / raw)
  To: jingjing.wu, helin.zhang; +Cc: dev, wenzhuo.lu
In-Reply-To: <1480679625-4157-1-git-send-email-beilei.xing@intel.com>

This patch adds handling RTE_ETH_FILTER_GENERIC filter type in
.filter_ctrl function, and result in a pointer to i40e_flow_ops.
This patch also adds flow validate ops.

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 997e2fe..c1623c4 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -52,6 +52,7 @@
 #include <rte_eth_ctrl.h>
 #include <rte_tailq.h>
 #include <rte_hash_crc.h>
+#include <rte_flow_driver.h>
 
 #include "i40e_logs.h"
 #include "base/i40e_prototype.h"
@@ -490,6 +491,12 @@ static void i40e_tunnel_filter_restore(struct i40e_pf *pf);
 static void i40e_rss_hash_restore(struct i40e_pf *pf);
 static void i40e_filter_restore(struct i40e_pf *pf);
 
+static int i40e_flow_validate(__rte_unused struct rte_eth_dev *dev,
+			      const struct rte_flow_attr *attr,
+			      const struct rte_flow_item *pattern,
+			      const struct rte_flow_action *actions,
+			      struct rte_flow_error *error);
+
 static const struct rte_pci_id pci_id_i40e_map[] = {
 	{ RTE_PCI_DEVICE(I40E_INTEL_VENDOR_ID, I40E_DEV_ID_SFP_XL710) },
 	{ RTE_PCI_DEVICE(I40E_INTEL_VENDOR_ID, I40E_DEV_ID_QEMU) },
@@ -584,6 +591,10 @@ static const struct eth_dev_ops i40e_eth_dev_ops = {
 	.mtu_set                      = i40e_dev_mtu_set,
 };
 
+static const struct rte_flow_ops i40e_flow_ops = {
+	.validate = i40e_flow_validate,
+};
+
 /* store statistics names and its offset in stats structure */
 struct rte_i40e_xstats_name_off {
 	char name[RTE_ETH_XSTATS_NAME_SIZE];
@@ -8505,6 +8516,11 @@ i40e_dev_filter_ctrl(struct rte_eth_dev *dev,
 	case RTE_ETH_FILTER_FDIR:
 		ret = i40e_fdir_ctrl_func(dev, filter_op, arg);
 		break;
+	case RTE_ETH_FILTER_GENERIC:
+		if (filter_op != RTE_ETH_FILTER_GET)
+			return -EINVAL;
+		*(const void **)arg = &i40e_flow_ops;
+		break;
 	default:
 		PMD_DRV_LOG(WARNING, "Filter type (%d) not supported",
 							filter_type);
@@ -10223,3 +10239,21 @@ i40e_filter_restore(struct i40e_pf *pf)
 	i40e_fdir_filter_restore(pf);
 	i40e_rss_hash_restore(pf);
 }
+
+static int
+i40e_flow_validate(__rte_unused struct rte_eth_dev *dev,
+		   const struct rte_flow_attr *attr,
+		   const struct rte_flow_item *pattern,
+		   const struct rte_flow_action *actions,
+		   struct rte_flow_error *error)
+{
+	struct rte_eth_ethertype_filter ethertype_filter;
+	int ret;
+
+	ret = cons_parse_ethertype_filter(attr, pattern, actions,
+					  &ethertype_filter, error);
+	if (!ret)
+		return 0;
+
+	return ret;
+}
-- 
2.5.5

^ permalink raw reply related

* [PATCH 10/24] ethdev: parse ethertype filter
From: Beilei Xing @ 2016-12-02 11:53 UTC (permalink / raw)
  To: jingjing.wu, helin.zhang; +Cc: dev, wenzhuo.lu
In-Reply-To: <1480679625-4157-1-git-send-email-beilei.xing@intel.com>

Check if the rule is a ethertype rule, and get the ethertype
info BTW.

Signed-off-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
 lib/librte_ether/rte_flow.c        | 136 +++++++++++++++++++++++++++++++++++++
 lib/librte_ether/rte_flow_driver.h |  34 ++++++++++
 2 files changed, 170 insertions(+)

diff --git a/lib/librte_ether/rte_flow.c b/lib/librte_ether/rte_flow.c
index 064963d..acc9057 100644
--- a/lib/librte_ether/rte_flow.c
+++ b/lib/librte_ether/rte_flow.c
@@ -157,3 +157,139 @@ rte_flow_query(uint8_t port_id,
 			   NULL, rte_strerror(ENOTSUP));
 	return -rte_errno;
 }
+
+/**
+ * Parse the rule to see if it is a ethertype rule.
+ * And get the ethertype filter info BTW.
+ */
+int
+cons_parse_ethertype_filter(const struct rte_flow_attr *attr,
+			    const struct rte_flow_item *pattern,
+			    const struct rte_flow_action *actions,
+			    struct rte_eth_ethertype_filter *filter,
+			    struct rte_flow_error *error)
+{
+	const struct rte_flow_item *item;
+	const struct rte_flow_action *act;
+	const struct rte_flow_item_eth *eth_spec;
+	const struct rte_flow_item_eth *eth_mask;
+	const struct rte_flow_action_queue *act_q;
+	uint32_t i, j;
+
+	/************************************************
+	 * parse pattern
+	 ************************************************/
+	i = 0;
+
+	/* The first not void item should be MAC. */
+	PATTERN_SKIP_VOID(filter, struct rte_eth_ethertype_filter,
+			  RTE_FLOW_ERROR_TYPE_ITEM_NUM);
+	if (item->type != RTE_FLOW_ITEM_TYPE_ETH) {
+		error->type = RTE_FLOW_ERROR_TYPE_ITEM;
+		return -EINVAL;
+	}
+
+	/* Get the MAC info. */
+	if (!item->spec || !item->mask) {
+		error->type = RTE_FLOW_ERROR_TYPE_ITEM;
+		return -EINVAL;
+	}
+
+	eth_spec = (const struct rte_flow_item_eth *)item->spec;
+	eth_mask = (const struct rte_flow_item_eth *)item->mask;
+	/**
+	 * Source MAC address must be masked.
+	 * Destination MAC address must be totally masked or not.
+	 */
+	if (eth_mask->src.addr_bytes[0] ||
+	    (eth_mask->dst.addr_bytes[0] != 0xFF &&
+	     eth_mask->dst.addr_bytes[0])) {
+		error->type = RTE_FLOW_ERROR_TYPE_ITEM;
+		return -EINVAL;
+	}
+
+	for (j = 1; j < ETHER_ADDR_LEN; j++) {
+		if (eth_mask->src.addr_bytes[j] !=
+		    eth_mask->src.addr_bytes[0] ||
+		    eth_mask->dst.addr_bytes[j] !=
+		    eth_mask->dst.addr_bytes[0]) {
+			error->type = RTE_FLOW_ERROR_TYPE_ITEM;
+			return -EINVAL;
+		}
+	}
+
+	if ((eth_mask->type & 0xFFFF) != 0xFFFF) {
+		error->type = RTE_FLOW_ERROR_TYPE_ITEM;
+		return -EINVAL;
+	}
+
+	if (eth_mask->dst.addr_bytes[0]) {
+		filter->mac_addr = eth_spec->dst;
+		filter->flags |= RTE_ETHTYPE_FLAGS_MAC;
+	} else {
+		filter->flags &= ~RTE_ETHTYPE_FLAGS_MAC;
+	}
+	filter->ether_type = (uint16_t)eth_spec->type;
+
+	/* Check if the next not void item is END. */
+	i++;
+	PATTERN_SKIP_VOID(filter, struct rte_eth_ethertype_filter,
+			  RTE_FLOW_ERROR_TYPE_ITEM_NUM);
+	if (item->type != RTE_FLOW_ITEM_TYPE_END) {
+		error->type = RTE_FLOW_ERROR_TYPE_ITEM;
+		return -EINVAL;
+	}
+
+	/************************************************
+	 * parse action
+	 ************************************************/
+	i = 0;
+
+	/* Check if the first not void action is QUEUE or DROP. */
+	ACTION_SKIP_VOID(filter, struct rte_eth_ethertype_filter,
+			 RTE_FLOW_ERROR_TYPE_ACTION_NUM);
+	if (act->type != RTE_FLOW_ACTION_TYPE_QUEUE &&
+	    act->type != RTE_FLOW_ACTION_TYPE_DROP) {
+		error->type = RTE_FLOW_ERROR_TYPE_ACTION;
+		return -EINVAL;
+	}
+
+	if (act->type == RTE_FLOW_ACTION_TYPE_QUEUE) {
+		act_q = (const struct rte_flow_action_queue *)act->conf;
+		filter->queue = act_q->index;
+	} else {
+		filter->flags |= RTE_ETHTYPE_FLAGS_DROP;
+	}
+
+	/* Check if the next not void item is END */
+	i++;
+	ACTION_SKIP_VOID(filter, struct rte_eth_ethertype_filter,
+			 RTE_FLOW_ERROR_TYPE_ACTION_NUM);
+	if (act->type != RTE_FLOW_ACTION_TYPE_END) {
+		error->type = RTE_FLOW_ERROR_TYPE_ACTION;
+		return -EINVAL;
+	}
+
+	/************************************************
+	 * parse attr
+	 ************************************************/
+	/* Must be input direction */
+	if (!attr->ingress) {
+		error->type = RTE_FLOW_ERROR_TYPE_ATTR_INGRESS;
+		return -EINVAL;
+	}
+
+	/* Not supported */
+	if (attr->egress) {
+		error->type = RTE_FLOW_ERROR_TYPE_ATTR_EGRESS;
+		return -EINVAL;
+	}
+
+	/* Not supported */
+	if (attr->priority) {
+		error->type = RTE_FLOW_ERROR_TYPE_ATTR_PRIORITY;
+		return -EINVAL;
+	}
+
+	return 0;
+}
diff --git a/lib/librte_ether/rte_flow_driver.h b/lib/librte_ether/rte_flow_driver.h
index a88c621..2760c74 100644
--- a/lib/librte_ether/rte_flow_driver.h
+++ b/lib/librte_ether/rte_flow_driver.h
@@ -170,6 +170,40 @@ rte_flow_error_set(struct rte_flow_error *error,
 const struct rte_flow_ops *
 rte_flow_ops_get(uint8_t port_id, struct rte_flow_error *error);
 
+int cons_parse_ethertype_filter(const struct rte_flow_attr *attr,
+			    const struct rte_flow_item *pattern,
+			    const struct rte_flow_action *actions,
+			    struct rte_eth_ethertype_filter *filter,
+			    struct rte_flow_error *error);
+
+#define PATTERN_SKIP_VOID(filter, filter_struct, error_type)		\
+	do {								\
+		if (!pattern) {						\
+			memset(filter, 0, sizeof(filter_struct));	\
+			error->type = error_type;                       \
+			return -EINVAL;					\
+		}							\
+		item = pattern + i;					\
+		while (item->type == RTE_FLOW_ITEM_TYPE_VOID) {		\
+			i++;						\
+			item = pattern + i;				\
+		}							\
+	} while (0)
+
+#define ACTION_SKIP_VOID(filter, filter_struct, error_type)		\
+	do {								\
+		if (!actions) {						\
+			memset(filter, 0, sizeof(filter_struct));	\
+			error->type = error_type;			\
+			return -EINVAL;					\
+		}							\
+		act = actions + i;					\
+		while (act->type == RTE_FLOW_ACTION_TYPE_VOID) {	\
+			i++;						\
+			act = actions + i;				\
+		}							\
+	} while (0)
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.5.5

^ permalink raw reply related

* [PATCH 09/24] net/i40e: restore RSS hash info
From: Beilei Xing @ 2016-12-02 11:53 UTC (permalink / raw)
  To: jingjing.wu, helin.zhang; +Cc: dev, wenzhuo.lu
In-Reply-To: <1480679625-4157-1-git-send-email-beilei.xing@intel.com>

Add support of restoring RSS hash info, include looup table
and RSS configuration.

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index a47d141..997e2fe 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -487,6 +487,7 @@ static int i40e_sw_tunnel_filter_del(struct i40e_pf *pf,
 static void i40e_ethertype_filter_restore(struct i40e_pf *pf);
 static void i40e_macvlan_filter_restore(struct i40e_pf *pf);
 static void i40e_tunnel_filter_restore(struct i40e_pf *pf);
+static void i40e_rss_hash_restore(struct i40e_pf *pf);
 static void i40e_filter_restore(struct i40e_pf *pf);
 
 static const struct rte_pci_id pci_id_i40e_map[] = {
@@ -10197,6 +10198,22 @@ i40e_tunnel_filter_restore(struct i40e_pf *pf)
 	}
 }
 
+/* Restore hash filter */
+static void
+i40e_rss_hash_restore(struct i40e_pf *pf)
+{
+	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
+	uint16_t reta_size = hw->func_caps.rss_table_size;
+
+	if (pf->hash.reta) {
+		/* Restore lut to HW */
+		i40e_set_rss_lut(pf->main_vsi, pf->hash.reta, reta_size);
+
+		/* Restore RSS configuration to HW */
+		i40e_hw_rss_hash_set(pf, &pf->hash.rss_conf);
+	}
+}
+
 static void
 i40e_filter_restore(struct i40e_pf *pf)
 {
@@ -10204,4 +10221,5 @@ i40e_filter_restore(struct i40e_pf *pf)
 	i40e_macvlan_filter_restore(pf);
 	i40e_tunnel_filter_restore(pf);
 	i40e_fdir_filter_restore(pf);
+	i40e_rss_hash_restore(pf);
 }
-- 
2.5.5

^ permalink raw reply related

* [PATCH 08/24] net/i40e: restore flow director filter
From: Beilei Xing @ 2016-12-02 11:53 UTC (permalink / raw)
  To: jingjing.wu, helin.zhang; +Cc: dev, wenzhuo.lu
In-Reply-To: <1480679625-4157-1-git-send-email-beilei.xing@intel.com>

Add support of restoring flow director filter.

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c |  1 +
 drivers/net/i40e/i40e_ethdev.h |  1 +
 drivers/net/i40e/i40e_fdir.c   | 11 +++++++++++
 3 files changed, 13 insertions(+)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 8ca69f2..a47d141 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -10203,4 +10203,5 @@ i40e_filter_restore(struct i40e_pf *pf)
 	i40e_ethertype_filter_restore(pf);
 	i40e_macvlan_filter_restore(pf);
 	i40e_tunnel_filter_restore(pf);
+	i40e_fdir_filter_restore(pf);
 }
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index d40010a..35ac6d6 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -683,6 +683,7 @@ int i40e_fdir_ctrl_func(struct rte_eth_dev *dev,
 int i40e_select_filter_input_set(struct i40e_hw *hw,
 				 struct rte_eth_input_set_conf *conf,
 				 enum rte_filter_type filter);
+void i40e_fdir_filter_restore(struct i40e_pf *pf);
 int i40e_hash_filter_inset_select(struct i40e_hw *hw,
 			     struct rte_eth_input_set_conf *conf);
 int i40e_fdir_filter_inset_select(struct i40e_pf *pf,
diff --git a/drivers/net/i40e/i40e_fdir.c b/drivers/net/i40e/i40e_fdir.c
index 1913fe1..e47a949 100644
--- a/drivers/net/i40e/i40e_fdir.c
+++ b/drivers/net/i40e/i40e_fdir.c
@@ -1576,3 +1576,14 @@ i40e_fdir_ctrl_func(struct rte_eth_dev *dev,
 	}
 	return ret;
 }
+
+void
+i40e_fdir_filter_restore(struct i40e_pf *pf)
+{
+	struct i40e_fdir_filter_list *fdir_list = &pf->fdir.fdir_list;
+	struct i40e_fdir_filter *f;
+	struct rte_eth_dev *dev = I40E_VSI_TO_ETH_DEV(pf->main_vsi);
+
+	TAILQ_FOREACH(f, fdir_list, rules)
+		i40e_add_del_fdir_filter(dev, &f->fdir, TRUE);
+}
-- 
2.5.5

^ permalink raw reply related

* [PATCH 07/24] net/i40e: restore tunnel filter
From: Beilei Xing @ 2016-12-02 11:53 UTC (permalink / raw)
  To: jingjing.wu, helin.zhang; +Cc: dev, wenzhuo.lu
In-Reply-To: <1480679625-4157-1-git-send-email-beilei.xing@intel.com>

Add support of restoring tunnel filter.

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 119ff94..8ca69f2 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -486,6 +486,7 @@ static int i40e_sw_tunnel_filter_del(struct i40e_pf *pf,
 
 static void i40e_ethertype_filter_restore(struct i40e_pf *pf);
 static void i40e_macvlan_filter_restore(struct i40e_pf *pf);
+static void i40e_tunnel_filter_restore(struct i40e_pf *pf);
 static void i40e_filter_restore(struct i40e_pf *pf);
 
 static const struct rte_pci_id pci_id_i40e_map[] = {
@@ -10178,9 +10179,28 @@ i40e_macvlan_filter_restore(struct i40e_pf *pf)
 	}
 }
 
+/* Restore tunnel filter */
+static void
+i40e_tunnel_filter_restore(struct i40e_pf *pf)
+{
+	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
+	struct i40e_vsi *vsi = pf->main_vsi;
+	struct i40e_tunnel_filter_list
+		*tunnel_list = &pf->tunnel.tunnel_list;
+	struct i40e_tunnel_filter *f;
+	struct i40e_aqc_add_remove_cloud_filters_element_data cld_filter;
+
+	TAILQ_FOREACH(f, tunnel_list, rules) {
+		rte_memcpy(&cld_filter, &f->input, sizeof(f->input));
+		cld_filter.queue_number = f->queue;
+		i40e_aq_add_cloud_filters(hw, vsi->seid, &cld_filter, 1);
+	}
+}
+
 static void
 i40e_filter_restore(struct i40e_pf *pf)
 {
 	i40e_ethertype_filter_restore(pf);
 	i40e_macvlan_filter_restore(pf);
+	i40e_tunnel_filter_restore(pf);
 }
-- 
2.5.5

^ permalink raw reply related

* [PATCH 06/24] net/i40e: restore macvlan filter
From: Beilei Xing @ 2016-12-02 11:53 UTC (permalink / raw)
  To: jingjing.wu, helin.zhang; +Cc: dev, wenzhuo.lu
In-Reply-To: <1480679625-4157-1-git-send-email-beilei.xing@intel.com>

Add support of restoring macvlan filter.

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 11c4c64..119ff94 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -485,6 +485,7 @@ static int i40e_sw_tunnel_filter_del(struct i40e_pf *pf,
 			     struct i40e_tunnel_filter *tunnel_filter);
 
 static void i40e_ethertype_filter_restore(struct i40e_pf *pf);
+static void i40e_macvlan_filter_restore(struct i40e_pf *pf);
 static void i40e_filter_restore(struct i40e_pf *pf);
 
 static const struct rte_pci_id pci_id_i40e_map[] = {
@@ -10159,8 +10160,27 @@ i40e_ethertype_filter_restore(struct i40e_pf *pf)
 	}
 }
 
+/* Restore macvlan filter */
+static void
+i40e_macvlan_filter_restore(struct i40e_pf *pf)
+{
+	struct i40e_mac_filter *f;
+	struct i40e_mac_filter_info *mac_filter;
+	struct i40e_vsi *vsi;
+	int i;
+
+	for (i = 0; i < pf->vf_num; i++) {
+		vsi = pf->vfs[i].vsi;
+		TAILQ_FOREACH(f, &vsi->mac_list, next) {
+			mac_filter = &f->mac_info;
+			i40e_vsi_add_mac(vsi, mac_filter);
+		}
+	}
+}
+
 static void
 i40e_filter_restore(struct i40e_pf *pf)
 {
 	i40e_ethertype_filter_restore(pf);
+	i40e_macvlan_filter_restore(pf);
 }
-- 
2.5.5

^ permalink raw reply related

* [PATCH 05/24] net/i40e: restore ethertype filter
From: Beilei Xing @ 2016-12-02 11:53 UTC (permalink / raw)
  To: jingjing.wu, helin.zhang; +Cc: dev, wenzhuo.lu
In-Reply-To: <1480679625-4157-1-git-send-email-beilei.xing@intel.com>

Add support of restoring ethertype filter.

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 521e7bb..11c4c64 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -484,6 +484,9 @@ static int i40e_sw_tunnel_filter_insert(struct i40e_pf *pf,
 static int i40e_sw_tunnel_filter_del(struct i40e_pf *pf,
 			     struct i40e_tunnel_filter *tunnel_filter);
 
+static void i40e_ethertype_filter_restore(struct i40e_pf *pf);
+static void i40e_filter_restore(struct i40e_pf *pf);
+
 static const struct rte_pci_id pci_id_i40e_map[] = {
 	{ RTE_PCI_DEVICE(I40E_INTEL_VENDOR_ID, I40E_DEV_ID_SFP_XL710) },
 	{ RTE_PCI_DEVICE(I40E_INTEL_VENDOR_ID, I40E_DEV_ID_QEMU) },
@@ -1966,6 +1969,8 @@ i40e_dev_start(struct rte_eth_dev *dev)
 	/* enable uio intr after callback register */
 	rte_intr_enable(intr_handle);
 
+	i40e_filter_restore(pf);
+
 	return I40E_SUCCESS;
 
 err_up:
@@ -10125,3 +10130,37 @@ i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu)
 
 	return ret;
 }
+
+/* Restore ethertype filter */
+static void
+i40e_ethertype_filter_restore(struct i40e_pf *pf)
+{
+	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
+	struct i40e_ethertype_filter_list
+		*ethertype_list = &pf->ethertype.ethertype_list;
+	struct i40e_ethertype_filter *f;
+	struct i40e_control_filter_stats stats;
+	uint16_t flags;
+
+	TAILQ_FOREACH(f, ethertype_list, rules) {
+		flags = 0;
+		if (!(f->flags & RTE_ETHTYPE_FLAGS_MAC))
+			flags |= I40E_AQC_ADD_CONTROL_PACKET_FLAGS_IGNORE_MAC;
+		if (f->flags & RTE_ETHTYPE_FLAGS_DROP)
+			flags |= I40E_AQC_ADD_CONTROL_PACKET_FLAGS_DROP;
+		flags |= I40E_AQC_ADD_CONTROL_PACKET_FLAGS_TO_QUEUE;
+
+		memset(&stats, 0, sizeof(stats));
+		i40e_aq_add_rem_control_packet_filter(hw,
+					    f->input.mac_addr.addr_bytes,
+					    f->input.ether_type,
+					    flags, pf->main_vsi->seid,
+					    f->queue, 1, &stats, NULL);
+	}
+}
+
+static void
+i40e_filter_restore(struct i40e_pf *pf)
+{
+	i40e_ethertype_filter_restore(pf);
+}
-- 
2.5.5

^ permalink raw reply related

* [PATCH 04/24] net/i40e: store RSS hash info
From: Beilei Xing @ 2016-12-02 11:53 UTC (permalink / raw)
  To: jingjing.wu, helin.zhang; +Cc: dev, wenzhuo.lu
In-Reply-To: <1480679625-4157-1-git-send-email-beilei.xing@intel.com>

Add support of storing lookup table and RSS
configuration in SW.

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 39 ++++++++++++++++++++++++++++++++++-----
 drivers/net/i40e/i40e_ethdev.h |  6 ++++++
 2 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index c38536f..521e7bb 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -1356,6 +1356,9 @@ eth_i40e_dev_uninit(struct rte_eth_dev *dev)
 	if (hw->adapter_stopped == 0)
 		i40e_dev_close(dev);
 
+	if (pf->hash.reta)
+		rte_free(pf->hash.reta);
+
 	/* Remove all ethertype director rules and hash */
 	if (ethertype_info->hash_map)
 		rte_free(ethertype_info->hash_map);
@@ -3453,6 +3456,8 @@ i40e_dev_rss_reta_update(struct rte_eth_dev *dev,
 	}
 	ret = i40e_set_rss_lut(pf->main_vsi, lut, reta_size);
 
+	/* Store updated lut */
+	rte_memcpy(pf->hash.reta, lut, sizeof(*lut) * reta_size);
 out:
 	rte_free(lut);
 
@@ -6959,6 +6964,8 @@ i40e_pf_config_rss(struct i40e_pf *pf)
 	struct rte_eth_rss_conf rss_conf;
 	uint32_t i, lut = 0;
 	uint16_t j, num;
+	uint16_t reta_size = hw->func_caps.rss_table_size;
+	int ret = -EINVAL;
 
 	/*
 	 * If both VMDQ and RSS enabled, not all of PF queues are configured.
@@ -6978,7 +6985,7 @@ i40e_pf_config_rss(struct i40e_pf *pf)
 		return -ENOTSUP;
 	}
 
-	for (i = 0, j = 0; i < hw->func_caps.rss_table_size; i++, j++) {
+	for (i = 0, j = 0; i < reta_size; i++, j++) {
 		if (j == num)
 			j = 0;
 		lut = (lut << 8) | (j & ((0x1 <<
@@ -6987,6 +6994,19 @@ i40e_pf_config_rss(struct i40e_pf *pf)
 			I40E_WRITE_REG(hw, I40E_PFQF_HLUT(i >> 2), lut);
 	}
 
+	/* Store lut into SW */
+	uint8_t *reta;
+
+	reta = rte_zmalloc("i40e_rss_reta", reta_size, 0);
+	if (!reta) {
+		PMD_DRV_LOG(ERR, "No memory can be allocated");
+		return -ENOMEM;
+	}
+	pf->hash.reta = reta;
+	ret = i40e_get_rss_lut(pf->main_vsi, reta, reta_size);
+	if (ret < 0)
+		return ret;
+
 	rss_conf = pf->dev_data->dev_conf.rx_adv_conf.rss_conf;
 	if ((rss_conf.rss_hf & I40E_RSS_OFFLOAD_ALL) == 0) {
 		i40e_pf_disable_rss(pf);
@@ -7005,7 +7025,15 @@ i40e_pf_config_rss(struct i40e_pf *pf)
 							sizeof(uint32_t);
 	}
 
-	return i40e_hw_rss_hash_set(pf, &rss_conf);
+	ret = i40e_hw_rss_hash_set(pf, &rss_conf);
+	if (ret < 0)
+		return ret;
+
+	/* store rss configuration into SW */
+	ret = i40e_dev_rss_hash_conf_get(
+		I40E_VSI_TO_ETH_DEV(pf->main_vsi), &pf->hash.rss_conf);
+
+	return ret;
 }
 
 static int
@@ -7158,9 +7186,10 @@ i40e_pf_config_mq_rx(struct i40e_pf *pf)
 	enum rte_eth_rx_mq_mode mq_mode = pf->dev_data->dev_conf.rxmode.mq_mode;
 
 	/* RSS setup */
-	if (mq_mode & ETH_MQ_RX_RSS_FLAG)
-		ret = i40e_pf_config_rss(pf);
-	else
+	if (mq_mode & ETH_MQ_RX_RSS_FLAG) {
+		if (!pf->hash.reta)
+			ret = i40e_pf_config_rss(pf);
+	} else
 		i40e_pf_disable_rss(pf);
 
 	return ret;
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index b6eed6a..d40010a 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -466,6 +466,11 @@ struct i40e_tunnel_info {
 	struct rte_hash *hash_table;
 };
 
+struct i40e_hash_info {
+	uint8_t *reta;
+	struct rte_eth_rss_conf rss_conf;
+};
+
 #define I40E_MIRROR_MAX_ENTRIES_PER_RULE   64
 #define I40E_MAX_MIRROR_RULES           64
 /*
@@ -538,6 +543,7 @@ struct i40e_pf {
 	struct i40e_fdir_info fdir; /* flow director info */
 	struct i40e_ethertype_info ethertype; /* Ethertype filter info */
 	struct i40e_tunnel_info tunnel; /* Tunnel filter info */
+	struct i40e_hash_info hash; /* Hash filter info */
 	struct i40e_fc_conf fc_conf; /* Flow control conf */
 	struct i40e_mirror_rule_list mirror_list;
 	uint16_t nb_mirror_rule;   /* The number of mirror rules */
-- 
2.5.5

^ permalink raw reply related

* [PATCH 03/24] net/i40e: store flow director filter
From: Beilei Xing @ 2016-12-02 11:53 UTC (permalink / raw)
  To: jingjing.wu, helin.zhang; +Cc: dev, wenzhuo.lu
In-Reply-To: <1480679625-4157-1-git-send-email-beilei.xing@intel.com>

Add support for storing flow director filter in SW.

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 48 +++++++++++++++++++++
 drivers/net/i40e/i40e_ethdev.h | 12 ++++++
 drivers/net/i40e/i40e_fdir.c   | 95 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 155 insertions(+)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index b20a851..c38536f 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -962,6 +962,7 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
 	uint8_t aq_fail = 0;
 	struct i40e_ethertype_info *ethertype_info = &pf->ethertype;
 	struct i40e_tunnel_info *tunnel_info = &pf->tunnel;
+	struct i40e_fdir_info *fdir_info = &pf->fdir;
 
 	PMD_INIT_FUNC_TRACE();
 
@@ -981,6 +982,14 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
 		.hash_func = rte_hash_crc,
 	};
 
+	char fdir_hash_name[RTE_HASH_NAMESIZE];
+	struct rte_hash_parameters fdir_hash_params = {
+		.name = fdir_hash_name,
+		.entries = I40E_MAX_FDIR_FILTER_NUM,
+		.key_len = sizeof(struct rte_eth_fdir_input),
+		.hash_func = rte_hash_crc,
+	};
+
 	dev->dev_ops = &i40e_eth_dev_ops;
 	dev->rx_pkt_burst = i40e_recv_pkts;
 	dev->tx_pkt_burst = i40e_xmit_pkts;
@@ -1262,8 +1271,33 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
 		goto err_tunnel_hash_map_alloc;
 	}
 
+	/* Initialize flow director filter rule list and hash */
+	TAILQ_INIT(&fdir_info->fdir_list);
+	snprintf(fdir_hash_name, RTE_HASH_NAMESIZE,
+		 "fdir_%s", dev->data->name);
+	fdir_info->hash_table = rte_hash_create(&fdir_hash_params);
+	if (!fdir_info->hash_table) {
+		PMD_INIT_LOG(ERR, "Failed to create fdir hash table!");
+		ret = -EINVAL;
+		goto err_fdir_hash_table_create;
+	}
+	fdir_info->hash_map = rte_zmalloc("i40e_fdir_hash_map",
+					  sizeof(struct i40e_fdir_filter *) *
+					  I40E_MAX_FDIR_FILTER_NUM,
+					  0);
+	if (!fdir_info->hash_map) {
+		PMD_INIT_LOG(ERR,
+			     "Failed to allocate memory for fdir hash map!");
+		ret = -ENOMEM;
+		goto err_fdir_hash_map_alloc;
+	}
+
 	return 0;
 
+err_fdir_hash_map_alloc:
+	rte_hash_free(fdir_info->hash_table);
+err_fdir_hash_table_create:
+	rte_free(tunnel_info->hash_map);
 err_tunnel_hash_map_alloc:
 	rte_hash_free(tunnel_info->hash_table);
 err_tunnel_hash_table_create:
@@ -1300,10 +1334,12 @@ eth_i40e_dev_uninit(struct rte_eth_dev *dev)
 	struct i40e_filter_control_settings settings;
 	struct i40e_ethertype_filter *p_ethertype;
 	struct i40e_tunnel_filter *p_tunnel;
+	struct i40e_fdir_filter *p_fdir;
 	int ret;
 	uint8_t aq_fail = 0;
 	struct i40e_ethertype_info *ethertype_info;
 	struct i40e_tunnel_info *tunnel_info;
+	struct i40e_fdir_info *fdir_info;
 
 	PMD_INIT_FUNC_TRACE();
 
@@ -1315,6 +1351,7 @@ eth_i40e_dev_uninit(struct rte_eth_dev *dev)
 	pci_dev = dev->pci_dev;
 	ethertype_info = &pf->ethertype;
 	tunnel_info = &pf->tunnel;
+	fdir_info = &pf->fdir;
 
 	if (hw->adapter_stopped == 0)
 		i40e_dev_close(dev);
@@ -1342,6 +1379,17 @@ eth_i40e_dev_uninit(struct rte_eth_dev *dev)
 		rte_free(p_tunnel);
 	}
 
+	/* Remove all flow director rules and hash */
+	if (fdir_info->hash_map)
+		rte_free(fdir_info->hash_map);
+	if (fdir_info->hash_table)
+		rte_hash_free(fdir_info->hash_table);
+
+	while ((p_fdir = TAILQ_FIRST(&fdir_info->fdir_list))) {
+		TAILQ_REMOVE(&fdir_info->fdir_list, p_fdir, rules);
+		rte_free(p_fdir);
+	}
+
 	dev->dev_ops = NULL;
 	dev->rx_pkt_burst = NULL;
 	dev->tx_pkt_burst = NULL;
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index 5f9cddd..b6eed6a 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -377,6 +377,14 @@ struct i40e_fdir_flex_mask {
 };
 
 #define I40E_FILTER_PCTYPE_MAX 64
+#define I40E_MAX_FDIR_FILTER_NUM (1024 * 8)
+
+struct i40e_fdir_filter {
+	TAILQ_ENTRY(i40e_fdir_filter) rules;
+	struct rte_eth_fdir_filter fdir;
+};
+
+TAILQ_HEAD(i40e_fdir_filter_list, i40e_fdir_filter);
 /*
  *  A structure used to define fields of a FDIR related info.
  */
@@ -395,6 +403,10 @@ struct i40e_fdir_info {
 	 */
 	struct i40e_fdir_flex_pit flex_set[I40E_MAX_FLXPLD_LAYER * I40E_MAX_FLXPLD_FIED];
 	struct i40e_fdir_flex_mask flex_mask[I40E_FILTER_PCTYPE_MAX];
+
+	struct i40e_fdir_filter_list fdir_list;
+	struct i40e_fdir_filter **hash_map;
+	struct rte_hash *hash_table;
 };
 
 #define I40E_MAX_ETHERTYPE_FILTER_NUM 768
diff --git a/drivers/net/i40e/i40e_fdir.c b/drivers/net/i40e/i40e_fdir.c
index 335bf15..1913fe1 100644
--- a/drivers/net/i40e/i40e_fdir.c
+++ b/drivers/net/i40e/i40e_fdir.c
@@ -121,6 +121,16 @@ static int i40e_fdir_filter_programming(struct i40e_pf *pf,
 			bool add);
 static int i40e_fdir_flush(struct rte_eth_dev *dev);
 
+static int i40e_fdir_filter_convert(const struct rte_eth_fdir_filter *input,
+			 struct i40e_fdir_filter *filter);
+static struct i40e_fdir_filter *
+i40e_sw_fdir_filter_lookup(struct i40e_fdir_info *fdir_info,
+			const struct rte_eth_fdir_input *input);
+static int i40e_sw_fdir_filter_insert(struct i40e_pf *pf,
+				   struct i40e_fdir_filter *filter);
+static int i40e_sw_fdir_filter_del(struct i40e_pf *pf,
+				struct i40e_fdir_filter *filter);
+
 static int
 i40e_fdir_rx_queue_init(struct i40e_rx_queue *rxq)
 {
@@ -1017,6 +1027,66 @@ i40e_check_fdir_programming_status(struct i40e_rx_queue *rxq)
 	return ret;
 }
 
+static int
+i40e_fdir_filter_convert(const struct rte_eth_fdir_filter *input,
+			 struct i40e_fdir_filter *filter)
+{
+	rte_memcpy(&filter->fdir, input, sizeof(struct rte_eth_fdir_filter));
+	return 0;
+}
+
+static struct i40e_fdir_filter *
+i40e_sw_fdir_filter_lookup(struct i40e_fdir_info *fdir_info,
+			const struct rte_eth_fdir_input *input)
+{
+	int ret = 0;
+
+	ret = rte_hash_lookup(fdir_info->hash_table, (const void *)input);
+	if (ret < 0)
+		return NULL;
+
+	return fdir_info->hash_map[ret];
+}
+
+static int
+i40e_sw_fdir_filter_insert(struct i40e_pf *pf, struct i40e_fdir_filter *filter)
+{
+	struct i40e_fdir_info *fdir_info = &pf->fdir;
+	int ret = 0;
+
+	ret = rte_hash_add_key(fdir_info->hash_table,
+			       &filter->fdir.input);
+	if (ret < 0)
+		PMD_DRV_LOG(ERR,
+			    "Failed to insert fdir filter to hash table %d!",
+			    ret);
+	fdir_info->hash_map[ret] = filter;
+
+	TAILQ_INSERT_TAIL(&fdir_info->fdir_list, filter, rules);
+
+	return 0;
+}
+
+static int
+i40e_sw_fdir_filter_del(struct i40e_pf *pf, struct i40e_fdir_filter *filter)
+{
+	struct i40e_fdir_info *fdir_info = &pf->fdir;
+	int ret = 0;
+
+	ret = rte_hash_del_key(fdir_info->hash_table,
+			       &filter->fdir.input);
+	if (ret < 0)
+		PMD_DRV_LOG(ERR,
+			    "Failed to delete fdir filter to hash table %d!",
+			    ret);
+	fdir_info->hash_map[ret] = NULL;
+
+	TAILQ_REMOVE(&fdir_info->fdir_list, filter, rules);
+	rte_free(filter);
+
+	return 0;
+}
+
 /*
  * i40e_add_del_fdir_filter - add or remove a flow director filter.
  * @pf: board private structure
@@ -1032,6 +1102,8 @@ i40e_add_del_fdir_filter(struct rte_eth_dev *dev,
 	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
 	unsigned char *pkt = (unsigned char *)pf->fdir.prg_pkt;
 	enum i40e_filter_pctype pctype;
+	struct i40e_fdir_info *fdir_info = &pf->fdir;
+	struct i40e_fdir_filter *fdir_filter, *node;
 	int ret = 0;
 
 	if (dev->data->dev_conf.fdir_conf.mode != RTE_FDIR_MODE_PERFECT) {
@@ -1054,6 +1126,21 @@ i40e_add_del_fdir_filter(struct rte_eth_dev *dev,
 		return -EINVAL;
 	}
 
+	fdir_filter = rte_zmalloc("fdir_filter", sizeof(*fdir_filter), 0);
+	i40e_fdir_filter_convert(filter, fdir_filter);
+	node = i40e_sw_fdir_filter_lookup(fdir_info, &fdir_filter->fdir.input);
+	if (add && node) {
+		PMD_DRV_LOG(ERR,
+			    "Conflict with existing flow director rules!");
+		rte_free(fdir_filter);
+		return -EINVAL;
+	} else if (!add && !node) {
+		PMD_DRV_LOG(ERR,
+			    "There's no corresponding flow firector filter!");
+		rte_free(fdir_filter);
+		return -EINVAL;
+	}
+
 	memset(pkt, 0, I40E_FDIR_PKT_LEN);
 
 	ret = i40e_fdir_construct_pkt(pf, &filter->input, pkt);
@@ -1077,6 +1164,14 @@ i40e_add_del_fdir_filter(struct rte_eth_dev *dev,
 			    pctype);
 		return ret;
 	}
+
+	if (add)
+		ret = i40e_sw_fdir_filter_insert(pf, fdir_filter);
+	else {
+		ret = i40e_sw_fdir_filter_del(pf, node);
+		rte_free(fdir_filter);
+	}
+
 	return ret;
 }
 
-- 
2.5.5

^ permalink raw reply related

* [PATCH 02/24] net/i40e: store tunnel filter
From: Beilei Xing @ 2016-12-02 11:53 UTC (permalink / raw)
  To: jingjing.wu, helin.zhang; +Cc: dev, wenzhuo.lu
In-Reply-To: <1480679625-4157-1-git-send-email-beilei.xing@intel.com>

Add support of storing tunnel filter in SW.

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 161 ++++++++++++++++++++++++++++++++++++++++-
 drivers/net/i40e/i40e_ethdev.h |  35 +++++++++
 2 files changed, 193 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 30822a0..b20a851 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -473,6 +473,17 @@ static int i40e_sw_ethertype_filter_insert(struct i40e_pf *pf,
 static int i40e_sw_ethertype_filter_del(struct i40e_pf *pf,
 					struct i40e_ethertype_filter *filter);
 
+static int i40e_tunnel_filter_convert(
+	struct i40e_aqc_add_remove_cloud_filters_element_data *cld_filter,
+	struct i40e_tunnel_filter *tunnel_filter);
+static struct i40e_tunnel_filter *
+i40e_sw_tunnel_filter_lookup(struct i40e_tunnel_info *tunnel_info,
+			     const struct i40e_tunnel_filter_input *input);
+static int i40e_sw_tunnel_filter_insert(struct i40e_pf *pf,
+				struct i40e_tunnel_filter *tunnel_filter);
+static int i40e_sw_tunnel_filter_del(struct i40e_pf *pf,
+			     struct i40e_tunnel_filter *tunnel_filter);
+
 static const struct rte_pci_id pci_id_i40e_map[] = {
 	{ RTE_PCI_DEVICE(I40E_INTEL_VENDOR_ID, I40E_DEV_ID_SFP_XL710) },
 	{ RTE_PCI_DEVICE(I40E_INTEL_VENDOR_ID, I40E_DEV_ID_QEMU) },
@@ -950,6 +961,7 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
 	uint32_t len;
 	uint8_t aq_fail = 0;
 	struct i40e_ethertype_info *ethertype_info = &pf->ethertype;
+	struct i40e_tunnel_info *tunnel_info = &pf->tunnel;
 
 	PMD_INIT_FUNC_TRACE();
 
@@ -961,6 +973,14 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
 		.hash_func = rte_hash_crc,
 	};
 
+	char tunnel_hash_name[RTE_HASH_NAMESIZE];
+	struct rte_hash_parameters tunnel_hash_params = {
+		.name = tunnel_hash_name,
+		.entries = I40E_MAX_TUNNEL_FILTER_NUM,
+		.key_len = sizeof(struct i40e_tunnel_filter_input),
+		.hash_func = rte_hash_crc,
+	};
+
 	dev->dev_ops = &i40e_eth_dev_ops;
 	dev->rx_pkt_burst = i40e_recv_pkts;
 	dev->tx_pkt_burst = i40e_xmit_pkts;
@@ -1221,8 +1241,33 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
 		goto err_ethertype_hash_map_alloc;
 	}
 
+	/* Initialize tunnel filter rule list and hash */
+	TAILQ_INIT(&tunnel_info->tunnel_list);
+	snprintf(tunnel_hash_name, RTE_HASH_NAMESIZE,
+		 "tunnel_%s", dev->data->name);
+	tunnel_info->hash_table = rte_hash_create(&tunnel_hash_params);
+	if (!tunnel_info->hash_table) {
+		PMD_INIT_LOG(ERR, "Failed to create tunnel hash table!");
+		ret = -EINVAL;
+		goto err_tunnel_hash_table_create;
+	}
+	tunnel_info->hash_map = rte_zmalloc("i40e_tunnel_hash_map",
+				    sizeof(struct i40e_tunnel_filter *) *
+				    I40E_MAX_TUNNEL_FILTER_NUM,
+				    0);
+	if (!tunnel_info->hash_map) {
+		PMD_INIT_LOG(ERR,
+			     "Failed to allocate memory for tunnel hash map!");
+		ret = -ENOMEM;
+		goto err_tunnel_hash_map_alloc;
+	}
+
 	return 0;
 
+err_tunnel_hash_map_alloc:
+	rte_hash_free(tunnel_info->hash_table);
+err_tunnel_hash_table_create:
+	rte_free(ethertype_info->hash_map);
 err_ethertype_hash_map_alloc:
 	rte_hash_free(ethertype_info->hash_table);
 err_ethertype_hash_table_create:
@@ -1254,9 +1299,11 @@ eth_i40e_dev_uninit(struct rte_eth_dev *dev)
 	struct i40e_hw *hw;
 	struct i40e_filter_control_settings settings;
 	struct i40e_ethertype_filter *p_ethertype;
+	struct i40e_tunnel_filter *p_tunnel;
 	int ret;
 	uint8_t aq_fail = 0;
 	struct i40e_ethertype_info *ethertype_info;
+	struct i40e_tunnel_info *tunnel_info;
 
 	PMD_INIT_FUNC_TRACE();
 
@@ -1267,6 +1314,7 @@ eth_i40e_dev_uninit(struct rte_eth_dev *dev)
 	hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	pci_dev = dev->pci_dev;
 	ethertype_info = &pf->ethertype;
+	tunnel_info = &pf->tunnel;
 
 	if (hw->adapter_stopped == 0)
 		i40e_dev_close(dev);
@@ -1283,6 +1331,17 @@ eth_i40e_dev_uninit(struct rte_eth_dev *dev)
 		rte_free(p_ethertype);
 	}
 
+	/* Remove all tunnel director rules and hash */
+	if (tunnel_info->hash_map)
+		rte_free(tunnel_info->hash_map);
+	if (tunnel_info->hash_table)
+		rte_hash_free(tunnel_info->hash_table);
+
+	while ((p_tunnel = TAILQ_FIRST(&tunnel_info->tunnel_list))) {
+		TAILQ_REMOVE(&tunnel_info->tunnel_list, p_tunnel, rules);
+		rte_free(p_tunnel);
+	}
+
 	dev->dev_ops = NULL;
 	dev->rx_pkt_burst = NULL;
 	dev->tx_pkt_burst = NULL;
@@ -6491,6 +6550,79 @@ i40e_dev_get_filter_type(uint16_t filter_type, uint16_t *flag)
 }
 
 static int
+i40e_tunnel_filter_convert(struct i40e_aqc_add_remove_cloud_filters_element_data
+			   *cld_filter,
+			   struct i40e_tunnel_filter *tunnel_filter)
+{
+	ether_addr_copy((struct ether_addr *)&cld_filter->outer_mac,
+			(struct ether_addr *)&tunnel_filter->input.outer_mac);
+	ether_addr_copy((struct ether_addr *)&cld_filter->inner_mac,
+			(struct ether_addr *)&tunnel_filter->input.inner_mac);
+	tunnel_filter->input.inner_vlan = cld_filter->inner_vlan;
+	tunnel_filter->input.flags = cld_filter->flags;
+	tunnel_filter->input.tenant_id = cld_filter->tenant_id;
+	rte_memcpy(&tunnel_filter->input.ipaddr, &cld_filter->ipaddr,
+		   sizeof(tunnel_filter->input.ipaddr));
+	tunnel_filter->queue = cld_filter->queue_number;
+
+	return 0;
+}
+
+static struct i40e_tunnel_filter *
+i40e_sw_tunnel_filter_lookup(struct i40e_tunnel_info *tunnel_info,
+			     const struct i40e_tunnel_filter_input *input)
+{
+	int ret = 0;
+
+	ret = rte_hash_lookup(tunnel_info->hash_table, (const void *)input);
+	if (ret < 0)
+		return NULL;
+
+	return tunnel_info->hash_map[ret];
+}
+
+static int
+i40e_sw_tunnel_filter_insert(struct i40e_pf *pf,
+			     struct i40e_tunnel_filter *tunnel_filter)
+{
+	struct i40e_tunnel_info *tunnel_info = &pf->tunnel;
+	int ret = 0;
+
+	ret = rte_hash_add_key(tunnel_info->hash_table,
+			       &tunnel_filter->input);
+	if (ret < 0)
+		PMD_DRV_LOG(ERR,
+			    "Failed to insert tunnel filter to hash table %d!",
+			    ret);
+	tunnel_info->hash_map[ret] = tunnel_filter;
+
+	TAILQ_INSERT_TAIL(&tunnel_info->tunnel_list, tunnel_filter, rules);
+
+	return 0;
+}
+
+static int
+i40e_sw_tunnel_filter_del(struct i40e_pf *pf,
+			  struct i40e_tunnel_filter *tunnel_filter)
+{
+	struct i40e_tunnel_info *tunnel_info = &pf->tunnel;
+	int ret = 0;
+
+	ret = rte_hash_del_key(tunnel_info->hash_table,
+			       &tunnel_filter->input);
+	if (ret < 0)
+		PMD_DRV_LOG(ERR,
+			    "Failed to delete tunnel filter to hash table %d!",
+			    ret);
+	tunnel_info->hash_map[ret] = NULL;
+
+	TAILQ_REMOVE(&tunnel_info->tunnel_list, tunnel_filter, rules);
+	rte_free(tunnel_filter);
+
+	return 0;
+}
+
+static int
 i40e_dev_tunnel_filter_set(struct i40e_pf *pf,
 			struct rte_eth_tunnel_filter_conf *tunnel_filter,
 			uint8_t add)
@@ -6505,6 +6637,8 @@ i40e_dev_tunnel_filter_set(struct i40e_pf *pf,
 	struct i40e_vsi *vsi = pf->main_vsi;
 	struct i40e_aqc_add_remove_cloud_filters_element_data  *cld_filter;
 	struct i40e_aqc_add_remove_cloud_filters_element_data  *pfilter;
+	struct i40e_tunnel_info *tunnel_info = &pf->tunnel;
+	struct i40e_tunnel_filter *tunnel, *node;
 
 	cld_filter = rte_zmalloc("tunnel_filter",
 		sizeof(struct i40e_aqc_add_remove_cloud_filters_element_data),
@@ -6567,11 +6701,32 @@ i40e_dev_tunnel_filter_set(struct i40e_pf *pf,
 	pfilter->tenant_id = rte_cpu_to_le_32(tunnel_filter->tenant_id);
 	pfilter->queue_number = rte_cpu_to_le_16(tunnel_filter->queue_id);
 
-	if (add)
+	tunnel = rte_zmalloc("tunnel_filter", sizeof(*tunnel), 0);
+	i40e_tunnel_filter_convert(cld_filter, tunnel);
+	node = i40e_sw_tunnel_filter_lookup(tunnel_info, &tunnel->input);
+	if (add && node) {
+		PMD_DRV_LOG(ERR, "Conflict with existing tunnel rules!");
+		rte_free(tunnel);
+		return -EINVAL;
+	} else if (!add && !node) {
+		PMD_DRV_LOG(ERR, "There's no corresponding tunnel filter!");
+		rte_free(tunnel);
+		return -EINVAL;
+	}
+
+	if (add) {
 		ret = i40e_aq_add_cloud_filters(hw, vsi->seid, cld_filter, 1);
-	else
+		if (ret < 0)
+			return ret;
+		ret = i40e_sw_tunnel_filter_insert(pf, tunnel);
+	} else {
 		ret = i40e_aq_remove_cloud_filters(hw, vsi->seid,
-						cld_filter, 1);
+						   cld_filter, 1);
+		if (ret < 0)
+			return ret;
+		ret = i40e_sw_tunnel_filter_del(pf, node);
+		rte_free(tunnel);
+	}
 
 	rte_free(cld_filter);
 	return ret;
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index 8604198..5f9cddd 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -420,6 +420,40 @@ struct i40e_ethertype_info {
 	struct rte_hash *hash_table;
 };
 
+#define I40E_MAX_TUNNEL_FILTER_NUM 400
+
+/* Tunnel filter struct */
+struct i40e_tunnel_filter_input {
+	u8      outer_mac[6];
+	u8      inner_mac[6];
+	__le16  inner_vlan;
+	union {
+		struct {
+			u8 reserved[12];
+			u8 data[4];
+		} v4;
+		struct {
+			u8 data[16];
+		} v6;
+	} ipaddr;
+	__le16  flags;
+	__le32  tenant_id;
+};
+
+struct i40e_tunnel_filter {
+	TAILQ_ENTRY(i40e_tunnel_filter) rules;
+	struct i40e_tunnel_filter_input input;
+	uint16_t queue;
+};
+
+TAILQ_HEAD(i40e_tunnel_filter_list, i40e_tunnel_filter);
+
+struct i40e_tunnel_info {
+	struct i40e_tunnel_filter_list tunnel_list;
+	struct i40e_tunnel_filter  **hash_map;
+	struct rte_hash *hash_table;
+};
+
 #define I40E_MIRROR_MAX_ENTRIES_PER_RULE   64
 #define I40E_MAX_MIRROR_RULES           64
 /*
@@ -491,6 +525,7 @@ struct i40e_pf {
 
 	struct i40e_fdir_info fdir; /* flow director info */
 	struct i40e_ethertype_info ethertype; /* Ethertype filter info */
+	struct i40e_tunnel_info tunnel; /* Tunnel filter info */
 	struct i40e_fc_conf fc_conf; /* Flow control conf */
 	struct i40e_mirror_rule_list mirror_list;
 	uint16_t nb_mirror_rule;   /* The number of mirror rules */
-- 
2.5.5

^ permalink raw reply related

* [PATCH 01/24] net/i40e: store ethertype filter
From: Beilei Xing @ 2016-12-02 11:53 UTC (permalink / raw)
  To: jingjing.wu, helin.zhang; +Cc: dev, wenzhuo.lu
In-Reply-To: <1480679625-4157-1-git-send-email-beilei.xing@intel.com>

Add support of storing ethertype filter in SW.

Signed-off-by: Beilei Xing <beilei.xing@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 160 ++++++++++++++++++++++++++++++++++++++++-
 drivers/net/i40e/i40e_ethdev.h |  25 +++++++
 2 files changed, 184 insertions(+), 1 deletion(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 67778ba..30822a0 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -51,6 +51,7 @@
 #include <rte_dev.h>
 #include <rte_eth_ctrl.h>
 #include <rte_tailq.h>
+#include <rte_hash_crc.h>
 
 #include "i40e_logs.h"
 #include "base/i40e_prototype.h"
@@ -461,6 +462,17 @@ static void i40e_set_default_mac_addr(struct rte_eth_dev *dev,
 
 static int i40e_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);
 
+static int i40e_ethertype_filter_convert(
+	const struct rte_eth_ethertype_filter *input,
+	struct i40e_ethertype_filter *filter);
+static struct i40e_ethertype_filter *
+i40e_sw_ethertype_filter_lookup(struct i40e_ethertype_info *ethertype_info,
+			const struct i40e_ethertype_filter_input *input);
+static int i40e_sw_ethertype_filter_insert(struct i40e_pf *pf,
+				   struct i40e_ethertype_filter *filter);
+static int i40e_sw_ethertype_filter_del(struct i40e_pf *pf,
+					struct i40e_ethertype_filter *filter);
+
 static const struct rte_pci_id pci_id_i40e_map[] = {
 	{ RTE_PCI_DEVICE(I40E_INTEL_VENDOR_ID, I40E_DEV_ID_SFP_XL710) },
 	{ RTE_PCI_DEVICE(I40E_INTEL_VENDOR_ID, I40E_DEV_ID_QEMU) },
@@ -937,9 +949,18 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
 	int ret;
 	uint32_t len;
 	uint8_t aq_fail = 0;
+	struct i40e_ethertype_info *ethertype_info = &pf->ethertype;
 
 	PMD_INIT_FUNC_TRACE();
 
+	char ethertype_hash_name[RTE_HASH_NAMESIZE];
+	struct rte_hash_parameters ethertype_hash_params = {
+		.name = ethertype_hash_name,
+		.entries = I40E_MAX_ETHERTYPE_FILTER_NUM,
+		.key_len = sizeof(struct i40e_ethertype_filter_input),
+		.hash_func = rte_hash_crc,
+	};
+
 	dev->dev_ops = &i40e_eth_dev_ops;
 	dev->rx_pkt_burst = i40e_recv_pkts;
 	dev->tx_pkt_burst = i40e_xmit_pkts;
@@ -1179,8 +1200,33 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
 		pf->flags &= ~I40E_FLAG_DCB;
 	}
 
+	/* Initialize ethertype filter rule list and hash */
+	TAILQ_INIT(&ethertype_info->ethertype_list);
+	snprintf(ethertype_hash_name, RTE_HASH_NAMESIZE,
+		 "ethertype_%s", dev->data->name);
+	ethertype_info->hash_table = rte_hash_create(&ethertype_hash_params);
+	if (!ethertype_info->hash_table) {
+		PMD_INIT_LOG(ERR, "Failed to create ethertype hash table!");
+		ret = -EINVAL;
+		goto err_ethertype_hash_table_create;
+	}
+	ethertype_info->hash_map = rte_zmalloc("i40e_ethertype_hash_map",
+				       sizeof(struct i40e_ethertype_filter *) *
+				       I40E_MAX_ETHERTYPE_FILTER_NUM,
+				       0);
+	if (!ethertype_info->hash_map) {
+		PMD_INIT_LOG(ERR,
+		     "Failed to allocate memory for ethertype hash map!");
+		ret = -ENOMEM;
+		goto err_ethertype_hash_map_alloc;
+	}
+
 	return 0;
 
+err_ethertype_hash_map_alloc:
+	rte_hash_free(ethertype_info->hash_table);
+err_ethertype_hash_table_create:
+	rte_free(dev->data->mac_addrs);
 err_mac_alloc:
 	i40e_vsi_release(pf->main_vsi);
 err_setup_pf_switch:
@@ -1203,23 +1249,40 @@ eth_i40e_dev_init(struct rte_eth_dev *dev)
 static int
 eth_i40e_dev_uninit(struct rte_eth_dev *dev)
 {
+	struct i40e_pf *pf;
 	struct rte_pci_device *pci_dev;
 	struct i40e_hw *hw;
 	struct i40e_filter_control_settings settings;
+	struct i40e_ethertype_filter *p_ethertype;
 	int ret;
 	uint8_t aq_fail = 0;
+	struct i40e_ethertype_info *ethertype_info;
 
 	PMD_INIT_FUNC_TRACE();
 
 	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
 		return 0;
 
+	pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);
 	hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	pci_dev = dev->pci_dev;
+	ethertype_info = &pf->ethertype;
 
 	if (hw->adapter_stopped == 0)
 		i40e_dev_close(dev);
 
+	/* Remove all ethertype director rules and hash */
+	if (ethertype_info->hash_map)
+		rte_free(ethertype_info->hash_map);
+	if (ethertype_info->hash_table)
+		rte_hash_free(ethertype_info->hash_table);
+
+	while ((p_ethertype = TAILQ_FIRST(&ethertype_info->ethertype_list))) {
+		TAILQ_REMOVE(&ethertype_info->ethertype_list,
+			     p_ethertype, rules);
+		rte_free(p_ethertype);
+	}
+
 	dev->dev_ops = NULL;
 	dev->rx_pkt_burst = NULL;
 	dev->tx_pkt_burst = NULL;
@@ -7986,6 +8049,74 @@ i40e_hash_filter_ctrl(struct rte_eth_dev *dev,
 	return ret;
 }
 
+static int
+i40e_ethertype_filter_convert(const struct rte_eth_ethertype_filter *input,
+			      struct i40e_ethertype_filter *filter)
+{
+	rte_memcpy(&filter->input.mac_addr, &input->mac_addr, ETHER_ADDR_LEN);
+	filter->input.ether_type = input->ether_type;
+	filter->flags = input->flags;
+	filter->queue = input->queue;
+
+	return 0;
+}
+
+static struct i40e_ethertype_filter *
+i40e_sw_ethertype_filter_lookup(struct i40e_ethertype_info *ethertype_info,
+				const struct i40e_ethertype_filter_input *input)
+{
+	int ret = 0;
+
+	ret = rte_hash_lookup(ethertype_info->hash_table, (const void *)input);
+	if (ret < 0)
+		return NULL;
+
+	return ethertype_info->hash_map[ret];
+}
+
+static int
+i40e_sw_ethertype_filter_insert(struct i40e_pf *pf,
+				struct i40e_ethertype_filter *filter)
+{
+	struct i40e_ethertype_info *ethertype_info = &pf->ethertype;
+	int ret = 0;
+
+	ret = rte_hash_add_key(ethertype_info->hash_table,
+			       &filter->input);
+	if (ret < 0)
+		PMD_DRV_LOG(ERR,
+			    "Failed to insert ethertype filter"
+			    " to hash table %d!",
+			    ret);
+	ethertype_info->hash_map[ret] = filter;
+
+	TAILQ_INSERT_TAIL(&ethertype_info->ethertype_list, filter, rules);
+
+	return 0;
+}
+
+static int
+i40e_sw_ethertype_filter_del(struct i40e_pf *pf,
+			     struct i40e_ethertype_filter *filter)
+{
+	struct i40e_ethertype_info *ethertype_info = &pf->ethertype;
+	int ret = 0;
+
+	ret = rte_hash_del_key(ethertype_info->hash_table,
+			       &filter->input);
+	if (ret < 0)
+		PMD_DRV_LOG(ERR,
+			    "Failed to delete ethertype filter"
+			    " to hash table %d!",
+			    ret);
+	ethertype_info->hash_map[ret] = NULL;
+
+	TAILQ_REMOVE(&ethertype_info->ethertype_list, filter, rules);
+	rte_free(filter);
+
+	return 0;
+}
+
 /*
  * Configure ethertype filter, which can director packet by filtering
  * with mac address and ether_type or only ether_type
@@ -7996,6 +8127,8 @@ i40e_ethertype_filter_set(struct i40e_pf *pf,
 			bool add)
 {
 	struct i40e_hw *hw = I40E_PF_TO_HW(pf);
+	struct i40e_ethertype_info *ethertype_info = &pf->ethertype;
+	struct i40e_ethertype_filter *ethertype_filter, *node;
 	struct i40e_control_filter_stats stats;
 	uint16_t flags = 0;
 	int ret;
@@ -8014,6 +8147,22 @@ i40e_ethertype_filter_set(struct i40e_pf *pf,
 		PMD_DRV_LOG(WARNING, "filter vlan ether_type in first tag is"
 			" not supported.");
 
+	/* Check if there is the filter in SW list */
+	ethertype_filter = rte_zmalloc("ethertype_filter",
+				       sizeof(*ethertype_filter), 0);
+	i40e_ethertype_filter_convert(filter, ethertype_filter);
+	node = i40e_sw_ethertype_filter_lookup(ethertype_info,
+					       &ethertype_filter->input);
+	if (add && node) {
+		PMD_DRV_LOG(ERR, "Conflict with existing ethertype rules!");
+		rte_free(ethertype_filter);
+		return -EINVAL;
+	} else if (!add && !node) {
+		PMD_DRV_LOG(ERR, "There's no corresponding ethertype filter!");
+		rte_free(ethertype_filter);
+		return -EINVAL;
+	}
+
 	if (!(filter->flags & RTE_ETHTYPE_FLAGS_MAC))
 		flags |= I40E_AQC_ADD_CONTROL_PACKET_FLAGS_IGNORE_MAC;
 	if (filter->flags & RTE_ETHTYPE_FLAGS_DROP)
@@ -8034,7 +8183,16 @@ i40e_ethertype_filter_set(struct i40e_pf *pf,
 			 stats.mac_etype_free, stats.etype_free);
 	if (ret < 0)
 		return -ENOSYS;
-	return 0;
+
+	/* Add or delete a filter in SW list */
+	if (add)
+		ret = i40e_sw_ethertype_filter_insert(pf, ethertype_filter);
+	else {
+		ret = i40e_sw_ethertype_filter_del(pf, node);
+		rte_free(ethertype_filter);
+	}
+
+	return ret;
 }
 
 /*
diff --git a/drivers/net/i40e/i40e_ethdev.h b/drivers/net/i40e/i40e_ethdev.h
index 298cef4..8604198 100644
--- a/drivers/net/i40e/i40e_ethdev.h
+++ b/drivers/net/i40e/i40e_ethdev.h
@@ -37,6 +37,7 @@
 #include <rte_eth_ctrl.h>
 #include <rte_time.h>
 #include <rte_kvargs.h>
+#include <rte_hash.h>
 
 #define I40E_VLAN_TAG_SIZE        4
 
@@ -396,6 +397,29 @@ struct i40e_fdir_info {
 	struct i40e_fdir_flex_mask flex_mask[I40E_FILTER_PCTYPE_MAX];
 };
 
+#define I40E_MAX_ETHERTYPE_FILTER_NUM 768
+
+/* Ethertype filter struct */
+struct i40e_ethertype_filter_input {
+	struct ether_addr mac_addr;   /**< Mac address to match. */
+	uint16_t ether_type;          /**< Ether type to match */
+};
+
+struct i40e_ethertype_filter {
+	TAILQ_ENTRY(i40e_ethertype_filter) rules;
+	struct i40e_ethertype_filter_input input;
+	uint16_t flags;
+	uint16_t queue;
+};
+
+TAILQ_HEAD(i40e_ethertype_filter_list, i40e_ethertype_filter);
+
+struct i40e_ethertype_info {
+	struct i40e_ethertype_filter_list ethertype_list;
+	struct i40e_ethertype_filter  **hash_map;
+	struct rte_hash *hash_table;
+};
+
 #define I40E_MIRROR_MAX_ENTRIES_PER_RULE   64
 #define I40E_MAX_MIRROR_RULES           64
 /*
@@ -466,6 +490,7 @@ struct i40e_pf {
 	struct i40e_vmdq_info *vmdq;
 
 	struct i40e_fdir_info fdir; /* flow director info */
+	struct i40e_ethertype_info ethertype; /* Ethertype filter info */
 	struct i40e_fc_conf fc_conf; /* Flow control conf */
 	struct i40e_mirror_rule_list mirror_list;
 	uint16_t nb_mirror_rule;   /* The number of mirror rules */
-- 
2.5.5

^ permalink raw reply related

* [PATCH 00/24] net/i40e: Consistent filter API
From: Beilei Xing @ 2016-12-02 11:53 UTC (permalink / raw)
  To: jingjing.wu, helin.zhang; +Cc: dev, wenzhuo.lu

All pathes depend on Adrien's Generic flow API.

The patches mainly finish following functions:
1) Store and restore all kinds of filters.
2) Parse all kinds of filters.
3) Add flow validate function.
4) Add flow create function.
5) Add flow destroy function.
6) Add flow flush function.

Beilei Xing (24):
  net/i40e: store ethertype filter
  net/i40e: store tunnel filter
  net/i40e: store flow director filter
  net/i40e: store RSS hash info
  net/i40e: restore ethertype filter
  net/i40e: restore macvlan filter
  net/i40e: restore tunnel filter
  net/i40e: restore flow director filter
  net/i40e: restore RSS hash info
  ethdev: parse ethertype filter
  net/i40e: add flow validate function
  net/i40e: parse macvlan filter
  net/i40e: parse VXLAN filter
  net/i40e: parse NVGRE filter
  net/i40e: parse flow director filter
  net/i40e: add flow create function
  net/i40e: destroy ethertype filter
  net/i40e: destroy macvlan filter
  net/i40e: destroy tunnel filter
  net/i40e: destroy flow directory filter
  net/i40e: add flow flush function
  net/i40e: flush ethertype filters
  net/i40e: flush macvlan filters
  net/i40e: flush tunnel filters

 drivers/net/i40e/i40e_ethdev.c     | 2180 +++++++++++++++++++++++++++++++++++-
 drivers/net/i40e/i40e_ethdev.h     |   83 ++
 drivers/net/i40e/i40e_fdir.c       |  111 +-
 lib/librte_ether/rte_flow.c        |  136 +++
 lib/librte_ether/rte_flow.h        |   23 +
 lib/librte_ether/rte_flow_driver.h |   34 +
 6 files changed, 2555 insertions(+), 12 deletions(-)

-- 
2.5.5

^ permalink raw reply

* Re: [RFC PATCH] i40e: fix setting of default MAC address
From: Wu, Jingjing @ 2016-12-02  3:10 UTC (permalink / raw)
  To: Igor Ryzhov, dev@dpdk.org; +Cc: Zhang, Helin
In-Reply-To: <1479990879-26598-1-git-send-email-iryzhov@nfware.com>

Hi, Igor

Thanks for your contribute, my comments are in below, thanks.

> -----Original Message-----
> From: Igor Ryzhov [mailto:iryzhov@nfware.com]
> Sent: Thursday, November 24, 2016 8:35 PM
> To: dev@dpdk.org
> Cc: Zhang, Helin <helin.zhang@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>
> Subject: [RFC PATCH] i40e: fix setting of default MAC address
> 
> While testing X710 cards in our lab I found that setting of default MAC
> address doesn't work correctly for i40e driver. I compared DPDK driver
> implementation with Linux driver implementation and found that a lot of
> code is lost in DPDK.
> I tried to make DPDK implementation similar to Linux implementation and it
> worked for me – now everything is working. But I'm not sure that my
> changes are correct so, please, maintainers, check the patch very careful.
> 
> Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
> ---
>  drivers/net/i40e/i40e_ethdev.c | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 67778ba..b73f9c8 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -9694,6 +9694,7 @@ static int i40e_get_eeprom(struct rte_eth_dev
> *dev,  static void i40e_set_default_mac_addr(struct rte_eth_dev *dev,
>  				      struct ether_addr *mac_addr)
>  {
> +	struct i40e_vsi *vsi =
> +I40E_DEV_PRIVATE_TO_MAIN_VSI(dev->data->dev_private);
>  	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
> 
>  	if (!is_valid_assigned_ether_addr(mac_addr)) { @@ -9701,8 +9702,33
> @@ static void i40e_set_default_mac_addr(struct rte_eth_dev *dev,
>  		return;
>  	}
> 
> -	/* Flags: 0x3 updates port address */
> -	i40e_aq_mac_address_write(hw, 0x3, mac_addr->addr_bytes,
> NULL);
> +	i40e_aq_mac_address_write(hw, I40E_AQC_WRITE_TYPE_LAA_WOL,
> +mac_addr->addr_bytes, NULL);
> +
> +	if (!memcmp(&dev->data->mac_addrs[0].addr_bytes, hw-
> >mac.addr, ETH_ADDR_LEN)) {
> +		struct i40e_aqc_remove_macvlan_element_data element;
> +
> +		memset(&element, 0, sizeof(element));
> +		memcpy(element.mac_addr, &dev->data-
> >mac_addrs[0].addr_bytes, ETH_ADDR_LEN);
> +		element.flags = I40E_AQC_MACVLAN_DEL_PERFECT_MATCH;
> +		i40e_aq_remove_macvlan(hw, vsi->seid, &element, 1, NULL);
> +	} else {
> +		i40e_vsi_delete_mac(vsi, &dev->data->mac_addrs[0]);
> +	}
> +
> +	if (!memcmp(mac_addr->addr_bytes, hw->mac.addr,
> ETH_ADDR_LEN)) {

In rte_eth_dev_default_mac_addr_set, before call dev_ops->mac_addr_set,
The dev->data->mac_addrs[0] is already set to the input addr.

So the if loop is the same as above if, why not merge them? 


If you look the code in eth_i40e_dev_init, you will found the dev->data->mac_addrs[0]
Is just the hw->mac.addr. And I think we need to make them same all the time.
So hw->mac.addr may also need to be updated.


Thanks for figure it out!

/Jingjing

^ permalink raw reply

* [PATCH v2] app/testpmd: unify help strings
From: Ferruh Yigit @ 2016-12-02  1:58 UTC (permalink / raw)
  To: dev; +Cc: Pablo de Lara, Jingjing Wu
In-Reply-To: <20161202013727.2117-1-ferruh.yigit@intel.com>

Formatted as:
cmd fixed_string fixed|string|options <variable>: Description

If there is no description, final colon emitted.

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---

v2:
* checkpatch warnings fixed
---
 app/test-pmd/cmdline.c | 486 +++++++++++++++++++++++++++----------------------
 1 file changed, 267 insertions(+), 219 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 63b55dc..39fe2f5 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -127,7 +127,7 @@ cmdline_parse_token_string_t cmd_help_brief_help =
 cmdline_parse_inst_t cmd_help_brief = {
 	.f = cmd_help_brief_parsed,
 	.data = NULL,
-	.help_str = "show help",
+	.help_str = "help: Show help",
 	.tokens = {
 		(void *)&cmd_help_brief_help,
 		NULL,
@@ -824,7 +824,8 @@ cmdline_parse_token_string_t cmd_help_long_section =
 cmdline_parse_inst_t cmd_help_long = {
 	.f = cmd_help_long_parsed,
 	.data = NULL,
-	.help_str = "show help",
+	.help_str = "help all|control|display|config|ports|register|filters: "
+		"Show help",
 	.tokens = {
 		(void *)&cmd_help_long_help,
 		(void *)&cmd_help_long_section,
@@ -868,7 +869,7 @@ cmdline_parse_token_string_t cmd_operate_port_all_all =
 cmdline_parse_inst_t cmd_operate_port = {
 	.f = cmd_operate_port_parsed,
 	.data = NULL,
-	.help_str = "port start|stop|close all: start/stop/close all ports",
+	.help_str = "port start|stop|close all: Start/Stop/Close all ports",
 	.tokens = {
 		(void *)&cmd_operate_port_all_cmd,
 		(void *)&cmd_operate_port_all_port,
@@ -913,7 +914,7 @@ cmdline_parse_token_num_t cmd_operate_specific_port_id =
 cmdline_parse_inst_t cmd_operate_specific_port = {
 	.f = cmd_operate_specific_port_parsed,
 	.data = NULL,
-	.help_str = "port start|stop|close X: start/stop/close port X",
+	.help_str = "port start|stop|close <port_id>: Start/Stop/Close port_id",
 	.tokens = {
 		(void *)&cmd_operate_specific_port_cmd,
 		(void *)&cmd_operate_specific_port_port,
@@ -954,8 +955,8 @@ cmdline_parse_token_string_t cmd_operate_attach_port_identifier =
 cmdline_parse_inst_t cmd_operate_attach_port = {
 	.f = cmd_operate_attach_port_parsed,
 	.data = NULL,
-	.help_str = "port attach identifier, "
-		"identifier: pci address or virtual dev name",
+	.help_str = "port attach <identifier>: "
+		"(identifier: pci address or virtual dev name)",
 	.tokens = {
 		(void *)&cmd_operate_attach_port_port,
 		(void *)&cmd_operate_attach_port_keyword,
@@ -996,7 +997,7 @@ cmdline_parse_token_num_t cmd_operate_detach_port_port_id =
 cmdline_parse_inst_t cmd_operate_detach_port = {
 	.f = cmd_operate_detach_port_parsed,
 	.data = NULL,
-	.help_str = "port detach port_id",
+	.help_str = "port detach <port_id>",
 	.tokens = {
 		(void *)&cmd_operate_detach_port_port,
 		(void *)&cmd_operate_detach_port_keyword,
@@ -1189,7 +1190,7 @@ cmdline_parse_token_string_t cmd_config_speed_specific_value2 =
 cmdline_parse_inst_t cmd_config_speed_specific = {
 	.f = cmd_config_speed_specific_parsed,
 	.data = NULL,
-	.help_str = "port config X speed "
+	.help_str = "port config <port_id> speed "
 		"10|100|1000|10000|25000|40000|50000|100000|auto duplex "
 							"half|full|auto",
 	.tokens = {
@@ -1279,7 +1280,7 @@ cmdline_parse_token_num_t cmd_config_rx_tx_value =
 cmdline_parse_inst_t cmd_config_rx_tx = {
 	.f = cmd_config_rx_tx_parsed,
 	.data = NULL,
-	.help_str = "port config all rxq|txq|rxd|txd value",
+	.help_str = "port config all rxq|txq|rxd|txd <value>",
 	.tokens = {
 		(void *)&cmd_config_rx_tx_port,
 		(void *)&cmd_config_rx_tx_keyword,
@@ -1354,7 +1355,7 @@ cmdline_parse_token_num_t cmd_config_max_pkt_len_value =
 cmdline_parse_inst_t cmd_config_max_pkt_len = {
 	.f = cmd_config_max_pkt_len_parsed,
 	.data = NULL,
-	.help_str = "port config all max-pkt-len value",
+	.help_str = "port config all max-pkt-len <value>",
 	.tokens = {
 		(void *)&cmd_config_max_pkt_len_port,
 		(void *)&cmd_config_max_pkt_len_keyword,
@@ -1405,7 +1406,7 @@ cmdline_parse_token_num_t cmd_config_mtu_value =
 cmdline_parse_inst_t cmd_config_mtu = {
 	.f = cmd_config_mtu_parsed,
 	.data = NULL,
-	.help_str = "port config mtu port_id value",
+	.help_str = "port config mtu <port_id> <value>",
 	.tokens = {
 		(void *)&cmd_config_mtu_port,
 		(void *)&cmd_config_mtu_keyword,
@@ -1625,7 +1626,8 @@ cmdline_parse_token_string_t cmd_config_rss_value =
 cmdline_parse_inst_t cmd_config_rss = {
 	.f = cmd_config_rss_parsed,
 	.data = NULL,
-	.help_str = "port config all rss all|ip|tcp|udp|sctp|ether|port|vxlan|geneve|nvgre|none",
+	.help_str = "port config all rss "
+		"all|ip|tcp|udp|sctp|ether|port|vxlan|geneve|nvgre|none",
 	.tokens = {
 		(void *)&cmd_config_rss_port,
 		(void *)&cmd_config_rss_keyword,
@@ -1738,12 +1740,11 @@ cmdline_parse_token_string_t cmd_config_rss_hash_key_value =
 cmdline_parse_inst_t cmd_config_rss_hash_key = {
 	.f = cmd_config_rss_hash_key_parsed,
 	.data = NULL,
-	.help_str =
-		"port config X rss-hash-key ipv4|ipv4-frag|ipv4-tcp|ipv4-udp|"
-		"ipv4-sctp|ipv4-other|ipv6|ipv6-frag|ipv6-tcp|ipv6-udp|"
-		"ipv6-sctp|ipv6-other|l2-payload|"
-		"ipv6-ex|ipv6-tcp-ex|ipv6-udp-ex "
-		"<string of hexa digits (variable length, NIC dependent)>\n",
+	.help_str = "port config <port_id> rss-hash-key "
+		"ipv4|ipv4-frag|ipv4-tcp|ipv4-udp|ipv4-sctp|ipv4-other|"
+		"ipv6|ipv6-frag|ipv6-tcp|ipv6-udp|ipv6-sctp|ipv6-other|"
+		"l2-payload|ipv6-ex|ipv6-tcp-ex|ipv6-udp-ex "
+		"<string of hexa digits (variable length, NIC dependent)>",
 	.tokens = {
 		(void *)&cmd_config_rss_hash_key_port,
 		(void *)&cmd_config_rss_hash_key_config,
@@ -1838,7 +1839,7 @@ cmdline_parse_token_string_t cmd_config_rxtx_queue_opname =
 cmdline_parse_inst_t cmd_config_rxtx_queue = {
 	.f = cmd_config_rxtx_queue_parsed,
 	.data = NULL,
-	.help_str = "port X rxq|txq ID start|stop",
+	.help_str = "port <port_id> rxq|txq <queue_id> start|stop",
 	.tokens = {
 		(void *)&cmd_config_speed_all_port,
 		(void *)&cmd_config_rxtx_queue_portid,
@@ -1973,7 +1974,7 @@ cmdline_parse_token_string_t cmd_config_rss_reta_list_of_items =
 cmdline_parse_inst_t cmd_config_rss_reta = {
 	.f = cmd_set_rss_reta_parsed,
 	.data = NULL,
-	.help_str = "port config X rss reta (hash,queue)[,(hash,queue)]",
+	.help_str = "port config <port_id> rss reta <hash,queue[,hash,queue]*>",
 	.tokens = {
 		(void *)&cmd_config_rss_reta_port,
 		(void *)&cmd_config_rss_reta_keyword,
@@ -2080,7 +2081,7 @@ cmdline_parse_token_string_t cmd_showport_reta_list_of_items =
 cmdline_parse_inst_t cmd_showport_reta = {
 	.f = cmd_showport_reta_parsed,
 	.data = NULL,
-	.help_str = "show port X rss reta (size) (mask0,mask1,...)",
+	.help_str = "show port <port_id> rss reta <size> <mask0[,mask1]*>",
 	.tokens = {
 		(void *)&cmd_showport_reta_show,
 		(void *)&cmd_showport_reta_port,
@@ -2134,11 +2135,10 @@ cmdline_parse_token_string_t cmd_showport_rss_hash_rss_key =
 cmdline_parse_inst_t cmd_showport_rss_hash = {
 	.f = cmd_showport_rss_hash_parsed,
 	.data = NULL,
-	.help_str =
-		"show port X rss-hash ipv4|ipv4-frag|ipv4-tcp|ipv4-udp|"
-		"ipv4-sctp|ipv4-other|ipv6|ipv6-frag|ipv6-tcp|ipv6-udp|"
-		"ipv6-sctp|ipv6-other|l2-payload|"
-		"ipv6-ex|ipv6-tcp-ex|ipv6-udp-ex (X = port number)\n",
+	.help_str = "show port <port_id> rss-hash "
+		"ipv4|ipv4-frag|ipv4-tcp|ipv4-udp|ipv4-sctp|ipv4-other|"
+		"ipv6|ipv6-frag|ipv6-tcp|ipv6-udp|ipv6-sctp|ipv6-other|"
+		"l2-payload|ipv6-ex|ipv6-tcp-ex|ipv6-udp-ex",
 	.tokens = {
 		(void *)&cmd_showport_rss_hash_show,
 		(void *)&cmd_showport_rss_hash_port,
@@ -2152,11 +2152,10 @@ cmdline_parse_inst_t cmd_showport_rss_hash = {
 cmdline_parse_inst_t cmd_showport_rss_hash_key = {
 	.f = cmd_showport_rss_hash_parsed,
 	.data = (void *)1,
-	.help_str =
-		"show port X rss-hash ipv4|ipv4-frag|ipv4-tcp|ipv4-udp|"
-		"ipv4-sctp|ipv4-other|ipv6|ipv6-frag|ipv6-tcp|ipv6-udp|"
-		"ipv6-sctp|ipv6-other|l2-payload|"
-		"ipv6-ex|ipv6-tcp-ex|ipv6-udp-ex key (X = port number)\n",
+	.help_str = "show port <port_id> rss-hash "
+		"ipv4|ipv4-frag|ipv4-tcp|ipv4-udp|ipv4-sctp|ipv4-other|"
+		"ipv6|ipv6-frag|ipv6-tcp|ipv6-udp|ipv6-sctp|ipv6-other|"
+		"l2-payload|ipv6-ex|ipv6-tcp-ex|ipv6-udp-ex key",
 	.tokens = {
 		(void *)&cmd_showport_rss_hash_show,
 		(void *)&cmd_showport_rss_hash_port,
@@ -2253,10 +2252,10 @@ cmdline_parse_token_string_t cmd_config_dcb_pfc_en =
         TOKEN_STRING_INITIALIZER(struct cmd_config_dcb, pfc_en, "on#off");
 
 cmdline_parse_inst_t cmd_config_dcb = {
-        .f = cmd_config_dcb_parsed,
-        .data = NULL,
-        .help_str = "port config port-id dcb vt on|off nb-tcs pfc on|off",
-        .tokens = {
+	.f = cmd_config_dcb_parsed,
+	.data = NULL,
+	.help_str = "port config <port-id> dcb vt on|off <num_tcs> pfc on|off",
+	.tokens = {
 		(void *)&cmd_config_dcb_port,
 		(void *)&cmd_config_dcb_config,
 		(void *)&cmd_config_dcb_port_id,
@@ -2321,7 +2320,7 @@ cmdline_parse_token_num_t cmd_config_burst_value =
 cmdline_parse_inst_t cmd_config_burst = {
 	.f = cmd_config_burst_parsed,
 	.data = NULL,
-	.help_str = "port config all burst value",
+	.help_str = "port config all burst <value>",
 	.tokens = {
 		(void *)&cmd_config_burst_port,
 		(void *)&cmd_config_burst_keyword,
@@ -2390,7 +2389,7 @@ cmdline_parse_token_num_t cmd_config_thresh_value =
 cmdline_parse_inst_t cmd_config_thresh = {
 	.f = cmd_config_thresh_parsed,
 	.data = NULL,
-	.help_str = "port config all txpt|txht|txwt|rxpt|rxht|rxwt value",
+	.help_str = "port config all txpt|txht|txwt|rxpt|rxht|rxwt <value>",
 	.tokens = {
 		(void *)&cmd_config_thresh_port,
 		(void *)&cmd_config_thresh_keyword,
@@ -2454,7 +2453,7 @@ cmdline_parse_token_num_t cmd_config_threshold_value =
 cmdline_parse_inst_t cmd_config_threshold = {
 	.f = cmd_config_threshold_parsed,
 	.data = NULL,
-	.help_str = "port config all txfreet|txrst|rxfreet value",
+	.help_str = "port config all txfreet|txrst|rxfreet <value>",
 	.tokens = {
 		(void *)&cmd_config_threshold_port,
 		(void *)&cmd_config_threshold_keyword,
@@ -2483,7 +2482,7 @@ cmdline_parse_token_string_t cmd_stop_stop =
 cmdline_parse_inst_t cmd_stop = {
 	.f = cmd_stop_parsed,
 	.data = NULL,
-	.help_str = "stop - stop packet forwarding",
+	.help_str = "stop: Stop packet forwarding",
 	.tokens = {
 		(void *)&cmd_stop_stop,
 		NULL,
@@ -2613,7 +2612,7 @@ cmdline_parse_token_string_t cmd_set_list_of_items =
 cmdline_parse_inst_t cmd_set_fwd_list = {
 	.f = cmd_set_list_parsed,
 	.data = NULL,
-	.help_str = "set corelist|portlist x[,y]*",
+	.help_str = "set corelist|portlist <list0[,list1]*>",
 	.tokens = {
 		(void *)&cmd_set_list_keyword,
 		(void *)&cmd_set_list_name,
@@ -2660,7 +2659,7 @@ cmdline_parse_token_num_t cmd_setmask_value =
 cmdline_parse_inst_t cmd_set_fwd_mask = {
 	.f = cmd_set_mask_parsed,
 	.data = NULL,
-	.help_str = "set coremask|portmask hexadecimal value",
+	.help_str = "set coremask|portmask <hexadecimal value>",
 	.tokens = {
 		(void *)&cmd_setmask_set,
 		(void *)&cmd_setmask_mask,
@@ -2706,7 +2705,7 @@ cmdline_parse_token_num_t cmd_set_value =
 cmdline_parse_inst_t cmd_set_numbers = {
 	.f = cmd_set_parsed,
 	.data = NULL,
-	.help_str = "set nbport|nbcore|burst|verbose value",
+	.help_str = "set nbport|nbcore|burst|verbose <value>",
 	.tokens = {
 		(void *)&cmd_set_set,
 		(void *)&cmd_set_what,
@@ -2752,7 +2751,7 @@ cmdline_parse_token_string_t cmd_set_txpkts_lengths =
 cmdline_parse_inst_t cmd_set_txpkts = {
 	.f = cmd_set_txpkts_parsed,
 	.data = NULL,
-	.help_str = "set txpkts x[,y]*",
+	.help_str = "set txpkts <len0[,len1]*>",
 	.tokens = {
 		(void *)&cmd_set_txpkts_keyword,
 		(void *)&cmd_set_txpkts_name,
@@ -2859,7 +2858,7 @@ cmdline_parse_token_num_t cmd_config_txqflags_value =
 cmdline_parse_inst_t cmd_config_txqflags = {
 	.f = cmd_config_txqflags_parsed,
 	.data = NULL,
-	.help_str = "port config all txqflags value",
+	.help_str = "port config all txqflags <value>",
 	.tokens = {
 		(void *)&cmd_config_txqflags_port,
 		(void *)&cmd_config_txqflags_config,
@@ -2907,8 +2906,9 @@ cmdline_parse_token_num_t cmd_rx_vlan_filter_all_portid =
 cmdline_parse_inst_t cmd_rx_vlan_filter_all = {
 	.f = cmd_rx_vlan_filter_all_parsed,
 	.data = NULL,
-	.help_str = "add/remove all identifiers to/from the set of VLAN "
-	"Identifiers filtered by a port",
+	.help_str = "rx_vlan add|rm all <port_id>: "
+		"Add/Remove all identifiers to/from the set of VLAN "
+		"identifiers filtered by a port",
 	.tokens = {
 		(void *)&cmd_rx_vlan_filter_all_rx_vlan,
 		(void *)&cmd_rx_vlan_filter_all_what,
@@ -3007,8 +3007,9 @@ cmdline_parse_token_string_t cmd_vlan_offload_portid =
 cmdline_parse_inst_t cmd_vlan_offload = {
 	.f = cmd_vlan_offload_parsed,
 	.data = NULL,
-	.help_str = "set strip|filter|qinq|stripq on|off port_id[,queue_id], filter/strip for rx side"
-	" qinq(extended) for both rx/tx sides ",
+	.help_str = "vlan set strip|filter|qinq|stripq on|off "
+		"<port_id[,queue_id]>: "
+		"Filter/Strip for rx side qinq(extended) for both rx/tx sides",
 	.tokens = {
 		(void *)&cmd_vlan_offload_vlan,
 		(void *)&cmd_vlan_offload_set,
@@ -3070,8 +3071,8 @@ cmdline_parse_token_num_t cmd_vlan_tpid_portid =
 cmdline_parse_inst_t cmd_vlan_tpid = {
 	.f = cmd_vlan_tpid_parsed,
 	.data = NULL,
-	.help_str = "set inner|outer tpid tp_id port_id, set the VLAN "
-		    "Ether type",
+	.help_str = "vlan set inner|outer tpid <tp_id> <port_id>: "
+		"Set the VLAN Ether type",
 	.tokens = {
 		(void *)&cmd_vlan_tpid_vlan,
 		(void *)&cmd_vlan_tpid_set,
@@ -3120,8 +3121,9 @@ cmdline_parse_token_num_t cmd_rx_vlan_filter_portid =
 cmdline_parse_inst_t cmd_rx_vlan_filter = {
 	.f = cmd_rx_vlan_filter_parsed,
 	.data = NULL,
-	.help_str = "add/remove a VLAN identifier to/from the set of VLAN "
-	"Identifiers filtered by a port",
+	.help_str = "rx_vlan add|rm <vlan_id> <port_id>: "
+		"Add/Remove a VLAN identifier to/from the set of VLAN "
+		"identifiers filtered by a port",
 	.tokens = {
 		(void *)&cmd_rx_vlan_filter_rx_vlan,
 		(void *)&cmd_rx_vlan_filter_what,
@@ -3165,7 +3167,8 @@ cmdline_parse_token_num_t cmd_tx_vlan_set_vlanid =
 cmdline_parse_inst_t cmd_tx_vlan_set = {
 	.f = cmd_tx_vlan_set_parsed,
 	.data = NULL,
-	.help_str = "enable hardware insertion of a single VLAN header "
+	.help_str = "tx_vlan set <port_id> <vlan_id>: "
+		"Enable hardware insertion of a single VLAN header "
 		"with a given TAG Identifier in packets sent on a port",
 	.tokens = {
 		(void *)&cmd_tx_vlan_set_tx_vlan,
@@ -3214,7 +3217,8 @@ cmdline_parse_token_num_t cmd_tx_vlan_set_qinq_vlanid_outer =
 cmdline_parse_inst_t cmd_tx_vlan_set_qinq = {
 	.f = cmd_tx_vlan_set_qinq_parsed,
 	.data = NULL,
-	.help_str = "enable hardware insertion of double VLAN header "
+	.help_str = "tx_vlan set <port_id> <vlan_id> <outer_vlan_id>: "
+		"Enable hardware insertion of double VLAN header "
 		"with given TAG Identifiers in packets sent on a port",
 	.tokens = {
 		(void *)&cmd_tx_vlan_set_qinq_tx_vlan,
@@ -3271,7 +3275,7 @@ cmdline_parse_token_string_t cmd_tx_vlan_set_pvid_mode =
 cmdline_parse_inst_t cmd_tx_vlan_set_pvid = {
 	.f = cmd_tx_vlan_set_pvid_parsed,
 	.data = NULL,
-	.help_str = "tx_vlan set pvid port_id vlan_id (on|off)",
+	.help_str = "tx_vlan set pvid <port_id> <vlan_id> on|off",
 	.tokens = {
 		(void *)&cmd_tx_vlan_set_pvid_tx_vlan,
 		(void *)&cmd_tx_vlan_set_pvid_set,
@@ -3313,8 +3317,8 @@ cmdline_parse_token_num_t cmd_tx_vlan_reset_portid =
 cmdline_parse_inst_t cmd_tx_vlan_reset = {
 	.f = cmd_tx_vlan_reset_parsed,
 	.data = NULL,
-	.help_str = "disable hardware insertion of a VLAN header in packets "
-	"sent on a port",
+	.help_str = "tx_vlan reset <port_id>: Disable hardware insertion of a "
+		"VLAN header in packets sent on a port",
 	.tokens = {
 		(void *)&cmd_tx_vlan_reset_tx_vlan,
 		(void *)&cmd_tx_vlan_reset_reset,
@@ -3440,8 +3444,9 @@ cmdline_parse_token_num_t cmd_csum_portid =
 cmdline_parse_inst_t cmd_csum_set = {
 	.f = cmd_csum_parsed,
 	.data = NULL,
-	.help_str = "enable/disable hardware calculation of L3/L4 checksum when "
-		"using csum forward engine: csum set ip|tcp|udp|sctp|outer-ip hw|sw <port>",
+	.help_str = "csum set ip|tcp|udp|sctp|outer-ip hw|sw <port_id>: "
+		"Enable/Disable hardware calculation of L3/L4 checksum when "
+		"using csum forward engine",
 	.tokens = {
 		(void *)&cmd_csum_csum,
 		(void *)&cmd_csum_mode,
@@ -3459,7 +3464,7 @@ cmdline_parse_token_string_t cmd_csum_mode_show =
 cmdline_parse_inst_t cmd_csum_show = {
 	.f = cmd_csum_parsed,
 	.data = NULL,
-	.help_str = "show checksum offload configuration: csum show <port>",
+	.help_str = "csum show <port_id>: Show checksum offload configuration",
 	.tokens = {
 		(void *)&cmd_csum_csum,
 		(void *)&cmd_csum_mode_show,
@@ -3512,8 +3517,8 @@ cmdline_parse_token_num_t cmd_csum_tunnel_portid =
 cmdline_parse_inst_t cmd_csum_tunnel = {
 	.f = cmd_csum_tunnel_parsed,
 	.data = NULL,
-	.help_str = "enable/disable parsing of tunnels for csum engine: "
-	"csum parse_tunnel on|off <tx-port>",
+	.help_str = "csum parse_tunnel on|off <port_id>: "
+		"Enable/Disable parsing of tunnels for csum engine",
 	.tokens = {
 		(void *)&cmd_csum_tunnel_csum,
 		(void *)&cmd_csum_tunnel_parse,
@@ -3576,8 +3581,9 @@ cmdline_parse_token_num_t cmd_tso_set_portid =
 cmdline_parse_inst_t cmd_tso_set = {
 	.f = cmd_tso_set_parsed,
 	.data = NULL,
-	.help_str = "Set TSO segment size of non-tunneled packets "
-	"for csum engine (0 to disable): tso set <tso_segsz> <port>",
+	.help_str = "tso set <tso_segsz> <port_id>: "
+		"Set TSO segment size of non-tunneled packets for csum engine "
+		"(0 to disable)",
 	.tokens = {
 		(void *)&cmd_tso_set_tso,
 		(void *)&cmd_tso_set_mode,
@@ -3595,8 +3601,8 @@ cmdline_parse_token_string_t cmd_tso_show_mode =
 cmdline_parse_inst_t cmd_tso_show = {
 	.f = cmd_tso_set_parsed,
 	.data = NULL,
-	.help_str = "Show TSO segment size of non-tunneled packets "
-	"for csum engine: tso show <port>",
+	.help_str = "tso show <port_id>: "
+		"Show TSO segment size of non-tunneled packets for csum engine",
 	.tokens = {
 		(void *)&cmd_tso_set_tso,
 		(void *)&cmd_tso_show_mode,
@@ -3692,8 +3698,9 @@ cmdline_parse_token_num_t cmd_tunnel_tso_set_portid =
 cmdline_parse_inst_t cmd_tunnel_tso_set = {
 	.f = cmd_tunnel_tso_set_parsed,
 	.data = NULL,
-	.help_str = "Set TSO segment size of tunneled packets for csum engine "
-	"(0 to disable): tunnel_tso set <tso_segsz> <port>",
+	.help_str = "tunnel_tso set <tso_segsz> <port_id>: "
+		"Set TSO segment size of tunneled packets for csum engine "
+		"(0 to disable)",
 	.tokens = {
 		(void *)&cmd_tunnel_tso_set_tso,
 		(void *)&cmd_tunnel_tso_set_mode,
@@ -3711,8 +3718,8 @@ cmdline_parse_token_string_t cmd_tunnel_tso_show_mode =
 cmdline_parse_inst_t cmd_tunnel_tso_show = {
 	.f = cmd_tunnel_tso_set_parsed,
 	.data = NULL,
-	.help_str = "Show TSO segment size of tunneled packets "
-	"for csum engine: tunnel_tso show <port>",
+	.help_str = "tunnel_tso show <port_id> "
+		"Show TSO segment size of tunneled packets for csum engine",
 	.tokens = {
 		(void *)&cmd_tunnel_tso_set_tso,
 		(void *)&cmd_tunnel_tso_show_mode,
@@ -3750,7 +3757,7 @@ cmdline_parse_token_string_t cmd_setflushrx_mode =
 
 cmdline_parse_inst_t cmd_set_flush_rx = {
 	.f = cmd_set_flush_rx_parsed,
-	.help_str = "set flush_rx on|off: enable/disable flush on rx streams",
+	.help_str = "set flush_rx on|off: Enable/Disable flush on rx streams",
 	.data = NULL,
 	.tokens = {
 		(void *)&cmd_setflushrx_set,
@@ -3789,7 +3796,7 @@ cmdline_parse_token_string_t cmd_setlinkcheck_mode =
 
 cmdline_parse_inst_t cmd_set_link_check = {
 	.f = cmd_set_link_check_parsed,
-	.help_str = "set link_check on|off: enable/disable link status check "
+	.help_str = "set link_check on|off: Enable/Disable link status check "
 	            "when starting/stopping a port",
 	.data = NULL,
 	.tokens = {
@@ -3850,7 +3857,7 @@ cmdline_parse_token_num_t cmd_setbypass_mode_port =
 
 cmdline_parse_inst_t cmd_set_bypass_mode = {
 	.f = cmd_set_bypass_mode_parsed,
-	.help_str = "set bypass mode (normal|bypass|isolate) (port_id): "
+	.help_str = "set bypass mode normal|bypass|isolate <port_id>: "
 	            "Set the NIC bypass mode for port_id",
 	.data = NULL,
 	.tokens = {
@@ -3950,9 +3957,9 @@ cmdline_parse_token_num_t cmd_setbypass_event_port =
 
 cmdline_parse_inst_t cmd_set_bypass_event = {
 	.f = cmd_set_bypass_event_parsed,
-	.help_str = "set bypass event (timeout|os_on|os_off|power_on|power_off) "
-	            "mode (normal|bypass|isolate) (port_id): "
-	            "Set the NIC bypass event mode for port_id",
+	.help_str = "set bypass event none|timeout|os_on|os_off|power_on|"
+		"power_off mode normal|bypass|isolate <port_id>: "
+		"Set the NIC bypass event mode for port_id",
 	.data = NULL,
 	.tokens = {
 		(void *)&cmd_setbypass_event_set,
@@ -4015,8 +4022,8 @@ cmdline_parse_token_string_t cmd_setbypass_timeout_value =
 
 cmdline_parse_inst_t cmd_set_bypass_timeout = {
 	.f = cmd_set_bypass_timeout_parsed,
-	.help_str = "set bypass timeout (0|1.5|2|3|4|8|16|32) seconds: "
-	            "Set the NIC bypass watchdog timeout",
+	.help_str = "set bypass timeout 0|1.5|2|3|4|8|16|32: "
+		"Set the NIC bypass watchdog timeout in seconds",
 	.data = NULL,
 	.tokens = {
 		(void *)&cmd_setbypass_timeout_set,
@@ -4109,7 +4116,7 @@ cmdline_parse_token_num_t cmd_showbypass_config_port =
 
 cmdline_parse_inst_t cmd_show_bypass_config = {
 	.f = cmd_show_bypass_config_parsed,
-	.help_str = "show bypass config (port_id): "
+	.help_str = "show bypass config <port_id>: "
 	            "Show the NIC bypass config for port_id",
 	.data = NULL,
 	.tokens = {
@@ -4162,7 +4169,8 @@ TOKEN_NUM_INITIALIZER(struct cmd_set_bonding_mode_result,
 
 cmdline_parse_inst_t cmd_set_bonding_mode = {
 		.f = cmd_set_bonding_mode_parsed,
-		.help_str = "set bonding mode (mode_value) (port_id): Set the bonding mode for port_id",
+		.help_str = "set bonding mode <mode_value> <port_id>: "
+			"Set the bonding mode for port_id",
 		.data = NULL,
 		.tokens = {
 				(void *) &cmd_setbonding_mode_set,
@@ -4227,7 +4235,9 @@ TOKEN_STRING_INITIALIZER(struct cmd_set_bonding_balance_xmit_policy_result,
 
 cmdline_parse_inst_t cmd_set_balance_xmit_policy = {
 		.f = cmd_set_bonding_balance_xmit_policy_parsed,
-		.help_str = "set bonding balance_xmit_policy (port_id) (policy_value): Set the bonding balance_xmit_policy for port_id",
+		.help_str = "set bonding balance_xmit_policy <port_id> "
+			"l2|l23|l34: "
+			"Set the bonding balance_xmit_policy for port_id",
 		.data = NULL,
 		.tokens = {
 				(void *)&cmd_setbonding_balance_xmit_policy_set,
@@ -4353,7 +4363,8 @@ TOKEN_NUM_INITIALIZER(struct cmd_show_bonding_config_result,
 
 cmdline_parse_inst_t cmd_show_bonding_config = {
 		.f = cmd_show_bonding_config_parsed,
-		.help_str =	"show bonding config (port_id): Show the bonding config for port_id",
+		.help_str = "show bonding config <port_id>: "
+			"Show the bonding config for port_id",
 		.data = NULL,
 		.tokens = {
 				(void *)&cmd_showbonding_config_show,
@@ -4408,7 +4419,8 @@ TOKEN_NUM_INITIALIZER(struct cmd_set_bonding_primary_result,
 
 cmdline_parse_inst_t cmd_set_bonding_primary = {
 		.f = cmd_set_bonding_primary_parsed,
-		.help_str = "set bonding primary (slave_id) (port_id): Set the primary slave for port_id",
+		.help_str = "set bonding primary <slave_id> <port_id>: "
+			"Set the primary slave for port_id",
 		.data = NULL,
 		.tokens = {
 				(void *)&cmd_setbonding_primary_set,
@@ -4465,7 +4477,8 @@ TOKEN_NUM_INITIALIZER(struct cmd_add_bonding_slave_result,
 
 cmdline_parse_inst_t cmd_add_bonding_slave = {
 		.f = cmd_add_bonding_slave_parsed,
-		.help_str = "add bonding slave (slave_id) (port_id): Add a slave device to a bonded device",
+		.help_str = "add bonding slave <slave_id> <port_id>: "
+			"Add a slave device to a bonded device",
 		.data = NULL,
 		.tokens = {
 				(void *)&cmd_addbonding_slave_add,
@@ -4522,7 +4535,8 @@ cmdline_parse_token_num_t cmd_removebonding_slave_port =
 
 cmdline_parse_inst_t cmd_remove_bonding_slave = {
 		.f = cmd_remove_bonding_slave_parsed,
-		.help_str = "remove bonding slave (slave_id) (port_id): Remove a slave device from a bonded device",
+		.help_str = "remove bonding slave <slave_id> <port_id>: "
+			"Remove a slave device from a bonded device",
 		.data = NULL,
 		.tokens = {
 				(void *)&cmd_removebonding_slave_remove,
@@ -4597,7 +4611,8 @@ cmdline_parse_token_num_t cmd_createbonded_device_socket =
 
 cmdline_parse_inst_t cmd_create_bonded_device = {
 		.f = cmd_create_bonded_device_parsed,
-		.help_str = "create bonded device (mode) (socket): Create a new bonded device with specific bonding mode and socket",
+		.help_str = "create bonded device <mode> <socket>: "
+			"Create a new bonded device with specific bonding mode and socket",
 		.data = NULL,
 		.tokens = {
 				(void *)&cmd_createbonded_device_create,
@@ -4651,7 +4666,7 @@ cmdline_parse_token_etheraddr_t cmd_set_bond_mac_addr_addr =
 cmdline_parse_inst_t cmd_set_bond_mac_addr = {
 		.f = cmd_set_bond_mac_addr_parsed,
 		.data = (void *) 0,
-		.help_str = "set bonding mac_addr (port_id) (address): ",
+		.help_str = "set bonding mac_addr <port_id> <mac_addr>",
 		.tokens = {
 				(void *)&cmd_set_bond_mac_addr_set,
 				(void *)&cmd_set_bond_mac_addr_bonding,
@@ -4710,7 +4725,7 @@ cmdline_parse_token_num_t cmd_set_bond_mon_period_period_ms =
 cmdline_parse_inst_t cmd_set_bond_mon_period = {
 		.f = cmd_set_bond_mon_period_parsed,
 		.data = (void *) 0,
-		.help_str = "set bonding mon_period (port_id) (period_ms): ",
+		.help_str = "set bonding mon_period <port_id> <period_ms>",
 		.tokens = {
 				(void *)&cmd_set_bond_mon_period_set,
 				(void *)&cmd_set_bond_mon_period_bonding,
@@ -4768,8 +4783,8 @@ static void cmd_set_fwd_mode_init(void)
 	cmdline_parse_token_string_t *token_struct;
 
 	modes = list_pkt_forwarding_modes();
-	snprintf(help, sizeof help, "set fwd %s - "
-		"set packet forwarding mode", modes);
+	snprintf(help, sizeof(help), "set fwd %s: "
+		"Set packet forwarding mode", modes);
 	cmd_set_fwd_mode.help_str = help;
 
 	/* string token separator is # */
@@ -4835,8 +4850,8 @@ static void cmd_set_fwd_retry_mode_init(void)
 	cmdline_parse_token_string_t *token_struct;
 
 	modes = list_pkt_forwarding_retry_modes();
-	snprintf(help, sizeof(help), "set fwd %s retry - "
-		"set packet forwarding mode with retry", modes);
+	snprintf(help, sizeof(help), "set fwd %s retry: "
+		"Set packet forwarding mode with retry", modes);
 	cmd_set_fwd_retry_mode.help_str = help;
 
 	/* string token separator is # */
@@ -4895,7 +4910,7 @@ cmdline_parse_token_num_t cmd_set_burst_tx_retry_retry_num =
 
 cmdline_parse_inst_t cmd_set_burst_tx_retry = {
 	.f = cmd_set_burst_tx_retry_parsed,
-	.help_str = "set burst tx delay (time_by_useconds) retry (retry_num)",
+	.help_str = "set burst tx delay <delay_usec> retry <num_retry>",
 	.tokens = {
 		(void *)&cmd_set_burst_tx_retry_set,
 		(void *)&cmd_set_burst_tx_retry_burst,
@@ -4965,7 +4980,7 @@ cmdline_parse_token_string_t cmd_setpromisc_mode =
 cmdline_parse_inst_t cmd_set_promisc_mode_all = {
 	.f = cmd_set_promisc_mode_parsed,
 	.data = (void *)1,
-	.help_str = "set promisc all on|off: set promisc mode for all ports",
+	.help_str = "set promisc all on|off: Set promisc mode for all ports",
 	.tokens = {
 		(void *)&cmd_setpromisc_set,
 		(void *)&cmd_setpromisc_promisc,
@@ -4978,7 +4993,7 @@ cmdline_parse_inst_t cmd_set_promisc_mode_all = {
 cmdline_parse_inst_t cmd_set_promisc_mode_one = {
 	.f = cmd_set_promisc_mode_parsed,
 	.data = (void *)0,
-	.help_str = "set promisc X on|off: set promisc mode on port X",
+	.help_str = "set promisc <port_id> on|off: Set promisc mode on port_id",
 	.tokens = {
 		(void *)&cmd_setpromisc_set,
 		(void *)&cmd_setpromisc_promisc,
@@ -5045,7 +5060,7 @@ cmdline_parse_token_string_t cmd_setallmulti_mode =
 cmdline_parse_inst_t cmd_set_allmulti_mode_all = {
 	.f = cmd_set_allmulti_mode_parsed,
 	.data = (void *)1,
-	.help_str = "set allmulti all on|off: set allmulti mode for all ports",
+	.help_str = "set allmulti all on|off: Set allmulti mode for all ports",
 	.tokens = {
 		(void *)&cmd_setallmulti_set,
 		(void *)&cmd_setallmulti_allmulti,
@@ -5058,7 +5073,8 @@ cmdline_parse_inst_t cmd_set_allmulti_mode_all = {
 cmdline_parse_inst_t cmd_set_allmulti_mode_one = {
 	.f = cmd_set_allmulti_mode_parsed,
 	.data = (void *)0,
-	.help_str = "set allmulti X on|off: set allmulti mode on port X",
+	.help_str = "set allmulti <port_id> on|off: "
+		"Set allmulti mode on port_id",
 	.tokens = {
 		(void *)&cmd_setallmulti_set,
 		(void *)&cmd_setallmulti_allmulti,
@@ -5157,9 +5173,9 @@ cmd_link_flow_ctrl_set_parsed(void *parsed_result, struct cmdline *cl,
 cmdline_parse_inst_t cmd_link_flow_control_set = {
 	.f = cmd_link_flow_ctrl_set_parsed,
 	.data = NULL,
-	.help_str = "Configure the Ethernet flow control: set flow_ctrl rx on|off \
-tx on|off high_water low_water pause_time send_xon mac_ctrl_frame_fwd on|off \
-autoneg on|off port_id",
+	.help_str = "set flow_ctrl rx on|off tx on|off <high_water> "
+		"<low_water> <pause_time> <send_xon> mac_ctrl_frame_fwd on|off "
+		"autoneg on|off <port_id>: Configure the Ethernet flow control",
 	.tokens = {
 		(void *)&cmd_lfc_set_set,
 		(void *)&cmd_lfc_set_flow_ctrl,
@@ -5183,8 +5199,8 @@ autoneg on|off port_id",
 cmdline_parse_inst_t cmd_link_flow_control_set_rx = {
 	.f = cmd_link_flow_ctrl_set_parsed,
 	.data = (void *)&cmd_link_flow_control_set_rx,
-	.help_str = "Change rx flow control parameter: set flow_ctrl "
-		    "rx on|off port_id",
+	.help_str = "set flow_ctrl rx on|off <port_id>: "
+		"Change rx flow control parameter",
 	.tokens = {
 		(void *)&cmd_lfc_set_set,
 		(void *)&cmd_lfc_set_flow_ctrl,
@@ -5198,8 +5214,8 @@ cmdline_parse_inst_t cmd_link_flow_control_set_rx = {
 cmdline_parse_inst_t cmd_link_flow_control_set_tx = {
 	.f = cmd_link_flow_ctrl_set_parsed,
 	.data = (void *)&cmd_link_flow_control_set_tx,
-	.help_str = "Change tx flow control parameter: set flow_ctrl "
-		    "tx on|off port_id",
+	.help_str = "set flow_ctrl tx on|off <port_id>: "
+		"Change tx flow control parameter",
 	.tokens = {
 		(void *)&cmd_lfc_set_set,
 		(void *)&cmd_lfc_set_flow_ctrl,
@@ -5213,8 +5229,8 @@ cmdline_parse_inst_t cmd_link_flow_control_set_tx = {
 cmdline_parse_inst_t cmd_link_flow_control_set_hw = {
 	.f = cmd_link_flow_ctrl_set_parsed,
 	.data = (void *)&cmd_link_flow_control_set_hw,
-	.help_str = "Change high water flow control parameter: set flow_ctrl "
-		    "high_water value port_id",
+	.help_str = "set flow_ctrl high_water <value> <port_id>: "
+		"Change high water flow control parameter",
 	.tokens = {
 		(void *)&cmd_lfc_set_set,
 		(void *)&cmd_lfc_set_flow_ctrl,
@@ -5228,8 +5244,8 @@ cmdline_parse_inst_t cmd_link_flow_control_set_hw = {
 cmdline_parse_inst_t cmd_link_flow_control_set_lw = {
 	.f = cmd_link_flow_ctrl_set_parsed,
 	.data = (void *)&cmd_link_flow_control_set_lw,
-	.help_str = "Change low water flow control parameter: set flow_ctrl "
-		    "low_water value port_id",
+	.help_str = "set flow_ctrl low_water <value> <port_id>: "
+		"Change low water flow control parameter",
 	.tokens = {
 		(void *)&cmd_lfc_set_set,
 		(void *)&cmd_lfc_set_flow_ctrl,
@@ -5243,8 +5259,8 @@ cmdline_parse_inst_t cmd_link_flow_control_set_lw = {
 cmdline_parse_inst_t cmd_link_flow_control_set_pt = {
 	.f = cmd_link_flow_ctrl_set_parsed,
 	.data = (void *)&cmd_link_flow_control_set_pt,
-	.help_str = "Change pause time flow control parameter: set flow_ctrl "
-		    "pause_time value port_id",
+	.help_str = "set flow_ctrl pause_time <value> <port_id>: "
+		"Change pause time flow control parameter",
 	.tokens = {
 		(void *)&cmd_lfc_set_set,
 		(void *)&cmd_lfc_set_flow_ctrl,
@@ -5258,8 +5274,8 @@ cmdline_parse_inst_t cmd_link_flow_control_set_pt = {
 cmdline_parse_inst_t cmd_link_flow_control_set_xon = {
 	.f = cmd_link_flow_ctrl_set_parsed,
 	.data = (void *)&cmd_link_flow_control_set_xon,
-	.help_str = "Change send_xon flow control parameter: set flow_ctrl "
-		    "send_xon value port_id",
+	.help_str = "set flow_ctrl send_xon <value> <port_id>: "
+		"Change send_xon flow control parameter",
 	.tokens = {
 		(void *)&cmd_lfc_set_set,
 		(void *)&cmd_lfc_set_flow_ctrl,
@@ -5273,8 +5289,8 @@ cmdline_parse_inst_t cmd_link_flow_control_set_xon = {
 cmdline_parse_inst_t cmd_link_flow_control_set_macfwd = {
 	.f = cmd_link_flow_ctrl_set_parsed,
 	.data = (void *)&cmd_link_flow_control_set_macfwd,
-	.help_str = "Change mac ctrl fwd flow control parameter: set flow_ctrl "
-		    "mac_ctrl_frame_fwd on|off port_id",
+	.help_str = "set flow_ctrl mac_ctrl_frame_fwd on|off <port_id>: "
+		"Change mac ctrl fwd flow control parameter",
 	.tokens = {
 		(void *)&cmd_lfc_set_set,
 		(void *)&cmd_lfc_set_flow_ctrl,
@@ -5288,8 +5304,8 @@ cmdline_parse_inst_t cmd_link_flow_control_set_macfwd = {
 cmdline_parse_inst_t cmd_link_flow_control_set_autoneg = {
 	.f = cmd_link_flow_ctrl_set_parsed,
 	.data = (void *)&cmd_link_flow_control_set_autoneg,
-	.help_str = "Change autoneg flow control parameter: set flow_ctrl "
-		    "autoneg on|off port_id",
+	.help_str = "set flow_ctrl autoneg on|off <port_id>: "
+		"Change autoneg flow control parameter",
 	.tokens = {
 		(void *)&cmd_lfc_set_set,
 		(void *)&cmd_lfc_set_flow_ctrl,
@@ -5459,8 +5475,9 @@ cmdline_parse_token_num_t cmd_pfc_set_portid =
 cmdline_parse_inst_t cmd_priority_flow_control_set = {
 	.f = cmd_priority_flow_ctrl_set_parsed,
 	.data = NULL,
-	.help_str = "Configure the Ethernet priority flow control: set pfc_ctrl rx on|off\n\
-			tx on|off high_water low_water pause_time priority port_id",
+	.help_str = "set pfc_ctrl rx on|off tx on|off <high_water> <low_water> "
+		"<pause_time> <priority> <port_id>: "
+		"Configure the Ethernet priority flow control",
 	.tokens = {
 		(void *)&cmd_pfc_set_set,
 		(void *)&cmd_pfc_set_flow_ctrl,
@@ -5500,7 +5517,7 @@ cmdline_parse_token_string_t cmd_reset_def =
 cmdline_parse_inst_t cmd_reset = {
 	.f = cmd_reset_parsed,
 	.data = NULL,
-	.help_str = "set default: reset default forwarding configuration",
+	.help_str = "set default: Reset default forwarding configuration",
 	.tokens = {
 		(void *)&cmd_reset_set,
 		(void *)&cmd_reset_def,
@@ -5526,7 +5543,7 @@ static void cmd_start_parsed(__attribute__((unused)) void *parsed_result,
 cmdline_parse_inst_t cmd_start = {
 	.f = cmd_start_parsed,
 	.data = NULL,
-	.help_str = "start packet forwarding",
+	.help_str = "start: Start packet forwarding",
 	.tokens = {
 		(void *)&cmd_start_start,
 		NULL,
@@ -5557,7 +5574,8 @@ cmdline_parse_token_string_t cmd_start_tx_first_tx_first =
 cmdline_parse_inst_t cmd_start_tx_first = {
 	.f = cmd_start_tx_first_parsed,
 	.data = NULL,
-	.help_str = "start packet forwarding, after sending 1 burst of packets",
+	.help_str = "start tx_first: Start packet forwarding, "
+		"after sending 1 burst of packets",
 	.tokens = {
 		(void *)&cmd_start_tx_first_start,
 		(void *)&cmd_start_tx_first_tx_first,
@@ -5595,8 +5613,8 @@ cmdline_parse_token_num_t cmd_start_tx_first_n_tx_num =
 cmdline_parse_inst_t cmd_start_tx_first_n = {
 	.f = cmd_start_tx_first_n_parsed,
 	.data = NULL,
-	.help_str = "start packet forwarding, after sending <num> "
-		"bursts of packets",
+	.help_str = "start tx_first <num>: "
+		"packet forwarding, after sending <num> bursts of packets",
 	.tokens = {
 		(void *)&cmd_start_tx_first_n_start,
 		(void *)&cmd_start_tx_first_n_tx_first,
@@ -5634,7 +5652,7 @@ static void cmd_set_link_up_parsed(__attribute__((unused)) void *parsed_result,
 cmdline_parse_inst_t cmd_set_link_up = {
 	.f = cmd_set_link_up_parsed,
 	.data = NULL,
-	.help_str = "set link-up port (port id)",
+	.help_str = "set link-up port <port id>",
 	.tokens = {
 		(void *)&cmd_set_link_up_set,
 		(void *)&cmd_set_link_up_link_up,
@@ -5674,7 +5692,7 @@ static void cmd_set_link_down_parsed(
 cmdline_parse_inst_t cmd_set_link_down = {
 	.f = cmd_set_link_down_parsed,
 	.data = NULL,
-	.help_str = "set link-down port (port id)",
+	.help_str = "set link-down port <port id>",
 	.tokens = {
 		(void *)&cmd_set_link_down_set,
 		(void *)&cmd_set_link_down_link_down,
@@ -5837,7 +5855,8 @@ cmdline_parse_token_num_t cmd_showport_portnum =
 cmdline_parse_inst_t cmd_showport = {
 	.f = cmd_showport_parsed,
 	.data = NULL,
-	.help_str = "show|clear port info|stats|xstats|fdir|stat_qmap|dcb_tc X (X = port number)",
+	.help_str = "show|clear port info|stats|xstats|fdir|stat_qmap|dcb_tc "
+		"<port_id>",
 	.tokens = {
 		(void *)&cmd_showport_show,
 		(void *)&cmd_showport_port,
@@ -5883,7 +5902,7 @@ cmdline_parse_token_num_t cmd_showqueue_queuenum =
 cmdline_parse_inst_t cmd_showqueue = {
 	.f = cmd_showqueue_parsed,
 	.data = NULL,
-	.help_str = "show rxq|txq info <port number> <queue_number>",
+	.help_str = "show rxq|txq info <port_id> <queue_id>",
 	.tokens = {
 		(void *)&cmd_showqueue_show,
 		(void *)&cmd_showqueue_type,
@@ -5923,7 +5942,7 @@ cmdline_parse_token_num_t cmd_read_reg_reg_off =
 cmdline_parse_inst_t cmd_read_reg = {
 	.f = cmd_read_reg_parsed,
 	.data = NULL,
-	.help_str = "read reg port_id reg_off",
+	.help_str = "read reg <port_id> <reg_off>",
 	.tokens = {
 		(void *)&cmd_read_reg_read,
 		(void *)&cmd_read_reg_reg,
@@ -5975,8 +5994,8 @@ cmdline_parse_token_num_t cmd_read_reg_bit_field_bit2_pos =
 cmdline_parse_inst_t cmd_read_reg_bit_field = {
 	.f = cmd_read_reg_bit_field_parsed,
 	.data = NULL,
-	.help_str = "read regfield port_id reg_off bit_x bit_y "
-	"(read register bit field between bit_x and bit_y included)",
+	.help_str = "read regfield <port_id> <reg_off> <bit_x> <bit_y>: "
+	"Read register bit field between bit_x and bit_y included",
 	.tokens = {
 		(void *)&cmd_read_reg_bit_field_read,
 		(void *)&cmd_read_reg_bit_field_regfield,
@@ -6021,7 +6040,7 @@ cmdline_parse_token_num_t cmd_read_reg_bit_bit_pos =
 cmdline_parse_inst_t cmd_read_reg_bit = {
 	.f = cmd_read_reg_bit_parsed,
 	.data = NULL,
-	.help_str = "read regbit port_id reg_off bit_x (0 <= bit_x <= 31)",
+	.help_str = "read regbit <port_id> <reg_off> <bit_x>: 0 <= bit_x <= 31",
 	.tokens = {
 		(void *)&cmd_read_reg_bit_read,
 		(void *)&cmd_read_reg_bit_regbit,
@@ -6064,7 +6083,7 @@ cmdline_parse_token_num_t cmd_write_reg_value =
 cmdline_parse_inst_t cmd_write_reg = {
 	.f = cmd_write_reg_parsed,
 	.data = NULL,
-	.help_str = "write reg port_id reg_off reg_value",
+	.help_str = "write reg <port_id> <reg_off> <reg_value>",
 	.tokens = {
 		(void *)&cmd_write_reg_write,
 		(void *)&cmd_write_reg_reg,
@@ -6121,8 +6140,9 @@ cmdline_parse_token_num_t cmd_write_reg_bit_field_value =
 cmdline_parse_inst_t cmd_write_reg_bit_field = {
 	.f = cmd_write_reg_bit_field_parsed,
 	.data = NULL,
-	.help_str = "write regfield port_id reg_off bit_x bit_y reg_value"
-	"(set register bit field between bit_x and bit_y included)",
+	.help_str = "write regfield <port_id> <reg_off> <bit_x> <bit_y> "
+		"<reg_value>: "
+		"Set register bit field between bit_x and bit_y included",
 	.tokens = {
 		(void *)&cmd_write_reg_bit_field_write,
 		(void *)&cmd_write_reg_bit_field_regfield,
@@ -6172,7 +6192,8 @@ cmdline_parse_token_num_t cmd_write_reg_bit_value =
 cmdline_parse_inst_t cmd_write_reg_bit = {
 	.f = cmd_write_reg_bit_parsed,
 	.data = NULL,
-	.help_str = "write regbit port_id reg_off bit_x 0/1 (0 <= bit_x <= 31)",
+	.help_str = "write regbit <port_id> <reg_off> <bit_x> 0|1: "
+		"0 <= bit_x <= 31",
 	.tokens = {
 		(void *)&cmd_write_reg_bit_write,
 		(void *)&cmd_write_reg_bit_regbit,
@@ -6221,7 +6242,7 @@ cmdline_parse_token_num_t cmd_read_rxd_txd_desc_id =
 cmdline_parse_inst_t cmd_read_rxd_txd = {
 	.f = cmd_read_rxd_txd_parsed,
 	.data = NULL,
-	.help_str = "read rxd|txd port_id queue_id rxd_id",
+	.help_str = "read rxd|txd <port_id> <queue_id> <desc_id>",
 	.tokens = {
 		(void *)&cmd_read_rxd_txd_read,
 		(void *)&cmd_read_rxd_txd_rxd_txd,
@@ -6251,7 +6272,7 @@ cmdline_parse_token_string_t cmd_quit_quit =
 cmdline_parse_inst_t cmd_quit = {
 	.f = cmd_quit_parsed,
 	.data = NULL,
-	.help_str = "exit application",
+	.help_str = "quit: Exit application",
 	.tokens = {
 		(void *)&cmd_quit_quit,
 		NULL,
@@ -6298,8 +6319,8 @@ cmdline_parse_token_etheraddr_t cmd_mac_addr_addr =
 cmdline_parse_inst_t cmd_mac_addr = {
 	.f = cmd_mac_addr_parsed,
 	.data = (void *)0,
-	.help_str = "mac_addr add|remove X <address>: "
-			"add/remove MAC address on port X",
+	.help_str = "mac_addr add|remove <port_id> <mac_addr>: "
+			"Add/Remove MAC address on port_id",
 	.tokens = {
 		(void *)&cmd_mac_addr_cmd,
 		(void *)&cmd_mac_addr_what,
@@ -6353,7 +6374,8 @@ cmdline_parse_token_num_t cmd_setqmap_mapvalue =
 cmdline_parse_inst_t cmd_set_qmap = {
 	.f = cmd_set_qmap_parsed,
 	.data = NULL,
-	.help_str = "Set statistics mapping value on tx|rx queue_id of port_id",
+	.help_str = "set stat_qmap rx|tx <port_id> <queue_id> <map_value>: "
+		"Set statistics mapping value on tx|rx queue_id of port_id",
 	.tokens = {
 		(void *)&cmd_setqmap_set,
 		(void *)&cmd_setqmap_qmap,
@@ -6415,7 +6437,7 @@ cmdline_parse_token_string_t cmd_set_uc_hash_mode =
 cmdline_parse_inst_t cmd_set_uc_hash_filter = {
 	.f = cmd_set_uc_hash_parsed,
 	.data = NULL,
-	.help_str = "set port X uta Y on|off(X = port number,Y = MAC address)",
+	.help_str = "set port <port_id> uta <mac_addr> on|off)",
 	.tokens = {
 		(void *)&cmd_set_uc_hash_set,
 		(void *)&cmd_set_uc_hash_port,
@@ -6476,7 +6498,7 @@ cmdline_parse_token_string_t cmd_set_uc_all_hash_mode =
 cmdline_parse_inst_t cmd_set_uc_all_hash_filter = {
 	.f = cmd_set_uc_all_hash_parsed,
 	.data = NULL,
-	.help_str = "set port X uta all on|off (X = port number)",
+	.help_str = "set port <port_id> uta all on|off",
 	.tokens = {
 		(void *)&cmd_set_uc_all_hash_set,
 		(void *)&cmd_set_uc_all_hash_port,
@@ -6575,12 +6597,10 @@ cmdline_parse_token_string_t cmd_set_vf_macvlan_mode =
 cmdline_parse_inst_t cmd_set_vf_macvlan_filter = {
 	.f = cmd_set_vf_macvlan_parsed,
 	.data = NULL,
-	.help_str = "set port (portid) vf (vfid) (mac-addr) "
-			"(exact-mac|exact-mac-vlan|hashmac|hashmac-vlan) "
-			"on|off\n"
-			"exact match rule:exact match of MAC or MAC and VLAN; "
-			"hash match rule: hash match of MAC and exact match "
-			"of VLAN",
+	.help_str = "set port <port_id> vf <vf_id> <mac_addr> "
+		"exact-mac|exact-mac-vlan|hashmac|hashmac-vlan on|off: "
+		"Exact match rule: exact match of MAC or MAC and VLAN; "
+		"hash match rule: hash match of MAC and exact match of VLAN",
 	.tokens = {
 		(void *)&cmd_set_vf_macvlan_set,
 		(void *)&cmd_set_vf_macvlan_port,
@@ -6642,8 +6662,7 @@ cmdline_parse_token_string_t cmd_setvf_traffic_mode =
 cmdline_parse_inst_t cmd_set_vf_traffic = {
 	.f = cmd_set_vf_traffic_parsed,
 	.data = NULL,
-	.help_str = "set port X vf Y rx|tx on|off"
-			"(X = port number,Y = vf id)",
+	.help_str = "set port <port_id> vf <vf_id> rx|tx on|off",
 	.tokens = {
 		(void *)&cmd_setvf_traffic_set,
 		(void *)&cmd_setvf_traffic_port,
@@ -6723,7 +6742,8 @@ cmdline_parse_token_string_t cmd_set_vf_rxmode_on =
 cmdline_parse_inst_t cmd_set_vf_rxmode = {
 	.f = cmd_set_vf_rxmode_parsed,
 	.data = NULL,
-	.help_str = "set port X vf Y rxmode AUPE|ROPE|BAM|MPE on|off",
+	.help_str = "set port <port_id> vf <vf_id> rxmode "
+		"AUPE|ROPE|BAM|MPE on|off",
 	.tokens = {
 		(void *)&cmd_set_vf_rxmode_set,
 		(void *)&cmd_set_vf_rxmode_port,
@@ -6788,8 +6808,8 @@ cmdline_parse_token_etheraddr_t cmd_vf_mac_addr_addr =
 cmdline_parse_inst_t cmd_vf_mac_addr_filter = {
 	.f = cmd_vf_mac_addr_parsed,
 	.data = (void *)0,
-	.help_str = "mac_addr add port X vf Y ethaddr:(X = port number,"
-	"Y = VF number)add MAC address filtering for a VF on port X",
+	.help_str = "mac_addr add port <port_id> vf <vf_id> <mac_addr>: "
+		"Add MAC address filtering for a VF on port_id",
 	.tokens = {
 		(void *)&cmd_vf_mac_addr_cmd,
 		(void *)&cmd_vf_mac_addr_what,
@@ -6851,8 +6871,8 @@ cmdline_parse_token_num_t cmd_vf_rx_vlan_filter_vf_mask =
 cmdline_parse_inst_t cmd_vf_rxvlan_filter = {
 	.f = cmd_vf_rx_vlan_filter_parsed,
 	.data = NULL,
-	.help_str = "rx_vlan add|rm X port Y vf Z (X = VLAN ID,"
-		"Y = port number,Z = hexadecimal VF mask)",
+	.help_str = "rx_vlan add|rm <vlan_id> port <port_id> vf <vf_mask>: "
+		"(vf_mask = hexadecimal VF mask)",
 	.tokens = {
 		(void *)&cmd_vf_rx_vlan_filter_rx_vlan,
 		(void *)&cmd_vf_rx_vlan_filter_what,
@@ -6918,8 +6938,8 @@ cmdline_parse_token_num_t cmd_queue_rate_limit_ratenum =
 cmdline_parse_inst_t cmd_queue_rate_limit = {
 	.f = cmd_queue_rate_limit_parsed,
 	.data = (void *)0,
-	.help_str = "set port X queue Y rate Z:(X = port number,"
-	"Y = queue number,Z = rate number)set rate limit for a queue on port X",
+	.help_str = "set port <port_id> queue <queue_id> rate <rate_value>: "
+		"Set rate limit for a queue on port_id",
 	.tokens = {
 		(void *)&cmd_queue_rate_limit_set,
 		(void *)&cmd_queue_rate_limit_port,
@@ -6994,9 +7014,9 @@ cmdline_parse_token_num_t cmd_vf_rate_limit_q_msk_val =
 cmdline_parse_inst_t cmd_vf_rate_limit = {
 	.f = cmd_vf_rate_limit_parsed,
 	.data = (void *)0,
-	.help_str = "set port X vf Y rate Z queue_mask V:(X = port number,"
-	"Y = VF number,Z = rate number, V = queue mask value)set rate limit "
-	"for queues of VF on port X",
+	.help_str = "set port <port_id> vf <vf_id> rate <rate_value> "
+		"queue_mask <queue_mask_value>: "
+		"Set rate limit for queues of VF on port_id",
 	.tokens = {
 		(void *)&cmd_vf_rate_limit_set,
 		(void *)&cmd_vf_rate_limit_port,
@@ -7140,12 +7160,10 @@ cmdline_parse_token_num_t cmd_tunnel_filter_queue_num =
 cmdline_parse_inst_t cmd_tunnel_filter = {
 	.f = cmd_tunnel_filter_parsed,
 	.data = (void *)0,
-	.help_str = "add/rm tunnel filter of a port: "
-			"tunnel_filter add port_id outer_mac inner_mac ip "
-			"inner_vlan tunnel_type(vxlan|nvgre|ipingre) filter_type "
-			"(oip|iip|imac-ivlan|imac-ivlan-tenid|imac-tenid|"
-			"imac|omac-imac-tenid) "
-			"tenant_id queue_num",
+	.help_str = "tunnel_filter add|rm <port_id> <outer_mac> <inner_mac> "
+		"<ip> <inner_vlan> vxlan|nvgre|ipingre oip|iip|imac-ivlan|"
+		"imac-ivlan-tenid|imac-tenid|imac|omac-imac-tenid <tenant_id> "
+		"<queue_id>: Add/Rm tunnel filter of a port",
 	.tokens = {
 		(void *)&cmd_tunnel_filter_cmd,
 		(void *)&cmd_tunnel_filter_what,
@@ -7211,8 +7229,8 @@ cmdline_parse_token_num_t cmd_tunnel_udp_config_port_id =
 cmdline_parse_inst_t cmd_tunnel_udp_config = {
 	.f = cmd_tunnel_udp_config_parsed,
 	.data = (void *)0,
-	.help_str = "add/rm an tunneling UDP port filter: "
-			"rx_vxlan_port add udp_port port_id",
+	.help_str = "rx_vxlan_port add|rm <udp_port> <port_id>: "
+		"Add/Remove a tunneling UDP port filter",
 	.tokens = {
 		(void *)&cmd_tunnel_udp_config_cmd,
 		(void *)&cmd_tunnel_udp_config_what,
@@ -7263,7 +7281,7 @@ cmdline_parse_token_num_t cmd_global_config_gre_key_len =
 cmdline_parse_inst_t cmd_global_config = {
 	.f = cmd_global_config_parsed,
 	.data = (void *)NULL,
-	.help_str = "global_config <port_id> gre-key-len <number>",
+	.help_str = "global_config <port_id> gre-key-len <key_len>",
 	.tokens = {
 		(void *)&cmd_global_config_cmd,
 		(void *)&cmd_global_config_port_id,
@@ -7371,8 +7389,9 @@ cmd_set_mirror_mask_parsed(void *parsed_result,
 cmdline_parse_inst_t cmd_set_mirror_mask = {
 		.f = cmd_set_mirror_mask_parsed,
 		.data = NULL,
-		.help_str = "set port X mirror-rule Y pool-mirror-up|pool-mirror-down|vlan-mirror"
-			    " pool_mask|vlan_id[,vlan_id]* dst-pool Z on|off",
+		.help_str = "set port <port_id> mirror-rule <rule_id> "
+			"pool-mirror-up|pool-mirror-down|vlan-mirror "
+			"<pool_mask|vlan_id[,vlan_id]*> dst-pool <pool_id> on|off",
 		.tokens = {
 			(void *)&cmd_mirror_mask_set,
 			(void *)&cmd_mirror_mask_port,
@@ -7462,8 +7481,8 @@ cmd_set_mirror_link_parsed(void *parsed_result,
 cmdline_parse_inst_t cmd_set_mirror_link = {
 		.f = cmd_set_mirror_link_parsed,
 		.data = NULL,
-		.help_str = "set port X mirror-rule Y uplink-mirror|"
-			"downlink-mirror dst-pool Z on|off",
+		.help_str = "set port <port_id> mirror-rule <rule_id> "
+			"uplink-mirror|downlink-mirror dst-pool <pool_id> on|off",
 		.tokens = {
 			(void *)&cmd_mirror_link_set,
 			(void *)&cmd_mirror_link_port,
@@ -7519,7 +7538,7 @@ cmd_reset_mirror_rule_parsed(void *parsed_result,
 cmdline_parse_inst_t cmd_reset_mirror_rule = {
 		.f = cmd_reset_mirror_rule_parsed,
 		.data = NULL,
-		.help_str = "reset port X mirror-rule Y",
+		.help_str = "reset port <port_id> mirror-rule <rule_id>",
 		.tokens = {
 			(void *)&cmd_rm_mirror_rule_reset,
 			(void *)&cmd_rm_mirror_rule_port,
@@ -7578,7 +7597,7 @@ cmdline_parse_token_string_t cmd_dump_dump =
 cmdline_parse_inst_t cmd_dump = {
 	.f = cmd_dump_parsed,  /* function to call */
 	.data = NULL,      /* 2nd arg of func */
-	.help_str = "dump status",
+	.help_str = "Dump status",
 	.tokens = {        /* token list, NULL terminated */
 		(void *)&cmd_dump_dump,
 		NULL,
@@ -7626,7 +7645,7 @@ cmdline_parse_token_string_t cmd_dump_one_name =
 cmdline_parse_inst_t cmd_dump_one = {
 	.f = cmd_dump_one_parsed,  /* function to call */
 	.data = NULL,      /* 2nd arg of func */
-	.help_str = "dump one ring/mempool: dump_ring|dump_mempool <name>",
+	.help_str = "dump_ring|dump_mempool <name>: Dump one ring/mempool",
 	.tokens = {        /* token list, NULL terminated */
 		(void *)&cmd_dump_one_dump,
 		(void *)&cmd_dump_one_name,
@@ -7711,7 +7730,8 @@ cmdline_parse_token_num_t cmd_syn_filter_queue_id =
 cmdline_parse_inst_t cmd_syn_filter = {
 	.f = cmd_syn_filter_parsed,
 	.data = NULL,
-	.help_str = "add/delete syn filter",
+	.help_str = "syn_filter <port_id> add|del priority high|low queue "
+		"<queue_id>: Add/Delete syn filter",
 	.tokens = {
 		(void *)&cmd_syn_filter_filter,
 		(void *)&cmd_syn_filter_port_id,
@@ -7850,7 +7870,9 @@ cmdline_parse_token_num_t cmd_2tuple_filter_queue_id =
 cmdline_parse_inst_t cmd_2tuple_filter = {
 	.f = cmd_2tuple_filter_parsed,
 	.data = NULL,
-	.help_str = "add a 2tuple filter",
+	.help_str = "2tuple_filter <port_id> add|del dst_port <value> protocol "
+		"<value> mask <value> tcp_flags <value> priority <value> queue "
+		"<queue_id>: Add a 2tuple filter",
 	.tokens = {
 		(void *)&cmd_2tuple_filter_filter,
 		(void *)&cmd_2tuple_filter_port_id,
@@ -8045,7 +8067,10 @@ cmdline_parse_token_num_t cmd_5tuple_filter_queue_id =
 cmdline_parse_inst_t cmd_5tuple_filter = {
 	.f = cmd_5tuple_filter_parsed,
 	.data = NULL,
-	.help_str = "add/del a 5tuple filter",
+	.help_str = "5tuple_filter <port_id> add|del dst_ip <value> "
+		"src_ip <value> dst_port <value> src_port <value> "
+		"protocol <value>  mask <value> tcp_flags <value> "
+		"priority <value> queue <queue_id>: Add/Del a 5tuple filter",
 	.tokens = {
 		(void *)&cmd_5tuple_filter_filter,
 		(void *)&cmd_5tuple_filter_port_id,
@@ -8240,7 +8265,9 @@ cmdline_parse_token_num_t cmd_flex_filter_queue_id =
 cmdline_parse_inst_t cmd_flex_filter = {
 	.f = cmd_flex_filter_parsed,
 	.data = NULL,
-	.help_str = "add/del a flex filter",
+	.help_str = "flex_filter <port_id> add|del len <value> bytes "
+		"<value> mask <value> priority <value> queue <queue_id>: "
+		"Add/Del a flex filter",
 	.tokens = {
 		(void *)&cmd_flex_filter_filter,
 		(void *)&cmd_flex_filter_port_id,
@@ -8352,7 +8379,9 @@ cmd_ethertype_filter_parsed(void *parsed_result,
 cmdline_parse_inst_t cmd_ethertype_filter = {
 	.f = cmd_ethertype_filter_parsed,
 	.data = NULL,
-	.help_str = "add or delete an ethertype filter entry",
+	.help_str = "ethertype_filter <port_id> add|del mac_addr|mac_ignr "
+		"<mac_addr> ethertype <value> drop|fw queue <queue_id>: "
+		"Add or delete an ethertype filter entry",
 	.tokens = {
 		(void *)&cmd_ethertype_filter_filter,
 		(void *)&cmd_ethertype_filter_port_id,
@@ -8840,7 +8869,14 @@ cmdline_parse_token_num_t cmd_flow_director_tunnel_id_value =
 cmdline_parse_inst_t cmd_add_del_ip_flow_director = {
 	.f = cmd_flow_director_filter_parsed,
 	.data = NULL,
-	.help_str = "add or delete an ip flow director entry on NIC",
+	.help_str = "flow_director_filter <port_id> mode IP add|del|update flow"
+		" ipv4-other|ipv4-frag|ipv4-tcp|ipv4-udp|ipv4-sctp|"
+		"ipv6-other|ipv6-frag|ipv6-tcp|ipv6-udp|ipv6-sctp|"
+		"l2_payload src <src_ip> dst <dst_ip> tos <tos_value> "
+		"proto <proto_value> ttl <ttl_value> vlan <vlan_value> "
+		"flexbytes <flexbyte_vaues> drop|fw <pf_vf> queue <queue_id> "
+		"fd_id <fd_id_value>: "
+		"Add or delete an ip flow director entry on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_filter,
 		(void *)&cmd_flow_director_port_id,
@@ -8876,7 +8912,8 @@ cmdline_parse_inst_t cmd_add_del_ip_flow_director = {
 cmdline_parse_inst_t cmd_add_del_udp_flow_director = {
 	.f = cmd_flow_director_filter_parsed,
 	.data = NULL,
-	.help_str = "add or delete an udp/tcp flow director entry on NIC",
+	.help_str = "flow_director_filter ... : Add or delete an udp/tcp flow "
+		"director entry on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_filter,
 		(void *)&cmd_flow_director_port_id,
@@ -8912,7 +8949,8 @@ cmdline_parse_inst_t cmd_add_del_udp_flow_director = {
 cmdline_parse_inst_t cmd_add_del_sctp_flow_director = {
 	.f = cmd_flow_director_filter_parsed,
 	.data = NULL,
-	.help_str = "add or delete a sctp flow director entry on NIC",
+	.help_str = "flow_director_filter ... : Add or delete a sctp flow "
+		"director entry on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_filter,
 		(void *)&cmd_flow_director_port_id,
@@ -8950,7 +8988,8 @@ cmdline_parse_inst_t cmd_add_del_sctp_flow_director = {
 cmdline_parse_inst_t cmd_add_del_l2_flow_director = {
 	.f = cmd_flow_director_filter_parsed,
 	.data = NULL,
-	.help_str = "add or delete a L2 flow director entry on NIC",
+	.help_str = "flow_director_filter ... : Add or delete a L2 flow "
+		"director entry on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_filter,
 		(void *)&cmd_flow_director_port_id,
@@ -8976,7 +9015,8 @@ cmdline_parse_inst_t cmd_add_del_l2_flow_director = {
 cmdline_parse_inst_t cmd_add_del_mac_vlan_flow_director = {
 	.f = cmd_flow_director_filter_parsed,
 	.data = NULL,
-	.help_str = "add or delete a MAC VLAN flow director entry on NIC",
+	.help_str = "flow_director_filter ... : Add or delete a MAC VLAN flow "
+		"director entry on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_filter,
 		(void *)&cmd_flow_director_port_id,
@@ -9001,7 +9041,8 @@ cmdline_parse_inst_t cmd_add_del_mac_vlan_flow_director = {
 cmdline_parse_inst_t cmd_add_del_tunnel_flow_director = {
 	.f = cmd_flow_director_filter_parsed,
 	.data = NULL,
-	.help_str = "add or delete a tunnel flow director entry on NIC",
+	.help_str = "flow_director_filter ... : Add or delete a tunnel flow "
+		"director entry on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_filter,
 		(void *)&cmd_flow_director_port_id,
@@ -9064,7 +9105,8 @@ cmd_flush_flow_director_parsed(void *parsed_result,
 cmdline_parse_inst_t cmd_flush_flow_director = {
 	.f = cmd_flush_flow_director_parsed,
 	.data = NULL,
-	.help_str = "flush all flow director entries of a device on NIC",
+	.help_str = "flush_flow_director <port_id>: "
+		"Flush all flow director entries of a device on NIC",
 	.tokens = {
 		(void *)&cmd_flush_flow_director_flush,
 		(void *)&cmd_flush_flow_director_port_id,
@@ -9225,7 +9267,8 @@ cmdline_parse_token_num_t cmd_flow_director_mask_tunnel_id_value =
 cmdline_parse_inst_t cmd_set_flow_director_ip_mask = {
 	.f = cmd_flow_director_mask_parsed,
 	.data = NULL,
-	.help_str = "set IP mode flow director's mask on NIC",
+	.help_str = "flow_director_mask ... : "
+		"Set IP mode flow director's mask on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_mask,
 		(void *)&cmd_flow_director_mask_port_id,
@@ -9248,7 +9291,8 @@ cmdline_parse_inst_t cmd_set_flow_director_ip_mask = {
 cmdline_parse_inst_t cmd_set_flow_director_mac_vlan_mask = {
 	.f = cmd_flow_director_mask_parsed,
 	.data = NULL,
-	.help_str = "set MAC VLAN mode flow director's mask on NIC",
+	.help_str = "flow_director_mask ... : Set MAC VLAN mode "
+		"flow director's mask on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_mask,
 		(void *)&cmd_flow_director_mask_port_id,
@@ -9263,7 +9307,8 @@ cmdline_parse_inst_t cmd_set_flow_director_mac_vlan_mask = {
 cmdline_parse_inst_t cmd_set_flow_director_tunnel_mask = {
 	.f = cmd_flow_director_mask_parsed,
 	.data = NULL,
-	.help_str = "set tunnel mode flow director's mask on NIC",
+	.help_str = "flow_director_mask ... : Set tunnel mode "
+		"flow director's mask on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_mask,
 		(void *)&cmd_flow_director_mask_port_id,
@@ -9391,7 +9436,8 @@ cmdline_parse_token_string_t cmd_flow_director_flexmask_mask =
 cmdline_parse_inst_t cmd_set_flow_director_flex_mask = {
 	.f = cmd_flow_director_flex_mask_parsed,
 	.data = NULL,
-	.help_str = "set flow director's flex mask on NIC",
+	.help_str = "flow_director_flex_mask ... : "
+		"Set flow director's flex mask on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_flexmask,
 		(void *)&cmd_flow_director_flexmask_port_id,
@@ -9509,7 +9555,8 @@ cmdline_parse_token_string_t cmd_flow_director_flexpayload_payload_cfg =
 cmdline_parse_inst_t cmd_set_flow_director_flex_payload = {
 	.f = cmd_flow_director_flxpld_parsed,
 	.data = NULL,
-	.help_str = "set flow director's flex payload on NIC",
+	.help_str = "flow_director_flexpayload ... : "
+		"Set flow director's flex payload on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_flexpayload,
 		(void *)&cmd_flow_director_flexpayload_port_id,
@@ -9567,7 +9614,7 @@ cmdline_parse_token_num_t cmd_get_sym_hash_ena_per_port_port_id =
 cmdline_parse_inst_t cmd_get_sym_hash_ena_per_port = {
 	.f = cmd_get_sym_hash_per_port_parsed,
 	.data = NULL,
-	.help_str = "get_sym_hash_ena_per_port port_id",
+	.help_str = "get_sym_hash_ena_per_port <port_id>",
 	.tokens = {
 		(void *)&cmd_get_sym_hash_ena_per_port_all,
 		(void *)&cmd_get_sym_hash_ena_per_port_port_id,
@@ -9626,7 +9673,7 @@ cmdline_parse_token_string_t cmd_set_sym_hash_ena_per_port_enable =
 cmdline_parse_inst_t cmd_set_sym_hash_ena_per_port = {
 	.f = cmd_set_sym_hash_per_port_parsed,
 	.data = NULL,
-	.help_str = "set_sym_hash_ena_per_port port_id enable|disable",
+	.help_str = "set_sym_hash_ena_per_port <port_id> enable|disable",
 	.tokens = {
 		(void *)&cmd_set_sym_hash_ena_per_port_all,
 		(void *)&cmd_set_sym_hash_ena_per_port_port_id,
@@ -9744,7 +9791,7 @@ cmdline_parse_token_num_t cmd_get_hash_global_config_port_id =
 cmdline_parse_inst_t cmd_get_hash_global_config = {
 	.f = cmd_get_hash_global_config_parsed,
 	.data = NULL,
-	.help_str = "get_hash_global_config port_id",
+	.help_str = "get_hash_global_config <port_id>",
 	.tokens = {
 		(void *)&cmd_get_hash_global_config_all,
 		(void *)&cmd_get_hash_global_config_port_id,
@@ -9827,11 +9874,11 @@ cmdline_parse_token_string_t cmd_set_hash_global_config_enable =
 cmdline_parse_inst_t cmd_set_hash_global_config = {
 	.f = cmd_set_hash_global_config_parsed,
 	.data = NULL,
-	.help_str = "set_hash_global_config port_id "
+	.help_str = "set_hash_global_config <port_id> "
 		"toeplitz|simple_xor|default "
-		"ipv4|ipv4-frag|ipv4-tcp|ipv4-udp|ipv4-sctp|ipv4-other|ipv6|"
-		"ipv6-frag|ipv6-tcp|ipv6-udp|ipv6-sctp|ipv6-other|l2_payload "
-		"enable|disable",
+		"ipv4|ipv4-frag|ipv4-tcp|ipv4-udp|ipv4-sctp|ipv4-other|"
+		"ipv6|ipv6-frag|ipv6-tcp|ipv6-udp|ipv6-sctp|ipv6-other|"
+		"l2_payload enable|disable",
 	.tokens = {
 		(void *)&cmd_set_hash_global_config_all,
 		(void *)&cmd_set_hash_global_config_port_id,
@@ -10082,7 +10129,8 @@ cmdline_parse_token_etheraddr_t cmd_mcast_addr_addr =
 cmdline_parse_inst_t cmd_mcast_addr = {
 	.f = cmd_mcast_addr_parsed,
 	.data = (void *)0,
-	.help_str = "mcast_addr add|remove X <mcast_addr>: add/remove multicast MAC address on port X",
+	.help_str = "mcast_addr add|remove <port_id> <mcast_addr>: "
+		"Add/Remove multicast MAC address on port_id",
 	.tokens = {
 		(void *)&cmd_mcast_addr_cmd,
 		(void *)&cmd_mcast_addr_what,
@@ -10182,7 +10230,7 @@ cmd_config_l2_tunnel_eth_type_all_parsed
 cmdline_parse_inst_t cmd_config_l2_tunnel_eth_type_all = {
 	.f = cmd_config_l2_tunnel_eth_type_all_parsed,
 	.data = NULL,
-	.help_str = "port config all l2-tunnel ether-type",
+	.help_str = "port config all l2-tunnel E-tag ether-type <value>",
 	.tokens = {
 		(void *)&cmd_config_l2_tunnel_eth_type_port,
 		(void *)&cmd_config_l2_tunnel_eth_type_config,
@@ -10218,7 +10266,7 @@ cmd_config_l2_tunnel_eth_type_specific_parsed(
 cmdline_parse_inst_t cmd_config_l2_tunnel_eth_type_specific = {
 	.f = cmd_config_l2_tunnel_eth_type_specific_parsed,
 	.data = NULL,
-	.help_str = "port config l2-tunnel ether-type",
+	.help_str = "port config <port_id> l2-tunnel E-tag ether-type <value>",
 	.tokens = {
 		(void *)&cmd_config_l2_tunnel_eth_type_port,
 		(void *)&cmd_config_l2_tunnel_eth_type_config,
@@ -10301,7 +10349,7 @@ cmd_config_l2_tunnel_en_dis_all_parsed(
 cmdline_parse_inst_t cmd_config_l2_tunnel_en_dis_all = {
 	.f = cmd_config_l2_tunnel_en_dis_all_parsed,
 	.data = NULL,
-	.help_str = "port config all l2-tunnel enable/disable",
+	.help_str = "port config all l2-tunnel E-tag enable|disable",
 	.tokens = {
 		(void *)&cmd_config_l2_tunnel_en_dis_port,
 		(void *)&cmd_config_l2_tunnel_en_dis_config,
@@ -10344,7 +10392,7 @@ cmd_config_l2_tunnel_en_dis_specific_parsed(
 cmdline_parse_inst_t cmd_config_l2_tunnel_en_dis_specific = {
 	.f = cmd_config_l2_tunnel_en_dis_specific_parsed,
 	.data = NULL,
-	.help_str = "port config l2-tunnel enable/disable",
+	.help_str = "port config <port_id> l2-tunnel E-tag enable|disable",
 	.tokens = {
 		(void *)&cmd_config_l2_tunnel_en_dis_port,
 		(void *)&cmd_config_l2_tunnel_en_dis_config,
@@ -10517,7 +10565,7 @@ cmd_config_e_tag_insertion_dis_parsed(
 cmdline_parse_inst_t cmd_config_e_tag_insertion_en = {
 	.f = cmd_config_e_tag_insertion_en_parsed,
 	.data = NULL,
-	.help_str = "E-tag insertion enable",
+	.help_str = "E-tag ... : E-tag insertion enable",
 	.tokens = {
 		(void *)&cmd_config_e_tag_e_tag,
 		(void *)&cmd_config_e_tag_set,
@@ -10536,7 +10584,7 @@ cmdline_parse_inst_t cmd_config_e_tag_insertion_en = {
 cmdline_parse_inst_t cmd_config_e_tag_insertion_dis = {
 	.f = cmd_config_e_tag_insertion_dis_parsed,
 	.data = NULL,
-	.help_str = "E-tag insertion disable",
+	.help_str = "E-tag ... : E-tag insertion disable",
 	.tokens = {
 		(void *)&cmd_config_e_tag_e_tag,
 		(void *)&cmd_config_e_tag_set,
@@ -10583,7 +10631,7 @@ cmd_config_e_tag_stripping_parsed(
 cmdline_parse_inst_t cmd_config_e_tag_stripping_en_dis = {
 	.f = cmd_config_e_tag_stripping_parsed,
 	.data = NULL,
-	.help_str = "E-tag stripping enable/disable",
+	.help_str = "E-tag ... : E-tag stripping enable/disable",
 	.tokens = {
 		(void *)&cmd_config_e_tag_e_tag,
 		(void *)&cmd_config_e_tag_set,
@@ -10627,7 +10675,7 @@ cmd_config_e_tag_forwarding_parsed(
 cmdline_parse_inst_t cmd_config_e_tag_forwarding_en_dis = {
 	.f = cmd_config_e_tag_forwarding_parsed,
 	.data = NULL,
-	.help_str = "E-tag forwarding enable/disable",
+	.help_str = "E-tag ... : E-tag forwarding enable/disable",
 	.tokens = {
 		(void *)&cmd_config_e_tag_e_tag,
 		(void *)&cmd_config_e_tag_set,
@@ -10682,7 +10730,7 @@ cmd_config_e_tag_filter_add_parsed(
 cmdline_parse_inst_t cmd_config_e_tag_filter_add = {
 	.f = cmd_config_e_tag_filter_add_parsed,
 	.data = NULL,
-	.help_str = "E-tag filter add",
+	.help_str = "E-tag ... : E-tag filter add",
 	.tokens = {
 		(void *)&cmd_config_e_tag_e_tag,
 		(void *)&cmd_config_e_tag_set,
@@ -10739,7 +10787,7 @@ cmd_config_e_tag_filter_del_parsed(
 cmdline_parse_inst_t cmd_config_e_tag_filter_del = {
 	.f = cmd_config_e_tag_filter_del_parsed,
 	.data = NULL,
-	.help_str = "E-tag filter delete",
+	.help_str = "E-tag ... : E-tag filter delete",
 	.tokens = {
 		(void *)&cmd_config_e_tag_e_tag,
 		(void *)&cmd_config_e_tag_set,
@@ -10826,7 +10874,7 @@ cmd_set_vf_vlan_anti_spoof_parsed(
 cmdline_parse_inst_t cmd_set_vf_vlan_anti_spoof = {
 	.f = cmd_set_vf_vlan_anti_spoof_parsed,
 	.data = NULL,
-	.help_str = "set vf vlan antispoof port_id vf_id on|off",
+	.help_str = "set vf vlan antispoof <port_id> <vf_id> on|off",
 	.tokens = {
 		(void *)&cmd_vf_vlan_anti_spoof_set,
 		(void *)&cmd_vf_vlan_anti_spoof_vf,
@@ -10911,7 +10959,7 @@ cmd_set_vf_mac_anti_spoof_parsed(
 cmdline_parse_inst_t cmd_set_vf_mac_anti_spoof = {
 	.f = cmd_set_vf_mac_anti_spoof_parsed,
 	.data = NULL,
-	.help_str = "set vf mac antispoof port_id vf_id on|off",
+	.help_str = "set vf mac antispoof <port_id> <vf_id> on|off",
 	.tokens = {
 		(void *)&cmd_vf_mac_anti_spoof_set,
 		(void *)&cmd_vf_mac_anti_spoof_vf,
@@ -10995,7 +11043,7 @@ cmd_set_vf_vlan_stripq_parsed(
 cmdline_parse_inst_t cmd_set_vf_vlan_stripq = {
 	.f = cmd_set_vf_vlan_stripq_parsed,
 	.data = NULL,
-	.help_str = "set vf vlan stripq port_id vf_id on|off",
+	.help_str = "set vf vlan stripq <port_id> <vf_id> on|off",
 	.tokens = {
 		(void *)&cmd_vf_vlan_stripq_set,
 		(void *)&cmd_vf_vlan_stripq_vf,
@@ -11078,7 +11126,7 @@ cmd_set_vf_vlan_insert_parsed(
 cmdline_parse_inst_t cmd_set_vf_vlan_insert = {
 	.f = cmd_set_vf_vlan_insert_parsed,
 	.data = NULL,
-	.help_str = "set vf vlan insert port_id vf_id vlan_id",
+	.help_str = "set vf vlan insert <port_id> <vf_id> <vlan_id>",
 	.tokens = {
 		(void *)&cmd_vf_vlan_insert_set,
 		(void *)&cmd_vf_vlan_insert_vf,
@@ -11152,7 +11200,7 @@ cmd_set_tx_loopback_parsed(
 cmdline_parse_inst_t cmd_set_tx_loopback = {
 	.f = cmd_set_tx_loopback_parsed,
 	.data = NULL,
-	.help_str = "set tx loopback port_id on|off",
+	.help_str = "set tx loopback <port_id> on|off",
 	.tokens = {
 		(void *)&cmd_tx_loopback_set,
 		(void *)&cmd_tx_loopback_tx,
@@ -11229,7 +11277,7 @@ cmd_set_all_queues_drop_en_parsed(
 cmdline_parse_inst_t cmd_set_all_queues_drop_en = {
 	.f = cmd_set_all_queues_drop_en_parsed,
 	.data = NULL,
-	.help_str = "set all queues drop port_id on|off",
+	.help_str = "set all queues drop <port_id> on|off",
 	.tokens = {
 		(void *)&cmd_all_queues_drop_en_set,
 		(void *)&cmd_all_queues_drop_en_all,
@@ -11313,7 +11361,7 @@ cmd_set_vf_split_drop_en_parsed(
 cmdline_parse_inst_t cmd_set_vf_split_drop_en = {
 	.f = cmd_set_vf_split_drop_en_parsed,
 	.data = NULL,
-	.help_str = "set vf split drop port_id vf_id on|off",
+	.help_str = "set vf split drop <port_id> <vf_id> on|off",
 	.tokens = {
 		(void *)&cmd_vf_split_drop_en_set,
 		(void *)&cmd_vf_split_drop_en_vf,
@@ -11397,7 +11445,7 @@ cmd_set_vf_mac_addr_parsed(
 cmdline_parse_inst_t cmd_set_vf_mac_addr = {
 	.f = cmd_set_vf_mac_addr_parsed,
 	.data = NULL,
-	.help_str = "set vf mac addr port_id vf_id xx:xx:xx:xx:xx:xx",
+	.help_str = "set vf mac addr <port_id> <vf_id> <mac_addr>",
 	.tokens = {
 		(void *)&cmd_set_vf_mac_addr_set,
 		(void *)&cmd_set_vf_mac_addr_vf,
-- 
2.9.3

^ permalink raw reply related

* [PATCH] app/testpmd: unify help strings
From: Ferruh Yigit @ 2016-12-02  1:37 UTC (permalink / raw)
  To: dev; +Cc: Pablo de Lara, Jingjing Wu

Formatted as:
cmd fixed_string fixed|string|options <variable>: Description

If there is no description, final colon emitted.

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 app/test-pmd/cmdline.c | 465 ++++++++++++++++++++++++++-----------------------
 1 file changed, 250 insertions(+), 215 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 63b55dc..47da22b 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -127,7 +127,7 @@ cmdline_parse_token_string_t cmd_help_brief_help =
 cmdline_parse_inst_t cmd_help_brief = {
 	.f = cmd_help_brief_parsed,
 	.data = NULL,
-	.help_str = "show help",
+	.help_str = "help: Show help",
 	.tokens = {
 		(void *)&cmd_help_brief_help,
 		NULL,
@@ -824,7 +824,8 @@ cmdline_parse_token_string_t cmd_help_long_section =
 cmdline_parse_inst_t cmd_help_long = {
 	.f = cmd_help_long_parsed,
 	.data = NULL,
-	.help_str = "show help",
+	.help_str = "help all|control|display|config|ports|register|filters: "
+		"Show help",
 	.tokens = {
 		(void *)&cmd_help_long_help,
 		(void *)&cmd_help_long_section,
@@ -868,7 +869,7 @@ cmdline_parse_token_string_t cmd_operate_port_all_all =
 cmdline_parse_inst_t cmd_operate_port = {
 	.f = cmd_operate_port_parsed,
 	.data = NULL,
-	.help_str = "port start|stop|close all: start/stop/close all ports",
+	.help_str = "port start|stop|close all: Start/Stop/Close all ports",
 	.tokens = {
 		(void *)&cmd_operate_port_all_cmd,
 		(void *)&cmd_operate_port_all_port,
@@ -913,7 +914,7 @@ cmdline_parse_token_num_t cmd_operate_specific_port_id =
 cmdline_parse_inst_t cmd_operate_specific_port = {
 	.f = cmd_operate_specific_port_parsed,
 	.data = NULL,
-	.help_str = "port start|stop|close X: start/stop/close port X",
+	.help_str = "port start|stop|close <port_id>: Start/Stop/Close port_id",
 	.tokens = {
 		(void *)&cmd_operate_specific_port_cmd,
 		(void *)&cmd_operate_specific_port_port,
@@ -954,8 +955,8 @@ cmdline_parse_token_string_t cmd_operate_attach_port_identifier =
 cmdline_parse_inst_t cmd_operate_attach_port = {
 	.f = cmd_operate_attach_port_parsed,
 	.data = NULL,
-	.help_str = "port attach identifier, "
-		"identifier: pci address or virtual dev name",
+	.help_str = "port attach <identifier>: "
+		"(identifier: pci address or virtual dev name)",
 	.tokens = {
 		(void *)&cmd_operate_attach_port_port,
 		(void *)&cmd_operate_attach_port_keyword,
@@ -996,7 +997,7 @@ cmdline_parse_token_num_t cmd_operate_detach_port_port_id =
 cmdline_parse_inst_t cmd_operate_detach_port = {
 	.f = cmd_operate_detach_port_parsed,
 	.data = NULL,
-	.help_str = "port detach port_id",
+	.help_str = "port detach <port_id>",
 	.tokens = {
 		(void *)&cmd_operate_detach_port_port,
 		(void *)&cmd_operate_detach_port_keyword,
@@ -1189,7 +1190,7 @@ cmdline_parse_token_string_t cmd_config_speed_specific_value2 =
 cmdline_parse_inst_t cmd_config_speed_specific = {
 	.f = cmd_config_speed_specific_parsed,
 	.data = NULL,
-	.help_str = "port config X speed "
+	.help_str = "port config <port_id> speed "
 		"10|100|1000|10000|25000|40000|50000|100000|auto duplex "
 							"half|full|auto",
 	.tokens = {
@@ -1279,7 +1280,7 @@ cmdline_parse_token_num_t cmd_config_rx_tx_value =
 cmdline_parse_inst_t cmd_config_rx_tx = {
 	.f = cmd_config_rx_tx_parsed,
 	.data = NULL,
-	.help_str = "port config all rxq|txq|rxd|txd value",
+	.help_str = "port config all rxq|txq|rxd|txd <value>",
 	.tokens = {
 		(void *)&cmd_config_rx_tx_port,
 		(void *)&cmd_config_rx_tx_keyword,
@@ -1354,7 +1355,7 @@ cmdline_parse_token_num_t cmd_config_max_pkt_len_value =
 cmdline_parse_inst_t cmd_config_max_pkt_len = {
 	.f = cmd_config_max_pkt_len_parsed,
 	.data = NULL,
-	.help_str = "port config all max-pkt-len value",
+	.help_str = "port config all max-pkt-len <value>",
 	.tokens = {
 		(void *)&cmd_config_max_pkt_len_port,
 		(void *)&cmd_config_max_pkt_len_keyword,
@@ -1405,7 +1406,7 @@ cmdline_parse_token_num_t cmd_config_mtu_value =
 cmdline_parse_inst_t cmd_config_mtu = {
 	.f = cmd_config_mtu_parsed,
 	.data = NULL,
-	.help_str = "port config mtu port_id value",
+	.help_str = "port config mtu <port_id> <value>",
 	.tokens = {
 		(void *)&cmd_config_mtu_port,
 		(void *)&cmd_config_mtu_keyword,
@@ -1625,7 +1626,8 @@ cmdline_parse_token_string_t cmd_config_rss_value =
 cmdline_parse_inst_t cmd_config_rss = {
 	.f = cmd_config_rss_parsed,
 	.data = NULL,
-	.help_str = "port config all rss all|ip|tcp|udp|sctp|ether|port|vxlan|geneve|nvgre|none",
+	.help_str = "port config all rss "
+		"all|ip|tcp|udp|sctp|ether|port|vxlan|geneve|nvgre|none",
 	.tokens = {
 		(void *)&cmd_config_rss_port,
 		(void *)&cmd_config_rss_keyword,
@@ -1738,12 +1740,11 @@ cmdline_parse_token_string_t cmd_config_rss_hash_key_value =
 cmdline_parse_inst_t cmd_config_rss_hash_key = {
 	.f = cmd_config_rss_hash_key_parsed,
 	.data = NULL,
-	.help_str =
-		"port config X rss-hash-key ipv4|ipv4-frag|ipv4-tcp|ipv4-udp|"
-		"ipv4-sctp|ipv4-other|ipv6|ipv6-frag|ipv6-tcp|ipv6-udp|"
-		"ipv6-sctp|ipv6-other|l2-payload|"
-		"ipv6-ex|ipv6-tcp-ex|ipv6-udp-ex "
-		"<string of hexa digits (variable length, NIC dependent)>\n",
+	.help_str = "port config <port_id> rss-hash-key "
+		"ipv4|ipv4-frag|ipv4-tcp|ipv4-udp|ipv4-sctp|ipv4-other|"
+		"ipv6|ipv6-frag|ipv6-tcp|ipv6-udp|ipv6-sctp|ipv6-other|"
+		"l2-payload|ipv6-ex|ipv6-tcp-ex|ipv6-udp-ex "
+		"<string of hexa digits (variable length, NIC dependent)>",
 	.tokens = {
 		(void *)&cmd_config_rss_hash_key_port,
 		(void *)&cmd_config_rss_hash_key_config,
@@ -1838,7 +1839,7 @@ cmdline_parse_token_string_t cmd_config_rxtx_queue_opname =
 cmdline_parse_inst_t cmd_config_rxtx_queue = {
 	.f = cmd_config_rxtx_queue_parsed,
 	.data = NULL,
-	.help_str = "port X rxq|txq ID start|stop",
+	.help_str = "port <port_id> rxq|txq <queue_id> start|stop",
 	.tokens = {
 		(void *)&cmd_config_speed_all_port,
 		(void *)&cmd_config_rxtx_queue_portid,
@@ -1973,7 +1974,7 @@ cmdline_parse_token_string_t cmd_config_rss_reta_list_of_items =
 cmdline_parse_inst_t cmd_config_rss_reta = {
 	.f = cmd_set_rss_reta_parsed,
 	.data = NULL,
-	.help_str = "port config X rss reta (hash,queue)[,(hash,queue)]",
+	.help_str = "port config <port_id> rss reta <hash,queue[,hash,queue]*>",
 	.tokens = {
 		(void *)&cmd_config_rss_reta_port,
 		(void *)&cmd_config_rss_reta_keyword,
@@ -2080,7 +2081,7 @@ cmdline_parse_token_string_t cmd_showport_reta_list_of_items =
 cmdline_parse_inst_t cmd_showport_reta = {
 	.f = cmd_showport_reta_parsed,
 	.data = NULL,
-	.help_str = "show port X rss reta (size) (mask0,mask1,...)",
+	.help_str = "show port <port_id> rss reta <size> <mask0[,mask1]*>",
 	.tokens = {
 		(void *)&cmd_showport_reta_show,
 		(void *)&cmd_showport_reta_port,
@@ -2134,11 +2135,10 @@ cmdline_parse_token_string_t cmd_showport_rss_hash_rss_key =
 cmdline_parse_inst_t cmd_showport_rss_hash = {
 	.f = cmd_showport_rss_hash_parsed,
 	.data = NULL,
-	.help_str =
-		"show port X rss-hash ipv4|ipv4-frag|ipv4-tcp|ipv4-udp|"
-		"ipv4-sctp|ipv4-other|ipv6|ipv6-frag|ipv6-tcp|ipv6-udp|"
-		"ipv6-sctp|ipv6-other|l2-payload|"
-		"ipv6-ex|ipv6-tcp-ex|ipv6-udp-ex (X = port number)\n",
+	.help_str = "show port <port_id> rss-hash "
+		"ipv4|ipv4-frag|ipv4-tcp|ipv4-udp|ipv4-sctp|ipv4-other|"
+		"ipv6|ipv6-frag|ipv6-tcp|ipv6-udp|ipv6-sctp|ipv6-other|"
+		"l2-payload|ipv6-ex|ipv6-tcp-ex|ipv6-udp-ex",
 	.tokens = {
 		(void *)&cmd_showport_rss_hash_show,
 		(void *)&cmd_showport_rss_hash_port,
@@ -2152,11 +2152,10 @@ cmdline_parse_inst_t cmd_showport_rss_hash = {
 cmdline_parse_inst_t cmd_showport_rss_hash_key = {
 	.f = cmd_showport_rss_hash_parsed,
 	.data = (void *)1,
-	.help_str =
-		"show port X rss-hash ipv4|ipv4-frag|ipv4-tcp|ipv4-udp|"
-		"ipv4-sctp|ipv4-other|ipv6|ipv6-frag|ipv6-tcp|ipv6-udp|"
-		"ipv6-sctp|ipv6-other|l2-payload|"
-		"ipv6-ex|ipv6-tcp-ex|ipv6-udp-ex key (X = port number)\n",
+	.help_str = "show port <port_id> rss-hash "
+		"ipv4|ipv4-frag|ipv4-tcp|ipv4-udp|ipv4-sctp|ipv4-other|"
+		"ipv6|ipv6-frag|ipv6-tcp|ipv6-udp|ipv6-sctp|ipv6-other|"
+		"l2-payload|ipv6-ex|ipv6-tcp-ex|ipv6-udp-ex key",
 	.tokens = {
 		(void *)&cmd_showport_rss_hash_show,
 		(void *)&cmd_showport_rss_hash_port,
@@ -2255,7 +2254,7 @@ cmdline_parse_token_string_t cmd_config_dcb_pfc_en =
 cmdline_parse_inst_t cmd_config_dcb = {
         .f = cmd_config_dcb_parsed,
         .data = NULL,
-        .help_str = "port config port-id dcb vt on|off nb-tcs pfc on|off",
+        .help_str = "port config <port-id> dcb vt on|off <num_tcs> pfc on|off",
         .tokens = {
 		(void *)&cmd_config_dcb_port,
 		(void *)&cmd_config_dcb_config,
@@ -2321,7 +2320,7 @@ cmdline_parse_token_num_t cmd_config_burst_value =
 cmdline_parse_inst_t cmd_config_burst = {
 	.f = cmd_config_burst_parsed,
 	.data = NULL,
-	.help_str = "port config all burst value",
+	.help_str = "port config all burst <value>",
 	.tokens = {
 		(void *)&cmd_config_burst_port,
 		(void *)&cmd_config_burst_keyword,
@@ -2390,7 +2389,7 @@ cmdline_parse_token_num_t cmd_config_thresh_value =
 cmdline_parse_inst_t cmd_config_thresh = {
 	.f = cmd_config_thresh_parsed,
 	.data = NULL,
-	.help_str = "port config all txpt|txht|txwt|rxpt|rxht|rxwt value",
+	.help_str = "port config all txpt|txht|txwt|rxpt|rxht|rxwt <value>",
 	.tokens = {
 		(void *)&cmd_config_thresh_port,
 		(void *)&cmd_config_thresh_keyword,
@@ -2454,7 +2453,7 @@ cmdline_parse_token_num_t cmd_config_threshold_value =
 cmdline_parse_inst_t cmd_config_threshold = {
 	.f = cmd_config_threshold_parsed,
 	.data = NULL,
-	.help_str = "port config all txfreet|txrst|rxfreet value",
+	.help_str = "port config all txfreet|txrst|rxfreet <value>",
 	.tokens = {
 		(void *)&cmd_config_threshold_port,
 		(void *)&cmd_config_threshold_keyword,
@@ -2483,7 +2482,7 @@ cmdline_parse_token_string_t cmd_stop_stop =
 cmdline_parse_inst_t cmd_stop = {
 	.f = cmd_stop_parsed,
 	.data = NULL,
-	.help_str = "stop - stop packet forwarding",
+	.help_str = "stop: Stop packet forwarding",
 	.tokens = {
 		(void *)&cmd_stop_stop,
 		NULL,
@@ -2613,7 +2612,7 @@ cmdline_parse_token_string_t cmd_set_list_of_items =
 cmdline_parse_inst_t cmd_set_fwd_list = {
 	.f = cmd_set_list_parsed,
 	.data = NULL,
-	.help_str = "set corelist|portlist x[,y]*",
+	.help_str = "set corelist|portlist <list0[,list1]*>",
 	.tokens = {
 		(void *)&cmd_set_list_keyword,
 		(void *)&cmd_set_list_name,
@@ -2660,7 +2659,7 @@ cmdline_parse_token_num_t cmd_setmask_value =
 cmdline_parse_inst_t cmd_set_fwd_mask = {
 	.f = cmd_set_mask_parsed,
 	.data = NULL,
-	.help_str = "set coremask|portmask hexadecimal value",
+	.help_str = "set coremask|portmask <hexadecimal value>",
 	.tokens = {
 		(void *)&cmd_setmask_set,
 		(void *)&cmd_setmask_mask,
@@ -2706,7 +2705,7 @@ cmdline_parse_token_num_t cmd_set_value =
 cmdline_parse_inst_t cmd_set_numbers = {
 	.f = cmd_set_parsed,
 	.data = NULL,
-	.help_str = "set nbport|nbcore|burst|verbose value",
+	.help_str = "set nbport|nbcore|burst|verbose <value>",
 	.tokens = {
 		(void *)&cmd_set_set,
 		(void *)&cmd_set_what,
@@ -2752,7 +2751,7 @@ cmdline_parse_token_string_t cmd_set_txpkts_lengths =
 cmdline_parse_inst_t cmd_set_txpkts = {
 	.f = cmd_set_txpkts_parsed,
 	.data = NULL,
-	.help_str = "set txpkts x[,y]*",
+	.help_str = "set txpkts <len0[,len1]*>",
 	.tokens = {
 		(void *)&cmd_set_txpkts_keyword,
 		(void *)&cmd_set_txpkts_name,
@@ -2859,7 +2858,7 @@ cmdline_parse_token_num_t cmd_config_txqflags_value =
 cmdline_parse_inst_t cmd_config_txqflags = {
 	.f = cmd_config_txqflags_parsed,
 	.data = NULL,
-	.help_str = "port config all txqflags value",
+	.help_str = "port config all txqflags <value>",
 	.tokens = {
 		(void *)&cmd_config_txqflags_port,
 		(void *)&cmd_config_txqflags_config,
@@ -2907,8 +2906,9 @@ cmdline_parse_token_num_t cmd_rx_vlan_filter_all_portid =
 cmdline_parse_inst_t cmd_rx_vlan_filter_all = {
 	.f = cmd_rx_vlan_filter_all_parsed,
 	.data = NULL,
-	.help_str = "add/remove all identifiers to/from the set of VLAN "
-	"Identifiers filtered by a port",
+	.help_str = "rx_vlan add|rm all <port_id>: "
+		"Add/Remove all identifiers to/from the set of VLAN "
+		"identifiers filtered by a port",
 	.tokens = {
 		(void *)&cmd_rx_vlan_filter_all_rx_vlan,
 		(void *)&cmd_rx_vlan_filter_all_what,
@@ -3007,8 +3007,8 @@ cmdline_parse_token_string_t cmd_vlan_offload_portid =
 cmdline_parse_inst_t cmd_vlan_offload = {
 	.f = cmd_vlan_offload_parsed,
 	.data = NULL,
-	.help_str = "set strip|filter|qinq|stripq on|off port_id[,queue_id], filter/strip for rx side"
-	" qinq(extended) for both rx/tx sides ",
+	.help_str = "vlan set strip|filter|qinq|stripq on|off <port_id[,queue_id]>: "
+		"Filter/Strip for rx side qinq(extended) for both rx/tx sides",
 	.tokens = {
 		(void *)&cmd_vlan_offload_vlan,
 		(void *)&cmd_vlan_offload_set,
@@ -3070,8 +3070,8 @@ cmdline_parse_token_num_t cmd_vlan_tpid_portid =
 cmdline_parse_inst_t cmd_vlan_tpid = {
 	.f = cmd_vlan_tpid_parsed,
 	.data = NULL,
-	.help_str = "set inner|outer tpid tp_id port_id, set the VLAN "
-		    "Ether type",
+	.help_str = "vlan set inner|outer tpid <tp_id> <port_id>: "
+		"Set the VLAN Ether type",
 	.tokens = {
 		(void *)&cmd_vlan_tpid_vlan,
 		(void *)&cmd_vlan_tpid_set,
@@ -3120,8 +3120,9 @@ cmdline_parse_token_num_t cmd_rx_vlan_filter_portid =
 cmdline_parse_inst_t cmd_rx_vlan_filter = {
 	.f = cmd_rx_vlan_filter_parsed,
 	.data = NULL,
-	.help_str = "add/remove a VLAN identifier to/from the set of VLAN "
-	"Identifiers filtered by a port",
+	.help_str = "rx_vlan add|rm <vlan_id> <port_id>: "
+		"Add/Remove a VLAN identifier to/from the set of VLAN "
+		"identifiers filtered by a port",
 	.tokens = {
 		(void *)&cmd_rx_vlan_filter_rx_vlan,
 		(void *)&cmd_rx_vlan_filter_what,
@@ -3165,7 +3166,8 @@ cmdline_parse_token_num_t cmd_tx_vlan_set_vlanid =
 cmdline_parse_inst_t cmd_tx_vlan_set = {
 	.f = cmd_tx_vlan_set_parsed,
 	.data = NULL,
-	.help_str = "enable hardware insertion of a single VLAN header "
+	.help_str = "tx_vlan set <port_id> <vlan_id>: "
+		"Enable hardware insertion of a single VLAN header "
 		"with a given TAG Identifier in packets sent on a port",
 	.tokens = {
 		(void *)&cmd_tx_vlan_set_tx_vlan,
@@ -3214,7 +3216,8 @@ cmdline_parse_token_num_t cmd_tx_vlan_set_qinq_vlanid_outer =
 cmdline_parse_inst_t cmd_tx_vlan_set_qinq = {
 	.f = cmd_tx_vlan_set_qinq_parsed,
 	.data = NULL,
-	.help_str = "enable hardware insertion of double VLAN header "
+	.help_str = "tx_vlan set <port_id> <vlan_id> <outer_vlan_id>: "
+		"Enable hardware insertion of double VLAN header "
 		"with given TAG Identifiers in packets sent on a port",
 	.tokens = {
 		(void *)&cmd_tx_vlan_set_qinq_tx_vlan,
@@ -3271,7 +3274,7 @@ cmdline_parse_token_string_t cmd_tx_vlan_set_pvid_mode =
 cmdline_parse_inst_t cmd_tx_vlan_set_pvid = {
 	.f = cmd_tx_vlan_set_pvid_parsed,
 	.data = NULL,
-	.help_str = "tx_vlan set pvid port_id vlan_id (on|off)",
+	.help_str = "tx_vlan set pvid <port_id> <vlan_id> on|off",
 	.tokens = {
 		(void *)&cmd_tx_vlan_set_pvid_tx_vlan,
 		(void *)&cmd_tx_vlan_set_pvid_set,
@@ -3313,8 +3316,8 @@ cmdline_parse_token_num_t cmd_tx_vlan_reset_portid =
 cmdline_parse_inst_t cmd_tx_vlan_reset = {
 	.f = cmd_tx_vlan_reset_parsed,
 	.data = NULL,
-	.help_str = "disable hardware insertion of a VLAN header in packets "
-	"sent on a port",
+	.help_str = "tx_vlan reset <port_id>: Disable hardware insertion of a "
+		"VLAN header in packets sent on a port",
 	.tokens = {
 		(void *)&cmd_tx_vlan_reset_tx_vlan,
 		(void *)&cmd_tx_vlan_reset_reset,
@@ -3440,8 +3443,9 @@ cmdline_parse_token_num_t cmd_csum_portid =
 cmdline_parse_inst_t cmd_csum_set = {
 	.f = cmd_csum_parsed,
 	.data = NULL,
-	.help_str = "enable/disable hardware calculation of L3/L4 checksum when "
-		"using csum forward engine: csum set ip|tcp|udp|sctp|outer-ip hw|sw <port>",
+	.help_str = "csum set ip|tcp|udp|sctp|outer-ip hw|sw <port_id>: "
+		"Enable/Disable hardware calculation of L3/L4 checksum when "
+		"using csum forward engine",
 	.tokens = {
 		(void *)&cmd_csum_csum,
 		(void *)&cmd_csum_mode,
@@ -3459,7 +3463,7 @@ cmdline_parse_token_string_t cmd_csum_mode_show =
 cmdline_parse_inst_t cmd_csum_show = {
 	.f = cmd_csum_parsed,
 	.data = NULL,
-	.help_str = "show checksum offload configuration: csum show <port>",
+	.help_str = "csum show <port_id>: Show checksum offload configuration",
 	.tokens = {
 		(void *)&cmd_csum_csum,
 		(void *)&cmd_csum_mode_show,
@@ -3512,8 +3516,8 @@ cmdline_parse_token_num_t cmd_csum_tunnel_portid =
 cmdline_parse_inst_t cmd_csum_tunnel = {
 	.f = cmd_csum_tunnel_parsed,
 	.data = NULL,
-	.help_str = "enable/disable parsing of tunnels for csum engine: "
-	"csum parse_tunnel on|off <tx-port>",
+	.help_str = "csum parse_tunnel on|off <port_id>: "
+		"Enable/Disable parsing of tunnels for csum engine",
 	.tokens = {
 		(void *)&cmd_csum_tunnel_csum,
 		(void *)&cmd_csum_tunnel_parse,
@@ -3576,8 +3580,9 @@ cmdline_parse_token_num_t cmd_tso_set_portid =
 cmdline_parse_inst_t cmd_tso_set = {
 	.f = cmd_tso_set_parsed,
 	.data = NULL,
-	.help_str = "Set TSO segment size of non-tunneled packets "
-	"for csum engine (0 to disable): tso set <tso_segsz> <port>",
+	.help_str = "tso set <tso_segsz> <port_id>: "
+		"Set TSO segment size of non-tunneled packets for csum engine "
+		"(0 to disable)",
 	.tokens = {
 		(void *)&cmd_tso_set_tso,
 		(void *)&cmd_tso_set_mode,
@@ -3595,8 +3600,8 @@ cmdline_parse_token_string_t cmd_tso_show_mode =
 cmdline_parse_inst_t cmd_tso_show = {
 	.f = cmd_tso_set_parsed,
 	.data = NULL,
-	.help_str = "Show TSO segment size of non-tunneled packets "
-	"for csum engine: tso show <port>",
+	.help_str = "tso show <port_id>: "
+		"Show TSO segment size of non-tunneled packets for csum engine",
 	.tokens = {
 		(void *)&cmd_tso_set_tso,
 		(void *)&cmd_tso_show_mode,
@@ -3692,8 +3697,9 @@ cmdline_parse_token_num_t cmd_tunnel_tso_set_portid =
 cmdline_parse_inst_t cmd_tunnel_tso_set = {
 	.f = cmd_tunnel_tso_set_parsed,
 	.data = NULL,
-	.help_str = "Set TSO segment size of tunneled packets for csum engine "
-	"(0 to disable): tunnel_tso set <tso_segsz> <port>",
+	.help_str = "tunnel_tso set <tso_segsz> <port_id>: "
+		"Set TSO segment size of tunneled packets for csum engine "
+		"(0 to disable)",
 	.tokens = {
 		(void *)&cmd_tunnel_tso_set_tso,
 		(void *)&cmd_tunnel_tso_set_mode,
@@ -3711,8 +3717,8 @@ cmdline_parse_token_string_t cmd_tunnel_tso_show_mode =
 cmdline_parse_inst_t cmd_tunnel_tso_show = {
 	.f = cmd_tunnel_tso_set_parsed,
 	.data = NULL,
-	.help_str = "Show TSO segment size of tunneled packets "
-	"for csum engine: tunnel_tso show <port>",
+	.help_str = "tunnel_tso show <port_id> "
+		"Show TSO segment size of tunneled packets for csum engine",
 	.tokens = {
 		(void *)&cmd_tunnel_tso_set_tso,
 		(void *)&cmd_tunnel_tso_show_mode,
@@ -3750,7 +3756,7 @@ cmdline_parse_token_string_t cmd_setflushrx_mode =
 
 cmdline_parse_inst_t cmd_set_flush_rx = {
 	.f = cmd_set_flush_rx_parsed,
-	.help_str = "set flush_rx on|off: enable/disable flush on rx streams",
+	.help_str = "set flush_rx on|off: Enable/Disable flush on rx streams",
 	.data = NULL,
 	.tokens = {
 		(void *)&cmd_setflushrx_set,
@@ -3789,7 +3795,7 @@ cmdline_parse_token_string_t cmd_setlinkcheck_mode =
 
 cmdline_parse_inst_t cmd_set_link_check = {
 	.f = cmd_set_link_check_parsed,
-	.help_str = "set link_check on|off: enable/disable link status check "
+	.help_str = "set link_check on|off: Enable/Disable link status check "
 	            "when starting/stopping a port",
 	.data = NULL,
 	.tokens = {
@@ -3850,7 +3856,7 @@ cmdline_parse_token_num_t cmd_setbypass_mode_port =
 
 cmdline_parse_inst_t cmd_set_bypass_mode = {
 	.f = cmd_set_bypass_mode_parsed,
-	.help_str = "set bypass mode (normal|bypass|isolate) (port_id): "
+	.help_str = "set bypass mode normal|bypass|isolate <port_id>: "
 	            "Set the NIC bypass mode for port_id",
 	.data = NULL,
 	.tokens = {
@@ -3950,8 +3956,8 @@ cmdline_parse_token_num_t cmd_setbypass_event_port =
 
 cmdline_parse_inst_t cmd_set_bypass_event = {
 	.f = cmd_set_bypass_event_parsed,
-	.help_str = "set bypass event (timeout|os_on|os_off|power_on|power_off) "
-	            "mode (normal|bypass|isolate) (port_id): "
+	.help_str = "set bypass event none|timeout|os_on|os_off|power_on|power_off "
+	            "mode normal|bypass|isolate <port_id>: "
 	            "Set the NIC bypass event mode for port_id",
 	.data = NULL,
 	.tokens = {
@@ -4015,8 +4021,8 @@ cmdline_parse_token_string_t cmd_setbypass_timeout_value =
 
 cmdline_parse_inst_t cmd_set_bypass_timeout = {
 	.f = cmd_set_bypass_timeout_parsed,
-	.help_str = "set bypass timeout (0|1.5|2|3|4|8|16|32) seconds: "
-	            "Set the NIC bypass watchdog timeout",
+	.help_str = "set bypass timeout 0|1.5|2|3|4|8|16|32: "
+	            "Set the NIC bypass watchdog timeout in seconds",
 	.data = NULL,
 	.tokens = {
 		(void *)&cmd_setbypass_timeout_set,
@@ -4109,7 +4115,7 @@ cmdline_parse_token_num_t cmd_showbypass_config_port =
 
 cmdline_parse_inst_t cmd_show_bypass_config = {
 	.f = cmd_show_bypass_config_parsed,
-	.help_str = "show bypass config (port_id): "
+	.help_str = "show bypass config <port_id>: "
 	            "Show the NIC bypass config for port_id",
 	.data = NULL,
 	.tokens = {
@@ -4162,7 +4168,8 @@ TOKEN_NUM_INITIALIZER(struct cmd_set_bonding_mode_result,
 
 cmdline_parse_inst_t cmd_set_bonding_mode = {
 		.f = cmd_set_bonding_mode_parsed,
-		.help_str = "set bonding mode (mode_value) (port_id): Set the bonding mode for port_id",
+		.help_str = "set bonding mode <mode_value> <port_id>: "
+			"Set the bonding mode for port_id",
 		.data = NULL,
 		.tokens = {
 				(void *) &cmd_setbonding_mode_set,
@@ -4227,7 +4234,8 @@ TOKEN_STRING_INITIALIZER(struct cmd_set_bonding_balance_xmit_policy_result,
 
 cmdline_parse_inst_t cmd_set_balance_xmit_policy = {
 		.f = cmd_set_bonding_balance_xmit_policy_parsed,
-		.help_str = "set bonding balance_xmit_policy (port_id) (policy_value): Set the bonding balance_xmit_policy for port_id",
+		.help_str = "set bonding balance_xmit_policy <port_id> l2|l23|l34: "
+			"Set the bonding balance_xmit_policy for port_id",
 		.data = NULL,
 		.tokens = {
 				(void *)&cmd_setbonding_balance_xmit_policy_set,
@@ -4353,7 +4361,8 @@ TOKEN_NUM_INITIALIZER(struct cmd_show_bonding_config_result,
 
 cmdline_parse_inst_t cmd_show_bonding_config = {
 		.f = cmd_show_bonding_config_parsed,
-		.help_str =	"show bonding config (port_id): Show the bonding config for port_id",
+		.help_str = "show bonding config <port_id>: "
+			"Show the bonding config for port_id",
 		.data = NULL,
 		.tokens = {
 				(void *)&cmd_showbonding_config_show,
@@ -4408,7 +4417,8 @@ TOKEN_NUM_INITIALIZER(struct cmd_set_bonding_primary_result,
 
 cmdline_parse_inst_t cmd_set_bonding_primary = {
 		.f = cmd_set_bonding_primary_parsed,
-		.help_str = "set bonding primary (slave_id) (port_id): Set the primary slave for port_id",
+		.help_str = "set bonding primary <slave_id> <port_id>: "
+			"Set the primary slave for port_id",
 		.data = NULL,
 		.tokens = {
 				(void *)&cmd_setbonding_primary_set,
@@ -4465,7 +4475,8 @@ TOKEN_NUM_INITIALIZER(struct cmd_add_bonding_slave_result,
 
 cmdline_parse_inst_t cmd_add_bonding_slave = {
 		.f = cmd_add_bonding_slave_parsed,
-		.help_str = "add bonding slave (slave_id) (port_id): Add a slave device to a bonded device",
+		.help_str = "add bonding slave <slave_id> <port_id>: "
+			"Add a slave device to a bonded device",
 		.data = NULL,
 		.tokens = {
 				(void *)&cmd_addbonding_slave_add,
@@ -4522,7 +4533,8 @@ cmdline_parse_token_num_t cmd_removebonding_slave_port =
 
 cmdline_parse_inst_t cmd_remove_bonding_slave = {
 		.f = cmd_remove_bonding_slave_parsed,
-		.help_str = "remove bonding slave (slave_id) (port_id): Remove a slave device from a bonded device",
+		.help_str = "remove bonding slave <slave_id> <port_id>: "
+			"Remove a slave device from a bonded device",
 		.data = NULL,
 		.tokens = {
 				(void *)&cmd_removebonding_slave_remove,
@@ -4597,7 +4609,8 @@ cmdline_parse_token_num_t cmd_createbonded_device_socket =
 
 cmdline_parse_inst_t cmd_create_bonded_device = {
 		.f = cmd_create_bonded_device_parsed,
-		.help_str = "create bonded device (mode) (socket): Create a new bonded device with specific bonding mode and socket",
+		.help_str = "create bonded device <mode> <socket>: "
+			"Create a new bonded device with specific bonding mode and socket",
 		.data = NULL,
 		.tokens = {
 				(void *)&cmd_createbonded_device_create,
@@ -4651,7 +4664,7 @@ cmdline_parse_token_etheraddr_t cmd_set_bond_mac_addr_addr =
 cmdline_parse_inst_t cmd_set_bond_mac_addr = {
 		.f = cmd_set_bond_mac_addr_parsed,
 		.data = (void *) 0,
-		.help_str = "set bonding mac_addr (port_id) (address): ",
+		.help_str = "set bonding mac_addr <port_id> <mac_addr>",
 		.tokens = {
 				(void *)&cmd_set_bond_mac_addr_set,
 				(void *)&cmd_set_bond_mac_addr_bonding,
@@ -4710,7 +4723,7 @@ cmdline_parse_token_num_t cmd_set_bond_mon_period_period_ms =
 cmdline_parse_inst_t cmd_set_bond_mon_period = {
 		.f = cmd_set_bond_mon_period_parsed,
 		.data = (void *) 0,
-		.help_str = "set bonding mon_period (port_id) (period_ms): ",
+		.help_str = "set bonding mon_period <port_id> <period_ms>",
 		.tokens = {
 				(void *)&cmd_set_bond_mon_period_set,
 				(void *)&cmd_set_bond_mon_period_bonding,
@@ -4768,8 +4781,8 @@ static void cmd_set_fwd_mode_init(void)
 	cmdline_parse_token_string_t *token_struct;
 
 	modes = list_pkt_forwarding_modes();
-	snprintf(help, sizeof help, "set fwd %s - "
-		"set packet forwarding mode", modes);
+	snprintf(help, sizeof help, "set fwd %s: "
+		"Set packet forwarding mode", modes);
 	cmd_set_fwd_mode.help_str = help;
 
 	/* string token separator is # */
@@ -4835,8 +4848,8 @@ static void cmd_set_fwd_retry_mode_init(void)
 	cmdline_parse_token_string_t *token_struct;
 
 	modes = list_pkt_forwarding_retry_modes();
-	snprintf(help, sizeof(help), "set fwd %s retry - "
-		"set packet forwarding mode with retry", modes);
+	snprintf(help, sizeof(help), "set fwd %s retry: "
+		"Set packet forwarding mode with retry", modes);
 	cmd_set_fwd_retry_mode.help_str = help;
 
 	/* string token separator is # */
@@ -4895,7 +4908,7 @@ cmdline_parse_token_num_t cmd_set_burst_tx_retry_retry_num =
 
 cmdline_parse_inst_t cmd_set_burst_tx_retry = {
 	.f = cmd_set_burst_tx_retry_parsed,
-	.help_str = "set burst tx delay (time_by_useconds) retry (retry_num)",
+	.help_str = "set burst tx delay <delay_usec> retry <num_retry>",
 	.tokens = {
 		(void *)&cmd_set_burst_tx_retry_set,
 		(void *)&cmd_set_burst_tx_retry_burst,
@@ -4965,7 +4978,7 @@ cmdline_parse_token_string_t cmd_setpromisc_mode =
 cmdline_parse_inst_t cmd_set_promisc_mode_all = {
 	.f = cmd_set_promisc_mode_parsed,
 	.data = (void *)1,
-	.help_str = "set promisc all on|off: set promisc mode for all ports",
+	.help_str = "set promisc all on|off: Set promisc mode for all ports",
 	.tokens = {
 		(void *)&cmd_setpromisc_set,
 		(void *)&cmd_setpromisc_promisc,
@@ -4978,7 +4991,7 @@ cmdline_parse_inst_t cmd_set_promisc_mode_all = {
 cmdline_parse_inst_t cmd_set_promisc_mode_one = {
 	.f = cmd_set_promisc_mode_parsed,
 	.data = (void *)0,
-	.help_str = "set promisc X on|off: set promisc mode on port X",
+	.help_str = "set promisc <port_id> on|off: Set promisc mode on port_id",
 	.tokens = {
 		(void *)&cmd_setpromisc_set,
 		(void *)&cmd_setpromisc_promisc,
@@ -5045,7 +5058,7 @@ cmdline_parse_token_string_t cmd_setallmulti_mode =
 cmdline_parse_inst_t cmd_set_allmulti_mode_all = {
 	.f = cmd_set_allmulti_mode_parsed,
 	.data = (void *)1,
-	.help_str = "set allmulti all on|off: set allmulti mode for all ports",
+	.help_str = "set allmulti all on|off: Set allmulti mode for all ports",
 	.tokens = {
 		(void *)&cmd_setallmulti_set,
 		(void *)&cmd_setallmulti_allmulti,
@@ -5058,7 +5071,7 @@ cmdline_parse_inst_t cmd_set_allmulti_mode_all = {
 cmdline_parse_inst_t cmd_set_allmulti_mode_one = {
 	.f = cmd_set_allmulti_mode_parsed,
 	.data = (void *)0,
-	.help_str = "set allmulti X on|off: set allmulti mode on port X",
+	.help_str = "set allmulti <port_id> on|off: Set allmulti mode on port_id",
 	.tokens = {
 		(void *)&cmd_setallmulti_set,
 		(void *)&cmd_setallmulti_allmulti,
@@ -5157,9 +5170,9 @@ cmd_link_flow_ctrl_set_parsed(void *parsed_result, struct cmdline *cl,
 cmdline_parse_inst_t cmd_link_flow_control_set = {
 	.f = cmd_link_flow_ctrl_set_parsed,
 	.data = NULL,
-	.help_str = "Configure the Ethernet flow control: set flow_ctrl rx on|off \
-tx on|off high_water low_water pause_time send_xon mac_ctrl_frame_fwd on|off \
-autoneg on|off port_id",
+	.help_str = "set flow_ctrl rx on|off tx on|off <high_water> "
+		"<low_water> <pause_time> <send_xon> mac_ctrl_frame_fwd on|off "
+		"autoneg on|off <port_id>: Configure the Ethernet flow control",
 	.tokens = {
 		(void *)&cmd_lfc_set_set,
 		(void *)&cmd_lfc_set_flow_ctrl,
@@ -5183,8 +5196,8 @@ autoneg on|off port_id",
 cmdline_parse_inst_t cmd_link_flow_control_set_rx = {
 	.f = cmd_link_flow_ctrl_set_parsed,
 	.data = (void *)&cmd_link_flow_control_set_rx,
-	.help_str = "Change rx flow control parameter: set flow_ctrl "
-		    "rx on|off port_id",
+	.help_str = "set flow_ctrl rx on|off <port_id>: "
+		"Change rx flow control parameter",
 	.tokens = {
 		(void *)&cmd_lfc_set_set,
 		(void *)&cmd_lfc_set_flow_ctrl,
@@ -5198,8 +5211,8 @@ cmdline_parse_inst_t cmd_link_flow_control_set_rx = {
 cmdline_parse_inst_t cmd_link_flow_control_set_tx = {
 	.f = cmd_link_flow_ctrl_set_parsed,
 	.data = (void *)&cmd_link_flow_control_set_tx,
-	.help_str = "Change tx flow control parameter: set flow_ctrl "
-		    "tx on|off port_id",
+	.help_str = "set flow_ctrl tx on|off <port_id>: "
+		"Change tx flow control parameter",
 	.tokens = {
 		(void *)&cmd_lfc_set_set,
 		(void *)&cmd_lfc_set_flow_ctrl,
@@ -5213,8 +5226,8 @@ cmdline_parse_inst_t cmd_link_flow_control_set_tx = {
 cmdline_parse_inst_t cmd_link_flow_control_set_hw = {
 	.f = cmd_link_flow_ctrl_set_parsed,
 	.data = (void *)&cmd_link_flow_control_set_hw,
-	.help_str = "Change high water flow control parameter: set flow_ctrl "
-		    "high_water value port_id",
+	.help_str = "set flow_ctrl high_water <value> <port_id>: "
+		"Change high water flow control parameter",
 	.tokens = {
 		(void *)&cmd_lfc_set_set,
 		(void *)&cmd_lfc_set_flow_ctrl,
@@ -5228,8 +5241,8 @@ cmdline_parse_inst_t cmd_link_flow_control_set_hw = {
 cmdline_parse_inst_t cmd_link_flow_control_set_lw = {
 	.f = cmd_link_flow_ctrl_set_parsed,
 	.data = (void *)&cmd_link_flow_control_set_lw,
-	.help_str = "Change low water flow control parameter: set flow_ctrl "
-		    "low_water value port_id",
+	.help_str = "set flow_ctrl low_water <value> <port_id>: "
+		"Change low water flow control parameter",
 	.tokens = {
 		(void *)&cmd_lfc_set_set,
 		(void *)&cmd_lfc_set_flow_ctrl,
@@ -5243,8 +5256,8 @@ cmdline_parse_inst_t cmd_link_flow_control_set_lw = {
 cmdline_parse_inst_t cmd_link_flow_control_set_pt = {
 	.f = cmd_link_flow_ctrl_set_parsed,
 	.data = (void *)&cmd_link_flow_control_set_pt,
-	.help_str = "Change pause time flow control parameter: set flow_ctrl "
-		    "pause_time value port_id",
+	.help_str = "set flow_ctrl pause_time <value> <port_id>: "
+		"Change pause time flow control parameter",
 	.tokens = {
 		(void *)&cmd_lfc_set_set,
 		(void *)&cmd_lfc_set_flow_ctrl,
@@ -5258,8 +5271,8 @@ cmdline_parse_inst_t cmd_link_flow_control_set_pt = {
 cmdline_parse_inst_t cmd_link_flow_control_set_xon = {
 	.f = cmd_link_flow_ctrl_set_parsed,
 	.data = (void *)&cmd_link_flow_control_set_xon,
-	.help_str = "Change send_xon flow control parameter: set flow_ctrl "
-		    "send_xon value port_id",
+	.help_str = "set flow_ctrl send_xon <value> <port_id>: "
+		"Change send_xon flow control parameter",
 	.tokens = {
 		(void *)&cmd_lfc_set_set,
 		(void *)&cmd_lfc_set_flow_ctrl,
@@ -5273,8 +5286,8 @@ cmdline_parse_inst_t cmd_link_flow_control_set_xon = {
 cmdline_parse_inst_t cmd_link_flow_control_set_macfwd = {
 	.f = cmd_link_flow_ctrl_set_parsed,
 	.data = (void *)&cmd_link_flow_control_set_macfwd,
-	.help_str = "Change mac ctrl fwd flow control parameter: set flow_ctrl "
-		    "mac_ctrl_frame_fwd on|off port_id",
+	.help_str = "set flow_ctrl mac_ctrl_frame_fwd on|off <port_id>: "
+		"Change mac ctrl fwd flow control parameter",
 	.tokens = {
 		(void *)&cmd_lfc_set_set,
 		(void *)&cmd_lfc_set_flow_ctrl,
@@ -5288,8 +5301,8 @@ cmdline_parse_inst_t cmd_link_flow_control_set_macfwd = {
 cmdline_parse_inst_t cmd_link_flow_control_set_autoneg = {
 	.f = cmd_link_flow_ctrl_set_parsed,
 	.data = (void *)&cmd_link_flow_control_set_autoneg,
-	.help_str = "Change autoneg flow control parameter: set flow_ctrl "
-		    "autoneg on|off port_id",
+	.help_str = "set flow_ctrl autoneg on|off <port_id>: "
+		"Change autoneg flow control parameter",
 	.tokens = {
 		(void *)&cmd_lfc_set_set,
 		(void *)&cmd_lfc_set_flow_ctrl,
@@ -5459,8 +5472,9 @@ cmdline_parse_token_num_t cmd_pfc_set_portid =
 cmdline_parse_inst_t cmd_priority_flow_control_set = {
 	.f = cmd_priority_flow_ctrl_set_parsed,
 	.data = NULL,
-	.help_str = "Configure the Ethernet priority flow control: set pfc_ctrl rx on|off\n\
-			tx on|off high_water low_water pause_time priority port_id",
+	.help_str = "set pfc_ctrl rx on|off tx on|off <high_water> <low_water> "
+		"<pause_time> <priority> <port_id>: "
+		"Configure the Ethernet priority flow control",
 	.tokens = {
 		(void *)&cmd_pfc_set_set,
 		(void *)&cmd_pfc_set_flow_ctrl,
@@ -5500,7 +5514,7 @@ cmdline_parse_token_string_t cmd_reset_def =
 cmdline_parse_inst_t cmd_reset = {
 	.f = cmd_reset_parsed,
 	.data = NULL,
-	.help_str = "set default: reset default forwarding configuration",
+	.help_str = "set default: Reset default forwarding configuration",
 	.tokens = {
 		(void *)&cmd_reset_set,
 		(void *)&cmd_reset_def,
@@ -5526,7 +5540,7 @@ static void cmd_start_parsed(__attribute__((unused)) void *parsed_result,
 cmdline_parse_inst_t cmd_start = {
 	.f = cmd_start_parsed,
 	.data = NULL,
-	.help_str = "start packet forwarding",
+	.help_str = "start: Start packet forwarding",
 	.tokens = {
 		(void *)&cmd_start_start,
 		NULL,
@@ -5557,7 +5571,8 @@ cmdline_parse_token_string_t cmd_start_tx_first_tx_first =
 cmdline_parse_inst_t cmd_start_tx_first = {
 	.f = cmd_start_tx_first_parsed,
 	.data = NULL,
-	.help_str = "start packet forwarding, after sending 1 burst of packets",
+	.help_str = "start tx_first: Start packet forwarding, "
+		"after sending 1 burst of packets",
 	.tokens = {
 		(void *)&cmd_start_tx_first_start,
 		(void *)&cmd_start_tx_first_tx_first,
@@ -5595,8 +5610,8 @@ cmdline_parse_token_num_t cmd_start_tx_first_n_tx_num =
 cmdline_parse_inst_t cmd_start_tx_first_n = {
 	.f = cmd_start_tx_first_n_parsed,
 	.data = NULL,
-	.help_str = "start packet forwarding, after sending <num> "
-		"bursts of packets",
+	.help_str = "start tx_first <num>: "
+		"packet forwarding, after sending <num> bursts of packets",
 	.tokens = {
 		(void *)&cmd_start_tx_first_n_start,
 		(void *)&cmd_start_tx_first_n_tx_first,
@@ -5634,7 +5649,7 @@ static void cmd_set_link_up_parsed(__attribute__((unused)) void *parsed_result,
 cmdline_parse_inst_t cmd_set_link_up = {
 	.f = cmd_set_link_up_parsed,
 	.data = NULL,
-	.help_str = "set link-up port (port id)",
+	.help_str = "set link-up port <port id>",
 	.tokens = {
 		(void *)&cmd_set_link_up_set,
 		(void *)&cmd_set_link_up_link_up,
@@ -5674,7 +5689,7 @@ static void cmd_set_link_down_parsed(
 cmdline_parse_inst_t cmd_set_link_down = {
 	.f = cmd_set_link_down_parsed,
 	.data = NULL,
-	.help_str = "set link-down port (port id)",
+	.help_str = "set link-down port <port id>",
 	.tokens = {
 		(void *)&cmd_set_link_down_set,
 		(void *)&cmd_set_link_down_link_down,
@@ -5837,7 +5852,7 @@ cmdline_parse_token_num_t cmd_showport_portnum =
 cmdline_parse_inst_t cmd_showport = {
 	.f = cmd_showport_parsed,
 	.data = NULL,
-	.help_str = "show|clear port info|stats|xstats|fdir|stat_qmap|dcb_tc X (X = port number)",
+	.help_str = "show|clear port info|stats|xstats|fdir|stat_qmap|dcb_tc <port_id>",
 	.tokens = {
 		(void *)&cmd_showport_show,
 		(void *)&cmd_showport_port,
@@ -5883,7 +5898,7 @@ cmdline_parse_token_num_t cmd_showqueue_queuenum =
 cmdline_parse_inst_t cmd_showqueue = {
 	.f = cmd_showqueue_parsed,
 	.data = NULL,
-	.help_str = "show rxq|txq info <port number> <queue_number>",
+	.help_str = "show rxq|txq info <port_id> <queue_id>",
 	.tokens = {
 		(void *)&cmd_showqueue_show,
 		(void *)&cmd_showqueue_type,
@@ -5923,7 +5938,7 @@ cmdline_parse_token_num_t cmd_read_reg_reg_off =
 cmdline_parse_inst_t cmd_read_reg = {
 	.f = cmd_read_reg_parsed,
 	.data = NULL,
-	.help_str = "read reg port_id reg_off",
+	.help_str = "read reg <port_id> <reg_off>",
 	.tokens = {
 		(void *)&cmd_read_reg_read,
 		(void *)&cmd_read_reg_reg,
@@ -5975,8 +5990,8 @@ cmdline_parse_token_num_t cmd_read_reg_bit_field_bit2_pos =
 cmdline_parse_inst_t cmd_read_reg_bit_field = {
 	.f = cmd_read_reg_bit_field_parsed,
 	.data = NULL,
-	.help_str = "read regfield port_id reg_off bit_x bit_y "
-	"(read register bit field between bit_x and bit_y included)",
+	.help_str = "read regfield <port_id> <reg_off> <bit_x> <bit_y>: "
+	"Read register bit field between bit_x and bit_y included",
 	.tokens = {
 		(void *)&cmd_read_reg_bit_field_read,
 		(void *)&cmd_read_reg_bit_field_regfield,
@@ -6021,7 +6036,7 @@ cmdline_parse_token_num_t cmd_read_reg_bit_bit_pos =
 cmdline_parse_inst_t cmd_read_reg_bit = {
 	.f = cmd_read_reg_bit_parsed,
 	.data = NULL,
-	.help_str = "read regbit port_id reg_off bit_x (0 <= bit_x <= 31)",
+	.help_str = "read regbit <port_id> <reg_off> <bit_x>: 0 <= bit_x <= 31",
 	.tokens = {
 		(void *)&cmd_read_reg_bit_read,
 		(void *)&cmd_read_reg_bit_regbit,
@@ -6064,7 +6079,7 @@ cmdline_parse_token_num_t cmd_write_reg_value =
 cmdline_parse_inst_t cmd_write_reg = {
 	.f = cmd_write_reg_parsed,
 	.data = NULL,
-	.help_str = "write reg port_id reg_off reg_value",
+	.help_str = "write reg <port_id> <reg_off> <reg_value>",
 	.tokens = {
 		(void *)&cmd_write_reg_write,
 		(void *)&cmd_write_reg_reg,
@@ -6121,8 +6136,8 @@ cmdline_parse_token_num_t cmd_write_reg_bit_field_value =
 cmdline_parse_inst_t cmd_write_reg_bit_field = {
 	.f = cmd_write_reg_bit_field_parsed,
 	.data = NULL,
-	.help_str = "write regfield port_id reg_off bit_x bit_y reg_value"
-	"(set register bit field between bit_x and bit_y included)",
+	.help_str = "write regfield <port_id> <reg_off> <bit_x> <bit_y> <reg_value>: "
+	"Set register bit field between bit_x and bit_y included",
 	.tokens = {
 		(void *)&cmd_write_reg_bit_field_write,
 		(void *)&cmd_write_reg_bit_field_regfield,
@@ -6172,7 +6187,7 @@ cmdline_parse_token_num_t cmd_write_reg_bit_value =
 cmdline_parse_inst_t cmd_write_reg_bit = {
 	.f = cmd_write_reg_bit_parsed,
 	.data = NULL,
-	.help_str = "write regbit port_id reg_off bit_x 0/1 (0 <= bit_x <= 31)",
+	.help_str = "write regbit <port_id> <reg_off> <bit_x> 0|1: 0 <= bit_x <= 31",
 	.tokens = {
 		(void *)&cmd_write_reg_bit_write,
 		(void *)&cmd_write_reg_bit_regbit,
@@ -6221,7 +6236,7 @@ cmdline_parse_token_num_t cmd_read_rxd_txd_desc_id =
 cmdline_parse_inst_t cmd_read_rxd_txd = {
 	.f = cmd_read_rxd_txd_parsed,
 	.data = NULL,
-	.help_str = "read rxd|txd port_id queue_id rxd_id",
+	.help_str = "read rxd|txd <port_id> <queue_id> <desc_id>",
 	.tokens = {
 		(void *)&cmd_read_rxd_txd_read,
 		(void *)&cmd_read_rxd_txd_rxd_txd,
@@ -6251,7 +6266,7 @@ cmdline_parse_token_string_t cmd_quit_quit =
 cmdline_parse_inst_t cmd_quit = {
 	.f = cmd_quit_parsed,
 	.data = NULL,
-	.help_str = "exit application",
+	.help_str = "quit: Exit application",
 	.tokens = {
 		(void *)&cmd_quit_quit,
 		NULL,
@@ -6298,8 +6313,8 @@ cmdline_parse_token_etheraddr_t cmd_mac_addr_addr =
 cmdline_parse_inst_t cmd_mac_addr = {
 	.f = cmd_mac_addr_parsed,
 	.data = (void *)0,
-	.help_str = "mac_addr add|remove X <address>: "
-			"add/remove MAC address on port X",
+	.help_str = "mac_addr add|remove <port_id> <mac_addr>: "
+			"Add/Remove MAC address on port_id",
 	.tokens = {
 		(void *)&cmd_mac_addr_cmd,
 		(void *)&cmd_mac_addr_what,
@@ -6353,7 +6368,8 @@ cmdline_parse_token_num_t cmd_setqmap_mapvalue =
 cmdline_parse_inst_t cmd_set_qmap = {
 	.f = cmd_set_qmap_parsed,
 	.data = NULL,
-	.help_str = "Set statistics mapping value on tx|rx queue_id of port_id",
+	.help_str = "set stat_qmap rx|tx <port_id> <queue_id> <map_value>: "
+		"Set statistics mapping value on tx|rx queue_id of port_id",
 	.tokens = {
 		(void *)&cmd_setqmap_set,
 		(void *)&cmd_setqmap_qmap,
@@ -6415,7 +6431,7 @@ cmdline_parse_token_string_t cmd_set_uc_hash_mode =
 cmdline_parse_inst_t cmd_set_uc_hash_filter = {
 	.f = cmd_set_uc_hash_parsed,
 	.data = NULL,
-	.help_str = "set port X uta Y on|off(X = port number,Y = MAC address)",
+	.help_str = "set port <port_id> uta <mac_addr> on|off)",
 	.tokens = {
 		(void *)&cmd_set_uc_hash_set,
 		(void *)&cmd_set_uc_hash_port,
@@ -6476,7 +6492,7 @@ cmdline_parse_token_string_t cmd_set_uc_all_hash_mode =
 cmdline_parse_inst_t cmd_set_uc_all_hash_filter = {
 	.f = cmd_set_uc_all_hash_parsed,
 	.data = NULL,
-	.help_str = "set port X uta all on|off (X = port number)",
+	.help_str = "set port <port_id> uta all on|off",
 	.tokens = {
 		(void *)&cmd_set_uc_all_hash_set,
 		(void *)&cmd_set_uc_all_hash_port,
@@ -6575,12 +6591,10 @@ cmdline_parse_token_string_t cmd_set_vf_macvlan_mode =
 cmdline_parse_inst_t cmd_set_vf_macvlan_filter = {
 	.f = cmd_set_vf_macvlan_parsed,
 	.data = NULL,
-	.help_str = "set port (portid) vf (vfid) (mac-addr) "
-			"(exact-mac|exact-mac-vlan|hashmac|hashmac-vlan) "
-			"on|off\n"
-			"exact match rule:exact match of MAC or MAC and VLAN; "
-			"hash match rule: hash match of MAC and exact match "
-			"of VLAN",
+	.help_str = "set port <port_id> vf <vf_id> <mac_addr> "
+		"exact-mac|exact-mac-vlan|hashmac|hashmac-vlan on|off: "
+		"Exact match rule: exact match of MAC or MAC and VLAN; "
+		"hash match rule: hash match of MAC and exact match of VLAN",
 	.tokens = {
 		(void *)&cmd_set_vf_macvlan_set,
 		(void *)&cmd_set_vf_macvlan_port,
@@ -6642,8 +6656,7 @@ cmdline_parse_token_string_t cmd_setvf_traffic_mode =
 cmdline_parse_inst_t cmd_set_vf_traffic = {
 	.f = cmd_set_vf_traffic_parsed,
 	.data = NULL,
-	.help_str = "set port X vf Y rx|tx on|off"
-			"(X = port number,Y = vf id)",
+	.help_str = "set port <port_id> vf <vf_id> rx|tx on|off",
 	.tokens = {
 		(void *)&cmd_setvf_traffic_set,
 		(void *)&cmd_setvf_traffic_port,
@@ -6723,7 +6736,7 @@ cmdline_parse_token_string_t cmd_set_vf_rxmode_on =
 cmdline_parse_inst_t cmd_set_vf_rxmode = {
 	.f = cmd_set_vf_rxmode_parsed,
 	.data = NULL,
-	.help_str = "set port X vf Y rxmode AUPE|ROPE|BAM|MPE on|off",
+	.help_str = "set port <port_id> vf <vf_id> rxmode AUPE|ROPE|BAM|MPE on|off",
 	.tokens = {
 		(void *)&cmd_set_vf_rxmode_set,
 		(void *)&cmd_set_vf_rxmode_port,
@@ -6788,8 +6801,8 @@ cmdline_parse_token_etheraddr_t cmd_vf_mac_addr_addr =
 cmdline_parse_inst_t cmd_vf_mac_addr_filter = {
 	.f = cmd_vf_mac_addr_parsed,
 	.data = (void *)0,
-	.help_str = "mac_addr add port X vf Y ethaddr:(X = port number,"
-	"Y = VF number)add MAC address filtering for a VF on port X",
+	.help_str = "mac_addr add port <port_id> vf <vf_id> <mac_addr>: "
+		"Add MAC address filtering for a VF on port_id",
 	.tokens = {
 		(void *)&cmd_vf_mac_addr_cmd,
 		(void *)&cmd_vf_mac_addr_what,
@@ -6851,8 +6864,8 @@ cmdline_parse_token_num_t cmd_vf_rx_vlan_filter_vf_mask =
 cmdline_parse_inst_t cmd_vf_rxvlan_filter = {
 	.f = cmd_vf_rx_vlan_filter_parsed,
 	.data = NULL,
-	.help_str = "rx_vlan add|rm X port Y vf Z (X = VLAN ID,"
-		"Y = port number,Z = hexadecimal VF mask)",
+	.help_str = "rx_vlan add|rm <vlan_id> port <port_id> vf <vf_mask>: "
+		"(vf_mask = hexadecimal VF mask)",
 	.tokens = {
 		(void *)&cmd_vf_rx_vlan_filter_rx_vlan,
 		(void *)&cmd_vf_rx_vlan_filter_what,
@@ -6918,8 +6931,8 @@ cmdline_parse_token_num_t cmd_queue_rate_limit_ratenum =
 cmdline_parse_inst_t cmd_queue_rate_limit = {
 	.f = cmd_queue_rate_limit_parsed,
 	.data = (void *)0,
-	.help_str = "set port X queue Y rate Z:(X = port number,"
-	"Y = queue number,Z = rate number)set rate limit for a queue on port X",
+	.help_str = "set port <port_id> queue <queue_id> rate <rate_value>: "
+		"Set rate limit for a queue on port_id",
 	.tokens = {
 		(void *)&cmd_queue_rate_limit_set,
 		(void *)&cmd_queue_rate_limit_port,
@@ -6994,9 +7007,9 @@ cmdline_parse_token_num_t cmd_vf_rate_limit_q_msk_val =
 cmdline_parse_inst_t cmd_vf_rate_limit = {
 	.f = cmd_vf_rate_limit_parsed,
 	.data = (void *)0,
-	.help_str = "set port X vf Y rate Z queue_mask V:(X = port number,"
-	"Y = VF number,Z = rate number, V = queue mask value)set rate limit "
-	"for queues of VF on port X",
+	.help_str = "set port <port_id> vf <vf_id> rate <rate_value> "
+		"queue_mask <queue_mask_value>: "
+		"Set rate limit for queues of VF on port_id",
 	.tokens = {
 		(void *)&cmd_vf_rate_limit_set,
 		(void *)&cmd_vf_rate_limit_port,
@@ -7140,12 +7153,10 @@ cmdline_parse_token_num_t cmd_tunnel_filter_queue_num =
 cmdline_parse_inst_t cmd_tunnel_filter = {
 	.f = cmd_tunnel_filter_parsed,
 	.data = (void *)0,
-	.help_str = "add/rm tunnel filter of a port: "
-			"tunnel_filter add port_id outer_mac inner_mac ip "
-			"inner_vlan tunnel_type(vxlan|nvgre|ipingre) filter_type "
-			"(oip|iip|imac-ivlan|imac-ivlan-tenid|imac-tenid|"
-			"imac|omac-imac-tenid) "
-			"tenant_id queue_num",
+	.help_str = "tunnel_filter add|rm <port_id> <outer_mac> <inner_mac> "
+		"<ip> <inner_vlan> vxlan|nvgre|ipingre oip|iip|imac-ivlan|"
+		"imac-ivlan-tenid|imac-tenid|imac|omac-imac-tenid <tenant_id> "
+		"<queue_id>: Add/Rm tunnel filter of a port",
 	.tokens = {
 		(void *)&cmd_tunnel_filter_cmd,
 		(void *)&cmd_tunnel_filter_what,
@@ -7211,8 +7222,8 @@ cmdline_parse_token_num_t cmd_tunnel_udp_config_port_id =
 cmdline_parse_inst_t cmd_tunnel_udp_config = {
 	.f = cmd_tunnel_udp_config_parsed,
 	.data = (void *)0,
-	.help_str = "add/rm an tunneling UDP port filter: "
-			"rx_vxlan_port add udp_port port_id",
+	.help_str = "rx_vxlan_port add|rm <udp_port> <port_id>: "
+		"Add/Remove a tunneling UDP port filter",
 	.tokens = {
 		(void *)&cmd_tunnel_udp_config_cmd,
 		(void *)&cmd_tunnel_udp_config_what,
@@ -7263,7 +7274,7 @@ cmdline_parse_token_num_t cmd_global_config_gre_key_len =
 cmdline_parse_inst_t cmd_global_config = {
 	.f = cmd_global_config_parsed,
 	.data = (void *)NULL,
-	.help_str = "global_config <port_id> gre-key-len <number>",
+	.help_str = "global_config <port_id> gre-key-len <key_len>",
 	.tokens = {
 		(void *)&cmd_global_config_cmd,
 		(void *)&cmd_global_config_port_id,
@@ -7371,8 +7382,9 @@ cmd_set_mirror_mask_parsed(void *parsed_result,
 cmdline_parse_inst_t cmd_set_mirror_mask = {
 		.f = cmd_set_mirror_mask_parsed,
 		.data = NULL,
-		.help_str = "set port X mirror-rule Y pool-mirror-up|pool-mirror-down|vlan-mirror"
-			    " pool_mask|vlan_id[,vlan_id]* dst-pool Z on|off",
+		.help_str = "set port <port_id> mirror-rule <rule_id> "
+			"pool-mirror-up|pool-mirror-down|vlan-mirror "
+			"<pool_mask|vlan_id[,vlan_id]*> dst-pool <pool_id> on|off",
 		.tokens = {
 			(void *)&cmd_mirror_mask_set,
 			(void *)&cmd_mirror_mask_port,
@@ -7462,8 +7474,8 @@ cmd_set_mirror_link_parsed(void *parsed_result,
 cmdline_parse_inst_t cmd_set_mirror_link = {
 		.f = cmd_set_mirror_link_parsed,
 		.data = NULL,
-		.help_str = "set port X mirror-rule Y uplink-mirror|"
-			"downlink-mirror dst-pool Z on|off",
+		.help_str = "set port <port_id> mirror-rule <rule_id> "
+			"uplink-mirror|downlink-mirror dst-pool <pool_id> on|off",
 		.tokens = {
 			(void *)&cmd_mirror_link_set,
 			(void *)&cmd_mirror_link_port,
@@ -7519,7 +7531,7 @@ cmd_reset_mirror_rule_parsed(void *parsed_result,
 cmdline_parse_inst_t cmd_reset_mirror_rule = {
 		.f = cmd_reset_mirror_rule_parsed,
 		.data = NULL,
-		.help_str = "reset port X mirror-rule Y",
+		.help_str = "reset port <port_id> mirror-rule <rule_id>",
 		.tokens = {
 			(void *)&cmd_rm_mirror_rule_reset,
 			(void *)&cmd_rm_mirror_rule_port,
@@ -7578,7 +7590,7 @@ cmdline_parse_token_string_t cmd_dump_dump =
 cmdline_parse_inst_t cmd_dump = {
 	.f = cmd_dump_parsed,  /* function to call */
 	.data = NULL,      /* 2nd arg of func */
-	.help_str = "dump status",
+	.help_str = "Dump status",
 	.tokens = {        /* token list, NULL terminated */
 		(void *)&cmd_dump_dump,
 		NULL,
@@ -7626,7 +7638,7 @@ cmdline_parse_token_string_t cmd_dump_one_name =
 cmdline_parse_inst_t cmd_dump_one = {
 	.f = cmd_dump_one_parsed,  /* function to call */
 	.data = NULL,      /* 2nd arg of func */
-	.help_str = "dump one ring/mempool: dump_ring|dump_mempool <name>",
+	.help_str = "dump_ring|dump_mempool <name>: Dump one ring/mempool",
 	.tokens = {        /* token list, NULL terminated */
 		(void *)&cmd_dump_one_dump,
 		(void *)&cmd_dump_one_name,
@@ -7711,7 +7723,8 @@ cmdline_parse_token_num_t cmd_syn_filter_queue_id =
 cmdline_parse_inst_t cmd_syn_filter = {
 	.f = cmd_syn_filter_parsed,
 	.data = NULL,
-	.help_str = "add/delete syn filter",
+	.help_str = "syn_filter <port_id> add|del priority high|low queue "
+		"<queue_id>: Add/Delete syn filter",
 	.tokens = {
 		(void *)&cmd_syn_filter_filter,
 		(void *)&cmd_syn_filter_port_id,
@@ -7850,7 +7863,9 @@ cmdline_parse_token_num_t cmd_2tuple_filter_queue_id =
 cmdline_parse_inst_t cmd_2tuple_filter = {
 	.f = cmd_2tuple_filter_parsed,
 	.data = NULL,
-	.help_str = "add a 2tuple filter",
+	.help_str = "2tuple_filter <port_id> add|del dst_port <value> protocol "
+		"<value> mask <value> tcp_flags <value> priority <value> queue "
+		"<queue_id>: Add a 2tuple filter",
 	.tokens = {
 		(void *)&cmd_2tuple_filter_filter,
 		(void *)&cmd_2tuple_filter_port_id,
@@ -8045,7 +8060,10 @@ cmdline_parse_token_num_t cmd_5tuple_filter_queue_id =
 cmdline_parse_inst_t cmd_5tuple_filter = {
 	.f = cmd_5tuple_filter_parsed,
 	.data = NULL,
-	.help_str = "add/del a 5tuple filter",
+	.help_str = "5tuple_filter <port_id> add|del dst_ip <value> "
+		"src_ip <value> dst_port <value> src_port <value> "
+		"protocol <value>  mask <value> tcp_flags <value> "
+		"priority <value> queue <queue_id>: Add/Del a 5tuple filter",
 	.tokens = {
 		(void *)&cmd_5tuple_filter_filter,
 		(void *)&cmd_5tuple_filter_port_id,
@@ -8240,7 +8258,9 @@ cmdline_parse_token_num_t cmd_flex_filter_queue_id =
 cmdline_parse_inst_t cmd_flex_filter = {
 	.f = cmd_flex_filter_parsed,
 	.data = NULL,
-	.help_str = "add/del a flex filter",
+	.help_str = "flex_filter <port_id> add|del len <value> bytes "
+		"<value> mask <value> priority <value> queue <queue_id>: "
+		"Add/Del a flex filter",
 	.tokens = {
 		(void *)&cmd_flex_filter_filter,
 		(void *)&cmd_flex_filter_port_id,
@@ -8352,7 +8372,9 @@ cmd_ethertype_filter_parsed(void *parsed_result,
 cmdline_parse_inst_t cmd_ethertype_filter = {
 	.f = cmd_ethertype_filter_parsed,
 	.data = NULL,
-	.help_str = "add or delete an ethertype filter entry",
+	.help_str = "ethertype_filter <port_id> add|del mac_addr|mac_ignr "
+		"<mac_addr> ethertype <value> drop|fw queue <queue_id>: "
+		"Add or delete an ethertype filter entry",
 	.tokens = {
 		(void *)&cmd_ethertype_filter_filter,
 		(void *)&cmd_ethertype_filter_port_id,
@@ -8840,7 +8862,13 @@ cmdline_parse_token_num_t cmd_flow_director_tunnel_id_value =
 cmdline_parse_inst_t cmd_add_del_ip_flow_director = {
 	.f = cmd_flow_director_filter_parsed,
 	.data = NULL,
-	.help_str = "add or delete an ip flow director entry on NIC",
+	.help_str = "flow_director_filter <port_id> mode IP add|del|update flow "
+		"ipv4-other|ipv4-frag|ipv4-tcp|ipv4-udp|ipv4-sctp|"
+		"ipv6-other|ipv6-frag|ipv6-tcp|ipv6-udp|ipv6-sctp|"
+		"l2_payload src <src_ip> dst <dst_ip> tos <tos_value> "
+		"proto <proto_value> ttl <ttl_value> vlan <vlan_value> "
+		"flexbytes <flexbyte_vaues> drop|fw <pf_vf> queue <queue_id> "
+		"fd_id <fd_id_value>: Add or delete an ip flow director entry on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_filter,
 		(void *)&cmd_flow_director_port_id,
@@ -8876,7 +8904,8 @@ cmdline_parse_inst_t cmd_add_del_ip_flow_director = {
 cmdline_parse_inst_t cmd_add_del_udp_flow_director = {
 	.f = cmd_flow_director_filter_parsed,
 	.data = NULL,
-	.help_str = "add or delete an udp/tcp flow director entry on NIC",
+	.help_str = "flow_director_filter ... : Add or delete an udp/tcp flow "
+		"director entry on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_filter,
 		(void *)&cmd_flow_director_port_id,
@@ -8912,7 +8941,8 @@ cmdline_parse_inst_t cmd_add_del_udp_flow_director = {
 cmdline_parse_inst_t cmd_add_del_sctp_flow_director = {
 	.f = cmd_flow_director_filter_parsed,
 	.data = NULL,
-	.help_str = "add or delete a sctp flow director entry on NIC",
+	.help_str = "flow_director_filter ... : Add or delete a sctp flow "
+		"director entry on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_filter,
 		(void *)&cmd_flow_director_port_id,
@@ -8950,7 +8980,8 @@ cmdline_parse_inst_t cmd_add_del_sctp_flow_director = {
 cmdline_parse_inst_t cmd_add_del_l2_flow_director = {
 	.f = cmd_flow_director_filter_parsed,
 	.data = NULL,
-	.help_str = "add or delete a L2 flow director entry on NIC",
+	.help_str = "flow_director_filter ... : Add or delete a L2 flow "
+		"director entry on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_filter,
 		(void *)&cmd_flow_director_port_id,
@@ -8976,7 +9007,8 @@ cmdline_parse_inst_t cmd_add_del_l2_flow_director = {
 cmdline_parse_inst_t cmd_add_del_mac_vlan_flow_director = {
 	.f = cmd_flow_director_filter_parsed,
 	.data = NULL,
-	.help_str = "add or delete a MAC VLAN flow director entry on NIC",
+	.help_str = "flow_director_filter ... : Add or delete a MAC VLAN flow "
+		"director entry on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_filter,
 		(void *)&cmd_flow_director_port_id,
@@ -9001,7 +9033,8 @@ cmdline_parse_inst_t cmd_add_del_mac_vlan_flow_director = {
 cmdline_parse_inst_t cmd_add_del_tunnel_flow_director = {
 	.f = cmd_flow_director_filter_parsed,
 	.data = NULL,
-	.help_str = "add or delete a tunnel flow director entry on NIC",
+	.help_str = "flow_director_filter ... : Add or delete a tunnel flow "
+		"director entry on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_filter,
 		(void *)&cmd_flow_director_port_id,
@@ -9064,7 +9097,8 @@ cmd_flush_flow_director_parsed(void *parsed_result,
 cmdline_parse_inst_t cmd_flush_flow_director = {
 	.f = cmd_flush_flow_director_parsed,
 	.data = NULL,
-	.help_str = "flush all flow director entries of a device on NIC",
+	.help_str = "flush_flow_director <port_id>: "
+		"Flush all flow director entries of a device on NIC",
 	.tokens = {
 		(void *)&cmd_flush_flow_director_flush,
 		(void *)&cmd_flush_flow_director_port_id,
@@ -9225,7 +9259,7 @@ cmdline_parse_token_num_t cmd_flow_director_mask_tunnel_id_value =
 cmdline_parse_inst_t cmd_set_flow_director_ip_mask = {
 	.f = cmd_flow_director_mask_parsed,
 	.data = NULL,
-	.help_str = "set IP mode flow director's mask on NIC",
+	.help_str = "flow_director_mask ... : Set IP mode flow director's mask on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_mask,
 		(void *)&cmd_flow_director_mask_port_id,
@@ -9248,7 +9282,7 @@ cmdline_parse_inst_t cmd_set_flow_director_ip_mask = {
 cmdline_parse_inst_t cmd_set_flow_director_mac_vlan_mask = {
 	.f = cmd_flow_director_mask_parsed,
 	.data = NULL,
-	.help_str = "set MAC VLAN mode flow director's mask on NIC",
+	.help_str = "flow_director_mask ... : Set MAC VLAN mode flow director's mask on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_mask,
 		(void *)&cmd_flow_director_mask_port_id,
@@ -9263,7 +9297,7 @@ cmdline_parse_inst_t cmd_set_flow_director_mac_vlan_mask = {
 cmdline_parse_inst_t cmd_set_flow_director_tunnel_mask = {
 	.f = cmd_flow_director_mask_parsed,
 	.data = NULL,
-	.help_str = "set tunnel mode flow director's mask on NIC",
+	.help_str = "flow_director_mask ... : Set tunnel mode flow director's mask on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_mask,
 		(void *)&cmd_flow_director_mask_port_id,
@@ -9391,7 +9425,7 @@ cmdline_parse_token_string_t cmd_flow_director_flexmask_mask =
 cmdline_parse_inst_t cmd_set_flow_director_flex_mask = {
 	.f = cmd_flow_director_flex_mask_parsed,
 	.data = NULL,
-	.help_str = "set flow director's flex mask on NIC",
+	.help_str = "flow_director_flex_mask ... : Set flow director's flex mask on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_flexmask,
 		(void *)&cmd_flow_director_flexmask_port_id,
@@ -9509,7 +9543,7 @@ cmdline_parse_token_string_t cmd_flow_director_flexpayload_payload_cfg =
 cmdline_parse_inst_t cmd_set_flow_director_flex_payload = {
 	.f = cmd_flow_director_flxpld_parsed,
 	.data = NULL,
-	.help_str = "set flow director's flex payload on NIC",
+	.help_str = "flow_director_flexpayload ... : Set flow director's flex payload on NIC",
 	.tokens = {
 		(void *)&cmd_flow_director_flexpayload,
 		(void *)&cmd_flow_director_flexpayload_port_id,
@@ -9567,7 +9601,7 @@ cmdline_parse_token_num_t cmd_get_sym_hash_ena_per_port_port_id =
 cmdline_parse_inst_t cmd_get_sym_hash_ena_per_port = {
 	.f = cmd_get_sym_hash_per_port_parsed,
 	.data = NULL,
-	.help_str = "get_sym_hash_ena_per_port port_id",
+	.help_str = "get_sym_hash_ena_per_port <port_id>",
 	.tokens = {
 		(void *)&cmd_get_sym_hash_ena_per_port_all,
 		(void *)&cmd_get_sym_hash_ena_per_port_port_id,
@@ -9626,7 +9660,7 @@ cmdline_parse_token_string_t cmd_set_sym_hash_ena_per_port_enable =
 cmdline_parse_inst_t cmd_set_sym_hash_ena_per_port = {
 	.f = cmd_set_sym_hash_per_port_parsed,
 	.data = NULL,
-	.help_str = "set_sym_hash_ena_per_port port_id enable|disable",
+	.help_str = "set_sym_hash_ena_per_port <port_id> enable|disable",
 	.tokens = {
 		(void *)&cmd_set_sym_hash_ena_per_port_all,
 		(void *)&cmd_set_sym_hash_ena_per_port_port_id,
@@ -9744,7 +9778,7 @@ cmdline_parse_token_num_t cmd_get_hash_global_config_port_id =
 cmdline_parse_inst_t cmd_get_hash_global_config = {
 	.f = cmd_get_hash_global_config_parsed,
 	.data = NULL,
-	.help_str = "get_hash_global_config port_id",
+	.help_str = "get_hash_global_config <port_id>",
 	.tokens = {
 		(void *)&cmd_get_hash_global_config_all,
 		(void *)&cmd_get_hash_global_config_port_id,
@@ -9827,11 +9861,11 @@ cmdline_parse_token_string_t cmd_set_hash_global_config_enable =
 cmdline_parse_inst_t cmd_set_hash_global_config = {
 	.f = cmd_set_hash_global_config_parsed,
 	.data = NULL,
-	.help_str = "set_hash_global_config port_id "
+	.help_str = "set_hash_global_config <port_id> "
 		"toeplitz|simple_xor|default "
-		"ipv4|ipv4-frag|ipv4-tcp|ipv4-udp|ipv4-sctp|ipv4-other|ipv6|"
-		"ipv6-frag|ipv6-tcp|ipv6-udp|ipv6-sctp|ipv6-other|l2_payload "
-		"enable|disable",
+		"ipv4|ipv4-frag|ipv4-tcp|ipv4-udp|ipv4-sctp|ipv4-other|"
+		"ipv6|ipv6-frag|ipv6-tcp|ipv6-udp|ipv6-sctp|ipv6-other|"
+		"l2_payload enable|disable",
 	.tokens = {
 		(void *)&cmd_set_hash_global_config_all,
 		(void *)&cmd_set_hash_global_config_port_id,
@@ -10082,7 +10116,8 @@ cmdline_parse_token_etheraddr_t cmd_mcast_addr_addr =
 cmdline_parse_inst_t cmd_mcast_addr = {
 	.f = cmd_mcast_addr_parsed,
 	.data = (void *)0,
-	.help_str = "mcast_addr add|remove X <mcast_addr>: add/remove multicast MAC address on port X",
+	.help_str = "mcast_addr add|remove <port_id> <mcast_addr>: "
+		"Add/Remove multicast MAC address on port_id",
 	.tokens = {
 		(void *)&cmd_mcast_addr_cmd,
 		(void *)&cmd_mcast_addr_what,
@@ -10182,7 +10217,7 @@ cmd_config_l2_tunnel_eth_type_all_parsed
 cmdline_parse_inst_t cmd_config_l2_tunnel_eth_type_all = {
 	.f = cmd_config_l2_tunnel_eth_type_all_parsed,
 	.data = NULL,
-	.help_str = "port config all l2-tunnel ether-type",
+	.help_str = "port config all l2-tunnel E-tag ether-type <value>",
 	.tokens = {
 		(void *)&cmd_config_l2_tunnel_eth_type_port,
 		(void *)&cmd_config_l2_tunnel_eth_type_config,
@@ -10218,7 +10253,7 @@ cmd_config_l2_tunnel_eth_type_specific_parsed(
 cmdline_parse_inst_t cmd_config_l2_tunnel_eth_type_specific = {
 	.f = cmd_config_l2_tunnel_eth_type_specific_parsed,
 	.data = NULL,
-	.help_str = "port config l2-tunnel ether-type",
+	.help_str = "port config <port_id> l2-tunnel E-tag ether-type <value>",
 	.tokens = {
 		(void *)&cmd_config_l2_tunnel_eth_type_port,
 		(void *)&cmd_config_l2_tunnel_eth_type_config,
@@ -10301,7 +10336,7 @@ cmd_config_l2_tunnel_en_dis_all_parsed(
 cmdline_parse_inst_t cmd_config_l2_tunnel_en_dis_all = {
 	.f = cmd_config_l2_tunnel_en_dis_all_parsed,
 	.data = NULL,
-	.help_str = "port config all l2-tunnel enable/disable",
+	.help_str = "port config all l2-tunnel E-tag enable|disable",
 	.tokens = {
 		(void *)&cmd_config_l2_tunnel_en_dis_port,
 		(void *)&cmd_config_l2_tunnel_en_dis_config,
@@ -10344,7 +10379,7 @@ cmd_config_l2_tunnel_en_dis_specific_parsed(
 cmdline_parse_inst_t cmd_config_l2_tunnel_en_dis_specific = {
 	.f = cmd_config_l2_tunnel_en_dis_specific_parsed,
 	.data = NULL,
-	.help_str = "port config l2-tunnel enable/disable",
+	.help_str = "port config <port_id> l2-tunnel E-tag enable|disable",
 	.tokens = {
 		(void *)&cmd_config_l2_tunnel_en_dis_port,
 		(void *)&cmd_config_l2_tunnel_en_dis_config,
@@ -10517,7 +10552,7 @@ cmd_config_e_tag_insertion_dis_parsed(
 cmdline_parse_inst_t cmd_config_e_tag_insertion_en = {
 	.f = cmd_config_e_tag_insertion_en_parsed,
 	.data = NULL,
-	.help_str = "E-tag insertion enable",
+	.help_str = "E-tag ... : E-tag insertion enable",
 	.tokens = {
 		(void *)&cmd_config_e_tag_e_tag,
 		(void *)&cmd_config_e_tag_set,
@@ -10536,7 +10571,7 @@ cmdline_parse_inst_t cmd_config_e_tag_insertion_en = {
 cmdline_parse_inst_t cmd_config_e_tag_insertion_dis = {
 	.f = cmd_config_e_tag_insertion_dis_parsed,
 	.data = NULL,
-	.help_str = "E-tag insertion disable",
+	.help_str = "E-tag ... : E-tag insertion disable",
 	.tokens = {
 		(void *)&cmd_config_e_tag_e_tag,
 		(void *)&cmd_config_e_tag_set,
@@ -10583,7 +10618,7 @@ cmd_config_e_tag_stripping_parsed(
 cmdline_parse_inst_t cmd_config_e_tag_stripping_en_dis = {
 	.f = cmd_config_e_tag_stripping_parsed,
 	.data = NULL,
-	.help_str = "E-tag stripping enable/disable",
+	.help_str = "E-tag ... : E-tag stripping enable/disable",
 	.tokens = {
 		(void *)&cmd_config_e_tag_e_tag,
 		(void *)&cmd_config_e_tag_set,
@@ -10627,7 +10662,7 @@ cmd_config_e_tag_forwarding_parsed(
 cmdline_parse_inst_t cmd_config_e_tag_forwarding_en_dis = {
 	.f = cmd_config_e_tag_forwarding_parsed,
 	.data = NULL,
-	.help_str = "E-tag forwarding enable/disable",
+	.help_str = "E-tag ... : E-tag forwarding enable/disable",
 	.tokens = {
 		(void *)&cmd_config_e_tag_e_tag,
 		(void *)&cmd_config_e_tag_set,
@@ -10682,7 +10717,7 @@ cmd_config_e_tag_filter_add_parsed(
 cmdline_parse_inst_t cmd_config_e_tag_filter_add = {
 	.f = cmd_config_e_tag_filter_add_parsed,
 	.data = NULL,
-	.help_str = "E-tag filter add",
+	.help_str = "E-tag ... : E-tag filter add",
 	.tokens = {
 		(void *)&cmd_config_e_tag_e_tag,
 		(void *)&cmd_config_e_tag_set,
@@ -10739,7 +10774,7 @@ cmd_config_e_tag_filter_del_parsed(
 cmdline_parse_inst_t cmd_config_e_tag_filter_del = {
 	.f = cmd_config_e_tag_filter_del_parsed,
 	.data = NULL,
-	.help_str = "E-tag filter delete",
+	.help_str = "E-tag ... : E-tag filter delete",
 	.tokens = {
 		(void *)&cmd_config_e_tag_e_tag,
 		(void *)&cmd_config_e_tag_set,
@@ -10826,7 +10861,7 @@ cmd_set_vf_vlan_anti_spoof_parsed(
 cmdline_parse_inst_t cmd_set_vf_vlan_anti_spoof = {
 	.f = cmd_set_vf_vlan_anti_spoof_parsed,
 	.data = NULL,
-	.help_str = "set vf vlan antispoof port_id vf_id on|off",
+	.help_str = "set vf vlan antispoof <port_id> <vf_id> on|off",
 	.tokens = {
 		(void *)&cmd_vf_vlan_anti_spoof_set,
 		(void *)&cmd_vf_vlan_anti_spoof_vf,
@@ -10911,7 +10946,7 @@ cmd_set_vf_mac_anti_spoof_parsed(
 cmdline_parse_inst_t cmd_set_vf_mac_anti_spoof = {
 	.f = cmd_set_vf_mac_anti_spoof_parsed,
 	.data = NULL,
-	.help_str = "set vf mac antispoof port_id vf_id on|off",
+	.help_str = "set vf mac antispoof <port_id> <vf_id> on|off",
 	.tokens = {
 		(void *)&cmd_vf_mac_anti_spoof_set,
 		(void *)&cmd_vf_mac_anti_spoof_vf,
@@ -10995,7 +11030,7 @@ cmd_set_vf_vlan_stripq_parsed(
 cmdline_parse_inst_t cmd_set_vf_vlan_stripq = {
 	.f = cmd_set_vf_vlan_stripq_parsed,
 	.data = NULL,
-	.help_str = "set vf vlan stripq port_id vf_id on|off",
+	.help_str = "set vf vlan stripq <port_id> <vf_id> on|off",
 	.tokens = {
 		(void *)&cmd_vf_vlan_stripq_set,
 		(void *)&cmd_vf_vlan_stripq_vf,
@@ -11078,7 +11113,7 @@ cmd_set_vf_vlan_insert_parsed(
 cmdline_parse_inst_t cmd_set_vf_vlan_insert = {
 	.f = cmd_set_vf_vlan_insert_parsed,
 	.data = NULL,
-	.help_str = "set vf vlan insert port_id vf_id vlan_id",
+	.help_str = "set vf vlan insert <port_id> <vf_id> <vlan_id>",
 	.tokens = {
 		(void *)&cmd_vf_vlan_insert_set,
 		(void *)&cmd_vf_vlan_insert_vf,
@@ -11152,7 +11187,7 @@ cmd_set_tx_loopback_parsed(
 cmdline_parse_inst_t cmd_set_tx_loopback = {
 	.f = cmd_set_tx_loopback_parsed,
 	.data = NULL,
-	.help_str = "set tx loopback port_id on|off",
+	.help_str = "set tx loopback <port_id> on|off",
 	.tokens = {
 		(void *)&cmd_tx_loopback_set,
 		(void *)&cmd_tx_loopback_tx,
@@ -11229,7 +11264,7 @@ cmd_set_all_queues_drop_en_parsed(
 cmdline_parse_inst_t cmd_set_all_queues_drop_en = {
 	.f = cmd_set_all_queues_drop_en_parsed,
 	.data = NULL,
-	.help_str = "set all queues drop port_id on|off",
+	.help_str = "set all queues drop <port_id> on|off",
 	.tokens = {
 		(void *)&cmd_all_queues_drop_en_set,
 		(void *)&cmd_all_queues_drop_en_all,
@@ -11313,7 +11348,7 @@ cmd_set_vf_split_drop_en_parsed(
 cmdline_parse_inst_t cmd_set_vf_split_drop_en = {
 	.f = cmd_set_vf_split_drop_en_parsed,
 	.data = NULL,
-	.help_str = "set vf split drop port_id vf_id on|off",
+	.help_str = "set vf split drop <port_id> <vf_id> on|off",
 	.tokens = {
 		(void *)&cmd_vf_split_drop_en_set,
 		(void *)&cmd_vf_split_drop_en_vf,
@@ -11397,7 +11432,7 @@ cmd_set_vf_mac_addr_parsed(
 cmdline_parse_inst_t cmd_set_vf_mac_addr = {
 	.f = cmd_set_vf_mac_addr_parsed,
 	.data = NULL,
-	.help_str = "set vf mac addr port_id vf_id xx:xx:xx:xx:xx:xx",
+	.help_str = "set vf mac addr <port_id> <vf_id> <mac_addr>",
 	.tokens = {
 		(void *)&cmd_set_vf_mac_addr_set,
 		(void *)&cmd_set_vf_mac_addr_vf,
-- 
2.9.3

^ permalink raw reply related

* Re: [PATCH v12 1/6] ethdev: add Tx preparation
From: Ananyev, Konstantin @ 2016-12-02  1:06 UTC (permalink / raw)
  To: Thomas Monjalon, Kulasek, TomaszX; +Cc: dev@dpdk.org, olivier.matz@6wind.com
In-Reply-To: <7834627.cBDVu3uoNi@xps13>


> 
> 2016-11-23 18:36, Tomasz Kulasek:
> > +/**
> > + * Process a burst of output packets on a transmit queue of an Ethernet device.
> > + *
> > + * The rte_eth_tx_prepare() function is invoked to prepare output packets to be
> > + * transmitted on the output queue *queue_id* of the Ethernet device designated
> > + * by its *port_id*.
> > + * The *nb_pkts* parameter is the number of packets to be prepared which are
> > + * supplied in the *tx_pkts* array of *rte_mbuf* structures, each of them
> > + * allocated from a pool created with rte_pktmbuf_pool_create().
> > + * For each packet to send, the rte_eth_tx_prepare() function performs
> > + * the following operations:
> > + *
> > + * - Check if packet meets devices requirements for tx offloads.
> > + *
> > + * - Check limitations about number of segments.
> > + *
> > + * - Check additional requirements when debug is enabled.
> > + *
> > + * - Update and/or reset required checksums when tx offload is set for packet.
> > + *
> > + * Since this function can modify packet data, provided mbufs must be safely
> > + * writable (e.g. modified data cannot be in shared segment).
> 
> I think we will have to remove this limitation in next releases.
> As we don't know how it could affect the API, I suggest to declare this
> API EXPERIMENTAL.

While I don't really mind to mart it as experimental, I don't really understand the reasoning:
Why " this function can modify packet data, provided mbufs must be safely writable" suddenly becomes a problem?
That seems like and obvious limitation to me and let say tx_burst() has the same one.
Second, I don't see how you are going to remove it without introducing a heavy performance impact.
Konstantin
  

^ permalink raw reply

* Re: [PATCH v12 0/6] add Tx preparation
From: Ananyev, Konstantin @ 2016-12-02  1:00 UTC (permalink / raw)
  To: Adrien Mazarguil
  Cc: Thomas Monjalon, dev@dpdk.org, Rahul Lakkireddy, Stephen Hurd,
	Jan Medala, Jakub Palider, John Daley, Alejandro Lucero,
	Harish Patil, Rasesh Mody, Jerin Jacob, Yuanhan Liu, Yong Wang,
	Kulasek, TomaszX, olivier.matz@6wind.com
In-Reply-To: <20161201071518.GG10340@6wind.com>


Hi Adrien,

> 
> Hi Konstantin,
> 
> On Wed, Nov 30, 2016 at 10:54:50AM +0000, Ananyev, Konstantin wrote:
> [...]
> > > Something is definitely needed here, and only PMDs can provide it. I think
> > > applications should not have to clear checksum fields or initialize them to
> > > some magic value, same goes for any other offload or hardware limitation
> > > that needs to be worked around.
> > >
> > > tx_prep() is one possible answer to this issue, however as mentioned in the
> > > original patch it can be very expensive if exposed by the PMD.
> > >
> > > Another issue I'm more concerned about is the way limitations are managed
> > > (struct rte_eth_desc_lim). While not officially tied to tx_prep(), this
> > > structure contains new fields that are only relevant to a few devices, and I
> > > fear it will keep growing with each new hardware quirk to manage, breaking
> > > ABIs in the process.
> >
> > Well, if some new HW capability/limitation would arise and we'd like to support
> > it in DPDK, then yes we probably would need to think how to incorporate it here.
> > Do you have anything particular in mind here?
> 
> Nothing in particular, so for the sake of the argument, let's suppose that I
> would like to add a field to expose some limitation that only applies to my
> PMD during TX but looks generic enough to make sense, e.g. maximum packet
> size when VLAN tagging is requested.

Hmm, I didn't hear about such limitations so far, but if it is real case -
sure, feel free to submit the patch.   

> PMDs are free to set that field to some
> special value (say, 0) if they do not care.
> 
> Since that field exists however, conscious applications should check its
> value for each packet that needs to be transmitted. This extra code causes a
> slowdown just by sitting in the data path. Since it is not the only field in
> that structure, the performance impact can be significant.
> 
> Even though this code is inside applications, it remains unfair to PMDs for
> which these tests are irrelevant. This problem is identified and addressed
> by tx_prepare().

I suppose the question is why do we need:
uint16_t nb_seg_max;
uint16_t nb_mtu_seg_max;
as we now have tx_prepare(), right?

For two reasons:
1. Some people might feel that tx_prepare() is not good (smart/fast) enough
for them and would prefer to do necessary preparations for TX offloads themselves.
2. Even if people do use tx_prepare() they still should take this information into accout.
As an example ixgbe can't TX packets with then 40 segments.
Obviously ixbge_tx_prep() performs that check and returns an error.
But it wouldn't try to merge/reallocate mbufs for you.
User still has to do it himself, or just prevent creating such long chains somehow.

> 
> Thanks to tx_prepare(), these checks are moved back into PMDs where they
> belong. PMDs that do not need them do not have to provide support for
> tx_prepare() and do not suffer any performance impact as result;
> applications only have to make sure tx_prepare() is always called at some
> point before tx_burst().
> 
> Once you reach this stage, you've effectively made tx_prepare() mandatory
> before tx_burst(). If some bug occurs, then perhaps you forgot to call
> tx_prepare(), you just need to add it. The total cost for doing TX is
> therefore tx_prepare() + tx_burst().
> 
> I'm perhaps a bit pessimistic mind you, but I do not think tx_prepare() will
> remain optional for long. Sure, PMDs that do not implement it do not care,
> I'm focusing on applications, for which the performance impact of calling
> tx_prepare() followed by tx_burst() is higher than a single tx_burst()
> performing all the necessary preparation at once.
> 
> [...]
> > > Following the same logic, why can't such a thing be made part of the TX
> > > burst function as well (through a direct call to rte_phdr_cksum_fix()
> > > whenever necessary). From an application standpoint, what are the advantages
> > > of having to:
> > >
> > >  if (tx_prep()) // iterate and update mbufs as needed
> > >      tx_burst(); // iterate and send
> > >
> > > Compared to:
> > >
> > >  tx_burst(); // iterate, update as needed and send
> >
> > I think that was discussed extensively quite a lot previously here:
> > As Thomas already replied - main motivation is to allow user
> > to execute them on different stages of packet TX pipeline,
> > and probably on different cores.
> > I think that provides better flexibility to the user to when/where
> > do these preparations and hopefully would lead to better performance.
> 
> And I agree, I think this use case is valid but does not warrant such a high
> penalty when your application does not need that much flexibility. Simple
> (yet conscious) applications need the highest performance. Complex ones as
> you described already suffer quite a bit from IPCs and won't mind a couple
> of extra CPU cycles right?

It would mean an extra cache-miss for every packet, so I think performance hit
would be quite significant. 
About the 'simple' case when tx_prep() and tx_burst() are called on the same core,
Why do you believe that:
tx_prep(); tx_burst(); would be much slower than tx_burst() {tx_prep(), ...}?
tx_prep() itself is quite expensive, let say for Intel HW it includes:
- read mbuf fileds (2 cache-lines),
- read packet header (1/2 cache-lines)
- calculate pseudo-header csum
 - update packet header 
Comparing to that price of extra function call seems neglectable
(if we TX packets in bursts of course). 

> 
> Yes they will, therefore we need a method that satisfies both cases.
> 
> As a possible solution, a special mbuf flag could be added to each mbuf
> having gone through tx_prepare(). That way, tx_burst() could skip some
> checks and things it would otherwise have done.

That's an interesting idea, but it has one drawback:
As I understand, it means that from now on if user doing preparations on his own,
he had to setup this flag, otherwise tx_burst() would do extra unnecessary work.
So any existing applications that using TX offloads and do preparation by themselves
would have to be modified to avoid performance loss.

> 
> Another possibility, telling the PMD first that you always intend to use
> tx_prepare() and getting a simpler/faster tx_burst() callback as a result.

That what we have right now (at least for Intel HW):  
it is a user responsibility to do the necessary preparations/checks before calling tx_burst().  
With tx_prepare() we just remove from user the headache to implement tx_prepare() on his own.
Now he can use a 'proper' PMD provided function.
My vote still would be for that model.

Konstantin

^ permalink raw reply

* Re: [PATCH v12 1/6] ethdev: add Tx preparation
From: Ananyev, Konstantin @ 2016-12-02  0:10 UTC (permalink / raw)
  To: Kulasek, TomaszX, Thomas Monjalon
  Cc: dev@dpdk.org, olivier.matz@6wind.com, Richardson, Bruce
In-Reply-To: <3042915272161B4EB253DA4D77EB373A14F57CDE@IRSMSX102.ger.corp.intel.com>



> 
> Hi Thomas,
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Thursday, December 1, 2016 20:52
> > To: Kulasek, TomaszX <tomaszx.kulasek@intel.com>
> > Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> > olivier.matz@6wind.com; Richardson, Bruce <bruce.richardson@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
> >
> > 2016-12-01 19:20, Kulasek, TomaszX:
> > > Hi Thomas,
> > >
> > > Sorry, I have answered for this question in another thread and I missed
> > about this one. Detailed answer is below.
> >
> > Yes you already gave this answer.
> > And I will continue asking the question until you understand it.
> >
> > > > 2016-11-28 11:54, Thomas Monjalon:
> > > > > Hi,
> > > > >
> > > > > 2016-11-23 18:36, Tomasz Kulasek:
> > > > > > --- a/config/common_base
> > > > > > +++ b/config/common_base
> > > > > > @@ -120,6 +120,7 @@ CONFIG_RTE_MAX_QUEUES_PER_PORT=1024
> > > > > >  CONFIG_RTE_LIBRTE_IEEE1588=n
> > > > > >  CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
> > > > > >  CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
> > > > > > +CONFIG_RTE_ETHDEV_TX_PREPARE=y
> > > > >
> > > > > Please, remind me why is there a configuration here.
> > > > > It should be the responsibility of the application to call
> > > > > tx_prepare or not. If the application choose to use this new API
> > > > > but it is disabled, then the packets won't be prepared and there is
> > no error code:
> > > > >
> > > > > > +#else
> > > > > > +
> > > > > > +static inline uint16_t
> > > > > > +rte_eth_tx_prepare(__rte_unused uint8_t port_id, __rte_unused
> > > > uint16_t queue_id,
> > > > > > +               __rte_unused struct rte_mbuf **tx_pkts, uint16_t
> > > > > > +nb_pkts) {
> > > > > > +       return nb_pkts;
> > > > > > +}
> > > > > > +
> > > > > > +#endif
> > > > >
> > > > > So the application is not aware of the issue and it will not use
> > > > > any fallback.
> > >
> > > tx_prepare mechanism can be turned off by compilation flag (as discussed
> > with Jerin in http://dpdk.org/dev/patchwork/patch/15770/) to provide real
> > NOOP functionality (e.g. for low-end CPUs, where even unnecessary memory
> > dereference and check can have significant impact on performance).
> > >
> > > Jerin observed that on some architectures (e.g. low-end ARM with
> > embedded NIC), just reading and comparing 'dev->tx_pkt_prepare' may cause
> > significant performance drop, so he proposed to introduce this
> > configuration flag to provide real NOOP when tx_prepare functionality is
> > not required, and can be turned on based on the _target_ configuration.
> > >
> > > For other cases, when this flag is turned on (by default), and
> > tx_prepare is not implemented, functional NOOP is used based on comparison
> > (dev->tx_pkt_prepare == NULL).
> >
> > So if the application call this function and if it is disabled, it simply
> > won't work. Packets won't be prepared, checksum won't be computed.
> >
> > I give up, I just NACK.
> 
> It is not to be turned on/off whatever someone wants, but only and only for the case, when platform developer knows, that his platform
> doesn't need this callback, so, he may turn off it and then save some performance (this option is per target).
> 
> For this case, the behavior of tx_prepare will be exactly the same when it is turned on or off. If is not the same, there's no sense to turn it
> off. There were long topic, where we've tried to convince you, that it should be turned on for all devices.

As Tomasz pointed out the RTE_ETHDEV_TX_PREPARE was introduced to fulfill Jerin request.
>From here:
"Low-end ARMv7,ARMv8 targets may not have PCIE-RC support and it may have
only integrated NIC controller. On those targets/configs, where integrated NIC
controller does not use tx_prep service it can made it as NOOP to save
cycles on following "rte_eth_tx_prep" and associated "if (unlikely(nb_prep
< nb_rx))" checks in the application."
According to the measurements he done it can save ~7% on some low-end ARM machine.
You can read whole story here:
http://dpdk.org/dev/patchwork/patch/15770/
Though, if now you guys believe that this is not good enough reason,
I have absolutely no problem to remove the RTE_ETHDEV_TX_PREPARE and associated logic.
I personally don't use ARM boxes and don't plan to,
and in theory users can still do conditional compilation at the upper layer, if they want to. 
Konstantin

^ permalink raw reply

* Re: [PATCH v12 1/6] ethdev: add Tx preparation
From: Thomas Monjalon @ 2016-12-01 23:50 UTC (permalink / raw)
  To: Kulasek, TomaszX
  Cc: dev, Ananyev, Konstantin, olivier.matz, Richardson, Bruce
In-Reply-To: <3042915272161B4EB253DA4D77EB373A14F57CDE@IRSMSX102.ger.corp.intel.com>

2016-12-01 22:31, Kulasek, TomaszX:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2016-12-01 19:20, Kulasek, TomaszX:
> > > Hi Thomas,
> > >
> > > Sorry, I have answered for this question in another thread and I missed
> > about this one. Detailed answer is below.
> > 
> > Yes you already gave this answer.
> > And I will continue asking the question until you understand it.
> > 
> > > > 2016-11-28 11:54, Thomas Monjalon:
> > > > > Hi,
> > > > >
> > > > > 2016-11-23 18:36, Tomasz Kulasek:
> > > > > > --- a/config/common_base
> > > > > > +++ b/config/common_base
> > > > > > @@ -120,6 +120,7 @@ CONFIG_RTE_MAX_QUEUES_PER_PORT=1024
> > > > > >  CONFIG_RTE_LIBRTE_IEEE1588=n
> > > > > >  CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
> > > > > >  CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
> > > > > > +CONFIG_RTE_ETHDEV_TX_PREPARE=y
> > > > >
> > > > > Please, remind me why is there a configuration here.
> > > > > It should be the responsibility of the application to call
> > > > > tx_prepare or not. If the application choose to use this new API
> > > > > but it is disabled, then the packets won't be prepared and there is
> > no error code:
> > > > >
> > > > > > +#else
> > > > > > +
> > > > > > +static inline uint16_t
> > > > > > +rte_eth_tx_prepare(__rte_unused uint8_t port_id, __rte_unused
> > > > uint16_t queue_id,
> > > > > > +               __rte_unused struct rte_mbuf **tx_pkts, uint16_t
> > > > > > +nb_pkts) {
> > > > > > +       return nb_pkts;
> > > > > > +}
> > > > > > +
> > > > > > +#endif
> > > > >
> > > > > So the application is not aware of the issue and it will not use
> > > > > any fallback.
> > >
> > > tx_prepare mechanism can be turned off by compilation flag (as discussed
> > with Jerin in http://dpdk.org/dev/patchwork/patch/15770/) to provide real
> > NOOP functionality (e.g. for low-end CPUs, where even unnecessary memory
> > dereference and check can have significant impact on performance).
> > >
> > > Jerin observed that on some architectures (e.g. low-end ARM with
> > embedded NIC), just reading and comparing 'dev->tx_pkt_prepare' may cause
> > significant performance drop, so he proposed to introduce this
> > configuration flag to provide real NOOP when tx_prepare functionality is
> > not required, and can be turned on based on the _target_ configuration.
> > >
> > > For other cases, when this flag is turned on (by default), and
> > tx_prepare is not implemented, functional NOOP is used based on comparison
> > (dev->tx_pkt_prepare == NULL).
> > 
> > So if the application call this function and if it is disabled, it simply
> > won't work. Packets won't be prepared, checksum won't be computed.
> > 
> > I give up, I just NACK.
> 
> It is not to be turned on/off whatever someone wants, but only and only for the case, when platform developer knows, that his platform doesn't need this callback, so, he may turn off it and then save some performance (this option is per target).

How may he know? There is no comment in the config file, no documentation.

> For this case, the behavior of tx_prepare will be exactly the same when it is turned on or off. If is not the same, there's no sense to turn it off. There were long topic, where we've tried to convince you, that it should be turned on for all devices.

Really? You tried to convince me to turn it on?
No you were trying to convince Jerin.
I think it is a wrong idea to allow disabling this function.
I didn't comment in first discussion because Jerin told it was really
important for small hardware with fixed NIC, and I thought it would
be implemented in a way the application cannot be misleaded.

The only solution I see here is to add some comments in the configuration
file, below the #else and in the doc.
Have you checked doc/guides/prog_guide/poll_mode_drv.rst?

^ permalink raw reply

* Re: [PATCH v12 1/6] ethdev: add Tx preparation
From: Kulasek, TomaszX @ 2016-12-01 22:31 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: dev@dpdk.org, Ananyev, Konstantin, olivier.matz@6wind.com,
	Richardson, Bruce
In-Reply-To: <2505996.o0gdCe9Hsd@xps13>

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, December 1, 2016 20:52
> To: Kulasek, TomaszX <tomaszx.kulasek@intel.com>
> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> olivier.matz@6wind.com; Richardson, Bruce <bruce.richardson@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
> 
> 2016-12-01 19:20, Kulasek, TomaszX:
> > Hi Thomas,
> >
> > Sorry, I have answered for this question in another thread and I missed
> about this one. Detailed answer is below.
> 
> Yes you already gave this answer.
> And I will continue asking the question until you understand it.
> 
> > > 2016-11-28 11:54, Thomas Monjalon:
> > > > Hi,
> > > >
> > > > 2016-11-23 18:36, Tomasz Kulasek:
> > > > > --- a/config/common_base
> > > > > +++ b/config/common_base
> > > > > @@ -120,6 +120,7 @@ CONFIG_RTE_MAX_QUEUES_PER_PORT=1024
> > > > >  CONFIG_RTE_LIBRTE_IEEE1588=n
> > > > >  CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
> > > > >  CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
> > > > > +CONFIG_RTE_ETHDEV_TX_PREPARE=y
> > > >
> > > > Please, remind me why is there a configuration here.
> > > > It should be the responsibility of the application to call
> > > > tx_prepare or not. If the application choose to use this new API
> > > > but it is disabled, then the packets won't be prepared and there is
> no error code:
> > > >
> > > > > +#else
> > > > > +
> > > > > +static inline uint16_t
> > > > > +rte_eth_tx_prepare(__rte_unused uint8_t port_id, __rte_unused
> > > uint16_t queue_id,
> > > > > +               __rte_unused struct rte_mbuf **tx_pkts, uint16_t
> > > > > +nb_pkts) {
> > > > > +       return nb_pkts;
> > > > > +}
> > > > > +
> > > > > +#endif
> > > >
> > > > So the application is not aware of the issue and it will not use
> > > > any fallback.
> >
> > tx_prepare mechanism can be turned off by compilation flag (as discussed
> with Jerin in http://dpdk.org/dev/patchwork/patch/15770/) to provide real
> NOOP functionality (e.g. for low-end CPUs, where even unnecessary memory
> dereference and check can have significant impact on performance).
> >
> > Jerin observed that on some architectures (e.g. low-end ARM with
> embedded NIC), just reading and comparing 'dev->tx_pkt_prepare' may cause
> significant performance drop, so he proposed to introduce this
> configuration flag to provide real NOOP when tx_prepare functionality is
> not required, and can be turned on based on the _target_ configuration.
> >
> > For other cases, when this flag is turned on (by default), and
> tx_prepare is not implemented, functional NOOP is used based on comparison
> (dev->tx_pkt_prepare == NULL).
> 
> So if the application call this function and if it is disabled, it simply
> won't work. Packets won't be prepared, checksum won't be computed.
> 
> I give up, I just NACK.

It is not to be turned on/off whatever someone wants, but only and only for the case, when platform developer knows, that his platform doesn't need this callback, so, he may turn off it and then save some performance (this option is per target).

For this case, the behavior of tx_prepare will be exactly the same when it is turned on or off. If is not the same, there's no sense to turn it off. There were long topic, where we've tried to convince you, that it should be turned on for all devices.

Tomasz

^ permalink raw reply

* Re: [PATCH v12 0/6] add Tx preparation
From: Jerin Jacob @ 2016-12-01 22:03 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: Adrien Mazarguil, Ananyev, Konstantin, dev, Rahul Lakkireddy,
	Stephen Hurd, Jan Medala, Jakub Palider, John Daley,
	Alejandro Lucero, Harish Patil, Rasesh Mody, Yuanhan Liu,
	Yong Wang, Kulasek, TomaszX, olivier.matz
In-Reply-To: <4746171.WvdnAfCX13@xps13>

On Thu, Dec 01, 2016 at 09:58:31AM +0100, Thomas Monjalon wrote:
> 2016-12-01 08:15, Adrien Mazarguil:
> > I'm perhaps a bit pessimistic mind you, but I do not think tx_prepare() will
> > remain optional for long. Sure, PMDs that do not implement it do not care,
> > I'm focusing on applications, for which the performance impact of calling
> > tx_prepare() followed by tx_burst() is higher than a single tx_burst()
> > performing all the necessary preparation at once.
> 
> I agree that tx_prepare() should become mandatory shortly.

I agree. The tx_prepare has to be mandatory. Application will have no
idea on how PMD drivers use this hook to fix up PMD tx side limitations.
On other side, if it turns out to be mandatory, what real benefit it is
going to have compared to existing scheme of just tx_burst.

> 
> > [...]
> > > > Following the same logic, why can't such a thing be made part of the TX
> > > > burst function as well (through a direct call to rte_phdr_cksum_fix()
> > > > whenever necessary). From an application standpoint, what are the advantages
> > > > of having to:
> > > > 
> > > >  if (tx_prep()) // iterate and update mbufs as needed
> > > >      tx_burst(); // iterate and send
> > > > 
> > > > Compared to:
> > > > 
> > > >  tx_burst(); // iterate, update as needed and send
> > > 
> > > I think that was discussed extensively quite a lot previously here:
> > > As Thomas already replied - main motivation is to allow user
> > > to execute them on different stages of packet TX pipeline,
> > > and probably on different cores.
> > > I think that provides better flexibility to the user to when/where
> > > do these preparations and hopefully would lead to better performance.
> > 
> > And I agree, I think this use case is valid but does not warrant such a high
> > penalty when your application does not need that much flexibility. Simple
> > (yet conscious) applications need the highest performance. Complex ones as
> > you described already suffer quite a bit from IPCs and won't mind a couple
> > of extra CPU cycles right?
> > 
> > Yes they will, therefore we need a method that satisfies both cases.
> > 
> > As a possible solution, a special mbuf flag could be added to each mbuf
> > having gone through tx_prepare(). That way, tx_burst() could skip some
> > checks and things it would otherwise have done.
> 
> I like this idea!
> 

^ permalink raw reply

* Re: [PATCH v12 1/6] ethdev: add Tx preparation
From: Jerin Jacob @ 2016-12-01 21:56 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: Kulasek, TomaszX, dev, Ananyev, Konstantin, olivier.matz,
	Richardson, Bruce
In-Reply-To: <2505996.o0gdCe9Hsd@xps13>

On Thu, Dec 01, 2016 at 08:52:22PM +0100, Thomas Monjalon wrote:
> 2016-12-01 19:20, Kulasek, TomaszX:
> > Hi Thomas,
> > 
> > Sorry, I have answered for this question in another thread and I missed about this one. Detailed answer is below.
> 
> Yes you already gave this answer.
> And I will continue asking the question until you understand it.
> 
> > > 2016-11-28 11:54, Thomas Monjalon:
> > > > Hi,
> > > >
> > > > 2016-11-23 18:36, Tomasz Kulasek:
> > > > > --- a/config/common_base
> > > > > +++ b/config/common_base
> > > > > @@ -120,6 +120,7 @@ CONFIG_RTE_MAX_QUEUES_PER_PORT=1024
> > > > >  CONFIG_RTE_LIBRTE_IEEE1588=n
> > > > >  CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
> > > > >  CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
> > > > > +CONFIG_RTE_ETHDEV_TX_PREPARE=y
> > > >
> > > > Please, remind me why is there a configuration here.
> > > > It should be the responsibility of the application to call tx_prepare
> > > > or not. If the application choose to use this new API but it is
> > > > disabled, then the packets won't be prepared and there is no error code:
> > > >
> > > > > +#else
> > > > > +
> > > > > +static inline uint16_t
> > > > > +rte_eth_tx_prepare(__rte_unused uint8_t port_id, __rte_unused
> > > uint16_t queue_id,
> > > > > +               __rte_unused struct rte_mbuf **tx_pkts, uint16_t
> > > > > +nb_pkts) {
> > > > > +       return nb_pkts;
> > > > > +}
> > > > > +
> > > > > +#endif
> > > >
> > > > So the application is not aware of the issue and it will not use any
> > > > fallback.
> > 
> > tx_prepare mechanism can be turned off by compilation flag (as discussed with Jerin in http://dpdk.org/dev/patchwork/patch/15770/) to provide real NOOP functionality (e.g. for low-end CPUs, where even unnecessary memory dereference and check can have significant impact on performance).
> > 
> > Jerin observed that on some architectures (e.g. low-end ARM with embedded NIC), just reading and comparing 'dev->tx_pkt_prepare' may cause significant performance drop, so he proposed to introduce this configuration flag to provide real NOOP when tx_prepare functionality is not required, and can be turned on based on the _target_ configuration.
> > 
> > For other cases, when this flag is turned on (by default), and tx_prepare is not implemented, functional NOOP is used based on comparison (dev->tx_pkt_prepare == NULL).
> 
> So if the application call this function and if it is disabled, it simply
> won't work. Packets won't be prepared, checksum won't be computed.
The use case I was referring  was "integrated NIC" case where
- DPDK target with no external NW PCI card support
AND
- The "integrated NIC" does not need tx_prepare

^ permalink raw reply

* Re: [PATCH v12 1/6] ethdev: add Tx preparation
From: Thomas Monjalon @ 2016-12-01 19:52 UTC (permalink / raw)
  To: Kulasek, TomaszX
  Cc: dev, Ananyev, Konstantin, olivier.matz, Richardson, Bruce
In-Reply-To: <3042915272161B4EB253DA4D77EB373A14F57CAE@IRSMSX102.ger.corp.intel.com>

2016-12-01 19:20, Kulasek, TomaszX:
> Hi Thomas,
> 
> Sorry, I have answered for this question in another thread and I missed about this one. Detailed answer is below.

Yes you already gave this answer.
And I will continue asking the question until you understand it.

> > 2016-11-28 11:54, Thomas Monjalon:
> > > Hi,
> > >
> > > 2016-11-23 18:36, Tomasz Kulasek:
> > > > --- a/config/common_base
> > > > +++ b/config/common_base
> > > > @@ -120,6 +120,7 @@ CONFIG_RTE_MAX_QUEUES_PER_PORT=1024
> > > >  CONFIG_RTE_LIBRTE_IEEE1588=n
> > > >  CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
> > > >  CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
> > > > +CONFIG_RTE_ETHDEV_TX_PREPARE=y
> > >
> > > Please, remind me why is there a configuration here.
> > > It should be the responsibility of the application to call tx_prepare
> > > or not. If the application choose to use this new API but it is
> > > disabled, then the packets won't be prepared and there is no error code:
> > >
> > > > +#else
> > > > +
> > > > +static inline uint16_t
> > > > +rte_eth_tx_prepare(__rte_unused uint8_t port_id, __rte_unused
> > uint16_t queue_id,
> > > > +               __rte_unused struct rte_mbuf **tx_pkts, uint16_t
> > > > +nb_pkts) {
> > > > +       return nb_pkts;
> > > > +}
> > > > +
> > > > +#endif
> > >
> > > So the application is not aware of the issue and it will not use any
> > > fallback.
> 
> tx_prepare mechanism can be turned off by compilation flag (as discussed with Jerin in http://dpdk.org/dev/patchwork/patch/15770/) to provide real NOOP functionality (e.g. for low-end CPUs, where even unnecessary memory dereference and check can have significant impact on performance).
> 
> Jerin observed that on some architectures (e.g. low-end ARM with embedded NIC), just reading and comparing 'dev->tx_pkt_prepare' may cause significant performance drop, so he proposed to introduce this configuration flag to provide real NOOP when tx_prepare functionality is not required, and can be turned on based on the _target_ configuration.
> 
> For other cases, when this flag is turned on (by default), and tx_prepare is not implemented, functional NOOP is used based on comparison (dev->tx_pkt_prepare == NULL).

So if the application call this function and if it is disabled, it simply
won't work. Packets won't be prepared, checksum won't be computed.

I give up, I just NACK.

^ permalink raw reply

* Re: [PATCH v12 1/6] ethdev: add Tx preparation
From: Kulasek, TomaszX @ 2016-12-01 19:20 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: dev@dpdk.org, Ananyev, Konstantin, olivier.matz@6wind.com,
	Richardson, Bruce
In-Reply-To: <1734448.0id6dCbsBT@xps13>

Hi Thomas,

Sorry, I have answered for this question in another thread and I missed about this one. Detailed answer is below.

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, December 1, 2016 17:24
> To: Kulasek, TomaszX <tomaszx.kulasek@intel.com>
> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> olivier.matz@6wind.com; Richardson, Bruce <bruce.richardson@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v12 1/6] ethdev: add Tx preparation
> 
> Please, a reply to this question would be greatly appreciated.
> 
> 2016-11-28 11:54, Thomas Monjalon:
> > Hi,
> >
> > 2016-11-23 18:36, Tomasz Kulasek:
> > > --- a/config/common_base
> > > +++ b/config/common_base
> > > @@ -120,6 +120,7 @@ CONFIG_RTE_MAX_QUEUES_PER_PORT=1024
> > >  CONFIG_RTE_LIBRTE_IEEE1588=n
> > >  CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
> > >  CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
> > > +CONFIG_RTE_ETHDEV_TX_PREPARE=y
> >
> > Please, remind me why is there a configuration here.
> > It should be the responsibility of the application to call tx_prepare
> > or not. If the application choose to use this new API but it is
> > disabled, then the packets won't be prepared and there is no error code:
> >
> > > +#else
> > > +
> > > +static inline uint16_t
> > > +rte_eth_tx_prepare(__rte_unused uint8_t port_id, __rte_unused
> uint16_t queue_id,
> > > +               __rte_unused struct rte_mbuf **tx_pkts, uint16_t
> > > +nb_pkts) {
> > > +       return nb_pkts;
> > > +}
> > > +
> > > +#endif
> >
> > So the application is not aware of the issue and it will not use any
> > fallback.

tx_prepare mechanism can be turned off by compilation flag (as discussed with Jerin in http://dpdk.org/dev/patchwork/patch/15770/) to provide real NOOP functionality (e.g. for low-end CPUs, where even unnecessary memory dereference and check can have significant impact on performance).

Jerin observed that on some architectures (e.g. low-end ARM with embedded NIC), just reading and comparing 'dev->tx_pkt_prepare' may cause significant performance drop, so he proposed to introduce this configuration flag to provide real NOOP when tx_prepare functionality is not required, and can be turned on based on the _target_ configuration.

For other cases, when this flag is turned on (by default), and tx_prepare is not implemented, functional NOOP is used based on comparison (dev->tx_pkt_prepare == NULL).

Tomasz

^ permalink raw reply

* Re: [PATCH v12 1/6] ethdev: add Tx preparation
From: Thomas Monjalon @ 2016-12-01 16:28 UTC (permalink / raw)
  To: Tomasz Kulasek; +Cc: dev, konstantin.ananyev, olivier.matz
In-Reply-To: <1479922585-8640-2-git-send-email-tomaszx.kulasek@intel.com>

2016-11-23 18:36, Tomasz Kulasek:
> +/**
> + * Process a burst of output packets on a transmit queue of an Ethernet device.
> + *
> + * The rte_eth_tx_prepare() function is invoked to prepare output packets to be
> + * transmitted on the output queue *queue_id* of the Ethernet device designated
> + * by its *port_id*.
> + * The *nb_pkts* parameter is the number of packets to be prepared which are
> + * supplied in the *tx_pkts* array of *rte_mbuf* structures, each of them
> + * allocated from a pool created with rte_pktmbuf_pool_create().
> + * For each packet to send, the rte_eth_tx_prepare() function performs
> + * the following operations:
> + *
> + * - Check if packet meets devices requirements for tx offloads.
> + *
> + * - Check limitations about number of segments.
> + *
> + * - Check additional requirements when debug is enabled.
> + *
> + * - Update and/or reset required checksums when tx offload is set for packet.
> + *
> + * Since this function can modify packet data, provided mbufs must be safely
> + * writable (e.g. modified data cannot be in shared segment).

I think we will have to remove this limitation in next releases.
As we don't know how it could affect the API, I suggest to declare this
API EXPERIMENTAL.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox