From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 8F1441B7CC for ; Wed, 25 Oct 2017 23:11:50 +0200 (CEST) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Oct 2017 14:11:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,433,1503385200"; d="scan'208";a="167610612" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.241.225.45]) ([10.241.225.45]) by fmsmga006.fm.intel.com with ESMTP; 25 Oct 2017 14:11:48 -0700 To: "Richardson, Bruce" , "Roger B. Melton" Cc: "Lu, Wenzhuo" , "dev@dpdk.org" , "Ananyev, Konstantin" , Qian Xu , "Zhang, Helin" References: <20171012172435.34700-1-rmelton@cisco.com> <1e8e5766-6c52-d09d-d7ac-11492b946c90@intel.com> <3b4d34fa-309e-10d4-e992-3d82034e4c41@cisco.com> <20171025201614.GA12968@bricha3-MOBL3.ger.corp.intel.com> <6256a52f-43f1-3e3f-61af-932b4bd26955@cisco.com> <59AF69C657FD0841A61C55336867B5B0721ED1C2@IRSMSX103.ger.corp.intel.com> From: Ferruh Yigit Message-ID: Date: Wed, 25 Oct 2017 14:11:48 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <59AF69C657FD0841A61C55336867B5B0721ED1C2@IRSMSX103.ger.corp.intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2] net/e1000: correct VLAN tag byte order for i35x LB packets X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Oct 2017 21:11:51 -0000 On 10/25/2017 1:48 PM, Richardson, Bruce wrote: > > >> -----Original Message----- >> From: Roger B. Melton [mailto:rmelton@cisco.com] >> Sent: Wednesday, October 25, 2017 9:45 PM >> To: Yigit, Ferruh ; Richardson, Bruce >> >> Cc: Lu, Wenzhuo ; dev@dpdk.org; Ananyev, Konstantin >> >> Subject: Re: [dpdk-dev] [PATCH v2] net/e1000: correct VLAN tag byte order >> for i35x LB packets >> >> On 10/25/17 4:22 PM, Ferruh Yigit wrote: >>> On 10/25/2017 1:16 PM, Bruce Richardson wrote: >>>> On Wed, Oct 25, 2017 at 11:11:08AM -0700, Ferruh Yigit wrote: >>>>> On 10/23/2017 10:42 AM, Roger B. Melton wrote: >>>>>> On 10/20/17 3:04 PM, Ferruh Yigit wrote: >>>>>>> On 10/12/2017 10:24 AM, Roger B Melton wrote: >>>>>>>> When copying VLAN tags from the RX descriptor to the vlan_tci >>>>>>>> field in the mbuf header, igb_rxtx.c:eth_igb_recv_pkts() and >>>>>>>> eth_igb_recv_scattered_pkts() both assume that the VLAN tag is >>>>>>>> always little endian. While i350, i354 and /i350vf VLAN >>>>>>>> non-loopback packets are stored little endian, VLAN tags in >>>>>>>> loopback packets for those devices are big endian. >>>>>>>> >>>>>>>> For i350, i354 and i350vf VLAN loopback packets, swap the tag >>>>>>>> when copying from the RX descriptor to the mbuf header. This >>>>>>>> will ensure that the mbuf vlan_tci is always little endian. >>>>>>>> >>>>>>>> Signed-off-by: Roger B Melton >>>>>>> <...> >>>>>>> >>>>>>>> @@ -946,9 +954,16 @@ eth_igb_recv_pkts(void *rx_queue, struct >>>>>>>> rte_mbuf **rx_pkts, >>>>>>>> >>>>>>>> rxm->hash.rss = rxd.wb.lower.hi_dword.rss; >>>>>>>> hlen_type_rss = >> rte_le_to_cpu_32(rxd.wb.lower.lo_dword.data); >>>>>>>> - /* Only valid if PKT_RX_VLAN_PKT set in pkt_flags */ >>>>>>>> - rxm->vlan_tci = rte_le_to_cpu_16(rxd.wb.upper.vlan); >>>>>>>> - >>>>>>>> + /* >>>>>>>> + * The vlan_tci field is only valid when PKT_RX_VLAN_PKT >> is >>>>>>>> + * set in the pkt_flags field and must be in CPU byte >> order. >>>>>>>> + */ >>>>>>>> + if ((staterr & >> rte_cpu_to_le_32(E1000_RXDEXT_STATERR_LB)) && >>>>>>>> + (rxq->flags & IGB_RXQ_FLAG_LB_BSWAP_VLAN)) { >>>>>>> This is adding more condition checks into Rx path. >>>>>>> What is the performance cost of this addition? >>>>>> I have not measured the performance cost, but I can collect data. >>>>>> What specifically are you looking for? >>>>>> >>>>>> To be clear the current implementation incorrect as it does not >>>>>> normalize the vlan tag to CPU byte order before copying it into >>>>>> mbuf and applications have no visibility to determine if the tag in >>>>>> the mbuf is big or little endian. >>>>>> >>>>>> Do you have any suggestions for an alternative approach to avoid rx >>>>>> patch checks? >>>>> No suggestion indeed. And correctness matters. >>>>> >>>>> But this add a cost and I wonder how much it is, based on that >>>>> result it may be possible to do more investigation for alternate >> solutions or trade-offs. >>>>> >>>>> Konstantin, Bruce, Wenzhuo, >>>>> >>>>> What do you think, do you have any comment? >>>>> >>>> For a 1G driver, is performance really that big an issue? >>> I don't know. So is this an Ack from you for the patch? > > No, I don't know much about this driver to comment. But it's an indication that > I don't have any objections to it either. :-) > >> >> I can tell you that from the perspective of my application the performance >> impact for 1G is not a concern. > > That's kinda what I would expect. > >> >> FWIW, I did go through a few iterations with Wenzhou to minimize the >> performance impact before we settled on this implementation, and Wenzhou >> did Ack it btw. Taking into account that Wenzhuo acked and there is no outstanding objection, I will get this. But I believe it would be good to run some regression tests on PMD after rc2. >> >> I'm hoping we can get this into 17.11. >> >> Thanks, >> -Roger >> >>> >>>> Unless you >>>> have a *lot* of 1G ports, I would expect most platforms not to notice >>>> an extra couple of cycles when dealing with 1G line rates. >>>> >>>> /Bruce >>>> >>> . >>> >