Re: [PATCH] examples/l3fwd: fix using packet type blindly

["Tan, Jianfeng" <jianfeng.tan@intel.com>]

Hi Konstantin,

On 3/1/2016 9:51 PM, Ananyev, Konstantin wrote:
> Hi Jianfeng,
>
>> -----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.
[...]
>> +
>> +	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.

>> +
>> +	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.

>> +	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.

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