diff mbox

[dpdk-dev,v2] net/e1000: correct VLAN tag byte order for i35x LB packets

Message ID 20171012172435.34700-1-rmelton@cisco.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Roger Melton Oct. 12, 2017, 5:24 p.m. UTC 
  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 <rmelton@cisco.com>
---
v2:
* PF: Limit swap to i350 and i354
* VF: Limit swap to i350VF

 drivers/net/e1000/igb_rxtx.c | 55 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 50 insertions(+), 5 deletions(-)

Comments

Wenzhuo Lu Oct. 16, 2017, 12:43 a.m. UTC | #1 
Hi,

> -----Original Message-----
> From: Roger B Melton [mailto:rmelton@cisco.com]
> Sent: Friday, October 13, 2017 1:25 AM
> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Cc: dev@dpdk.org; Roger B Melton <rmelton@cisco.com>
> Subject: [PATCH v2] net/e1000: correct VLAN tag byte order for i35x LB
> packets
> 
> 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 <rmelton@cisco.com>
Acked-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
Ferruh Yigit Oct. 20, 2017, 7:04 p.m. UTC | #2 
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 <rmelton@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?

> +			rxm->vlan_tci = rte_be_to_cpu_16(rxd.wb.upper.vlan);
> +		} else {
> +			rxm->vlan_tci = rte_le_to_cpu_16(rxd.wb.upper.vlan);
> +		}
>  		pkt_flags = rx_desc_hlen_type_rss_to_pkt_flags(rxq, hlen_type_rss);
>  		pkt_flags = pkt_flags | rx_desc_status_to_pkt_flags(staterr);
>  		pkt_flags = pkt_flags | rx_desc_error_to_pkt_flags(staterr);
<...>
Roger Melton Oct. 23, 2017, 5:42 p.m. UTC | #3 
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 <rmelton@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?

Thanks,
Roger


>
>> +			rxm->vlan_tci = rte_be_to_cpu_16(rxd.wb.upper.vlan);
>> +		} else {
>> +			rxm->vlan_tci = rte_le_to_cpu_16(rxd.wb.upper.vlan);
>> +		}
>>   		pkt_flags = rx_desc_hlen_type_rss_to_pkt_flags(rxq, hlen_type_rss);
>>   		pkt_flags = pkt_flags | rx_desc_status_to_pkt_flags(staterr);
>>   		pkt_flags = pkt_flags | rx_desc_error_to_pkt_flags(staterr);
> <...>
> .
>
Ferruh Yigit Oct. 25, 2017, 6:11 p.m. UTC | #4 
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 <rmelton@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?

Thanks,
ferruh

> 
> Thanks,
> Roger
> 
> 
>>
>>> +			rxm->vlan_tci = rte_be_to_cpu_16(rxd.wb.upper.vlan);
>>> +		} else {
>>> +			rxm->vlan_tci = rte_le_to_cpu_16(rxd.wb.upper.vlan);
>>> +		}
>>>   		pkt_flags = rx_desc_hlen_type_rss_to_pkt_flags(rxq, hlen_type_rss);
>>>   		pkt_flags = pkt_flags | rx_desc_status_to_pkt_flags(staterr);
>>>   		pkt_flags = pkt_flags | rx_desc_error_to_pkt_flags(staterr);
>> <...>
>> .
>>
> 
>
Bruce Richardson Oct. 25, 2017, 8:16 p.m. UTC | #5 
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 <rmelton@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? 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
Ferruh Yigit Oct. 25, 2017, 8:22 p.m. UTC | #6 
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 <rmelton@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?

> 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
>
Roger Melton Oct. 25, 2017, 8:45 p.m. UTC | #7 
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 <rmelton@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?

I can tell you that from the perspective of my application the 
performance impact for 1G is not a concern.

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.

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
>>
> .
>
Bruce Richardson Oct. 25, 2017, 8:48 p.m. UTC | #8 
> -----Original Message-----

> From: Roger B. Melton [mailto:rmelton@cisco.com]

> Sent: Wednesday, October 25, 2017 9:45 PM

> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Richardson, Bruce

> <bruce.richardson@intel.com>

> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org; Ananyev, Konstantin

> <konstantin.ananyev@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 <rmelton@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.

> 

> 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

> >>

> > .

> >
Ferruh Yigit Oct. 25, 2017, 9:11 p.m. UTC | #9 
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 <ferruh.yigit@intel.com>; Richardson, Bruce
>> <bruce.richardson@intel.com>
>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; dev@dpdk.org; Ananyev, Konstantin
>> <konstantin.ananyev@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 <rmelton@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
>>>>
>>> .
>>>
>
Ferruh Yigit Oct. 25, 2017, 9:37 p.m. UTC | #10 
On 10/15/2017 5:43 PM, Lu, Wenzhuo wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Roger B Melton [mailto:rmelton@cisco.com]
>> Sent: Friday, October 13, 2017 1:25 AM
>> To: Lu, Wenzhuo <wenzhuo.lu@intel.com>
>> Cc: dev@dpdk.org; Roger B Melton <rmelton@cisco.com>
>> Subject: [PATCH v2] net/e1000: correct VLAN tag byte order for i35x LB
>> packets
>>
>> 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 <rmelton@cisco.com>
> Acked-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

Applied to dpdk-next-net/master, thanks.
diff mbox

Patch

diff --git a/drivers/net/e1000/igb_rxtx.c b/drivers/net/e1000/igb_rxtx.c
index 1c80a2a..3eb547b 100644
--- a/drivers/net/e1000/igb_rxtx.c
+++ b/drivers/net/e1000/igb_rxtx.c
@@ -105,6 +105,13 @@  struct igb_tx_entry {
 };
 
 /**
+ * rx queue flags
+ */
+enum igb_rxq_flags {
+	IGB_RXQ_FLAG_LB_BSWAP_VLAN = 0x01,
+};
+
+/**
  * Structure associated with each RX queue.
  */
 struct igb_rx_queue {
@@ -128,6 +135,7 @@  struct igb_rx_queue {
 	uint8_t             wthresh;    /**< Write-back threshold register. */
 	uint8_t             crc_len;    /**< 0 if CRC stripped, 4 otherwise. */
 	uint8_t             drop_en;  /**< If not 0, set SRRCTL.Drop_En. */
+	uint32_t            flags;      /**< RX flags. */
 };
 
 /**
@@ -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)) {
+			rxm->vlan_tci = rte_be_to_cpu_16(rxd.wb.upper.vlan);
+		} else {
+			rxm->vlan_tci = rte_le_to_cpu_16(rxd.wb.upper.vlan);
+		}
 		pkt_flags = rx_desc_hlen_type_rss_to_pkt_flags(rxq, hlen_type_rss);
 		pkt_flags = pkt_flags | rx_desc_status_to_pkt_flags(staterr);
 		pkt_flags = pkt_flags | rx_desc_error_to_pkt_flags(staterr);
@@ -1181,9 +1196,16 @@  eth_igb_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 
 		/*
 		 * The vlan_tci field is only valid when PKT_RX_VLAN_PKT is
-		 * set in the pkt_flags field.
+		 * set in the pkt_flags field and must be in CPU byte order.
 		 */
-		first_seg->vlan_tci = rte_le_to_cpu_16(rxd.wb.upper.vlan);
+		if ((staterr & rte_cpu_to_le_32(E1000_RXDEXT_STATERR_LB)) &&
+			(rxq->flags & IGB_RXQ_FLAG_LB_BSWAP_VLAN)) {
+			first_seg->vlan_tci =
+				rte_be_to_cpu_16(rxd.wb.upper.vlan);
+		} else {
+			first_seg->vlan_tci =
+				rte_le_to_cpu_16(rxd.wb.upper.vlan);
+		}
 		hlen_type_rss = rte_le_to_cpu_32(rxd.wb.lower.lo_dword.data);
 		pkt_flags = rx_desc_hlen_type_rss_to_pkt_flags(rxq, hlen_type_rss);
 		pkt_flags = pkt_flags | rx_desc_status_to_pkt_flags(staterr);
@@ -2278,6 +2300,18 @@  eth_igb_rx_init(struct rte_eth_dev *dev)
 
 		rxq = dev->data->rx_queues[i];
 
+		rxq->flags = 0;
+		/*
+		 * i350 and i354 vlan packets have vlan tags byte swapped.
+		 */
+		if ((hw->mac.type == e1000_i350) ||
+			(hw->mac.type == e1000_i354)) {
+			rxq->flags |= IGB_RXQ_FLAG_LB_BSWAP_VLAN;
+			PMD_INIT_LOG(DEBUG, "IGB rx vlan bswap required");
+		} else {
+			PMD_INIT_LOG(DEBUG, "IGB rx vlan bswap not required");
+		}
+
 		/* Allocate buffers for descriptor rings and set up queue */
 		ret = igb_alloc_rx_queue_mbufs(rxq);
 		if (ret)
@@ -2557,6 +2591,17 @@  eth_igbvf_rx_init(struct rte_eth_dev *dev)
 
 		rxq = dev->data->rx_queues[i];
 
+		rxq->flags = 0;
+		/*
+		 * i350VF LB vlan packets have vlan tags byte swapped.
+		 */
+		if (hw->mac.type == e1000_vfadapt_i350) {
+			rxq->flags |= IGB_RXQ_FLAG_LB_BSWAP_VLAN;
+			PMD_INIT_LOG(DEBUG, "IGB rx vlan bswap required");
+		} else {
+			PMD_INIT_LOG(DEBUG, "IGB rx vlan bswap not required");
+		}
+
 		/* Allocate buffers for descriptor rings and set up queue */
 		ret = igb_alloc_rx_queue_mbufs(rxq);
 		if (ret)