From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerin Jacob Subject: Re: [PATCH] ethdev: fix queue mapping documentation Date: Tue, 10 Jul 2018 11:50:31 +0530 Message-ID: <20180710062023.GA2600@jerin> References: <20180629094443.26540-1-jerin.jacob@caviumnetworks.com> <22d74419-6842-044c-9c61-7855925bf41b@intel.com> <47d3c4aa-04f7-110b-f889-cfb07fecdfca@solarflare.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Andrew Rybchenko , dev@dpdk.org, thomas@monjalon.net, stable@dpdk.org To: Ferruh Yigit Return-path: Content-Disposition: inline In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" -----Original Message----- > Date: Mon, 2 Jul 2018 16:45:33 +0100 > From: Ferruh Yigit > To: Andrew Rybchenko , Jerin Jacob > , 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 > >>> --- > >>> 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.