From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 031F5960F for ; Tue, 1 Mar 2016 15:17:50 +0100 (CET) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga104.fm.intel.com with ESMTP; 01 Mar 2016 06:17:31 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,523,1449561600"; d="scan'208";a="57610824" Received: from tanjianf-mobl.ccr.corp.intel.com (HELO [10.239.202.13]) ([10.239.202.13]) by fmsmga004.fm.intel.com with ESMTP; 01 Mar 2016 06:17:27 -0800 To: "Ananyev, Konstantin" , "dev@dpdk.org" References: <1451544799-70776-1-git-send-email-jianfeng.tan@intel.com> <1456795416-118189-1-git-send-email-jianfeng.tan@intel.com> <2601191342CEEE43887BDE71AB97725836B0C99C@irsmsx105.ger.corp.intel.com> From: "Tan, Jianfeng" Message-ID: <56D5A476.6070500@intel.com> Date: Tue, 1 Mar 2016 22:17:26 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <2601191342CEEE43887BDE71AB97725836B0C99C@irsmsx105.ger.corp.intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] examples/l3fwd: fix using packet type blindly X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 01 Mar 2016 14:17:51 -0000 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 >