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

[Andrew Rybchenko <arybchenko@solarflare.com>]

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.
>>    */
>>