[dpdk-dev,v2] ixgbe: fix check for split packets

  The check for split packets to be reassembled in the vector ixgbe PMD
was incorrectly only checking the first 16 elements of the array instead
of all 32. This is fixed by changing the uint32_t values to be uint64_t
instead.

Fixes: cf4b4708a88a ("ixgbe: improve slow-path perf with vector scattered Rx")

Reported-by: Zoltan Kiss <zoltan.kiss@linaro.org>
Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

---
V2: Rename variable from split_fl32 to split_fl64 to match type change.
---
 drivers/net/ixgbe/ixgbe_rxtx_vec.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
  

Hi,

And what happens if someone changes RTE_IXGBE_VPMD_RX_BURST to something 
else than 32? I guess this bug were introduced when someone raised it 
from 16 to 32
I think you are better off with a for loop which uses this value. Or at 
least make a comment around RTE_IXGBE_VPMD_RX_BURST that if you change 
that value this check should be modified as well.

Regards,

Zoltan

On 22/07/15 10:13, Bruce Richardson wrote:
> The check for split packets to be reassembled in the vector ixgbe PMD
> was incorrectly only checking the first 16 elements of the array instead
> of all 32. This is fixed by changing the uint32_t values to be uint64_t
> instead.
>
> Fixes: cf4b4708a88a ("ixgbe: improve slow-path perf with vector scattered Rx")
>
> Reported-by: Zoltan Kiss <zoltan.kiss@linaro.org>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>
> ---
> V2: Rename variable from split_fl32 to split_fl64 to match type change.
> ---
>   drivers/net/ixgbe/ixgbe_rxtx_vec.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> index d3ac74a..f2052c6 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> @@ -549,10 +549,10 @@ ixgbe_recv_scattered_pkts_vec(void *rx_queue, struct rte_mbuf **rx_pkts,
>   		return 0;
>
>   	/* happy day case, full burst + no packets to be joined */
> -	const uint32_t *split_fl32 = (uint32_t *)split_flags;
> +	const uint64_t *split_fl64 = (uint64_t *)split_flags;
>   	if (rxq->pkt_first_seg == NULL &&
> -			split_fl32[0] == 0 && split_fl32[1] == 0 &&
> -			split_fl32[2] == 0 && split_fl32[3] == 0)
> +			split_fl64[0] == 0 && split_fl64[1] == 0 &&
> +			split_fl64[2] == 0 && split_fl64[3] == 0)
>   		return nb_bufs;
>
>   	/* reassemble any packets that need reassembly*/
>
  

On Wed, Jul 22, 2015 at 10:47:34AM +0100, Zoltan Kiss wrote:
> Hi,
> 
> And what happens if someone changes RTE_IXGBE_VPMD_RX_BURST to something
> else than 32? I guess this bug were introduced when someone raised it from
> 16 to 32

Actually, no, this bug is purely due to me getting my maths wrong when I 
wrote this function. The vector PMD has always worked in bursts of 32 at a
time.

> I think you are better off with a for loop which uses this value. Or at
> least make a comment around RTE_IXGBE_VPMD_RX_BURST that if you change that
> value this check should be modified as well.

The vector PMD always works off a fixed 32 burst size. Any change to that will
lead to many changes in the code, so I don't believe a loop is necessary.

Regards,
/Bruce

> 
> Regards,
> 
> Zoltan
> 
> On 22/07/15 10:13, Bruce Richardson wrote:
> >The check for split packets to be reassembled in the vector ixgbe PMD
> >was incorrectly only checking the first 16 elements of the array instead
> >of all 32. This is fixed by changing the uint32_t values to be uint64_t
> >instead.
> >
> >Fixes: cf4b4708a88a ("ixgbe: improve slow-path perf with vector scattered Rx")
> >
> >Reported-by: Zoltan Kiss <zoltan.kiss@linaro.org>
> >Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> >
> >---
> >V2: Rename variable from split_fl32 to split_fl64 to match type change.
> >---
> >  drivers/net/ixgbe/ixgbe_rxtx_vec.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> >index d3ac74a..f2052c6 100644
> >--- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> >+++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> >@@ -549,10 +549,10 @@ ixgbe_recv_scattered_pkts_vec(void *rx_queue, struct rte_mbuf **rx_pkts,
> >  		return 0;
> >
> >  	/* happy day case, full burst + no packets to be joined */
> >-	const uint32_t *split_fl32 = (uint32_t *)split_flags;
> >+	const uint64_t *split_fl64 = (uint64_t *)split_flags;
> >  	if (rxq->pkt_first_seg == NULL &&
> >-			split_fl32[0] == 0 && split_fl32[1] == 0 &&
> >-			split_fl32[2] == 0 && split_fl32[3] == 0)
> >+			split_fl64[0] == 0 && split_fl64[1] == 0 &&
> >+			split_fl64[2] == 0 && split_fl64[3] == 0)
> >  		return nb_bufs;
> >
> >  	/* reassemble any packets that need reassembly*/
> >
  

On 22/07/15 10:59, Bruce Richardson wrote:
> On Wed, Jul 22, 2015 at 10:47:34AM +0100, Zoltan Kiss wrote:
>> Hi,
>>
>> And what happens if someone changes RTE_IXGBE_VPMD_RX_BURST to something
>> else than 32? I guess this bug were introduced when someone raised it from
>> 16 to 32
>
> Actually, no, this bug is purely due to me getting my maths wrong when I
> wrote this function. The vector PMD has always worked in bursts of 32 at a
> time.
>
>> I think you are better off with a for loop which uses this value. Or at
>> least make a comment around RTE_IXGBE_VPMD_RX_BURST that if you change that
>> value this check should be modified as well.
>
> The vector PMD always works off a fixed 32 burst size. Any change to that will
> lead to many changes in the code, so I don't believe a loop is necessary.

Ok, then I suggest to make a comment around RTE_IXGBE_VPMD_RX_BURST that 
changing it needs a lot of other changes in the code elsewhere, e.g in 
this split_flags check.
Btw. vPMD was a bit misleading abbreviation for me, it took me a while 
until I realized 'v' stands for 'vector', not 'virtualization' as in 
most cases nowadays.

>
> Regards,
> /Bruce
>
>>
>> Regards,
>>
>> Zoltan
>>
>> On 22/07/15 10:13, Bruce Richardson wrote:
>>> The check for split packets to be reassembled in the vector ixgbe PMD
>>> was incorrectly only checking the first 16 elements of the array instead
>>> of all 32. This is fixed by changing the uint32_t values to be uint64_t
>>> instead.
>>>
>>> Fixes: cf4b4708a88a ("ixgbe: improve slow-path perf with vector scattered Rx")
>>>
>>> Reported-by: Zoltan Kiss <zoltan.kiss@linaro.org>
>>> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>>>
>>> ---
>>> V2: Rename variable from split_fl32 to split_fl64 to match type change.
>>> ---
>>>   drivers/net/ixgbe/ixgbe_rxtx_vec.c | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
>>> index d3ac74a..f2052c6 100644
>>> --- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
>>> +++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
>>> @@ -549,10 +549,10 @@ ixgbe_recv_scattered_pkts_vec(void *rx_queue, struct rte_mbuf **rx_pkts,
>>>   		return 0;
>>>
>>>   	/* happy day case, full burst + no packets to be joined */
>>> -	const uint32_t *split_fl32 = (uint32_t *)split_flags;
>>> +	const uint64_t *split_fl64 = (uint64_t *)split_flags;
>>>   	if (rxq->pkt_first_seg == NULL &&
>>> -			split_fl32[0] == 0 && split_fl32[1] == 0 &&
>>> -			split_fl32[2] == 0 && split_fl32[3] == 0)
>>> +			split_fl64[0] == 0 && split_fl64[1] == 0 &&
>>> +			split_fl64[2] == 0 && split_fl64[3] == 0)
>>>   		return nb_bufs;
>>>
>>>   	/* reassemble any packets that need reassembly*/
>>>
  

On 22/07/15 14:19, Zoltan Kiss wrote:
> Btw. vPMD was a bit misleading abbreviation for me, it took me a while
> until I realized 'v' stands for 'vector', not 'virtualization' as in
> most cases nowadays.
>

Though that's mostly my fault to not to check the documentation :)
  

> -----Original Message-----
> From: Zoltan Kiss [mailto:zoltan.kiss@linaro.org]
> Sent: Wednesday, July 22, 2015 2:20 PM
> To: Richardson, Bruce
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] ixgbe: fix check for split packets
> 
> 
> 
> On 22/07/15 10:59, Bruce Richardson wrote:
> > On Wed, Jul 22, 2015 at 10:47:34AM +0100, Zoltan Kiss wrote:
> >> Hi,
> >>
> >> And what happens if someone changes RTE_IXGBE_VPMD_RX_BURST to
> >> something else than 32? I guess this bug were introduced when someone
> >> raised it from
> >> 16 to 32
> >
> > Actually, no, this bug is purely due to me getting my maths wrong when
> > I wrote this function. The vector PMD has always worked in bursts of
> > 32 at a time.
> >
> >> I think you are better off with a for loop which uses this value. Or
> >> at least make a comment around RTE_IXGBE_VPMD_RX_BURST that if you
> >> change that value this check should be modified as well.
> >
> > The vector PMD always works off a fixed 32 burst size. Any change to
> > that will lead to many changes in the code, so I don't believe a loop is
> necessary.
> 
> Ok, then I suggest to make a comment around RTE_IXGBE_VPMD_RX_BURST that
> changing it needs a lot of other changes in the code elsewhere, e.g in
> this split_flags check.
> Btw. vPMD was a bit misleading abbreviation for me, it took me a while
> until I realized 'v' stands for 'vector', not 'virtualization' as in most
> cases nowadays.

Good idea. I'll try to submit a patch to add a comment if I get the chance - otherwise feel free to do so yourself.

As for the naming, yes, I suppose it can be confusing. :-) We possibly need to start calling these pieces of code SSE or AVX rather than just vector, since for some we may end up with multiple vector versions.

/Bruce

> 
> >
> > Regards,
> > /Bruce
> >
> >>
> >> Regards,
> >>
> >> Zoltan
> >>
> >> On 22/07/15 10:13, Bruce Richardson wrote:
> >>> The check for split packets to be reassembled in the vector ixgbe
> >>> PMD was incorrectly only checking the first 16 elements of the array
> >>> instead of all 32. This is fixed by changing the uint32_t values to
> >>> be uint64_t instead.
> >>>
> >>> Fixes: cf4b4708a88a ("ixgbe: improve slow-path perf with vector
> >>> scattered Rx")
> >>>
> >>> Reported-by: Zoltan Kiss <zoltan.kiss@linaro.org>
> >>> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> >>>
> >>> ---
> >>> V2: Rename variable from split_fl32 to split_fl64 to match type
> change.
> >>> ---
> >>>   drivers/net/ixgbe/ixgbe_rxtx_vec.c | 6 +++---
> >>>   1 file changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> >>> b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> >>> index d3ac74a..f2052c6 100644
> >>> --- a/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> >>> +++ b/drivers/net/ixgbe/ixgbe_rxtx_vec.c
> >>> @@ -549,10 +549,10 @@ ixgbe_recv_scattered_pkts_vec(void *rx_queue,
> struct rte_mbuf **rx_pkts,
> >>>   		return 0;
> >>>
> >>>   	/* happy day case, full burst + no packets to be joined */
> >>> -	const uint32_t *split_fl32 = (uint32_t *)split_flags;
> >>> +	const uint64_t *split_fl64 = (uint64_t *)split_flags;
> >>>   	if (rxq->pkt_first_seg == NULL &&
> >>> -			split_fl32[0] == 0 && split_fl32[1] == 0 &&
> >>> -			split_fl32[2] == 0 && split_fl32[3] == 0)
> >>> +			split_fl64[0] == 0 && split_fl64[1] == 0 &&
> >>> +			split_fl64[2] == 0 && split_fl64[3] == 0)
> >>>   		return nb_bufs;
> >>>
> >>>   	/* reassemble any packets that need reassembly*/
> >>>
  

2015-07-22 13:35, Richardson, Bruce:
> From: Zoltan Kiss [mailto:zoltan.kiss@linaro.org]
> > On 22/07/15 10:59, Bruce Richardson wrote:
> > > The vector PMD always works off a fixed 32 burst size. Any change to
> > > that will lead to many changes in the code, so I don't believe a loop is
> > necessary.
> > 
> > Ok, then I suggest to make a comment around RTE_IXGBE_VPMD_RX_BURST that
> > changing it needs a lot of other changes in the code elsewhere, e.g in
> > this split_flags check.
> > Btw. vPMD was a bit misleading abbreviation for me, it took me a while
> > until I realized 'v' stands for 'vector', not 'virtualization' as in most
> > cases nowadays.
> 
> Good idea. I'll try to submit a patch to add a comment if I get the chance
> - otherwise feel free to do so yourself.

Why not do it in this patch?
Is it not enough related?

> As for the naming, yes, I suppose it can be confusing. :-) We possibly need
> to start calling these pieces of code SSE or AVX rather than just vector,
> since for some we may end up with multiple vector versions.

+1 for SSE/AVX naming
  

> The check for split packets to be reassembled in the vector ixgbe PMD
> was incorrectly only checking the first 16 elements of the array instead
> of all 32. This is fixed by changing the uint32_t values to be uint64_t
> instead.
> 
> Fixes: cf4b4708a88a ("ixgbe: improve slow-path perf with vector scattered Rx")
> 
> Reported-by: Zoltan Kiss <zoltan.kiss@linaro.org>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> ---
> V2: Rename variable from split_fl32 to split_fl64 to match type change.

Applied, thanks