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 >