[dpdk-dev] [PATCH v1 0/3]: bug fixes in the ixgbe PF PMD Rx flow

[dpdk-dev] [PATCH v1 0/3]: bug fixes in the ixgbe PF PMD Rx flow

[Vlad Zolotarov]

This series contains some bug fixes that were found during my work on the ixgbe LRO
patches. Sending this series separately on Thomas request so that it may be integrated
into the 2.0 release.

Vlad Zolotarov (3):
  ixgbe: Use the rte_le_to_cpu_xx()/rte_cpu_to_le_xx() when
    reading/setting HW ring descriptor fields
  ixgbe: Bug fix: Properly configure Rx CRC stripping for x540 devices
  ixgbe: Unify the rx_pkt_bulk callback initialization

 lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h |   2 +
 lib/librte_pmd_ixgbe/ixgbe_ethdev.c     |  13 ++-
 lib/librte_pmd_ixgbe/ixgbe_rxtx.c       | 199 +++++++++++++++++++++-----------
 lib/librte_pmd_ixgbe/ixgbe_rxtx.h       |  22 +++-
 4 files changed, 161 insertions(+), 75 deletions(-)

-- 
2.1.0

[dpdk-dev] [PATCH v1 1/3] ixgbe: Use the rte_le_to_cpu_xx()/rte_cpu_to_le_xx() when reading/setting HW ring descriptor fields

[Vlad Zolotarov]

Fixed the above in ixgbe_rx_alloc_bufs() and in ixgbe_recv_scattered_pkts().

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
 lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
index 9ecf3e5..b033e04 100644
--- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
+++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
@@ -1028,7 +1028,7 @@ ixgbe_rx_alloc_bufs(struct igb_rx_queue *rxq)
 	struct igb_rx_entry *rxep;
 	struct rte_mbuf *mb;
 	uint16_t alloc_idx;
-	uint64_t dma_addr;
+	__le64 dma_addr;
 	int diag, i;
 
 	/* allocate buffers in bulk directly into the S/W ring */
@@ -1051,7 +1051,7 @@ ixgbe_rx_alloc_bufs(struct igb_rx_queue *rxq)
 		mb->port = rxq->port_id;
 
 		/* populate the descriptors */
-		dma_addr = (uint64_t)mb->buf_physaddr + RTE_PKTMBUF_HEADROOM;
+		dma_addr = rte_cpu_to_le_64(RTE_MBUF_DATA_DMA_ADDR_DEFAULT(mb));
 		rxdp[i].read.hdr_addr = dma_addr;
 		rxdp[i].read.pkt_addr = dma_addr;
 	}
@@ -1559,13 +1559,14 @@ ixgbe_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
 		first_seg->ol_flags = pkt_flags;
 
 		if (likely(pkt_flags & PKT_RX_RSS_HASH))
-			first_seg->hash.rss = rxd.wb.lower.hi_dword.rss;
+			first_seg->hash.rss =
+				    rte_le_to_cpu_32(rxd.wb.lower.hi_dword.rss);
 		else if (pkt_flags & PKT_RX_FDIR) {
 			first_seg->hash.fdir.hash =
-				(uint16_t)((rxd.wb.lower.hi_dword.csum_ip.csum)
-					   & IXGBE_ATR_HASH_MASK);
+			    rte_le_to_cpu_16(rxd.wb.lower.hi_dword.csum_ip.csum)
+					   & IXGBE_ATR_HASH_MASK;
 			first_seg->hash.fdir.id =
-				rxd.wb.lower.hi_dword.csum_ip.ip_id;
+			  rte_le_to_cpu_16(rxd.wb.lower.hi_dword.csum_ip.ip_id);
 		}
 
 		/* Prefetch data of first segment, if configured to do so. */
-- 
2.1.0

Re: [dpdk-dev] [PATCH v1 1/3] ixgbe: Use the rte_le_to_cpu_xx()/rte_cpu_to_le_xx() when reading/setting HW ring descriptor fields

[Ananyev, Konstantin]

Hi Vlad,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Vlad Zolotarov
> Sent: Monday, March 09, 2015 10:13 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v1 1/3] ixgbe: Use the rte_le_to_cpu_xx()/rte_cpu_to_le_xx() when reading/setting HW ring descriptor
> fields
> 
> Fixed the above in ixgbe_rx_alloc_bufs() and in ixgbe_recv_scattered_pkts().
> 
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
>  lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> index 9ecf3e5..b033e04 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> @@ -1028,7 +1028,7 @@ ixgbe_rx_alloc_bufs(struct igb_rx_queue *rxq)
>  	struct igb_rx_entry *rxep;
>  	struct rte_mbuf *mb;
>  	uint16_t alloc_idx;
> -	uint64_t dma_addr;
> +	__le64 dma_addr;

Wonder Why you changed from uint64_t to __le64 here?
Effectively __le64 is the same a uint64_t, and I think it is better to use always the same type across all PMD code for consistency.
Konstantin 


>  	int diag, i;
> 
>  	/* allocate buffers in bulk directly into the S/W ring */
> @@ -1051,7 +1051,7 @@ ixgbe_rx_alloc_bufs(struct igb_rx_queue *rxq)
>  		mb->port = rxq->port_id;
> 
>  		/* populate the descriptors */
> -		dma_addr = (uint64_t)mb->buf_physaddr + RTE_PKTMBUF_HEADROOM;
> +		dma_addr = rte_cpu_to_le_64(RTE_MBUF_DATA_DMA_ADDR_DEFAULT(mb));
>  		rxdp[i].read.hdr_addr = dma_addr;
>  		rxdp[i].read.pkt_addr = dma_addr;
>  	}
> @@ -1559,13 +1559,14 @@ ixgbe_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
>  		first_seg->ol_flags = pkt_flags;
> 
>  		if (likely(pkt_flags & PKT_RX_RSS_HASH))
> -			first_seg->hash.rss = rxd.wb.lower.hi_dword.rss;
> +			first_seg->hash.rss =
> +				    rte_le_to_cpu_32(rxd.wb.lower.hi_dword.rss);
>  		else if (pkt_flags & PKT_RX_FDIR) {
>  			first_seg->hash.fdir.hash =
> -				(uint16_t)((rxd.wb.lower.hi_dword.csum_ip.csum)
> -					   & IXGBE_ATR_HASH_MASK);
> +			    rte_le_to_cpu_16(rxd.wb.lower.hi_dword.csum_ip.csum)
> +					   & IXGBE_ATR_HASH_MASK;
>  			first_seg->hash.fdir.id =
> -				rxd.wb.lower.hi_dword.csum_ip.ip_id;
> +			  rte_le_to_cpu_16(rxd.wb.lower.hi_dword.csum_ip.ip_id);
>  		}
> 
>  		/* Prefetch data of first segment, if configured to do so. */
> --
> 2.1.0

Re: [dpdk-dev] [PATCH v1 1/3] ixgbe: Use the rte_le_to_cpu_xx()/rte_cpu_to_le_xx() when reading/setting HW ring descriptor fields

[Vlad Zolotarov]

On 03/09/15 12:29, Ananyev, Konstantin wrote:
> Hi Vlad,
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Vlad Zolotarov
>> Sent: Monday, March 09, 2015 10:13 AM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v1 1/3] ixgbe: Use the rte_le_to_cpu_xx()/rte_cpu_to_le_xx() when reading/setting HW ring descriptor
>> fields
>>
>> Fixed the above in ixgbe_rx_alloc_bufs() and in ixgbe_recv_scattered_pkts().
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>>   lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
>> index 9ecf3e5..b033e04 100644
>> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
>> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
>> @@ -1028,7 +1028,7 @@ ixgbe_rx_alloc_bufs(struct igb_rx_queue *rxq)
>>   	struct igb_rx_entry *rxep;
>>   	struct rte_mbuf *mb;
>>   	uint16_t alloc_idx;
>> -	uint64_t dma_addr;
>> +	__le64 dma_addr;
> Wonder Why you changed from uint64_t to __le64 here?
> Effectively __le64 is the same a uint64_t,

I'm afraid the above it's not completely correct. See below.

> and I think it is better to use always the same type across all PMD code for consistency.

Pls., note that "dma_addr" is only used (see below)...

> Konstantin
>
>
>>   	int diag, i;
>>
>>   	/* allocate buffers in bulk directly into the S/W ring */
>> @@ -1051,7 +1051,7 @@ ixgbe_rx_alloc_bufs(struct igb_rx_queue *rxq)
>>   		mb->port = rxq->port_id;
>>
>>   		/* populate the descriptors */
>> -		dma_addr = (uint64_t)mb->buf_physaddr + RTE_PKTMBUF_HEADROOM;
>> +		dma_addr = rte_cpu_to_le_64(RTE_MBUF_DATA_DMA_ADDR_DEFAULT(mb));
>>   		rxdp[i].read.hdr_addr = dma_addr;
>>   		rxdp[i].read.pkt_addr = dma_addr;

here. ;) And the type of both hdr_addr and pkt_addr is __le64.
I don't exactly understand what do u mean by "use the same type across 
all PMD code for consistency" - there are a lot of types used in the PMD 
code and __le64 is one of them... ;)

Now more seriously, let's recall what is the semantics of the __leXX 
types - they represent the integer in the "little endian" format. Here, 
NIC expects the physical address in a little endian format, thus the 
descriptor is defined the way it is defined - using __le64. The same 
relates to dma_addr local variable in this patch - it contains the 
physical (more correctly "DMA-able") address of the Rx buffer in the 
form NIC expects it to be written in the descriptor.

So, why to use __leXX anyway? - Debugging the (invalid) endianess is a 
real headache. Therefore there are a few static code analysis tools like 
"sparse" that allow to detect such inconsistencies (see here 
http://en.wikipedia.org/wiki/Sparse) and __leXX is a helper to allow 
tools like sparse to detect such problems.

In addition after spending some time writing patches for Linux netdev 
list u develop a strong habit for such stuff - Dave and others are very 
strict about such things... ;)

So, is it the same as uint64_t? I guess now it's clear why it is now... ;)

>>   	}
>> @@ -1559,13 +1559,14 @@ ixgbe_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
>>   		first_seg->ol_flags = pkt_flags;
>>
>>   		if (likely(pkt_flags & PKT_RX_RSS_HASH))
>> -			first_seg->hash.rss = rxd.wb.lower.hi_dword.rss;
>> +			first_seg->hash.rss =
>> +				    rte_le_to_cpu_32(rxd.wb.lower.hi_dword.rss);
>>   		else if (pkt_flags & PKT_RX_FDIR) {
>>   			first_seg->hash.fdir.hash =
>> -				(uint16_t)((rxd.wb.lower.hi_dword.csum_ip.csum)
>> -					   & IXGBE_ATR_HASH_MASK);
>> +			    rte_le_to_cpu_16(rxd.wb.lower.hi_dword.csum_ip.csum)
>> +					   & IXGBE_ATR_HASH_MASK;
>>   			first_seg->hash.fdir.id =
>> -				rxd.wb.lower.hi_dword.csum_ip.ip_id;
>> +			  rte_le_to_cpu_16(rxd.wb.lower.hi_dword.csum_ip.ip_id);
>>   		}
>>
>>   		/* Prefetch data of first segment, if configured to do so. */
>> --
>> 2.1.0

Re: [dpdk-dev] [PATCH v1 1/3] ixgbe: Use the rte_le_to_cpu_xx()/rte_cpu_to_le_xx() when reading/setting HW ring descriptor fields

[Ananyev, Konstantin]

> -----Original Message-----
> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
> Sent: Monday, March 09, 2015 12:43 PM
> To: Ananyev, Konstantin; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v1 1/3] ixgbe: Use the rte_le_to_cpu_xx()/rte_cpu_to_le_xx() when reading/setting HW ring
> descriptor fields
> 
> 
> 
> On 03/09/15 12:29, Ananyev, Konstantin wrote:
> > Hi Vlad,
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Vlad Zolotarov
> >> Sent: Monday, March 09, 2015 10:13 AM
> >> To: dev@dpdk.org
> >> Subject: [dpdk-dev] [PATCH v1 1/3] ixgbe: Use the rte_le_to_cpu_xx()/rte_cpu_to_le_xx() when reading/setting HW ring
> descriptor
> >> fields
> >>
> >> Fixed the above in ixgbe_rx_alloc_bufs() and in ixgbe_recv_scattered_pkts().
> >>
> >> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> >> ---
> >>   lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 13 +++++++------
> >>   1 file changed, 7 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> >> index 9ecf3e5..b033e04 100644
> >> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> >> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> >> @@ -1028,7 +1028,7 @@ ixgbe_rx_alloc_bufs(struct igb_rx_queue *rxq)
> >>   	struct igb_rx_entry *rxep;
> >>   	struct rte_mbuf *mb;
> >>   	uint16_t alloc_idx;
> >> -	uint64_t dma_addr;
> >> +	__le64 dma_addr;
> > Wonder Why you changed from uint64_t to __le64 here?
> > Effectively __le64 is the same a uint64_t,
> 
> I'm afraid the above it's not completely correct. See below.
> 
> > and I think it is better to use always the same type across all PMD code for consistency.
> 
> Pls., note that "dma_addr" is only used (see below)...
> 
> > Konstantin
> >
> >
> >>   	int diag, i;
> >>
> >>   	/* allocate buffers in bulk directly into the S/W ring */
> >> @@ -1051,7 +1051,7 @@ ixgbe_rx_alloc_bufs(struct igb_rx_queue *rxq)
> >>   		mb->port = rxq->port_id;
> >>
> >>   		/* populate the descriptors */
> >> -		dma_addr = (uint64_t)mb->buf_physaddr + RTE_PKTMBUF_HEADROOM;
> >> +		dma_addr = rte_cpu_to_le_64(RTE_MBUF_DATA_DMA_ADDR_DEFAULT(mb));
> >>   		rxdp[i].read.hdr_addr = dma_addr;
> >>   		rxdp[i].read.pkt_addr = dma_addr;
> 
> here. ;) And the type of both hdr_addr and pkt_addr is __le64.
> I don't exactly understand what do u mean by "use the same type across
> all PMD code for consistency" - there are a lot of types used in the PMD
> code and __le64 is one of them... ;)
> 

> Now more seriously, let's recall what is the semantics of the __leXX
> types - they represent the integer in the "little endian" format. Here,
> NIC expects the physical address in a little endian format, thus the
> descriptor is defined the way it is defined - using __le64. The same
> relates to dma_addr local variable in this patch - it contains the
> physical (more correctly "DMA-able") address of the Rx buffer in the
> form NIC expects it to be written in the descriptor.
> 
> So, why to use __leXX anyway? - Debugging the (invalid) endianess is a
> real headache. Therefore there are a few static code analysis tools like
> "sparse" that allow to detect such inconsistencies (see here
> http://en.wikipedia.org/wiki/Sparse) and __leXX is a helper to allow
> tools like sparse to detect such problems.

I meant that for librte_pmd_ixgbe these types are equivalent:
lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h:
#ifndef __le64
#define __le64  u64
#endif

lib/librte_pmd_ixgbe/ixgbe/ixgbe_osdep.h:
typedef uint64_t       u64;

So why not to use just uint64_t as the rest if librt_pmd_ixgbe/ixgbe_*.[c,h]?

Have to admit, didn't know about the sparse and that ability.
Seems like useful one.
Though, as I understand, to make any use of it with DPDK,
we'll have to use sparse specific attributes:
In one of our files define __le64 as '__attribute__((bitwise)) uint64_t' 
or something similar, right?
Otherwise there is no much point in using all these '__leXX' types,
except probably to show an intention, correct? 
Konstantin

> 
> In addition after spending some time writing patches for Linux netdev
> list u develop a strong habit for such stuff - Dave and others are very
> strict about such things... ;)
> 
> So, is it the same as uint64_t? I guess now it's clear why it is now... ;)
> 
> >>   	}
> >> @@ -1559,13 +1559,14 @@ ixgbe_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
> >>   		first_seg->ol_flags = pkt_flags;
> >>
> >>   		if (likely(pkt_flags & PKT_RX_RSS_HASH))
> >> -			first_seg->hash.rss = rxd.wb.lower.hi_dword.rss;
> >> +			first_seg->hash.rss =
> >> +				    rte_le_to_cpu_32(rxd.wb.lower.hi_dword.rss);
> >>   		else if (pkt_flags & PKT_RX_FDIR) {
> >>   			first_seg->hash.fdir.hash =
> >> -				(uint16_t)((rxd.wb.lower.hi_dword.csum_ip.csum)
> >> -					   & IXGBE_ATR_HASH_MASK);
> >> +			    rte_le_to_cpu_16(rxd.wb.lower.hi_dword.csum_ip.csum)
> >> +					   & IXGBE_ATR_HASH_MASK;
> >>   			first_seg->hash.fdir.id =
> >> -				rxd.wb.lower.hi_dword.csum_ip.ip_id;
> >> +			  rte_le_to_cpu_16(rxd.wb.lower.hi_dword.csum_ip.ip_id);
> >>   		}
> >>
> >>   		/* Prefetch data of first segment, if configured to do so. */
> >> --
> >> 2.1.0

Re: [dpdk-dev] [PATCH v1 1/3] ixgbe: Use the rte_le_to_cpu_xx()/rte_cpu_to_le_xx() when reading/setting HW ring descriptor fields

[Vlad Zolotarov]

On 03/09/15 18:35, Ananyev, Konstantin wrote:
>
>> -----Original Message-----
>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
>> Sent: Monday, March 09, 2015 12:43 PM
>> To: Ananyev, Konstantin; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v1 1/3] ixgbe: Use the rte_le_to_cpu_xx()/rte_cpu_to_le_xx() when reading/setting HW ring
>> descriptor fields
>>
>>
>>
>> On 03/09/15 12:29, Ananyev, Konstantin wrote:
>>> Hi Vlad,
>>>
>>>> -----Original Message-----
>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Vlad Zolotarov
>>>> Sent: Monday, March 09, 2015 10:13 AM
>>>> To: dev@dpdk.org
>>>> Subject: [dpdk-dev] [PATCH v1 1/3] ixgbe: Use the rte_le_to_cpu_xx()/rte_cpu_to_le_xx() when reading/setting HW ring
>> descriptor
>>>> fields
>>>>
>>>> Fixed the above in ixgbe_rx_alloc_bufs() and in ixgbe_recv_scattered_pkts().
>>>>
>>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>>> ---
>>>>    lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 13 +++++++------
>>>>    1 file changed, 7 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
>>>> index 9ecf3e5..b033e04 100644
>>>> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
>>>> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
>>>> @@ -1028,7 +1028,7 @@ ixgbe_rx_alloc_bufs(struct igb_rx_queue *rxq)
>>>>    	struct igb_rx_entry *rxep;
>>>>    	struct rte_mbuf *mb;
>>>>    	uint16_t alloc_idx;
>>>> -	uint64_t dma_addr;
>>>> +	__le64 dma_addr;
>>> Wonder Why you changed from uint64_t to __le64 here?
>>> Effectively __le64 is the same a uint64_t,
>> I'm afraid the above it's not completely correct. See below.
>>
>>> and I think it is better to use always the same type across all PMD code for consistency.
>> Pls., note that "dma_addr" is only used (see below)...
>>
>>> Konstantin
>>>
>>>
>>>>    	int diag, i;
>>>>
>>>>    	/* allocate buffers in bulk directly into the S/W ring */
>>>> @@ -1051,7 +1051,7 @@ ixgbe_rx_alloc_bufs(struct igb_rx_queue *rxq)
>>>>    		mb->port = rxq->port_id;
>>>>
>>>>    		/* populate the descriptors */
>>>> -		dma_addr = (uint64_t)mb->buf_physaddr + RTE_PKTMBUF_HEADROOM;
>>>> +		dma_addr = rte_cpu_to_le_64(RTE_MBUF_DATA_DMA_ADDR_DEFAULT(mb));
>>>>    		rxdp[i].read.hdr_addr = dma_addr;
>>>>    		rxdp[i].read.pkt_addr = dma_addr;
>> here. ;) And the type of both hdr_addr and pkt_addr is __le64.
>> I don't exactly understand what do u mean by "use the same type across
>> all PMD code for consistency" - there are a lot of types used in the PMD
>> code and __le64 is one of them... ;)
>>
>> Now more seriously, let's recall what is the semantics of the __leXX
>> types - they represent the integer in the "little endian" format. Here,
>> NIC expects the physical address in a little endian format, thus the
>> descriptor is defined the way it is defined - using __le64. The same
>> relates to dma_addr local variable in this patch - it contains the
>> physical (more correctly "DMA-able") address of the Rx buffer in the
>> form NIC expects it to be written in the descriptor.
>>
>> So, why to use __leXX anyway? - Debugging the (invalid) endianess is a
>> real headache. Therefore there are a few static code analysis tools like
>> "sparse" that allow to detect such inconsistencies (see here
>> http://en.wikipedia.org/wiki/Sparse) and __leXX is a helper to allow
>> tools like sparse to detect such problems.
> I meant that for librte_pmd_ixgbe these types are equivalent:
> lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h:
> #ifndef __le64
> #define __le64  u64
> #endif
>
> lib/librte_pmd_ixgbe/ixgbe/ixgbe_osdep.h:
> typedef uint64_t       u64;
>
> So why not to use just uint64_t as the rest if librt_pmd_ixgbe/ixgbe_*.[c,h]?

I'm sorry but it seems to me that I have already mentioned that it 
wasn't the first time __leXX is used in the ixgbe_*.[ch]. All structs 
describing the descriptors of HW rings in ixgbe_type.h use them, so I'm 
just continuing what has already been done.

>
> Have to admit, didn't know about the sparse and that ability.
> Seems like useful one.
> Though, as I understand, to make any use of it with DPDK,
> we'll have to use sparse specific attributes:
> In one of our files define __le64 as '__attribute__((bitwise)) uint64_t'
> or something similar, right?

Right.

> Otherwise there is no much point in using all these '__leXX' types,
> except probably to show an intention, correct?

Not exactly. If u use these types everywhere where it's needed it's only 
6 lines to patch (__le16,32,64 + __be16,32,64) to make sparse work. And 
if u don't - there are thousands of lines to check somehow.

thanks,
vlad
> Konstantin
>
>> In addition after spending some time writing patches for Linux netdev
>> list u develop a strong habit for such stuff - Dave and others are very
>> strict about such things... ;)
>>
>> So, is it the same as uint64_t? I guess now it's clear why it is now... ;)
>>
>>>>    	}
>>>> @@ -1559,13 +1559,14 @@ ixgbe_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
>>>>    		first_seg->ol_flags = pkt_flags;
>>>>
>>>>    		if (likely(pkt_flags & PKT_RX_RSS_HASH))
>>>> -			first_seg->hash.rss = rxd.wb.lower.hi_dword.rss;
>>>> +			first_seg->hash.rss =
>>>> +				    rte_le_to_cpu_32(rxd.wb.lower.hi_dword.rss);
>>>>    		else if (pkt_flags & PKT_RX_FDIR) {
>>>>    			first_seg->hash.fdir.hash =
>>>> -				(uint16_t)((rxd.wb.lower.hi_dword.csum_ip.csum)
>>>> -					   & IXGBE_ATR_HASH_MASK);
>>>> +			    rte_le_to_cpu_16(rxd.wb.lower.hi_dword.csum_ip.csum)
>>>> +					   & IXGBE_ATR_HASH_MASK;
>>>>    			first_seg->hash.fdir.id =
>>>> -				rxd.wb.lower.hi_dword.csum_ip.ip_id;
>>>> +			  rte_le_to_cpu_16(rxd.wb.lower.hi_dword.csum_ip.ip_id);
>>>>    		}
>>>>
>>>>    		/* Prefetch data of first segment, if configured to do so. */
>>>> --
>>>> 2.1.0

Re: [dpdk-dev] [PATCH v1 1/3] ixgbe: Use the rte_le_to_cpu_xx()/rte_cpu_to_le_xx() when reading/setting HW ring descriptor fields

[Ananyev, Konstantin]

> -----Original Message-----
> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
> Sent: Monday, March 09, 2015 6:51 PM
> To: Ananyev, Konstantin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v1 1/3] ixgbe: Use the rte_le_to_cpu_xx()/rte_cpu_to_le_xx() when reading/setting HW ring
> descriptor fields
> 
> 
> 
> On 03/09/15 18:35, Ananyev, Konstantin wrote:
> >
> >> -----Original Message-----
> >> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
> >> Sent: Monday, March 09, 2015 12:43 PM
> >> To: Ananyev, Konstantin; dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH v1 1/3] ixgbe: Use the rte_le_to_cpu_xx()/rte_cpu_to_le_xx() when reading/setting HW ring
> >> descriptor fields
> >>
> >>
> >>
> >> On 03/09/15 12:29, Ananyev, Konstantin wrote:
> >>> Hi Vlad,
> >>>
> >>>> -----Original Message-----
> >>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Vlad Zolotarov
> >>>> Sent: Monday, March 09, 2015 10:13 AM
> >>>> To: dev@dpdk.org
> >>>> Subject: [dpdk-dev] [PATCH v1 1/3] ixgbe: Use the rte_le_to_cpu_xx()/rte_cpu_to_le_xx() when reading/setting HW ring
> >> descriptor
> >>>> fields
> >>>>
> >>>> Fixed the above in ixgbe_rx_alloc_bufs() and in ixgbe_recv_scattered_pkts().
> >>>>
> >>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> >>>> ---
> >>>>    lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 13 +++++++------
> >>>>    1 file changed, 7 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> >>>> index 9ecf3e5..b033e04 100644
> >>>> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> >>>> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
> >>>> @@ -1028,7 +1028,7 @@ ixgbe_rx_alloc_bufs(struct igb_rx_queue *rxq)
> >>>>    	struct igb_rx_entry *rxep;
> >>>>    	struct rte_mbuf *mb;
> >>>>    	uint16_t alloc_idx;
> >>>> -	uint64_t dma_addr;
> >>>> +	__le64 dma_addr;
> >>> Wonder Why you changed from uint64_t to __le64 here?
> >>> Effectively __le64 is the same a uint64_t,
> >> I'm afraid the above it's not completely correct. See below.
> >>
> >>> and I think it is better to use always the same type across all PMD code for consistency.
> >> Pls., note that "dma_addr" is only used (see below)...
> >>
> >>> Konstantin
> >>>
> >>>
> >>>>    	int diag, i;
> >>>>
> >>>>    	/* allocate buffers in bulk directly into the S/W ring */
> >>>> @@ -1051,7 +1051,7 @@ ixgbe_rx_alloc_bufs(struct igb_rx_queue *rxq)
> >>>>    		mb->port = rxq->port_id;
> >>>>
> >>>>    		/* populate the descriptors */
> >>>> -		dma_addr = (uint64_t)mb->buf_physaddr + RTE_PKTMBUF_HEADROOM;
> >>>> +		dma_addr = rte_cpu_to_le_64(RTE_MBUF_DATA_DMA_ADDR_DEFAULT(mb));
> >>>>    		rxdp[i].read.hdr_addr = dma_addr;
> >>>>    		rxdp[i].read.pkt_addr = dma_addr;
> >> here. ;) And the type of both hdr_addr and pkt_addr is __le64.
> >> I don't exactly understand what do u mean by "use the same type across
> >> all PMD code for consistency" - there are a lot of types used in the PMD
> >> code and __le64 is one of them... ;)
> >>
> >> Now more seriously, let's recall what is the semantics of the __leXX
> >> types - they represent the integer in the "little endian" format. Here,
> >> NIC expects the physical address in a little endian format, thus the
> >> descriptor is defined the way it is defined - using __le64. The same
> >> relates to dma_addr local variable in this patch - it contains the
> >> physical (more correctly "DMA-able") address of the Rx buffer in the
> >> form NIC expects it to be written in the descriptor.
> >>
> >> So, why to use __leXX anyway? - Debugging the (invalid) endianess is a
> >> real headache. Therefore there are a few static code analysis tools like
> >> "sparse" that allow to detect such inconsistencies (see here
> >> http://en.wikipedia.org/wiki/Sparse) and __leXX is a helper to allow
> >> tools like sparse to detect such problems.
> > I meant that for librte_pmd_ixgbe these types are equivalent:
> > lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h:
> > #ifndef __le64
> > #define __le64  u64
> > #endif
> >
> > lib/librte_pmd_ixgbe/ixgbe/ixgbe_osdep.h:
> > typedef uint64_t       u64;
> >
> > So why not to use just uint64_t as the rest if librt_pmd_ixgbe/ixgbe_*.[c,h]?
> 
> I'm sorry but it seems to me that I have already mentioned that it
> wasn't the first time __leXX is used in the ixgbe_*.[ch]. All structs
> describing the descriptors of HW rings in ixgbe_type.h use them, so I'm
> just continuing what has already been done.
> 
> >
> > Have to admit, didn't know about the sparse and that ability.
> > Seems like useful one.
> > Though, as I understand, to make any use of it with DPDK,
> > we'll have to use sparse specific attributes:
> > In one of our files define __le64 as '__attribute__((bitwise)) uint64_t'
> > or something similar, right?
> 
> Right.
> 
> > Otherwise there is no much point in using all these '__leXX' types,
> > except probably to show an intention, correct?
> 
> Not exactly. If u use these types everywhere where it's needed it's only
> 6 lines to patch (__le16,32,64 + __be16,32,64) to make sparse work. And
> if u don't - there are thousands of lines to check somehow.

Yeh, though the thing is - we don't use it in all other similar places...
But probably you right and it is never too late to start with good habits. 
So I am convinced :)
Thanks
Konstantin

> 
> thanks,
> vlad
> > Konstantin
> >
> >> In addition after spending some time writing patches for Linux netdev
> >> list u develop a strong habit for such stuff - Dave and others are very
> >> strict about such things... ;)
> >>
> >> So, is it the same as uint64_t? I guess now it's clear why it is now... ;)
> >>
> >>>>    	}
> >>>> @@ -1559,13 +1559,14 @@ ixgbe_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
> >>>>    		first_seg->ol_flags = pkt_flags;
> >>>>
> >>>>    		if (likely(pkt_flags & PKT_RX_RSS_HASH))
> >>>> -			first_seg->hash.rss = rxd.wb.lower.hi_dword.rss;
> >>>> +			first_seg->hash.rss =
> >>>> +				    rte_le_to_cpu_32(rxd.wb.lower.hi_dword.rss);
> >>>>    		else if (pkt_flags & PKT_RX_FDIR) {
> >>>>    			first_seg->hash.fdir.hash =
> >>>> -				(uint16_t)((rxd.wb.lower.hi_dword.csum_ip.csum)
> >>>> -					   & IXGBE_ATR_HASH_MASK);
> >>>> +			    rte_le_to_cpu_16(rxd.wb.lower.hi_dword.csum_ip.csum)
> >>>> +					   & IXGBE_ATR_HASH_MASK;
> >>>>    			first_seg->hash.fdir.id =
> >>>> -				rxd.wb.lower.hi_dword.csum_ip.ip_id;
> >>>> +			  rte_le_to_cpu_16(rxd.wb.lower.hi_dword.csum_ip.ip_id);
> >>>>    		}
> >>>>
> >>>>    		/* Prefetch data of first segment, if configured to do so. */
> >>>> --
> >>>> 2.1.0

[dpdk-dev] [PATCH v1 2/3] ixgbe: Bug fix: Properly configure Rx CRC stripping for x540 devices

[Vlad Zolotarov]

According to x540 spec chapter 8.2.4.8.9 CRCSTRIP field of RDRXCTL should
be configured to the same value as HLREG0.RXCRCSTRP.

Clearing the RDRXCTL.RSCFRSTSIZE field for x540 is not required by the spec
but seems harmless.

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
 lib/librte_pmd_ixgbe/ixgbe_rxtx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
index b033e04..ce9658e 100644
--- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
+++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
@@ -3677,7 +3677,8 @@ ixgbe_dev_rx_init(struct rte_eth_dev *dev)
 
 	IXGBE_WRITE_REG(hw, IXGBE_RXCSUM, rxcsum);
 
-	if (hw->mac.type == ixgbe_mac_82599EB) {
+	if (hw->mac.type == ixgbe_mac_82599EB ||
+	    hw->mac.type == ixgbe_mac_X540) {
 		rdrxctl = IXGBE_READ_REG(hw, IXGBE_RDRXCTL);
 		if (dev->data->dev_conf.rxmode.hw_strip_crc)
 			rdrxctl |= IXGBE_RDRXCTL_CRCSTRIP;
-- 
2.1.0

[dpdk-dev] [PATCH v1 3/3] ixgbe: Unify the rx_pkt_bulk callback initialization

[Vlad Zolotarov]

   - Set the callback in a single function that is called from
     ixgbe_dev_rx_init() for a primary process and from eth_ixgbe_dev_init()
     for a secondary processes. This is instead of multiple, hard to track places.
   - Added ixgbe_hw.rx_bulk_alloc_allowed - see ixgbe_hw.rx_vec_allowed description below.
   - Added ixgbe_hw.rx_vec_allowed: like with Bulk Allocation, Vector Rx is
     enabled or disabled on a per-port level. All queues have to meet the appropriate
     preconditions and if any of them doesn't - the feature has to be disabled.
     Therefore ixgbe_hw.rx_vec_allowed will be updated during each queues configuration
     (rte_eth_rx_queue_setup()) and then used in rte_eth_dev_start() to configure the
     appropriate callbacks. The same happens with ixgbe_hw.rx_vec_allowed in a Bulk Allocation
     context.
   - Bugs fixed:
      - Vector scattered packets callback was called regardless the appropriate
        preconditions:
         - Vector Rx specific preconditions.
         - Bulk Allocation preconditions.
      - Vector Rx was enabled/disabled according to the last queue setting and not
        based on all queues setting (which may be different for each queue).

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
 lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h |   2 +
 lib/librte_pmd_ixgbe/ixgbe_ethdev.c     |  13 ++-
 lib/librte_pmd_ixgbe/ixgbe_rxtx.c       | 183 +++++++++++++++++++++-----------
 lib/librte_pmd_ixgbe/ixgbe_rxtx.h       |  22 +++-
 4 files changed, 152 insertions(+), 68 deletions(-)

diff --git a/lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h b/lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h
index c67d462..9a66370 100644
--- a/lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h
+++ b/lib/librte_pmd_ixgbe/ixgbe/ixgbe_type.h
@@ -3657,6 +3657,8 @@ struct ixgbe_hw {
 	bool force_full_reset;
 	bool allow_unsupported_sfp;
 	bool wol_enabled;
+	bool rx_bulk_alloc_allowed;
+	bool rx_vec_allowed;
 };
 
 #define ixgbe_call_func(hw, func, params, error) \
diff --git a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
index 9bdc046..9d3de1a 100644
--- a/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
+++ b/lib/librte_pmd_ixgbe/ixgbe_ethdev.c
@@ -760,8 +760,8 @@ eth_ixgbe_dev_init(__attribute__((unused)) struct eth_driver *eth_drv,
 			                   "Using default TX function.");
 		}
 
-		if (eth_dev->data->scattered_rx)
-			eth_dev->rx_pkt_burst = ixgbe_recv_scattered_pkts;
+		set_rx_function(eth_dev);
+
 		return 0;
 	}
 	pci_dev = eth_dev->pci_dev;
@@ -772,6 +772,13 @@ eth_ixgbe_dev_init(__attribute__((unused)) struct eth_driver *eth_drv,
 	hw->hw_addr = (void *)pci_dev->mem_resource[0].addr;
 	hw->allow_unsupported_sfp = 1;
 
+	/*
+	 * Initialize to TRUE. If any of Rx queues doesn't meet the bulk
+	 * allocation or vector Rx preconditions we will reset it.
+	 */
+	hw->rx_bulk_alloc_allowed = true;
+	hw->rx_vec_allowed = true;
+
 	/* Initialize the shared code (base driver) */
 #ifdef RTE_NIC_BYPASS
 	diag = ixgbe_bypass_init_shared_code(hw);
@@ -1641,6 +1648,8 @@ ixgbe_dev_stop(struct rte_eth_dev *dev)
 
 	/* Clear stored conf */
 	dev->data->scattered_rx = 0;
+	hw->rx_bulk_alloc_allowed = false;
+	hw->rx_vec_allowed = false;
 
 	/* Clear recorded link status */
 	memset(&link, 0, sizeof(link));
diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
index ce9658e..72c65df 100644
--- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
+++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.c
@@ -2074,12 +2074,12 @@ check_rx_burst_bulk_alloc_preconditions(__rte_unused struct igb_rx_queue *rxq)
 
 /* Reset dynamic igb_rx_queue fields back to defaults */
 static void
-ixgbe_reset_rx_queue(struct igb_rx_queue *rxq)
+ixgbe_reset_rx_queue(struct ixgbe_hw *hw, struct igb_rx_queue *rxq)
 {
 	static const union ixgbe_adv_rx_desc zeroed_desc = { .read = {
 			.pkt_addr = 0}};
 	unsigned i;
-	uint16_t len;
+	uint16_t len = rxq->nb_rx_desc;
 
 	/*
 	 * By default, the Rx queue setup function allocates enough memory for
@@ -2091,14 +2091,9 @@ ixgbe_reset_rx_queue(struct igb_rx_queue *rxq)
 	 * constraints here to see if we need to zero out memory after the end
 	 * of the H/W descriptor ring.
 	 */
-#ifdef RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC
-	if (check_rx_burst_bulk_alloc_preconditions(rxq) == 0)
+	if (hw->rx_bulk_alloc_allowed)
 		/* zero out extra memory */
-		len = (uint16_t)(rxq->nb_rx_desc + RTE_PMD_IXGBE_RX_MAX_BURST);
-	else
-#endif
-		/* do not zero out extra memory */
-		len = rxq->nb_rx_desc;
+		len += RTE_PMD_IXGBE_RX_MAX_BURST;
 
 	/*
 	 * Zero out HW ring memory. Zero out extra memory at the end of
@@ -2140,7 +2135,6 @@ ixgbe_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	const struct rte_memzone *rz;
 	struct igb_rx_queue *rxq;
 	struct ixgbe_hw     *hw;
-	int use_def_burst_func = 1;
 	uint16_t len;
 
 	PMD_INIT_FUNC_TRACE();
@@ -2221,15 +2215,27 @@ ixgbe_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	rxq->rx_ring = (union ixgbe_adv_rx_desc *) rz->addr;
 
 	/*
+	 * Certain constraints must be met in order to use the bulk buffer
+	 * allocation Rx burst function. If any of Rx queues doesn't meet them
+	 * the feature should be disabled for the whole port.
+	 */
+	if (check_rx_burst_bulk_alloc_preconditions(rxq)) {
+		PMD_INIT_LOG(DEBUG, "queue[%d] doesn't meet Rx Bulk Alloc "
+				    "preconditions - canceling the feature for "
+				    "the whole port[%d]",
+			     rxq->queue_id, rxq->port_id);
+		hw->rx_bulk_alloc_allowed = false;
+	}
+
+	/*
 	 * Allocate software ring. Allow for space at the end of the
 	 * S/W ring to make sure look-ahead logic in bulk alloc Rx burst
 	 * function does not access an invalid memory region.
 	 */
-#ifdef RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC
-	len = (uint16_t)(nb_desc + RTE_PMD_IXGBE_RX_MAX_BURST);
-#else
 	len = nb_desc;
-#endif
+	if (hw->rx_bulk_alloc_allowed)
+		len += RTE_PMD_IXGBE_RX_MAX_BURST;
+
 	rxq->sw_ring = rte_zmalloc_socket("rxq->sw_ring",
 					  sizeof(struct igb_rx_entry) * len,
 					  RTE_CACHE_LINE_SIZE, socket_id);
@@ -2240,42 +2246,18 @@ ixgbe_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	PMD_INIT_LOG(DEBUG, "sw_ring=%p hw_ring=%p dma_addr=0x%"PRIx64,
 		     rxq->sw_ring, rxq->rx_ring, rxq->rx_ring_phys_addr);
 
-	/*
-	 * Certain constraints must be met in order to use the bulk buffer
-	 * allocation Rx burst function.
-	 */
-	use_def_burst_func = check_rx_burst_bulk_alloc_preconditions(rxq);
+	if (!rte_is_power_of_2(nb_desc)) {
+		PMD_INIT_LOG(DEBUG, "queue[%d] doesn't meet Vector Rx "
+				    "preconditions - canceling the feature for "
+				    "the whole port[%d]",
+			     rxq->queue_id, rxq->port_id);
+		hw->rx_vec_allowed = false;
+	} else
+		ixgbe_rxq_vec_setup(rxq);
 
-#ifdef RTE_IXGBE_INC_VECTOR
-	ixgbe_rxq_vec_setup(rxq);
-#endif
-	/* Check if pre-conditions are satisfied, and no Scattered Rx */
-	if (!use_def_burst_func && !dev->data->scattered_rx) {
-#ifdef RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC
-		PMD_INIT_LOG(DEBUG, "Rx Burst Bulk Alloc Preconditions are "
-			     "satisfied. Rx Burst Bulk Alloc function will be "
-			     "used on port=%d, queue=%d.",
-			     rxq->port_id, rxq->queue_id);
-		dev->rx_pkt_burst = ixgbe_recv_pkts_bulk_alloc;
-#ifdef RTE_IXGBE_INC_VECTOR
-		if (!ixgbe_rx_vec_condition_check(dev) &&
-		    (rte_is_power_of_2(nb_desc))) {
-			PMD_INIT_LOG(INFO, "Vector rx enabled, please make "
-				     "sure RX burst size no less than 32.");
-			dev->rx_pkt_burst = ixgbe_recv_pkts_vec;
-		}
-#endif
-#endif
-	} else {
-		PMD_INIT_LOG(DEBUG, "Rx Burst Bulk Alloc Preconditions "
-			     "are not satisfied, Scattered Rx is requested, "
-			     "or RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC is not "
-			     "enabled (port=%d, queue=%d).",
-			     rxq->port_id, rxq->queue_id);
-	}
 	dev->data->rx_queues[queue_idx] = rxq;
 
-	ixgbe_reset_rx_queue(rxq);
+	ixgbe_reset_rx_queue(hw, rxq);
 
 	return 0;
 }
@@ -2329,6 +2311,7 @@ void
 ixgbe_dev_clear_queues(struct rte_eth_dev *dev)
 {
 	unsigned i;
+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	PMD_INIT_FUNC_TRACE();
 
@@ -2344,7 +2327,7 @@ ixgbe_dev_clear_queues(struct rte_eth_dev *dev)
 		struct igb_rx_queue *rxq = dev->data->rx_queues[i];
 		if (rxq != NULL) {
 			ixgbe_rx_queue_release_mbufs(rxq);
-			ixgbe_reset_rx_queue(rxq);
+			ixgbe_reset_rx_queue(hw, rxq);
 		}
 	}
 }
@@ -3506,6 +3489,58 @@ ixgbe_dev_mq_tx_configure(struct rte_eth_dev *dev)
 	return 0;
 }
 
+void set_rx_function(struct rte_eth_dev *dev)
+{
+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+
+	if (ixgbe_rx_vec_condition_check(dev)) {
+		PMD_INIT_LOG(DEBUG, "Port[%d] doesn't meet Vector Rx "
+				    "preconditions or RTE_IXGBE_INC_VECTOR is "
+				    "not enabled",
+			     dev->data->port_id);
+		hw->rx_vec_allowed = false;
+	}
+
+	/* Check if bulk alloc is allowed and no Scattered Rx */
+	if (hw->rx_bulk_alloc_allowed && !dev->data->scattered_rx) {
+		PMD_INIT_LOG(DEBUG, "Rx Burst Bulk Alloc Preconditions are "
+				    "satisfied. Rx Burst Bulk Alloc function "
+				    "will be used on port=%d.",
+			     dev->data->port_id);
+		dev->rx_pkt_burst = ixgbe_recv_pkts_bulk_alloc;
+
+		if (hw->rx_vec_allowed) {
+			PMD_INIT_LOG(INFO, "Vector rx enabled, please make "
+					   "sure RX burst size no less "
+					   "than 32.");
+			dev->rx_pkt_burst = ixgbe_recv_pkts_vec;
+		}
+	} else {
+		dev->rx_pkt_burst = ixgbe_recv_pkts;
+
+		PMD_INIT_LOG(DEBUG, "Rx Burst Bulk Alloc Preconditions are not "
+				    "satisfied, or Scattered Rx is requested, "
+				    "or RTE_LIBRTE_IXGBE_RX_ALLOW_BULK_ALLOC "
+				    "is not enabled (port=%d).",
+			     dev->data->port_id);
+	}
+
+	if (dev->data->scattered_rx) {
+		if (hw->rx_vec_allowed && hw->rx_bulk_alloc_allowed) {
+			PMD_INIT_LOG(DEBUG, "Using Vector Scattered Rx "
+					    "callback (port=%d).",
+				     dev->data->port_id);
+			dev->rx_pkt_burst = ixgbe_recv_scattered_pkts_vec;
+		} else {
+			PMD_INIT_LOG(DEBUG, "Using Regualr (non-vector) "
+					    "Scattered Rx callback "
+					    "(port=%d).",
+				     dev->data->port_id);
+			dev->rx_pkt_burst = ixgbe_recv_scattered_pkts;
+		}
+	}
+}
+
 /*
  * Initializes Receive Unit.
  */
@@ -3641,23 +3676,17 @@ ixgbe_dev_rx_init(struct rte_eth_dev *dev)
 		buf_size = (uint16_t) ((srrctl & IXGBE_SRRCTL_BSIZEPKT_MASK) <<
 				       IXGBE_SRRCTL_BSIZEPKT_SHIFT);
 
-		if (dev->data->dev_conf.rxmode.enable_scatter ||
-		    /* It adds dual VLAN length for supporting dual VLAN */
-		    (dev->data->dev_conf.rxmode.max_rx_pkt_len +
-				2 * IXGBE_VLAN_TAG_SIZE) > buf_size){
-			if (!dev->data->scattered_rx)
-				PMD_INIT_LOG(DEBUG, "forcing scatter mode");
+		/* It adds dual VLAN length for supporting dual VLAN */
+		if (dev->data->dev_conf.rxmode.max_rx_pkt_len +
+					    2 * IXGBE_VLAN_TAG_SIZE > buf_size)
 			dev->data->scattered_rx = 1;
-#ifdef RTE_IXGBE_INC_VECTOR
-			if (rte_is_power_of_2(rxq->nb_rx_desc))
-				dev->rx_pkt_burst =
-					ixgbe_recv_scattered_pkts_vec;
-			else
-#endif
-				dev->rx_pkt_burst = ixgbe_recv_scattered_pkts;
-		}
 	}
 
+	if (rx_conf->enable_scatter)
+		dev->data->scattered_rx = 1;
+
+	set_rx_function(dev);
+
 	/*
 	 * Device configured with multiple RX queues.
 	 */
@@ -3933,7 +3962,7 @@ ixgbe_dev_rx_queue_stop(struct rte_eth_dev *dev, uint16_t rx_queue_id)
 		rte_delay_us(RTE_IXGBE_WAIT_100_US);
 
 		ixgbe_rx_queue_release_mbufs(rxq);
-		ixgbe_reset_rx_queue(rxq);
+		ixgbe_reset_rx_queue(hw, rxq);
 	} else
 		return -1;
 
@@ -4289,3 +4318,31 @@ ixgbevf_dev_rxtx_start(struct rte_eth_dev *dev)
 
 	}
 }
+
+/* Stubs needed for linkage when CONFIG_RTE_IXGBE_INC_VECTOR is set to 'n' */
+#ifndef RTE_IXGBE_INC_VECTOR
+int ixgbe_rx_vec_condition_check(
+	struct rte_eth_dev __rte_unused *dev)
+{
+	return -1;
+}
+
+uint16_t
+ixgbe_recv_pkts_vec(void __rte_unused *rx_queue,
+		    struct rte_mbuf __rte_unused **rx_pkts,
+		    uint16_t __rte_unused nb_pkts)
+{
+	return 0;
+}
+
+uint16_t ixgbe_recv_scattered_pkts_vec(void __rte_unused *rx_queue,
+	struct rte_mbuf __rte_unused **rx_pkts, uint16_t __rte_unused nb_pkts)
+{
+	return 0;
+}
+
+int ixgbe_rxq_vec_setup(struct igb_rx_queue __rte_unused *rxq)
+{
+	return -1;
+}
+#endif
diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx.h b/lib/librte_pmd_ixgbe/ixgbe_rxtx.h
index 329007c..bbe5ff3 100644
--- a/lib/librte_pmd_ixgbe/ixgbe_rxtx.h
+++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx.h
@@ -255,16 +255,32 @@ struct ixgbe_txq_ops {
  */
 void set_tx_function(struct rte_eth_dev *dev, struct igb_tx_queue *txq);
 
-#ifdef RTE_IXGBE_INC_VECTOR
+/**
+ * Sets the rx_pkt_burst callback in the ixgbe rte_eth_dev instance.
+ *
+ * Sets the callback based on the device parameters:
+ *  - ixgbe_hw.rx_bulk_alloc_allowed
+ *  - rte_eth_dev_data.scattered_rx
+ *  - rte_eth_dev_data.lro
+ *  - conditions checked in ixgbe_rx_vec_condition_check()
+ *
+ *  This means that the parameters above have to be configured prior to calling
+ *  to this function.
+ *
+ * @dev rte_eth_dev handle
+ */
+void set_rx_function(struct rte_eth_dev *dev);
+
 uint16_t ixgbe_recv_pkts_vec(void *rx_queue, struct rte_mbuf **rx_pkts,
 		uint16_t nb_pkts);
 uint16_t ixgbe_recv_scattered_pkts_vec(void *rx_queue,
 		struct rte_mbuf **rx_pkts, uint16_t nb_pkts);
+int ixgbe_rx_vec_condition_check(struct rte_eth_dev *dev);
+int ixgbe_rxq_vec_setup(struct igb_rx_queue *rxq);
+#ifdef RTE_IXGBE_INC_VECTOR
 uint16_t ixgbe_xmit_pkts_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
 		uint16_t nb_pkts);
 int ixgbe_txq_vec_setup(struct igb_tx_queue *txq);
-int ixgbe_rxq_vec_setup(struct igb_rx_queue *rxq);
-int ixgbe_rx_vec_condition_check(struct rte_eth_dev *dev);
 #endif
 
 #endif
-- 
2.1.0

Re: [dpdk-dev] [PATCH v1 3/3] ixgbe: Unify the rx_pkt_bulk callback initialization

[Mcnamara, John]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Vlad Zolotarov
> Sent: Monday, March 9, 2015 10:13 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v1 3/3] ixgbe: Unify the rx_pkt_bulk callback
> initialization
> 
> @@ -3641,23 +3676,17 @@ ixgbe_dev_rx_init(struct rte_eth_dev *dev)
>  		buf_size = (uint16_t) ((srrctl & IXGBE_SRRCTL_BSIZEPKT_MASK)
> <<
>  				       IXGBE_SRRCTL_BSIZEPKT_SHIFT);
> 
> -		if (dev->data->dev_conf.rxmode.enable_scatter ||
> -		    /* It adds dual VLAN length for supporting dual VLAN */
> -		    (dev->data->dev_conf.rxmode.max_rx_pkt_len +
> -				2 * IXGBE_VLAN_TAG_SIZE) > buf_size){
> -			if (!dev->data->scattered_rx)
> -				PMD_INIT_LOG(DEBUG, "forcing scatter mode");
> +		/* It adds dual VLAN length for supporting dual VLAN */
> +		if (dev->data->dev_conf.rxmode.max_rx_pkt_len +
> +					    2 * IXGBE_VLAN_TAG_SIZE > buf_size)
>  			dev->data->scattered_rx = 1;
> -#ifdef RTE_IXGBE_INC_VECTOR
> -			if (rte_is_power_of_2(rxq->nb_rx_desc))
> -				dev->rx_pkt_burst =
> -					ixgbe_recv_scattered_pkts_vec;
> -			else
> -#endif
> -				dev->rx_pkt_burst = ixgbe_recv_scattered_pkts;
> -		}
>  	}
> 
> +	if (rx_conf->enable_scatter)
> +		dev->data->scattered_rx = 1;

Hi,

There is a compilation issue here when this patch is applied to the current master (with previous 2 patches applied as well):

make T=x86_64-native-linuxapp-gcc install CC=gcc
...
== Build lib/librte_pmd_ixgbe
  CC ixgbe_rxtx.o
/home/user/dpdk/lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function 'ixgbe_dev_rx_init':
/home/user/dpdk/lib/librte_pmd_ixgbe/ixgbe_rxtx.c:3689:6: error: 'rx_conf' undeclared (first use in this function)
/home/user/dpdk/lib/librte_pmd_ixgbe/ixgbe_rxtx.c:3689:6: note: each undeclared identifier is reported only once for each function it appears in
make[5]: *** [ixgbe_rxtx.o] Error 1

This is using the default DPDK linuxapp config.

John

Re: [dpdk-dev] [PATCH v1 3/3] ixgbe: Unify the rx_pkt_bulk callback initialization

[Vlad Zolotarov]

On 03/09/15 15:39, Mcnamara, John wrote:
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Vlad Zolotarov
>> Sent: Monday, March 9, 2015 10:13 AM
>> To: dev@dpdk.org
>> Subject: [dpdk-dev] [PATCH v1 3/3] ixgbe: Unify the rx_pkt_bulk callback
>> initialization
>>
>> @@ -3641,23 +3676,17 @@ ixgbe_dev_rx_init(struct rte_eth_dev *dev)
>>   		buf_size = (uint16_t) ((srrctl & IXGBE_SRRCTL_BSIZEPKT_MASK)
>> <<
>>   				       IXGBE_SRRCTL_BSIZEPKT_SHIFT);
>>
>> -		if (dev->data->dev_conf.rxmode.enable_scatter ||
>> -		    /* It adds dual VLAN length for supporting dual VLAN */
>> -		    (dev->data->dev_conf.rxmode.max_rx_pkt_len +
>> -				2 * IXGBE_VLAN_TAG_SIZE) > buf_size){
>> -			if (!dev->data->scattered_rx)
>> -				PMD_INIT_LOG(DEBUG, "forcing scatter mode");
>> +		/* It adds dual VLAN length for supporting dual VLAN */
>> +		if (dev->data->dev_conf.rxmode.max_rx_pkt_len +
>> +					    2 * IXGBE_VLAN_TAG_SIZE > buf_size)
>>   			dev->data->scattered_rx = 1;
>> -#ifdef RTE_IXGBE_INC_VECTOR
>> -			if (rte_is_power_of_2(rxq->nb_rx_desc))
>> -				dev->rx_pkt_burst =
>> -					ixgbe_recv_scattered_pkts_vec;
>> -			else
>> -#endif
>> -				dev->rx_pkt_burst = ixgbe_recv_scattered_pkts;
>> -		}
>>   	}
>>
>> +	if (rx_conf->enable_scatter)
>> +		dev->data->scattered_rx = 1;
> Hi,
>
> There is a compilation issue here when this patch is applied to the current master (with previous 2 patches applied as well):
>
> make T=x86_64-native-linuxapp-gcc install CC=gcc
> ...
> == Build lib/librte_pmd_ixgbe
>    CC ixgbe_rxtx.o
> /home/user/dpdk/lib/librte_pmd_ixgbe/ixgbe_rxtx.c: In function 'ixgbe_dev_rx_init':
> /home/user/dpdk/lib/librte_pmd_ixgbe/ixgbe_rxtx.c:3689:6: error: 'rx_conf' undeclared (first use in this function)
> /home/user/dpdk/lib/librte_pmd_ixgbe/ixgbe_rxtx.c:3689:6: note: each undeclared identifier is reported only once for each function it appears in
> make[5]: *** [ixgbe_rxtx.o] Error 1
>
> This is using the default DPDK linuxapp config.

v2 is out. Again, my apologize. ;)

>
> John
>