diff mbox

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

Message ID 1486650334-34300-1-git-send-email-yong.liu@intel.com (mailing list archive)
State Changes Requested, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel compilation success Compilation OK

Commit Message

Marvin Liu Feb. 9, 2017, 2:25 p.m. UTC 
  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>

Comments

Thomas Monjalon Feb. 9, 2017, 9:27 p.m. UTC | #1 
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?
Marvin Liu Feb. 10, 2017, 7:53 a.m. UTC | #2 
> -----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
Thomas Monjalon Feb. 10, 2017, 8:35 a.m. UTC | #3 
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).
Jianfeng Tan Feb. 10, 2017, 9 a.m. UTC | #4 
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
Marvin Liu Feb. 10, 2017, 9:02 a.m. UTC | #5 
> -----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
Thomas Monjalon April 4, 2017, 12:45 p.m. UTC | #6 
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
diff mbox

Patch

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++) {