[dpdk-dev] [PATCH] ethdev: fix queue mapping documentation

[dpdk-dev] [PATCH] ethdev: fix queue mapping documentation

[Jerin Jacob]

The RTE_MAX_ETHPORT_QUEUE_STATS_MAPS does not exists, change
to the correct definition(RTE_ETHDEV_QUEUE_STAT_CNTRS)

Fixes: 5de201df8927 ("ethdev: add stats per queue")
Cc: stable@dpdk.org

Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
---
 lib/librte_ethdev/rte_ethdev.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 36e3984ea..375ea24ce 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -2144,7 +2144,7 @@ void rte_eth_xstats_reset(uint16_t port_id);
  * @param stat_idx
  *   The per-queue packet statistics functionality number that the transmit
  *   queue is to be assigned.
- *   The value must be in the range [0, RTE_MAX_ETHPORT_QUEUE_STATS_MAPS - 1].
+ *   The value must be in the range [0, RTE_ETHDEV_QUEUE_STAT_CNTRS - 1].
  * @return
  *   Zero if successful. Non-zero otherwise.
  */
@@ -2164,7 +2164,7 @@ int rte_eth_dev_set_tx_queue_stats_mapping(uint16_t port_id,
  * @param stat_idx
  *   The per-queue packet statistics functionality number that the receive
  *   queue is to be assigned.
- *   The value must be in the range [0, RTE_MAX_ETHPORT_QUEUE_STATS_MAPS - 1].
+ *   The value must be in the range [0, RTE_ETHDEV_QUEUE_STAT_CNTRS - 1].
  * @return
  *   Zero if successful. Non-zero otherwise.
  */
-- 
2.18.0

Re: [dpdk-dev] [PATCH] ethdev: fix queue mapping documentation

[Andrew Rybchenko]

On 06/29/2018 12:44 PM, Jerin Jacob wrote:
> The RTE_MAX_ETHPORT_QUEUE_STATS_MAPS does not exists, change
> to the correct definition(RTE_ETHDEV_QUEUE_STAT_CNTRS)
>
> Fixes: 5de201df8927 ("ethdev: add stats per queue")
> Cc: stable@dpdk.org
>
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> ---
>   lib/librte_ethdev/rte_ethdev.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 36e3984ea..375ea24ce 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -2144,7 +2144,7 @@ void rte_eth_xstats_reset(uint16_t port_id);
>    * @param stat_idx
>    *   The per-queue packet statistics functionality number that the transmit
>    *   queue is to be assigned.
> - *   The value must be in the range [0, RTE_MAX_ETHPORT_QUEUE_STATS_MAPS - 1].
> + *   The value must be in the range [0, RTE_ETHDEV_QUEUE_STAT_CNTRS - 1].
>    * @return
>    *   Zero if successful. Non-zero otherwise.
>    */
> @@ -2164,7 +2164,7 @@ int rte_eth_dev_set_tx_queue_stats_mapping(uint16_t port_id,
>    * @param stat_idx
>    *   The per-queue packet statistics functionality number that the receive
>    *   queue is to be assigned.
> - *   The value must be in the range [0, RTE_MAX_ETHPORT_QUEUE_STATS_MAPS - 1].
> + *   The value must be in the range [0, RTE_ETHDEV_QUEUE_STAT_CNTRS - 1].
>    * @return
>    *   Zero if successful. Non-zero otherwise.
>    */

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

Re: [dpdk-dev] [PATCH] ethdev: fix queue mapping documentation

[Ferruh Yigit]

On 6/29/2018 4:16 PM, Andrew Rybchenko wrote:
> On 06/29/2018 12:44 PM, Jerin Jacob wrote:
>> The RTE_MAX_ETHPORT_QUEUE_STATS_MAPS does not exists, change
>> to the correct definition(RTE_ETHDEV_QUEUE_STAT_CNTRS)
>>
>> Fixes: 5de201df8927 ("ethdev: add stats per queue")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>
> Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

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

Re: [dpdk-dev] [PATCH] ethdev: fix queue mapping documentation

[Ferruh Yigit]

On 6/29/2018 10:44 AM, Jerin Jacob wrote:
> The RTE_MAX_ETHPORT_QUEUE_STATS_MAPS does not exists, change
> to the correct definition(RTE_ETHDEV_QUEUE_STAT_CNTRS)
> 
> Fixes: 5de201df8927 ("ethdev: add stats per queue")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 36e3984ea..375ea24ce 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -2144,7 +2144,7 @@ void rte_eth_xstats_reset(uint16_t port_id);
>   * @param stat_idx
>   *   The per-queue packet statistics functionality number that the transmit
>   *   queue is to be assigned.
> - *   The value must be in the range [0, RTE_MAX_ETHPORT_QUEUE_STATS_MAPS - 1].
> + *   The value must be in the range [0, RTE_ETHDEV_QUEUE_STAT_CNTRS - 1].

Yes RTE_MAX_ETHPORT_QUEUE_STATS_MAPS doesn't exits and comment is wrong, but
RTE_ETHDEV_QUEUE_STAT_CNTRS also slightly not correct.

I think how testpmd uses it increase the confusion.

In ixgbe there is no stats registers per queue, 128 queues are represented by 16
register set. stat_idx here is the index of that 16 registers. You map queue to
stats requester to get queue stats.

Also there is RTE_ETHDEV_QUEUE_STAT_CNTRS config in the ethdev API, which is the
hardcoded size of the queue stats, its default value is 16. This limits number
of the queues we can get stats from but saves allocated space. (Why not dynamic?)

You can increase the RTE_ETHDEV_QUEUE_STAT_CNTRS to the max supported number of
queue and ethdev code will be all valid. But "stat_idx" can't go beyond 16 (for
ixgbe) because it is hardware limitation and it may change from hw to hw.

Also technically it should be possible to reduce RTE_ETHDEV_QUEUE_STAT_CNTRS to
a low number, like 2, but in ixgbe map two queues into stat registers 14 & 15
and display those two set as queue stat 0 and 1. It seems current implementation
prevents this and forces the queues mapped should be less than
RTE_ETHDEV_QUEUE_STAT_CNTRS. Overall it seems there is a mixed used of
RTE_ETHDEV_QUEUE_STAT_CNTRS and stats queue index values, I assume because both
are same values.

I suggest updating it as:
"
The value must be in the range:
 [0 - MIN(HW Stat Registers Size, RTE_ETHDEV_QUEUE_STAT_CNTRS) - 1]
"

>   * @return
>   *   Zero if successful. Non-zero otherwise.
>   */
> @@ -2164,7 +2164,7 @@ int rte_eth_dev_set_tx_queue_stats_mapping(uint16_t port_id,
>   * @param stat_idx
>   *   The per-queue packet statistics functionality number that the receive
>   *   queue is to be assigned.
> - *   The value must be in the range [0, RTE_MAX_ETHPORT_QUEUE_STATS_MAPS - 1].
> + *   The value must be in the range [0, RTE_ETHDEV_QUEUE_STAT_CNTRS - 1].
>   * @return
>   *   Zero if successful. Non-zero otherwise.
>   */
> 

Re: [dpdk-dev] [PATCH] ethdev: fix queue mapping documentation

[Andrew Rybchenko]

On 07/02/2018 06:08 PM, Ferruh Yigit wrote:
> On 6/29/2018 10:44 AM, Jerin Jacob wrote:
>> The RTE_MAX_ETHPORT_QUEUE_STATS_MAPS does not exists, change
>> to the correct definition(RTE_ETHDEV_QUEUE_STAT_CNTRS)
>>
>> Fixes: 5de201df8927 ("ethdev: add stats per queue")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>> ---
>>   lib/librte_ethdev/rte_ethdev.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>> index 36e3984ea..375ea24ce 100644
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> @@ -2144,7 +2144,7 @@ void rte_eth_xstats_reset(uint16_t port_id);
>>    * @param stat_idx
>>    *   The per-queue packet statistics functionality number that the transmit
>>    *   queue is to be assigned.
>> - *   The value must be in the range [0, RTE_MAX_ETHPORT_QUEUE_STATS_MAPS - 1].
>> + *   The value must be in the range [0, RTE_ETHDEV_QUEUE_STAT_CNTRS - 1].
> Yes RTE_MAX_ETHPORT_QUEUE_STATS_MAPS doesn't exits and comment is wrong, but
> RTE_ETHDEV_QUEUE_STAT_CNTRS also slightly not correct.
>
> I think how testpmd uses it increase the confusion.
>
> In ixgbe there is no stats registers per queue, 128 queues are represented by 16
> register set. stat_idx here is the index of that 16 registers. You map queue to
> stats requester to get queue stats.
>
> Also there is RTE_ETHDEV_QUEUE_STAT_CNTRS config in the ethdev API, which is the
> hardcoded size of the queue stats, its default value is 16. This limits number
> of the queues we can get stats from but saves allocated space. (Why not dynamic?)
>
> You can increase the RTE_ETHDEV_QUEUE_STAT_CNTRS to the max supported number of
> queue and ethdev code will be all valid. But "stat_idx" can't go beyond 16 (for
> ixgbe) because it is hardware limitation and it may change from hw to hw.
>
> Also technically it should be possible to reduce RTE_ETHDEV_QUEUE_STAT_CNTRS to
> a low number, like 2, but in ixgbe map two queues into stat registers 14 & 15
> and display those two set as queue stat 0 and 1. It seems current implementation
> prevents this and forces the queues mapped should be less than
> RTE_ETHDEV_QUEUE_STAT_CNTRS. Overall it seems there is a mixed used of
> RTE_ETHDEV_QUEUE_STAT_CNTRS and stats queue index values, I assume because both
> are same values.
>
> I suggest updating it as:
> "
> The value must be in the range:
>   [0 - MIN(HW Stat Registers Size, RTE_ETHDEV_QUEUE_STAT_CNTRS) - 1]
> "

Technically I think it is not a problem to specify more than HW supports.
The function should simply return error. RTE_ETHDEV_QUEUE_STAT_CNTRS is
a hard limit which should be checked by ethdev.
The reasonable next question is how to find out what is the maximum for 
PMD/HW.
I think it deserves entry in dev_info. May be not now.
"HW Stats Registers size" is too HW specific. It could be not HW, but 
the PMD
limitation and limits for Rx and Tx could be different. So, may be 
something like:
"Device max per Tx queue stats"

>>    * @return
>>    *   Zero if successful. Non-zero otherwise.
>>    */
>> @@ -2164,7 +2164,7 @@ int rte_eth_dev_set_tx_queue_stats_mapping(uint16_t port_id,
>>    * @param stat_idx
>>    *   The per-queue packet statistics functionality number that the receive
>>    *   queue is to be assigned.
>> - *   The value must be in the range [0, RTE_MAX_ETHPORT_QUEUE_STATS_MAPS - 1].
>> + *   The value must be in the range [0, RTE_ETHDEV_QUEUE_STAT_CNTRS - 1].
>>    * @return
>>    *   Zero if successful. Non-zero otherwise.
>>    */
>>

Re: [dpdk-dev] [PATCH] ethdev: fix queue mapping documentation

[Ferruh Yigit]

On 7/2/2018 4:32 PM, Andrew Rybchenko wrote:
> On 07/02/2018 06:08 PM, Ferruh Yigit wrote:
>> On 6/29/2018 10:44 AM, Jerin Jacob wrote:
>>> The RTE_MAX_ETHPORT_QUEUE_STATS_MAPS does not exists, change
>>> to the correct definition(RTE_ETHDEV_QUEUE_STAT_CNTRS)
>>>
>>> Fixes: 5de201df8927 ("ethdev: add stats per queue")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>> ---
>>>  lib/librte_ethdev/rte_ethdev.h | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>>> index 36e3984ea..375ea24ce 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>> @@ -2144,7 +2144,7 @@ void rte_eth_xstats_reset(uint16_t port_id);
>>>   * @param stat_idx
>>>   *   The per-queue packet statistics functionality number that the transmit
>>>   *   queue is to be assigned.
>>> - *   The value must be in the range [0, RTE_MAX_ETHPORT_QUEUE_STATS_MAPS - 1].
>>> + *   The value must be in the range [0, RTE_ETHDEV_QUEUE_STAT_CNTRS - 1].
>> Yes RTE_MAX_ETHPORT_QUEUE_STATS_MAPS doesn't exits and comment is wrong, but
>> RTE_ETHDEV_QUEUE_STAT_CNTRS also slightly not correct.
>>
>> I think how testpmd uses it increase the confusion.
>>
>> In ixgbe there is no stats registers per queue, 128 queues are represented by 16
>> register set. stat_idx here is the index of that 16 registers. You map queue to
>> stats requester to get queue stats.
>>
>> Also there is RTE_ETHDEV_QUEUE_STAT_CNTRS config in the ethdev API, which is the
>> hardcoded size of the queue stats, its default value is 16. This limits number
>> of the queues we can get stats from but saves allocated space. (Why not dynamic?)
>>
>> You can increase the RTE_ETHDEV_QUEUE_STAT_CNTRS to the max supported number of
>> queue and ethdev code will be all valid. But "stat_idx" can't go beyond 16 (for
>> ixgbe) because it is hardware limitation and it may change from hw to hw.
>>
>> Also technically it should be possible to reduce RTE_ETHDEV_QUEUE_STAT_CNTRS to
>> a low number, like 2, but in ixgbe map two queues into stat registers 14 & 15
>> and display those two set as queue stat 0 and 1. It seems current implementation
>> prevents this and forces the queues mapped should be less than
>> RTE_ETHDEV_QUEUE_STAT_CNTRS. Overall it seems there is a mixed used of
>> RTE_ETHDEV_QUEUE_STAT_CNTRS and stats queue index values, I assume because both
>> are same values.
>>
>> I suggest updating it as:
>> "
>> The value must be in the range:
>>  [0 - MIN(HW Stat Registers Size, RTE_ETHDEV_QUEUE_STAT_CNTRS) - 1]
>> "
> 
> Technically I think it is not a problem to specify more than HW supports.
> The function should simply return error.  RTE_ETHDEV_QUEUE_STAT_CNTRS is
> a hard limit which should be checked by ethdev.
> The reasonable next question is how to find out what is the maximum for PMD/HW.
> I think it deserves entry in dev_info. May be not now.

Yes there is not a way to find out that limit by application, setting
RTE_ETHDEV_QUEUE_STAT_CNTRS to 16 and using it as limit solving the issue for now :)

> "HW Stats Registers size" is too HW specific. It could be not HW, but the PMD
> limitation and limits for Rx and Tx could be different. So, may be something like:
> "Device max per Tx queue stats"
> 
>>>   * @return
>>>   *   Zero if successful. Non-zero otherwise.
>>>   */
>>> @@ -2164,7 +2164,7 @@ int rte_eth_dev_set_tx_queue_stats_mapping(uint16_t port_id,
>>>   * @param stat_idx
>>>   *   The per-queue packet statistics functionality number that the receive
>>>   *   queue is to be assigned.
>>> - *   The value must be in the range [0, RTE_MAX_ETHPORT_QUEUE_STATS_MAPS - 1].
>>> + *   The value must be in the range [0, RTE_ETHDEV_QUEUE_STAT_CNTRS - 1].
>>>   * @return
>>>   *   Zero if successful. Non-zero otherwise.
>>>   */
>>>
> 

Re: [dpdk-dev] [PATCH] ethdev: fix queue mapping documentation

[Jerin Jacob]

-----Original Message-----
> Date: Mon, 2 Jul 2018 16:45:33 +0100
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> To: Andrew Rybchenko <arybchenko@solarflare.com>, Jerin Jacob
>  <jerin.jacob@caviumnetworks.com>, dev@dpdk.org
> CC: thomas@monjalon.net, stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] ethdev: fix queue mapping documentation
> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>  Thunderbird/52.8.0
> 
> 
> On 7/2/2018 4:32 PM, Andrew Rybchenko wrote:
> > On 07/02/2018 06:08 PM, Ferruh Yigit wrote:
> >> On 6/29/2018 10:44 AM, Jerin Jacob wrote:
> >>> The RTE_MAX_ETHPORT_QUEUE_STATS_MAPS does not exists, change
> >>> to the correct definition(RTE_ETHDEV_QUEUE_STAT_CNTRS)
> >>>
> >>> Fixes: 5de201df8927 ("ethdev: add stats per queue")
> >>> Cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> >>> ---
> >>>  lib/librte_ethdev/rte_ethdev.h | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> >>> index 36e3984ea..375ea24ce 100644
> >>> --- a/lib/librte_ethdev/rte_ethdev.h
> >>> +++ b/lib/librte_ethdev/rte_ethdev.h
> >>> @@ -2144,7 +2144,7 @@ void rte_eth_xstats_reset(uint16_t port_id);
> >>>   * @param stat_idx
> >>>   *   The per-queue packet statistics functionality number that the transmit
> >>>   *   queue is to be assigned.
> >>> - *   The value must be in the range [0, RTE_MAX_ETHPORT_QUEUE_STATS_MAPS - 1].
> >>> + *   The value must be in the range [0, RTE_ETHDEV_QUEUE_STAT_CNTRS - 1].
> >> Yes RTE_MAX_ETHPORT_QUEUE_STATS_MAPS doesn't exits and comment is wrong, but
> >> RTE_ETHDEV_QUEUE_STAT_CNTRS also slightly not correct.
> >>
> >> I think how testpmd uses it increase the confusion.
> >>
> >> In ixgbe there is no stats registers per queue, 128 queues are represented by 16
> >> register set. stat_idx here is the index of that 16 registers. You map queue to
> >> stats requester to get queue stats.
> >>
> >> Also there is RTE_ETHDEV_QUEUE_STAT_CNTRS config in the ethdev API, which is the
> >> hardcoded size of the queue stats, its default value is 16. This limits number
> >> of the queues we can get stats from but saves allocated space. (Why not dynamic?)
> >>
> >> You can increase the RTE_ETHDEV_QUEUE_STAT_CNTRS to the max supported number of
> >> queue and ethdev code will be all valid. But "stat_idx" can't go beyond 16 (for
> >> ixgbe) because it is hardware limitation and it may change from hw to hw.
> >>
> >> Also technically it should be possible to reduce RTE_ETHDEV_QUEUE_STAT_CNTRS to
> >> a low number, like 2, but in ixgbe map two queues into stat registers 14 & 15
> >> and display those two set as queue stat 0 and 1. It seems current implementation
> >> prevents this and forces the queues mapped should be less than
> >> RTE_ETHDEV_QUEUE_STAT_CNTRS. Overall it seems there is a mixed used of
> >> RTE_ETHDEV_QUEUE_STAT_CNTRS and stats queue index values, I assume because both
> >> are same values.
> >>
> >> I suggest updating it as:
> >> "
> >> The value must be in the range:
> >>  [0 - MIN(HW Stat Registers Size, RTE_ETHDEV_QUEUE_STAT_CNTRS) - 1]
> >> "
> >
> > Technically I think it is not a problem to specify more than HW supports.
> > The function should simply return error.  RTE_ETHDEV_QUEUE_STAT_CNTRS is
> > a hard limit which should be checked by ethdev.
> > The reasonable next question is how to find out what is the maximum for PMD/HW.
> > I think it deserves entry in dev_info. May be not now.
> 
> Yes there is not a way to find out that limit by application, setting
> RTE_ETHDEV_QUEUE_STAT_CNTRS to 16 and using it as limit solving the issue for now :)


If I understand it correctly, in the documentation, we are specify the
limits to avoid the segfault etc and if the specific PMD does not
support the range up to RTE_ETHDEV_QUEUE_STAT_CNTRS, It can simply return
error which makes the documentation semantically correct.

Considering the above point, I think this patch is correct considering
there is no way currently to detect the limit supported by PMD. So, 
1) Should we keep the patch as is?

--
The value must be in the range [0, RTE_ETHDEV_QUEUE_STAT_CNTRS - 1].
--

OR

2) Change to

--
The value must be in the range:
[0 - MIN(Device max per Tx queue stats, RTE_ETHDEV_QUEUE_STAT_CNTRS) - 1]
--

I prefer option 1. But I am okay send v2 if any ethdev maintainers prefer
option 2.

Let me know.

Re: [dpdk-dev] [PATCH] ethdev: fix queue mapping documentation

[Andrew Rybchenko]

On 10.07.2018 09:20, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Mon, 2 Jul 2018 16:45:33 +0100
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> To: Andrew Rybchenko <arybchenko@solarflare.com>, Jerin Jacob
>>   <jerin.jacob@caviumnetworks.com>, dev@dpdk.org
>> CC: thomas@monjalon.net, stable@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] ethdev: fix queue mapping documentation
>> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>>   Thunderbird/52.8.0
>>
>>
>> On 7/2/2018 4:32 PM, Andrew Rybchenko wrote:
>>> On 07/02/2018 06:08 PM, Ferruh Yigit wrote:
>>>> On 6/29/2018 10:44 AM, Jerin Jacob wrote:
>>>>> The RTE_MAX_ETHPORT_QUEUE_STATS_MAPS does not exists, change
>>>>> to the correct definition(RTE_ETHDEV_QUEUE_STAT_CNTRS)
>>>>>
>>>>> Fixes: 5de201df8927 ("ethdev: add stats per queue")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>>>> ---
>>>>>   lib/librte_ethdev/rte_ethdev.h | 4 ++--
>>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>>>>> index 36e3984ea..375ea24ce 100644
>>>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>>>> @@ -2144,7 +2144,7 @@ void rte_eth_xstats_reset(uint16_t port_id);
>>>>>    * @param stat_idx
>>>>>    *   The per-queue packet statistics functionality number that the transmit
>>>>>    *   queue is to be assigned.
>>>>> - *   The value must be in the range [0, RTE_MAX_ETHPORT_QUEUE_STATS_MAPS - 1].
>>>>> + *   The value must be in the range [0, RTE_ETHDEV_QUEUE_STAT_CNTRS - 1].
>>>> Yes RTE_MAX_ETHPORT_QUEUE_STATS_MAPS doesn't exits and comment is wrong, but
>>>> RTE_ETHDEV_QUEUE_STAT_CNTRS also slightly not correct.
>>>>
>>>> I think how testpmd uses it increase the confusion.
>>>>
>>>> In ixgbe there is no stats registers per queue, 128 queues are represented by 16
>>>> register set. stat_idx here is the index of that 16 registers. You map queue to
>>>> stats requester to get queue stats.
>>>>
>>>> Also there is RTE_ETHDEV_QUEUE_STAT_CNTRS config in the ethdev API, which is the
>>>> hardcoded size of the queue stats, its default value is 16. This limits number
>>>> of the queues we can get stats from but saves allocated space. (Why not dynamic?)
>>>>
>>>> You can increase the RTE_ETHDEV_QUEUE_STAT_CNTRS to the max supported number of
>>>> queue and ethdev code will be all valid. But "stat_idx" can't go beyond 16 (for
>>>> ixgbe) because it is hardware limitation and it may change from hw to hw.
>>>>
>>>> Also technically it should be possible to reduce RTE_ETHDEV_QUEUE_STAT_CNTRS to
>>>> a low number, like 2, but in ixgbe map two queues into stat registers 14 & 15
>>>> and display those two set as queue stat 0 and 1. It seems current implementation
>>>> prevents this and forces the queues mapped should be less than
>>>> RTE_ETHDEV_QUEUE_STAT_CNTRS. Overall it seems there is a mixed used of
>>>> RTE_ETHDEV_QUEUE_STAT_CNTRS and stats queue index values, I assume because both
>>>> are same values.
>>>>
>>>> I suggest updating it as:
>>>> "
>>>> The value must be in the range:
>>>>   [0 - MIN(HW Stat Registers Size, RTE_ETHDEV_QUEUE_STAT_CNTRS) - 1]
>>>> "
>>> Technically I think it is not a problem to specify more than HW supports.
>>> The function should simply return error.  RTE_ETHDEV_QUEUE_STAT_CNTRS is
>>> a hard limit which should be checked by ethdev.
>>> The reasonable next question is how to find out what is the maximum for PMD/HW.
>>> I think it deserves entry in dev_info. May be not now.
>> Yes there is not a way to find out that limit by application, setting
>> RTE_ETHDEV_QUEUE_STAT_CNTRS to 16 and using it as limit solving the issue for now :)
>
> If I understand it correctly, in the documentation, we are specify the
> limits to avoid the segfault etc and if the specific PMD does not
> support the range up to RTE_ETHDEV_QUEUE_STAT_CNTRS, It can simply return
> error which makes the documentation semantically correct.
>
> Considering the above point, I think this patch is correct considering
> there is no way currently to detect the limit supported by PMD. So,
> 1) Should we keep the patch as is?
>
> --
> The value must be in the range [0, RTE_ETHDEV_QUEUE_STAT_CNTRS - 1].
> --
>
> OR
>
> 2) Change to
>
> --
> The value must be in the range:
> [0 - MIN(Device max per Tx queue stats, RTE_ETHDEV_QUEUE_STAT_CNTRS) - 1]
> --
>
> I prefer option 1. But I am okay send v2 if any ethdev maintainers prefer
> option 2.
>
> Let me know.

I would prefer option 1 as well since right now we have no way to
advertise/find out "Device max per Tx queue stats".

Andrew.

Re: [dpdk-dev] [PATCH] ethdev: fix queue mapping documentation

[Ferruh Yigit]

On 7/10/2018 8:06 AM, Andrew Rybchenko wrote:
> On 10.07.2018 09:20, Jerin Jacob wrote:
>> -----Original Message-----
>>> Date: Mon, 2 Jul 2018 16:45:33 +0100
>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>>> To: Andrew Rybchenko <arybchenko@solarflare.com>, Jerin Jacob
>>>   <jerin.jacob@caviumnetworks.com>, dev@dpdk.org
>>> CC: thomas@monjalon.net, stable@dpdk.org
>>> Subject: Re: [dpdk-dev] [PATCH] ethdev: fix queue mapping documentation
>>> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
>>>   Thunderbird/52.8.0
>>>
>>>
>>> On 7/2/2018 4:32 PM, Andrew Rybchenko wrote:
>>>> On 07/02/2018 06:08 PM, Ferruh Yigit wrote:
>>>>> On 6/29/2018 10:44 AM, Jerin Jacob wrote:
>>>>>> The RTE_MAX_ETHPORT_QUEUE_STATS_MAPS does not exists, change
>>>>>> to the correct definition(RTE_ETHDEV_QUEUE_STAT_CNTRS)
>>>>>>
>>>>>> Fixes: 5de201df8927 ("ethdev: add stats per queue")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>>>>> ---
>>>>>>   lib/librte_ethdev/rte_ethdev.h | 4 ++--
>>>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>>>>>> index 36e3984ea..375ea24ce 100644
>>>>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>>>>> @@ -2144,7 +2144,7 @@ void rte_eth_xstats_reset(uint16_t port_id);
>>>>>>    * @param stat_idx
>>>>>>    *   The per-queue packet statistics functionality number that the transmit
>>>>>>    *   queue is to be assigned.
>>>>>> - *   The value must be in the range [0, RTE_MAX_ETHPORT_QUEUE_STATS_MAPS - 1].
>>>>>> + *   The value must be in the range [0, RTE_ETHDEV_QUEUE_STAT_CNTRS - 1].
>>>>> Yes RTE_MAX_ETHPORT_QUEUE_STATS_MAPS doesn't exits and comment is wrong, but
>>>>> RTE_ETHDEV_QUEUE_STAT_CNTRS also slightly not correct.
>>>>>
>>>>> I think how testpmd uses it increase the confusion.
>>>>>
>>>>> In ixgbe there is no stats registers per queue, 128 queues are represented by 16
>>>>> register set. stat_idx here is the index of that 16 registers. You map queue to
>>>>> stats requester to get queue stats.
>>>>>
>>>>> Also there is RTE_ETHDEV_QUEUE_STAT_CNTRS config in the ethdev API, which is the
>>>>> hardcoded size of the queue stats, its default value is 16. This limits number
>>>>> of the queues we can get stats from but saves allocated space. (Why not dynamic?)
>>>>>
>>>>> You can increase the RTE_ETHDEV_QUEUE_STAT_CNTRS to the max supported number of
>>>>> queue and ethdev code will be all valid. But "stat_idx" can't go beyond 16 (for
>>>>> ixgbe) because it is hardware limitation and it may change from hw to hw.
>>>>>
>>>>> Also technically it should be possible to reduce RTE_ETHDEV_QUEUE_STAT_CNTRS to
>>>>> a low number, like 2, but in ixgbe map two queues into stat registers 14 & 15
>>>>> and display those two set as queue stat 0 and 1. It seems current implementation
>>>>> prevents this and forces the queues mapped should be less than
>>>>> RTE_ETHDEV_QUEUE_STAT_CNTRS. Overall it seems there is a mixed used of
>>>>> RTE_ETHDEV_QUEUE_STAT_CNTRS and stats queue index values, I assume because both
>>>>> are same values.
>>>>>
>>>>> I suggest updating it as:
>>>>> "
>>>>> The value must be in the range:
>>>>>   [0 - MIN(HW Stat Registers Size, RTE_ETHDEV_QUEUE_STAT_CNTRS) - 1]
>>>>> "
>>>> Technically I think it is not a problem to specify more than HW supports.
>>>> The function should simply return error.  RTE_ETHDEV_QUEUE_STAT_CNTRS is
>>>> a hard limit which should be checked by ethdev.
>>>> The reasonable next question is how to find out what is the maximum for PMD/HW.
>>>> I think it deserves entry in dev_info. May be not now.
>>> Yes there is not a way to find out that limit by application, setting
>>> RTE_ETHDEV_QUEUE_STAT_CNTRS to 16 and using it as limit solving the issue for now :)
>>
>> If I understand it correctly, in the documentation, we are specify the
>> limits to avoid the segfault etc and if the specific PMD does not
>> support the range up to RTE_ETHDEV_QUEUE_STAT_CNTRS, It can simply return
>> error which makes the documentation semantically correct.
>>
>> Considering the above point, I think this patch is correct considering
>> there is no way currently to detect the limit supported by PMD. So,
>> 1) Should we keep the patch as is?
>>
>> --
>> The value must be in the range [0, RTE_ETHDEV_QUEUE_STAT_CNTRS - 1].
>> --
>>
>> OR
>>
>> 2) Change to
>>
>> --
>> The value must be in the range:
>> [0 - MIN(Device max per Tx queue stats, RTE_ETHDEV_QUEUE_STAT_CNTRS) - 1]
>> --
>>
>> I prefer option 1. But I am okay send v2 if any ethdev maintainers prefer
>> option 2.
>>
>> Let me know.
> 
> I would prefer option 1 as well since right now we have no way to
> advertise/find out "Device max per Tx queue stats".

OK, agreed/convinced on option 1, eventually it will be better than current
comment, so no new version required, thanks.