On 10/25/2017 1:48 PM, Richardson, Bruce wrote: > > >> -----Original Message----- >> From: Roger B. Melton [mailto:rmel...@cisco.com] >> Sent: Wednesday, October 25, 2017 9:45 PM >> To: Yigit, Ferruh <ferruh.yi...@intel.com>; Richardson, Bruce >> <bruce.richard...@intel.com> >> Cc: Lu, Wenzhuo <wenzhuo...@intel.com>; dev@dpdk.org; Ananyev, Konstantin >> <konstantin.anan...@intel.com> >> 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 <rmel...@cisco.com> >>>>>>> <...> >>>>>>> >>>>>>>> @@ -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 >>>> >>> . >>> >
