[dpdk-dev] [PATCH] examples/l3fwd: fix using packet type blindly
Tan, Jianfeng
jianfeng.tan at intel.com
Tue Mar 1 15:17:26 CET 2016
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 at dpdk.org
>> Cc: Zhang, Helin; Ananyev, Konstantin; nelio.laranjeiro at 6wind.com; adrien.mazarguil at 6wind.com; rahul.lakkireddy at chelsio.com;
>> pmatilai at 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
>
More information about the dev
mailing list