Hi Konstantin,

On 3/1/2016 9:51 PM, Ananyev, Konstantin wrote:

Hi Jianfeng,

quoted

-----Original Message-----
From: Tan, Jianfeng
Sent: Tuesday, March 01, 2016 1:24 AM
To: dev@dpdk.org
Cc: Zhang, Helin; Ananyev, Konstantin; nelio.laranjeiro@6wind.com; adrien.mazarguil@6wind.com; rahul.lakkireddy@chelsio.com;
pmatilai@redhat.com; Tan, Jianfeng
Subject: [PATCH] examples/l3fwd: fix using packet type blindly

As a example to use ptype info, l3fwd needs firstly to use
rte_eth_dev_get_ptype_info() API to check if device and/or
its PMD driver will parse and fill the needed packet type;
if not, use the newly added option, --parse-ptype, to
analyze it in the callback softly.

As the mode of EXACT_MATCH uses the 5 tuples to caculate
hash, so we narrow down its scope to:
   a. ip packets with no extensions, and
   b. L4 payload should be either tcp or udp.

[...]

quoted

+
+	if (ptype_l3_ipv4_ext == 0)
+		printf("port %d cannot parse RTE_PTYPE_L3_IPV4_EXT\n", portid);
+	if (ptype_l3_ipv6_ext == 0)
+		printf("port %d cannot parse RTE_PTYPE_L3_IPV6_EXT\n", portid);
+	if (ptype_l3_ipv4_ext && ptype_l3_ipv6_ext)
+		return 1;

Why return here?
You'll miss L4 ptype checks below.

Oops, should be: if (!ptype_l3_ipv4_ext || !ptype_l3_ipv6_ext) return 0; 
and continue check L4 ptype.

quoted

+
+	if (ptype_l4_tcp == 0)
+		printf("port %d cannot parse RTE_PTYPE_L4_TCP\n", portid);
+	if (ptype_l4_udp == 0)
+		printf("port %d cannot parse RTE_PTYPE_L4_UDP\n", portid);
+	if (ptype_l4_tcp || ptype_l4_udp)
+		return 1;
+
+	return 0;
+}
+
+void
+em_parse_ptype(struct rte_mbuf *m)
+{
+	struct ether_hdr *eth_hdr;
+	uint32_t packet_type = 0;
+	uint16_t ethertype;
+	void *l4;
+	int hdr_len;
+	struct ipv4_hdr *ipv4_hdr;
+	struct ipv6_hdr *ipv6_hdr;
+
+	eth_hdr = rte_pktmbuf_mtod(m, struct ether_hdr *);
+	ethertype = rte_be_to_cpu_16(eth_hdr->ether_type);
+	l4 = (uint8_t *)eth_hdr + sizeof(struct ether_hdr);

Just curious why l4? It looks like l3 :)

Yes, it's typo, should be l3. Thanks for pointing it out.

quoted

+	switch (ethertype) {
+	case ETHER_TYPE_IPv4:
+		ipv4_hdr = (struct ipv4_hdr *)l4;
+		hdr_len = (ipv4_hdr->version_ihl & IPV4_HDR_IHL_MASK) *
+			  IPV4_IHL_MULTIPLIER;
+		if (hdr_len == sizeof(struct ipv4_hdr) &&
+		    (ipv4_hdr->next_proto_id == IPPROTO_TCP ||
+		     ipv4_hdr->next_proto_id == IPPROTO_UDP))
+			packet_type |= RTE_PTYPE_L3_IPV4;

I think it needs to be something like:
If (hdr_len == sizeof(struct ipv4_hdr)) {
         packet_type = RTE_PTYPE_L3_IPV4;
        if (ipv4_hdr->next_proto_id == IPPROTO_TCP)
	packet_type |= RTE_PTYPE_L4_TCP;
        else if ipv4_hdr->next_proto_id == IPPROTO_UDP)
                 packet_type |= RTE_PTYPE_L4_TCP;
}

And then inside em forward check ptype to be sure that is IPV4 with no options and UDP/TCP packet.
Same for IPv6.

Got it, I'll change it and add this check in em forward function.

quoted

+		break;
+	case ETHER_TYPE_IPv6:
+		ipv6_hdr = (struct ipv6_hdr *)l4;
+		if (ipv6_hdr->proto == IPPROTO_TCP ||
+		    ipv6_hdr->proto == IPPROTO_UDP)
+			packet_type |= RTE_PTYPE_L3_IPV6;
+		break;
+	}
+
+	m->packet_type |= packet_type;
+}
+
  /* main processing loop */
  int
  em_main_loop(__attribute__((unused)) void *dummy)

diff --git a/examples/l3fwd/l3fwd_lpm.c b/examples/l3fwd/l3fwd_lpm.c
index e0ed3c4..981227a 100644
--- a/examples/l3fwd/l3fwd_lpm.c
+++ b/examples/l3fwd/l3fwd_lpm.c

@@ -280,6 +280,63 @@ setup_lpm(const int socketid)
  	}
  }

+int
+lpm_check_ptype(int portid)
+{
+	int i, ret;
+	int ptype_l3_ipv4 = 0, ptype_l3_ipv6 = 0;
+
+	ret = rte_eth_dev_get_ptype_info(portid, RTE_PTYPE_L3_MASK, NULL, 0);
+	if (ret <= 0)
+		return 0;
+
+	uint32_t ptypes[ret];
+
+	ret = rte_eth_dev_get_ptype_info(portid, RTE_PTYPE_L3_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 == 0)
+		printf("port %d cannot parse RTE_PTYPE_L3_IPV4\n", portid);
+
+	if (ptype_l3_ipv6 == 0)
+		printf("port %d cannot parse RTE_PTYPE_L3_IPV6\n", portid);
+
+	if (ptype_l3_ipv4 && ptype_l3_ipv6)
+		return 1;
+
+	return 0;
+
+}
+
+void
+lpm_parse_ptype(struct rte_mbuf *m)
+{
+	struct ether_hdr *eth_hdr;
+	uint32_t packet_type = 0;
+	uint16_t ethertype;
+
+	eth_hdr = rte_pktmbuf_mtod(m, struct ether_hdr *);
+	ethertype = rte_be_to_cpu_16(eth_hdr->ether_type);
+	switch (ethertype) {
+	case ETHER_TYPE_IPv4:
+		packet_type |= RTE_PTYPE_L3_IPV4_EXT_UNKNOWN;
+		break;
+	case ETHER_TYPE_IPv6:
+		packet_type |= RTE_PTYPE_L3_IPV6_EXT_UNKNOWN;
+		break;
+	default:
+		break;
+	}
+
+	m->packet_type |= packet_type;

Might be safer:
m->packet_type = packet_type;

Your previous comment on this says that the assignment will write off 
some ptype info PMDs have filled. But for l3fwd, I think it does not 
care other ptype info.

[...]

+static uint16_t
+cb_parse_packet_type(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 i;
+
+	for (i = 0; i < nb_pkts; ++i)
+		l3fwd_lkp.parse_ptype(pkts[i]);

No need to create callback chains.
That way you have extra call per packet.
Just 2 callbaclks: cb_lpm_parse_ptype & cb_em_parse_ptype,
and then register one depending on which mode are we in.
Would be simpler and faster, I believe.

Yes, I was considering it too. In addition, shall I use vector 
instruction here to accelerate it?

Thanks,
Jianfeng

Konstantin