From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4D0FAA04BB; Tue, 6 Oct 2020 10:33:32 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id C8F651023; Tue, 6 Oct 2020 10:33:30 +0200 (CEST) Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by dpdk.org (Postfix) with ESMTP id 40148160 for ; Tue, 6 Oct 2020 10:33:28 +0200 (CEST) Received: by mail-wr1-f68.google.com with SMTP id o5so12432375wrn.13 for ; Tue, 06 Oct 2020 01:33:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to :user-agent; bh=Jc2AzhR7oJM0GPW9/4Xi7LBxiFMLOOkgYwwB308eYN0=; b=cm+l9ezNomKq3eBvWyDNzJHufBsHWD9A+DJQR3MD+sdo2H7ki//4t9OIjZp6xe+jw0 X46MLB59d8HJIKVIFg9ewTeEjC2EMykOhut7Duy7q9Ygajg2bD+mUprBrPQqKGBnWE+8 V6hZJ29at3n4CXSWv7LvEHDSlHwzzmCI4JLDgE/Ay4ohogK1TZsrkraQktslvF/ak5nd yQGV30u2MI9X/oLNhsWi8A6Wd410jLCddgn4gcZTuqjFVqlv4zeFmALara5SG9bcvsRs 2QGJ610vxR+1kukyqkWCjK/Lrm3n1BQ1rCOja8qkKAk+8kK7D008OawOU+q5CPKDhDO1 lrzQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=Jc2AzhR7oJM0GPW9/4Xi7LBxiFMLOOkgYwwB308eYN0=; b=L4AZWCF57y86qh4gwW0BJxlWRb4l4LVpu11yi0sUQA+In+RF4xKFBe7/66aeTIfALC 67Ti44zShGfRXZY7ovYYAdnz8DaJ2FHMIntKjr9RCZC7wZpk3BF0Agm0ULk0RTQnw4DN 4a3X0WID5RiwHRB8u1iVGFMyfRRrASiD8wko+Of9DnGtl8tRvswdBumRjLHjuPSyz5jl NpiY8c/YJv71u48clUy3WjuxOhsdBfqiKqBh8YHOxgTi/MuI/B7Ewv8wIARteEYpqSBj peKGkPfROA5jdPKQdW19Z164ovcNEfD3KgSWJYLZ+uTj9FEHIGKhF77gl/YivimE/B1g i6qg== X-Gm-Message-State: AOAM530ZNMMKrYmCwc2A0AGlihfSojwDo0fwgWvgApQPx8N/jPaiWJe9 gPnj7ev3k398WK01XYSwbBxyNA== X-Google-Smtp-Source: ABdhPJwE9dfyTizIYq2y+Hy1/phmXd7gLMCK7EAjspef7J1uosNf968zW0D4u9B+Z2/aEp4dnnKXxw== X-Received: by 2002:adf:a28a:: with SMTP id s10mr3604030wra.300.1601973206867; Tue, 06 Oct 2020 01:33:26 -0700 (PDT) Received: from 6wind.com (2a01cb0c0005a600345636f7e65ed1a0.ipv6.abo.wanadoo.fr. [2a01:cb0c:5:a600:3456:36f7:e65e:d1a0]) by smtp.gmail.com with ESMTPSA id 67sm3056349wmb.31.2020.10.06.01.33.25 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 06 Oct 2020 01:33:25 -0700 (PDT) Date: Tue, 6 Oct 2020 10:33:25 +0200 From: Olivier Matz To: Ferruh Yigit Cc: Stephen Hemminger , Thomas Monjalon , "Min Hu (Connor)" , "techboard@dpdk.org" , bruce.richardson@intel.com, "jerinj@marvell.com" , Ray Kinsella , dev@dpdk.org Message-ID: <20201006083325.GF21395@platinum> References: <1598845317-55956-1-git-send-email-humin29@huawei.com> <32785804.XpyAPG8jY8@thomas> <9656261.dg2788STim@thomas> <20200928084357.3dfab7ab@hermes.local> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Subject: Re: [dpdk-dev] [dpdk-techboard] [PATCH V5 1/2] dpdk: resolve compiling errors for per-queue stats X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi, On Mon, Oct 05, 2020 at 01:23:08PM +0100, Ferruh Yigit wrote: > On 9/28/2020 4:43 PM, Stephen Hemminger wrote: > > On Mon, 28 Sep 2020 17:24:26 +0200 > > Thomas Monjalon wrote: > > > > > 28/09/2020 15:53, Ferruh Yigit: > > > > On 9/28/2020 10:16 AM, Thomas Monjalon wrote: > > > > > 28/09/2020 10:59, Ferruh Yigit: > > > > > > On 9/27/2020 4:16 AM, Min Hu (Connor) wrote: > > > > > > > From: Huisong Li > > > > > > > > > > > > > > Currently, only statistics of rx/tx queues with queue_id less than > > > > > > > RTE_ETHDEV_QUEUE_STAT_CNTRS can be displayed. If there is a certain > > > > > > > application scenario that it needs to use 256 or more than 256 queues > > > > > > > and display all statistics of rx/tx queue. At this moment, we have to > > > > > > > change the macro to be equaled to the queue number. > > > > > > > > > > > > > > However, modifying the macro to be greater than 256 will trigger > > > > > > > many errors and warnings from test-pmd, PMD drivers and librte_ethdev > > > > > > > during compiling dpdk project. But it is possible and permitted that > > > > > > > rx/tx queue number is greater than 256 and all statistics of rx/tx > > > > > > > queue need to be displayed. In addition, the data type of rx/tx queue > > > > > > > number in rte_eth_dev_configure API is 'uint16_t'. So It is unreasonable > > > > > > > to use the 'uint8_t' type for variables that control which per-queue > > > > > > > statistics can be displayed. > > > > > > > > > > The explanation is too much complex and misleading. > > > > > You mean you cannot increase RTE_ETHDEV_QUEUE_STAT_CNTRS > > > > > above 256 because it is an 8-bit type? > > > > > > > > > > [...] > > > > > > > --- a/lib/librte_ethdev/rte_ethdev.h > > > > > > > +++ b/lib/librte_ethdev/rte_ethdev.h > > > > > > > int rte_eth_dev_set_tx_queue_stats_mapping(uint16_t port_id, > > > > > > > - uint16_t tx_queue_id, uint8_t stat_idx); > > > > > > > + uint16_t tx_queue_id, uint16_t stat_idx); > > > > > [...] > > > > > > > int rte_eth_dev_set_rx_queue_stats_mapping(uint16_t port_id, > > > > > > > uint16_t rx_queue_id, > > > > > > > - uint8_t stat_idx); > > > > > > > + uint16_t stat_idx); > > > > > [...] > > > > > > cc'ed tech-board, > > > > > > > > > > > > The patch breaks the ethdev ABI without a deprecation notice from previous > > > > > > release(s). > > > > > > > > > > > > It is mainly a fix to the port_id storage type, which we have updated from > > > > > > uint8_t to uint16_t in past but some seems remained for > > > > > > 'rte_eth_dev_set_tx_queue_stats_mapping()' & > > > > > > 'rte_eth_dev_set_rx_queue_stats_mapping()' APIs. > > > > > > > > > > No, it is not related to the port id, but the number of limited stats. > > > > > > > > Right, it is not related to the port id, it is fixing the storage type for index > > > > used to map the queue stats. > > > > > > Since the ethdev library already heavily breaks the ABI this release, I am for > > > > > > getting this fix, instead of waiting the fix for one more year. > > > > > > > > > > If stats can be managed for more than 256 queues, I think it means > > > > > it is not limited. In this case, we probably don't need the API > > > > > *_queue_stats_mapping which was invented for a limitation of ixgbe. > > > > > > > > > > The problem is probably somewhere else (in testpmd), > > > > > that's why I am against this patch. > > > > > > > > This patch is not to fix queue stats mapping, I agree there are problems related > > > > to it, already shared as comment to this set. > > > > > > > > But this patch is to fix the build errors when 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > > > > needs to set more than 255. Where the build errors seems around the > > > > stats_mapping APIs. > > > > > > It is not said this API is supposed to manage more than 256 queues mapping. > > > In general we should not need this API. > > > I think it is solving the wrong problem. > > > > > > The original API is a band aid for the limited number of statistics counters > > in the Intel IXGBE hardware. It crept into to the DPDK as an API. I would rather > > have per-queue statistics and make ixgbe say "not supported" > > > > The current issue is not directly related to '*_queue_stats_mapping' APIs. > > Problem is not able to set 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255. > User may need to set the 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255, since it is > used to define size of the stats counter. > "uint64_t q_ipackets[RTE_ETHDEV_QUEUE_STAT_CNTRS];" > > When 'RTE_ETHDEV_QUEUE_STAT_CNTRS' > 255, it gives multiple build errors, > the one in the ethdev is like [1]. > > This can be fixed two ways, > a) increase the size of 'stat_idx' storage type to u16 in the > '*_queue_stats_mapping' APIs, this is what this patch does. > b) Fix with a casting in the comparison, without changing the APIs. > > I think both are OK, but is (b) more preferable? I think the patch (a) is ok, knowing that RTE_ETHDEV_QUEUE_STAT_CNTRS is not modified. On the substance, I agree with Thomas that the queue_stats_mapping API should be replaced by xstats. > > > [1] > ../lib/librte_ethdev/rte_ethdev.c: In function ‘set_queue_stats_mapping’: > ../lib/librte_ethdev/rte_ethdev.c:2943:15: warning: comparison is always > false due to limited range of data type [-Wtype-limits] > 2943 | if (stat_idx >= RTE_ETHDEV_QUEUE_STAT_CNTRS) > | ^~