[00/12] rxq q_errors[] statistics fixes

  According to the api, the q_errors[] per queue statistic is for reception
errors not transmit errors.
This is a first cleanup on statistics before looking at oerrors.
  

On 3/4/2019 11:18 AM, David Marchand wrote:
> According to the api, the q_errors[] per queue statistic is for reception
> errors not transmit errors.
> This is a first cleanup on statistics before looking at oerrors.
> 

Yes, the patchset looks aligned with the API documentation [1].

What can be the solution after cleanup? We can merge this cleanup and solution
next to each-other to not leave a gap?
1- Different variables for Rx and Tx errors?
2- Combine Rx & Tx into this single variable?

It can be good to find a solution because new PMDs doing same mistake because of
copy/paste...

[1]
https://git.dpdk.org/dpdk/tree/lib/librte_ethdev/rte_ethdev.h?h=v19.02#n263
  

On Mon, Mar 11, 2019 at 6:22 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 3/4/2019 11:18 AM, David Marchand wrote:
> > According to the api, the q_errors[] per queue statistic is for reception
> > errors not transmit errors.
> > This is a first cleanup on statistics before looking at oerrors.
> >
>
> Yes, the patchset looks aligned with the API documentation [1].
>
> What can be the solution after cleanup? We can merge this cleanup and
> solution
> next to each-other to not leave a gap?
> 1- Different variables for Rx and Tx errors?
> 2- Combine Rx & Tx into this single variable?
>
> It can be good to find a solution because new PMDs doing same mistake
> because of
> copy/paste...
>

Might not be feasible but how about we could introduce an internal stats
structure containing the needed field for tx.
pmd would use it but ethdev would translate it to the current exposed api
rte_eth_dev_stats_get ?
The additional field would be formatted by ethdev to be provided through
the xstats api.
  

On Mon, Mar 11, 2019 at 7:09 PM David Marchand <david.marchand@redhat.com>
wrote:

> On Mon, Mar 11, 2019 at 6:22 PM Ferruh Yigit <ferruh.yigit@intel.com>
> wrote:
>
>> On 3/4/2019 11:18 AM, David Marchand wrote:
>> > According to the api, the q_errors[] per queue statistic is for
>> reception
>> > errors not transmit errors.
>> > This is a first cleanup on statistics before looking at oerrors.
>> >
>>
>> Yes, the patchset looks aligned with the API documentation [1].
>>
>> What can be the solution after cleanup? We can merge this cleanup and
>> solution
>> next to each-other to not leave a gap?
>> 1- Different variables for Rx and Tx errors?
>> 2- Combine Rx & Tx into this single variable?
>>
>> It can be good to find a solution because new PMDs doing same mistake
>> because of
>> copy/paste...
>>
>
> Might not be feasible but how about we could introduce an internal stats
> structure containing the needed field for tx.
> pmd would use it but ethdev would translate it to the current exposed api
> rte_eth_dev_stats_get ?
> The additional field would be formatted by ethdev to be provided through
> the xstats api.
>

Sending RFC patches to have something to discuss on.
  

11/03/2019 18:22, Ferruh Yigit:
> On 3/4/2019 11:18 AM, David Marchand wrote:
> > According to the api, the q_errors[] per queue statistic is for reception
> > errors not transmit errors.
> > This is a first cleanup on statistics before looking at oerrors.
> > 
> 
> Yes, the patchset looks aligned with the API documentation [1].
> 
> What can be the solution after cleanup? We can merge this cleanup and solution
> next to each-other to not leave a gap?

I think we should merge those fixes in 19.05-rc2.

It seems there is a lot more work to achieve on stats, so better
to start without waiting for the full picture.
  

On 4/12/2019 4:07 PM, Thomas Monjalon wrote:
> 11/03/2019 18:22, Ferruh Yigit:
>> On 3/4/2019 11:18 AM, David Marchand wrote:
>>> According to the api, the q_errors[] per queue statistic is for reception
>>> errors not transmit errors.
>>> This is a first cleanup on statistics before looking at oerrors.
>>>
>>
>> Yes, the patchset looks aligned with the API documentation [1].
>>
>> What can be the solution after cleanup? We can merge this cleanup and solution
>> next to each-other to not leave a gap?
> 
> I think we should merge those fixes in 19.05-rc2.
> 
> It seems there is a lot more work to achieve on stats, so better
> to start without waiting for the full picture.
> 

The problem is "q_errors" is available only for Rx queues, and David's patch is
preventing drivers to put Tx error stats into "q_errors" field.

But it is clear that there is a need for a field for Tx queues errors. David has
another patch to using xstats for this. But I believe xstats is making solution
confusing, and now approach is unbalanced for Rx and Tx queues.

I am for adding a new field for Tx queues "q_errors", and this will make getting
stats and David's patch very simple.

The problem with the new fields is it breaks the ABI, but we already increased
the ABIVER for ethdev this release, I believe this is very good timing for this fix.
  

12/04/2019 17:38, Ferruh Yigit:
> On 4/12/2019 4:07 PM, Thomas Monjalon wrote:
> > 11/03/2019 18:22, Ferruh Yigit:
> >> On 3/4/2019 11:18 AM, David Marchand wrote:
> >>> According to the api, the q_errors[] per queue statistic is for reception
> >>> errors not transmit errors.
> >>> This is a first cleanup on statistics before looking at oerrors.
> >>>
> >>
> >> Yes, the patchset looks aligned with the API documentation [1].
> >>
> >> What can be the solution after cleanup? We can merge this cleanup and solution
> >> next to each-other to not leave a gap?
> > 
> > I think we should merge those fixes in 19.05-rc2.
> > 
> > It seems there is a lot more work to achieve on stats, so better
> > to start without waiting for the full picture.
> > 
> 
> The problem is "q_errors" is available only for Rx queues, and David's patch is
> preventing drivers to put Tx error stats into "q_errors" field.
> 
> But it is clear that there is a need for a field for Tx queues errors. David has
> another patch to using xstats for this. But I believe xstats is making solution
> confusing, and now approach is unbalanced for Rx and Tx queues.
> 
> I am for adding a new field for Tx queues "q_errors", and this will make getting
> stats and David's patch very simple.
> 
> The problem with the new fields is it breaks the ABI, but we already increased
> the ABIVER for ethdev this release, I believe this is very good timing for this fix.

If changing the stats API, we should increase the number of stats per queue:
	#define RTE_ETHDEV_QUEUE_STAT_CNTRS 16
What about 128 queues per port? 256?
  

On 4/12/2019 4:45 PM, Thomas Monjalon wrote:
> 12/04/2019 17:38, Ferruh Yigit:
>> On 4/12/2019 4:07 PM, Thomas Monjalon wrote:
>>> 11/03/2019 18:22, Ferruh Yigit:
>>>> On 3/4/2019 11:18 AM, David Marchand wrote:
>>>>> According to the api, the q_errors[] per queue statistic is for reception
>>>>> errors not transmit errors.
>>>>> This is a first cleanup on statistics before looking at oerrors.
>>>>>
>>>>
>>>> Yes, the patchset looks aligned with the API documentation [1].
>>>>
>>>> What can be the solution after cleanup? We can merge this cleanup and solution
>>>> next to each-other to not leave a gap?
>>>
>>> I think we should merge those fixes in 19.05-rc2.
>>>
>>> It seems there is a lot more work to achieve on stats, so better
>>> to start without waiting for the full picture.
>>>
>>
>> The problem is "q_errors" is available only for Rx queues, and David's patch is
>> preventing drivers to put Tx error stats into "q_errors" field.
>>
>> But it is clear that there is a need for a field for Tx queues errors. David has
>> another patch to using xstats for this. But I believe xstats is making solution
>> confusing, and now approach is unbalanced for Rx and Tx queues.
>>
>> I am for adding a new field for Tx queues "q_errors", and this will make getting
>> stats and David's patch very simple.
>>
>> The problem with the new fields is it breaks the ABI, but we already increased
>> the ABIVER for ethdev this release, I believe this is very good timing for this fix.
> 
> If changing the stats API, we should increase the number of stats per queue:
> 	#define RTE_ETHDEV_QUEUE_STAT_CNTRS 16
> What about 128 queues per port? 256?

We have 5 uint64_t using this [1], it will be 6 if we add new one.
So having 256 queues, will cost 12K memory, this is not a big number.

Is there any other concern of having large array other than possible memory waste?

[1]
uint64_t q_ipackets[RTE_ETHDEV_QUEUE_STAT_CNTRS];
uint64_t q_opackets[RTE_ETHDEV_QUEUE_STAT_CNTRS];
uint64_t q_ibytes[RTE_ETHDEV_QUEUE_STAT_CNTRS];
uint64_t q_obytes[RTE_ETHDEV_QUEUE_STAT_CNTRS];
uint64_t q_errors[RTE_ETHDEV_QUEUE_STAT_CNTRS];
  

On 3/4/2019 11:18 AM, David Marchand wrote:
> According to the api, the q_errors[] per queue statistic is for reception
> errors not transmit errors.
> This is a first cleanup on statistics before looking at oerrors.
> 

The fix is correct according the documentation but it was waiting for a solution
to capture the Tx queue errors which we are removing this support from some PMDs
because they were storing this information into wrong field.

Since the affected stats are Tx queue error stats, and all Tx errors still can
be stored on 'oerrors' I am for getting the fix, although we need to figure out
how to store Tx queue error stats.

For series,
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Series applied to dpdk-next-net/master, thanks.