[dpdk-dev] [PATCH v2] examples: fix ip_reassembly not work with some NICs

[dpdk-dev] [PATCH v2] examples: fix ip_reassembly not work with some NICs

[Yong Liu]

Some network device drivers like Fortville may not fill packet type by
default. Changed the method for detecting packet type from mbuf packet
type to ethernet header MAC type will make sure this example compatible
with all NICs.

Fixes: b84fb4cb88ff ("examples/ip_reassembly: overhaul")

v2:
* fix code style issue

Signed-off-by: Yong Liu <yong.liu@intel.com>

diff --git a/examples/ip_reassembly/main.c b/examples/ip_reassembly/main.c
index 50fe422..974fabc 100644
--- a/examples/ip_reassembly/main.c
+++ b/examples/ip_reassembly/main.c
@@ -356,7 +356,7 @@ struct rte_lpm6_config lpm6_config = {
 	dst_port = portid;
 
 	/* if packet is IPv4 */
-	if (RTE_ETH_IS_IPV4_HDR(m->packet_type)) {
+	if (eth_hdr->ether_type == rte_cpu_to_be_16(ETHER_TYPE_IPv4)) {
 		struct ipv4_hdr *ip_hdr;
 		uint32_t ip_dst;
 
@@ -395,8 +395,7 @@ struct rte_lpm6_config lpm6_config = {
 			dst_port = next_hop_ipv4;
 		}
 
-		eth_hdr->ether_type = rte_be_to_cpu_16(ETHER_TYPE_IPv4);
-	} else if (RTE_ETH_IS_IPV6_HDR(m->packet_type)) {
+	} else if (eth_hdr->ether_type == rte_cpu_to_be_16(ETHER_TYPE_IPv6)) {
 		/* if packet is IPv6 */
 		struct ipv6_extension_fragment *frag_hdr;
 		struct ipv6_hdr *ip_hdr;
@@ -431,8 +430,6 @@ struct rte_lpm6_config lpm6_config = {
 				(enabled_port_mask & 1 << next_hop_ipv6) != 0) {
 			dst_port = next_hop_ipv6;
 		}
-
-		eth_hdr->ether_type = rte_be_to_cpu_16(ETHER_TYPE_IPv6);
 	}
 	/* if packet wasn't IPv4 or IPv6, it's forwarded to the port it came from */
 
-- 
1.9.3

[dpdk-dev] [PATCH v3 1/3] examples/ip_reassembly: add parse-ptype option

[Marvin Liu]

Add new option parse-ptype in this sample in case of pmd driver
not provide packet type info. If this option enabled, packet type
will be analyzed in Rx callback function.

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
Signed-off-by: Yong Liu <yong.liu@intel.com>

diff --git a/examples/ip_reassembly/main.c b/examples/ip_reassembly/main.c
index 50fe422..82b6055 100644
--- a/examples/ip_reassembly/main.c
+++ b/examples/ip_reassembly/main.c
@@ -155,6 +155,9 @@
 
 static int rx_queue_per_lcore = 1;
 
+/* Parse packet type using rx callback and disabled by default */
+static int parse_ptype;
+
 struct mbuf_table {
 	uint32_t len;
 	uint32_t head;
@@ -541,13 +544,15 @@ struct rte_lpm6_config lpm6_config = {
 {
 	printf("%s [EAL options] -- -p PORTMASK [-q NQ]"
 		"  [--max-pkt-len PKTLEN]"
-		"  [--maxflows=<flows>]  [--flowttl=<ttl>[(s|ms)]]\n"
+		"  [--maxflows=<flows>]  [--flowttl=<ttl>[(s|ms)]]"
+		"  [--parse-ptype]\n"
 		"  -p PORTMASK: hexadecimal bitmask of ports to configure\n"
 		"  -q NQ: number of RX queues per lcore\n"
 		"  --maxflows=<flows>: optional, maximum number of flows "
 		"supported\n"
 		"  --flowttl=<ttl>[(s|ms)]: optional, maximum TTL for each "
-		"flow\n",
+		"flow\n"
+		"  --parse-ptype: Set to use software to analyze packet type\n",
 		prgname);
 }
 
@@ -636,6 +641,7 @@ struct rte_lpm6_config lpm6_config = {
 	return n;
 }
 
+#define CMD_LINE_OPT_PARSE_PTYPE "parse-ptype"
 /* Parse the argument given in the command line of the application */
 static int
 parse_args(int argc, char **argv)
@@ -648,6 +654,7 @@ struct rte_lpm6_config lpm6_config = {
 		{"max-pkt-len", 1, 0, 0},
 		{"maxflows", 1, 0, 0},
 		{"flowttl", 1, 0, 0},
+		{CMD_LINE_OPT_PARSE_PTYPE, 0, 0, 0},
 		{NULL, 0, 0, 0}
 	};
 
@@ -706,6 +713,13 @@ struct rte_lpm6_config lpm6_config = {
 				}
 			}
 
+			if (!strncmp(lgopts[option_index].name,
+					CMD_LINE_OPT_PARSE_PTYPE,
+					sizeof(CMD_LINE_OPT_PARSE_PTYPE))) {
+				printf("soft parse-ptype is enabled\n");
+				parse_ptype = 1;
+			}
+
 			break;
 
 		default:
@@ -730,6 +744,74 @@ struct rte_lpm6_config lpm6_config = {
 	printf("%s%s", name, buf);
 }
 
+static inline void
+parse_ptype_one(struct rte_mbuf *m)
+{
+	struct ether_hdr *eth_hdr;
+	uint32_t packet_type = RTE_PTYPE_UNKNOWN;
+	uint16_t ether_type;
+
+	eth_hdr = rte_pktmbuf_mtod(m, struct ether_hdr *);
+	ether_type = eth_hdr->ether_type;
+	if (ether_type == rte_cpu_to_be_16(ETHER_TYPE_IPv4))
+		packet_type |= RTE_PTYPE_L3_IPV4_EXT_UNKNOWN;
+	else if (ether_type == rte_cpu_to_be_16(ETHER_TYPE_IPv6))
+		packet_type |= RTE_PTYPE_L3_IPV6_EXT_UNKNOWN;
+
+	m->packet_type = packet_type;
+}
+
+static uint16_t
+cb_parse_ptype(uint8_t port __rte_unused, uint16_t queue __rte_unused,
+		struct rte_mbuf *pkts[], uint16_t nb_pkts,
+		uint16_t max_pkts __rte_unused,
+		void *user_param __rte_unused)
+{
+	unsigned int i;
+
+	for (i = 0; i < nb_pkts; ++i)
+		parse_ptype_one(pkts[i]);
+
+	return nb_pkts;
+}
+
+static int
+add_cb_parse_ptype(uint8_t portid, uint16_t queueid)
+{
+	printf("Port %d: softly parse packet type info\n", portid);
+	if (rte_eth_add_rx_callback(portid, queueid, cb_parse_ptype, NULL))
+		return 0;
+
+	printf("Failed to add rx callback: port=%d\n", portid);
+	return -1;
+}
+
+static int check_ptype(uint8_t portid)
+{
+	int i, ret;
+	int ptype_l3_ipv4 = 0;
+	int ptype_l3_ipv6 = 0;
+	uint32_t ptype_mask = RTE_PTYPE_L3_MASK;
+
+	ret = rte_eth_dev_get_supported_ptypes(portid, ptype_mask, NULL, 0);
+	if (ret <= 0)
+		return 0;
+
+	uint32_t ptypes[ret];
+	ret = rte_eth_dev_get_supported_ptypes(portid, ptype_mask, ptypes, ret);
+	for (i = 0; i < ret; ++i) {
+		if (ptypes[i] & RTE_PTYPE_L3_IPV4)
+			ptype_l3_ipv4 = 1;
+		if (ptypes[i] & RTE_PTYPE_L3_IPV6)
+			ptype_l3_ipv6 = 1;
+	}
+
+	if (ptype_l3_ipv4 && ptype_l3_ipv6)
+		return 1;
+
+	return 0;
+}
+
 /* Check the link status of all ports in up to 9s, and print them finally */
 static void
 check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
@@ -1117,6 +1199,14 @@ struct rte_lpm6_config lpm6_config = {
 		print_ethaddr(" Address:", &ports_eth_addr[portid]);
 		printf("\n");
 
+		if (parse_ptype) {
+			if (add_cb_parse_ptype(portid, queueid) < 0)
+				rte_exit(EXIT_FAILURE,
+					"Fail to add ptype cb\n");
+		} else if (!check_ptype(portid))
+			rte_exit(EXIT_FAILURE,
+				"PMD can not provide needed ptypes\n");
+
 		/* init one TX queue per couple (lcore,port) */
 		queueid = 0;
 		for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
-- 
1.9.1

[dpdk-dev] [PATCH v3 2/3] examples/ip_fragmentation: add parse-ptype option

[Marvin Liu]

Add new option parse-ptype in this sample in case of pmd driver
not provide packet type info. If this option enabled, packet type
will be analyzed in Rx callback function.

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
Signed-off-by: Yong Liu <yong.liu@intel.com>

diff --git a/examples/ip_fragmentation/main.c b/examples/ip_fragmentation/main.c
index e1e32c6..df0e73c 100644
--- a/examples/ip_fragmentation/main.c
+++ b/examples/ip_fragmentation/main.c
@@ -143,6 +143,10 @@
 
 static int rx_queue_per_lcore = 1;
 
+#define CMD_LINE_OPT_PARSE_PTYPE "parse-ptype"
+/* Parse packet type using rx callback and disabled by default */
+static int parse_ptype;
+
 #define MBUF_TABLE_SIZE  (2 * MAX(MAX_PKT_BURST, MAX_PACKET_FRAG))
 
 struct mbuf_table {
@@ -488,7 +492,9 @@ struct rte_lpm6_config lpm6_config = {
 {
 	printf("%s [EAL options] -- -p PORTMASK [-q NQ]\n"
 	       "  -p PORTMASK: hexadecimal bitmask of ports to configure\n"
-	       "  -q NQ: number of queue (=ports) per lcore (default is 1)\n",
+	       "  -q NQ: number of queue (=ports) per lcore (default is 1)\n"
+	       "  --"CMD_LINE_OPT_PARSE_PTYPE": Set to use software to "
+	       "analyze packet type\n",
 	       prgname);
 }
 
@@ -536,6 +542,7 @@ struct rte_lpm6_config lpm6_config = {
 	int option_index;
 	char *prgname = argv[0];
 	static struct option lgopts[] = {
+		{CMD_LINE_OPT_PARSE_PTYPE, 0, 0, 0},
 		{NULL, 0, 0, 0}
 	};
 
@@ -567,8 +574,13 @@ struct rte_lpm6_config lpm6_config = {
 
 		/* long options */
 		case 0:
-			print_usage(prgname);
-			return -1;
+			if (!strncmp(lgopts[option_index].name,
+					CMD_LINE_OPT_PARSE_PTYPE,
+					sizeof(CMD_LINE_OPT_PARSE_PTYPE))) {
+				printf("soft parse-ptype is enabled\n");
+				parse_ptype = 1;
+			}
+			break;
 
 		default:
 			print_usage(prgname);
@@ -598,6 +610,74 @@ struct rte_lpm6_config lpm6_config = {
 	printf("%s%s", name, buf);
 }
 
+static inline void
+parse_ptype_one(struct rte_mbuf *m)
+{
+	struct ether_hdr *eth_hdr;
+	uint32_t packet_type = RTE_PTYPE_UNKNOWN;
+	uint16_t ether_type;
+
+	eth_hdr = rte_pktmbuf_mtod(m, struct ether_hdr *);
+	ether_type = eth_hdr->ether_type;
+	if (ether_type == rte_cpu_to_be_16(ETHER_TYPE_IPv4))
+		packet_type |= RTE_PTYPE_L3_IPV4_EXT_UNKNOWN;
+	else if (ether_type == rte_cpu_to_be_16(ETHER_TYPE_IPv6))
+		packet_type |= RTE_PTYPE_L3_IPV6_EXT_UNKNOWN;
+
+	m->packet_type = packet_type;
+}
+
+static uint16_t
+cb_parse_ptype(uint8_t port __rte_unused, uint16_t queue __rte_unused,
+		struct rte_mbuf *pkts[], uint16_t nb_pkts,
+		uint16_t max_pkts __rte_unused,
+		void *user_param __rte_unused)
+{
+	unsigned int i;
+
+	for (i = 0; i < nb_pkts; ++i)
+		parse_ptype_one(pkts[i]);
+
+	return nb_pkts;
+}
+
+static int
+add_cb_parse_ptype(uint8_t portid, uint16_t queueid)
+{
+	printf("Port %d: softly parse packet type info\n", portid);
+	if (rte_eth_add_rx_callback(portid, queueid, cb_parse_ptype, NULL))
+		return 0;
+
+	printf("Failed to add rx callback: port=%d\n", portid);
+	return -1;
+}
+
+static int check_ptype(uint8_t portid)
+{
+	int i, ret;
+	int ptype_l3_ipv4 = 0;
+	int ptype_l3_ipv6 = 0;
+	uint32_t ptype_mask = RTE_PTYPE_L3_MASK;
+
+	ret = rte_eth_dev_get_supported_ptypes(portid, ptype_mask, NULL, 0);
+	if (ret <= 0)
+		return 0;
+
+	uint32_t ptypes[ret];
+	ret = rte_eth_dev_get_supported_ptypes(portid, ptype_mask, ptypes, ret);
+	for (i = 0; i < ret; ++i) {
+		if (ptypes[i] & RTE_PTYPE_L3_IPV4)
+			ptype_l3_ipv4 = 1;
+		if (ptypes[i] & RTE_PTYPE_L3_IPV6)
+			ptype_l3_ipv6 = 1;
+	}
+
+	if (ptype_l3_ipv4 && ptype_l3_ipv6)
+		return 1;
+
+	return 0;
+}
+
 /* Check the link status of all ports in up to 9s, and print them finally */
 static void
 check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
@@ -901,6 +981,14 @@ struct rte_lpm6_config lpm6_config = {
 		print_ethaddr(" Address:", &ports_eth_addr[portid]);
 		printf("\n");
 
+		if (parse_ptype) {
+			if (add_cb_parse_ptype(portid, 0) < 0)
+				rte_exit(EXIT_FAILURE,
+					"Fail to add ptype cb\n");
+		} else if (!check_ptype(portid))
+			rte_exit(EXIT_FAILURE,
+				"PMD can not provide needed ptypes\n");
+
 		/* init one TX queue per couple (lcore,port) */
 		queueid = 0;
 		for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++) {
-- 
1.9.1

[dpdk-dev] [PATCH v3 3/3] examples/l3fwd-acl: add parse-ptype option

[Marvin Liu]

Add new option parse-ptype in this sample in case of pmd driver
not provide packet type info. If this option enabled, packet type
will be analyzed in Rx callback function.

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
Signed-off-by: Yong Liu <yong.liu@intel.com>

diff --git a/examples/l3fwd-acl/main.c b/examples/l3fwd-acl/main.c
index 3cfbb40..dfa9031 100644
--- a/examples/l3fwd-acl/main.c
+++ b/examples/l3fwd-acl/main.c
@@ -200,6 +200,7 @@ struct lcore_params {
 #define OPTION_RULE_IPV4	"rule_ipv4"
 #define OPTION_RULE_IPV6	"rule_ipv6"
 #define OPTION_SCALAR		"scalar"
+#define OPTION_PARSE_PTYPE	"parse-ptype"
 #define ACL_DENY_SIGNATURE	0xf0000000
 #define RTE_LOGTYPE_L3FWDACL	RTE_LOGTYPE_USER3
 #define acl_log(format, ...)	RTE_LOG(ERR, L3FWDACL, format, ##__VA_ARGS__)
@@ -470,6 +471,7 @@ struct acl_search_t {
 	const char *rule_ipv4_name;
 	const char *rule_ipv6_name;
 	int scalar;
+	int parse_ptype;
 } parm_config;
 
 const char cb_port_delim[] = ":";
@@ -1563,7 +1565,9 @@ struct lcore_conf {
 		"character '%c'.\n"
 		"  --"OPTION_RULE_IPV6"=FILE: specify the ipv6 rules "
 		"entries file.\n"
-		"  --"OPTION_SCALAR": Use scalar function to do lookup\n",
+		"  --"OPTION_SCALAR": Use scalar function to do lookup.\n"
+		"  --"OPTION_PARSE_PTYPE": Set to use software to analyze "
+		"packet type.\n",
 		prgname, ACL_LEAD_CHAR, ROUTE_LEAD_CHAR);
 }
 
@@ -1671,6 +1675,7 @@ struct lcore_conf {
 		{OPTION_RULE_IPV4, 1, 0, 0},
 		{OPTION_RULE_IPV6, 1, 0, 0},
 		{OPTION_SCALAR, 0, 0, 0},
+		{OPTION_PARSE_PTYPE, 0, 0, 0},
 		{NULL, 0, 0, 0}
 	};
 
@@ -1763,6 +1768,12 @@ struct lcore_conf {
 					OPTION_SCALAR, sizeof(OPTION_SCALAR)))
 				parm_config.scalar = 1;
 
+			if (!strncmp(lgopts[option_index].name,
+					OPTION_PARSE_PTYPE,
+					sizeof(OPTION_PARSE_PTYPE))) {
+				printf("soft parse-ptype is enabled\n");
+				parm_config.parse_ptype = 1;
+			}
 
 			break;
 
@@ -1828,6 +1839,74 @@ struct lcore_conf {
 	return 0;
 }
 
+static inline void
+parse_ptype_one(struct rte_mbuf *m)
+{
+	struct ether_hdr *eth_hdr;
+	uint32_t packet_type = RTE_PTYPE_UNKNOWN;
+	uint16_t ether_type;
+
+	eth_hdr = rte_pktmbuf_mtod(m, struct ether_hdr *);
+	ether_type = eth_hdr->ether_type;
+	if (ether_type == rte_cpu_to_be_16(ETHER_TYPE_IPv4))
+		packet_type |= RTE_PTYPE_L3_IPV4_EXT_UNKNOWN;
+	else if (ether_type == rte_cpu_to_be_16(ETHER_TYPE_IPv6))
+		packet_type |= RTE_PTYPE_L3_IPV6_EXT_UNKNOWN;
+
+	m->packet_type = packet_type;
+}
+
+static uint16_t
+cb_parse_ptype(uint8_t port __rte_unused, uint16_t queue __rte_unused,
+		struct rte_mbuf *pkts[], uint16_t nb_pkts,
+		uint16_t max_pkts __rte_unused,
+		void *user_param __rte_unused)
+{
+	unsigned int i;
+
+	for (i = 0; i < nb_pkts; ++i)
+		parse_ptype_one(pkts[i]);
+
+	return nb_pkts;
+}
+
+static int
+add_cb_parse_ptype(uint8_t portid, uint16_t queueid)
+{
+	printf("Port %d: softly parse packet type info\n", portid);
+	if (rte_eth_add_rx_callback(portid, queueid, cb_parse_ptype, NULL))
+		return 0;
+
+	printf("Failed to add rx callback: port=%d\n", portid);
+	return -1;
+}
+
+static int check_ptype(uint8_t portid)
+{
+	int i, ret;
+	int ptype_l3_ipv4 = 0;
+	int ptype_l3_ipv6 = 0;
+	uint32_t ptype_mask = RTE_PTYPE_L3_MASK;
+
+	ret = rte_eth_dev_get_supported_ptypes(portid, ptype_mask, NULL, 0);
+	if (ret <= 0)
+		return 0;
+
+	uint32_t ptypes[ret];
+	ret = rte_eth_dev_get_supported_ptypes(portid, ptype_mask, ptypes, ret);
+	for (i = 0; i < ret; ++i) {
+		if (ptypes[i] & RTE_PTYPE_L3_IPV4)
+			ptype_l3_ipv4 = 1;
+		if (ptypes[i] & RTE_PTYPE_L3_IPV6)
+			ptype_l3_ipv6 = 1;
+	}
+
+	if (ptype_l3_ipv4 && ptype_l3_ipv6)
+		return 1;
+
+	return 0;
+}
+
 /* Check the link status of all ports in up to 9s, and print them finally */
 static void
 check_all_ports_link_status(uint8_t port_num, uint32_t port_mask)
@@ -2039,7 +2118,15 @@ struct lcore_conf {
 				rte_exit(EXIT_FAILURE,
 					"rte_eth_rx_queue_setup: err=%d,"
 					"port=%d\n", ret, portid);
-		}
+
+			if (parm_config.parse_ptype) {
+				if (add_cb_parse_ptype(portid, queueid) < 0)
+					rte_exit(EXIT_FAILURE,
+						"Fail to add ptype cb\n");
+			} else if (!check_ptype(portid))
+				rte_exit(EXIT_FAILURE,
+					"PMD can not provide needed ptypes\n");
+			}
 	}
 
 	printf("\n");
-- 
1.9.1

Re: [dpdk-dev] [PATCH v3 1/3] examples/ip_reassembly: add parse-ptype option

[Thomas Monjalon]

2017-02-09 22:25, Marvin Liu:
> Add new option parse-ptype in this sample in case of pmd driver
> not provide packet type info. If this option enabled, packet type
> will be analyzed in Rx callback function.
[...]
> +		if (parse_ptype) {
> +			if (add_cb_parse_ptype(portid, queueid) < 0)
> +				rte_exit(EXIT_FAILURE,
> +					"Fail to add ptype cb\n");
> +		} else if (!check_ptype(portid))
> +			rte_exit(EXIT_FAILURE,
> +				"PMD can not provide needed ptypes\n");

Instead of adding a new option, why not adding the callback automatically
if the packet type is not supported by the hardware?

Re: [dpdk-dev] [PATCH v3 1/3] examples/ip_reassembly: add parse-ptype option

[Liu, Yong]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Friday, February 10, 2017 5:27 AM
> To: Liu, Yong <yong.liu@intel.com>; Tan, Jianfeng <jianfeng.tan@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 1/3] examples/ip_reassembly: add parse-
> ptype option
> 
> 2017-02-09 22:25, Marvin Liu:
> > Add new option parse-ptype in this sample in case of pmd driver
> > not provide packet type info. If this option enabled, packet type
> > will be analyzed in Rx callback function.
> [...]
> > +		if (parse_ptype) {
> > +			if (add_cb_parse_ptype(portid, queueid) < 0)
> > +				rte_exit(EXIT_FAILURE,
> > +					"Fail to add ptype cb\n");
> > +		} else if (!check_ptype(portid))
> > +			rte_exit(EXIT_FAILURE,
> > +				"PMD can not provide needed ptypes\n");
> 
> Instead of adding a new option, why not adding the callback automatically
> if the packet type is not supported by the hardware?

Thomas,
We want to let user choice which kind of method for packet type parsing. 
If start application with parse-type option, is meaning user want to use software parsing otherwise will use hardware parsing.

BRs,
Marvin

Re: [dpdk-dev] [PATCH v3 1/3] examples/ip_reassembly: add parse-ptype option

[Thomas Monjalon]

2017-02-10 07:53, Liu, Yong:
> From: Thomas Monjalon
> > 2017-02-09 22:25, Marvin Liu:
> > > Add new option parse-ptype in this sample in case of pmd driver
> > > not provide packet type info. If this option enabled, packet type
> > > will be analyzed in Rx callback function.
> > [...]
> > > +		if (parse_ptype) {
> > > +			if (add_cb_parse_ptype(portid, queueid) < 0)
> > > +				rte_exit(EXIT_FAILURE,
> > > +					"Fail to add ptype cb\n");
> > > +		} else if (!check_ptype(portid))
> > > +			rte_exit(EXIT_FAILURE,
> > > +				"PMD can not provide needed ptypes\n");
> > 
> > Instead of adding a new option, why not adding the callback automatically
> > if the packet type is not supported by the hardware?
> 
> Thomas,
> We want to let user choice which kind of method for packet type parsing. 
> If start application with parse-type option, is meaning user want to use software parsing otherwise will use hardware parsing.

I do not understand why this user choice matters.
If it is available, hardware ptype is better, isn't it?
It it is not available, we need to be aware of this specific issue,
otherwise we have the error "PMD can not provide needed ptypes"
(without suggesting to use the option).

Re: [dpdk-dev] [PATCH v3 1/3] examples/ip_reassembly: add parse-ptype option

[Tan, Jianfeng]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, February 10, 2017 4:36 PM
> To: Liu, Yong
> Cc: Tan, Jianfeng; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 1/3] examples/ip_reassembly: add parse-
> ptype option
> 
> 2017-02-10 07:53, Liu, Yong:
> > From: Thomas Monjalon
> > > 2017-02-09 22:25, Marvin Liu:
> > > > Add new option parse-ptype in this sample in case of pmd driver
> > > > not provide packet type info. If this option enabled, packet type
> > > > will be analyzed in Rx callback function.
> > > [...]
> > > > +		if (parse_ptype) {
> > > > +			if (add_cb_parse_ptype(portid, queueid) < 0)
> > > > +				rte_exit(EXIT_FAILURE,
> > > > +					"Fail to add ptype cb\n");
> > > > +		} else if (!check_ptype(portid))
> > > > +			rte_exit(EXIT_FAILURE,
> > > > +				"PMD can not provide needed ptypes\n");
> > >
> > > Instead of adding a new option, why not adding the callback automatically
> > > if the packet type is not supported by the hardware?
> >
> > Thomas,
> > We want to let user choice which kind of method for packet type parsing.
> > If start application with parse-type option, is meaning user want to use
> software parsing otherwise will use hardware parsing.
> 
> I do not understand why this user choice matters.
> If it is available, hardware ptype is better, isn't it?
> It it is not available, we need to be aware of this specific issue,
> otherwise we have the error "PMD can not provide needed ptypes"
> (without suggesting to use the option).

Actually, Konstantin is suggesting this way, I quote here:
    1. if '--parse-ptype' present always use SW parsing;
    2. else check does HW support ptype recognition:
       - if yes, then use HW offload
       - else use SW

By this way, most case, user does not need to specify this option, except the case that, user wants to compare the performance of HW and SW ptype version when the NIC actually supports HW ptypes.

I agree with this way. How do you think?

Thanks,
Jianfeng

Re: [dpdk-dev] [PATCH v3 1/3] examples/ip_reassembly: add parse-ptype option

[Liu, Yong]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, February 10, 2017 4:36 PM
> To: Liu, Yong <yong.liu@intel.com>
> Cc: Tan, Jianfeng <jianfeng.tan@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 1/3] examples/ip_reassembly: add parse-
> ptype option
> 
> 2017-02-10 07:53, Liu, Yong:
> > From: Thomas Monjalon
> > > 2017-02-09 22:25, Marvin Liu:
> > > > Add new option parse-ptype in this sample in case of pmd driver
> > > > not provide packet type info. If this option enabled, packet type
> > > > will be analyzed in Rx callback function.
> > > [...]
> > > > +		if (parse_ptype) {
> > > > +			if (add_cb_parse_ptype(portid, queueid) < 0)
> > > > +				rte_exit(EXIT_FAILURE,
> > > > +					"Fail to add ptype cb\n");
> > > > +		} else if (!check_ptype(portid))
> > > > +			rte_exit(EXIT_FAILURE,
> > > > +				"PMD can not provide needed ptypes\n");
> > >
> > > Instead of adding a new option, why not adding the callback
> automatically
> > > if the packet type is not supported by the hardware?
> >
> > Thomas,
> > We want to let user choice which kind of method for packet type parsing.
> > If start application with parse-type option, is meaning user want to use
> software parsing otherwise will use hardware parsing.
> 
> I do not understand why this user choice matters.
> If it is available, hardware ptype is better, isn't it?
> It it is not available, we need to be aware of this specific issue,
> otherwise we have the error "PMD can not provide needed ptypes"
> (without suggesting to use the option).

Yes, hardware always has better performance than software. I think it matters in some performance measurement scenarios. 
Like l3fwd, we compared performance with software and hardware packet parsers and this option may not have much value in other samples.
I will rework this patch and fallback to software if hardware not support.	

BRs,
Marvin

Re: [dpdk-dev] [PATCH v3 1/3] examples/ip_reassembly: add parse-ptype option

[Thomas Monjalon]

2017-02-10 09:02, Liu, Yong:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2017-02-10 07:53, Liu, Yong:
> > > From: Thomas Monjalon
> > > > 2017-02-09 22:25, Marvin Liu:
> > > > > Add new option parse-ptype in this sample in case of pmd driver
> > > > > not provide packet type info. If this option enabled, packet type
> > > > > will be analyzed in Rx callback function.
> > > > [...]
> > > > > +		if (parse_ptype) {
> > > > > +			if (add_cb_parse_ptype(portid, queueid) < 0)
> > > > > +				rte_exit(EXIT_FAILURE,
> > > > > +					"Fail to add ptype cb\n");
> > > > > +		} else if (!check_ptype(portid))
> > > > > +			rte_exit(EXIT_FAILURE,
> > > > > +				"PMD can not provide needed ptypes\n");
> > > >
> > > > Instead of adding a new option, why not adding the callback
> > automatically
> > > > if the packet type is not supported by the hardware?
> > >
> > > Thomas,
> > > We want to let user choice which kind of method for packet type parsing.
> > > If start application with parse-type option, is meaning user want to use
> > software parsing otherwise will use hardware parsing.
> > 
> > I do not understand why this user choice matters.
> > If it is available, hardware ptype is better, isn't it?
> > It it is not available, we need to be aware of this specific issue,
> > otherwise we have the error "PMD can not provide needed ptypes"
> > (without suggesting to use the option).
> 
> Yes, hardware always has better performance than software. I think it matters in some performance measurement scenarios. 
> Like l3fwd, we compared performance with software and hardware packet parsers and this option may not have much value in other samples.
> I will rework this patch and fallback to software if hardware not support.	

The ip_fragmentation case has been reworked this way:
	http://dpdk.org/patch/21769

Re: [dpdk-dev] [PATCH v2] examples: fix ip_reassembly not work with some NICs

[Luca Boccassi]

Hi Thomas and Yong,

This patch:

https://patches.dpdk.org/patch/19868/

Fixes an error in the Intel regression tests when backported to the
16.11.x LTS branch, but was never committed to master.
It's marked as superseded, and the error does not appear on master.

Do you remember what other change superseded this patch? I can't
find anything on the mailing list.

Thanks!

Kind regards,
Luca Boccassi

Re: [dpdk-dev] [PATCH v2] examples: fix ip_reassembly not work with some NICs

[Liu, Yong]

Boccassi,
Packet type is supported in both vector and normal Rx functions of Fortville pmd. So I think this patch was superseded because of it do not fix the real issue.

Thanks,
Marvin

> -----Original Message-----
> From: Luca Boccassi [mailto:luca.boccassi@gmail.com]
> Sent: Monday, August 27, 2018 9:20 PM
> To: dev@dpdk.org
> Cc: Liu, Yong <yong.liu@intel.com>; thomas@monjalon.net; Yigit, Ferruh
> <ferruh.yigit@intel.com>
> Subject: Re: [PATCH v2] examples: fix ip_reassembly not work with some NICs
> 
> Hi Thomas and Yong,
> 
> This patch:
> 
> https://patches.dpdk.org/patch/19868/
> 
> Fixes an error in the Intel regression tests when backported to the
> 16.11.x LTS branch, but was never committed to master.
> It's marked as superseded, and the error does not appear on master.
> 
> Do you remember what other change superseded this patch? I can't
> find anything on the mailing list.
> 
> Thanks!
> 
> Kind regards,
> Luca Boccassi