From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [84.52.114.115]) by dpdk.org (Postfix) with ESMTP id B1B582A5D for ; Wed, 18 Oct 2017 13:10:24 +0200 (CEST) Received: by shelob.oktetlabs.ru (Postfix, from userid 122) id 37D697F74A; Wed, 18 Oct 2017 14:10:24 +0300 (MSK) X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on shelob.oktetlabs.ru X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=ALL_TRUSTED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FAKE_REPLY_C autolearn=unavailable autolearn_force=no version=3.4.0 Received: from oktetlabs.ru (shelob.oktetlabs.ru [192.168.34.2]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id AD91A7F4B0; Wed, 18 Oct 2017 14:10:20 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.9.2 shelob.oktetlabs.ru AD91A7F4B0 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1508325020; bh=0DMmejkyp+dHtCHM5JSLV2a9Si0XoDodw/Ji0OEjzXY=; h=Date:From:To:Cc:Subject; b=WGAgsCw3gImNqxhq7+saZsPX3h0j22z082bUkdBEdgRu/92kFnt1yiv6MQs+X2Zq+ Bj/n1g0w5sVTB85jXzvj/vGcisGMdsRSzlW3S4y6/I1sd7Y50x6FiTia6SO5fpyBlQ NbxYoYyfCwxzaHOYCzE30rTgbHw2ArXtMAm2Yelw= Date: Wed, 18 Oct 2017 14:10:19 +0300 From: Ivan Malov To: Lee Daly , thomas@monjalon.net Cc: dev@dpdk.org, kubax.kozak@intel.com, ferruh.yigit@intel.com Message-ID: <20171018111019.GA5352@oktetlabs.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.23 (2014-03-12) X-Mailman-Approved-At: Thu, 19 Oct 2017 18:19:03 +0200 Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix xstats retrieve by id API 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: , X-List-Received-Date: Wed, 18 Oct 2017 11:10:24 -0000 On 10/12/2017 2:31 PM, Lee Daly wrote: > From: Lee > > Fix xstats functions, rte_eth_xstats_get_names_by_id() > and rte_eth_xstats_get_by_id(), in current implementation > ethdev level reads all xstat values and filters out > the ones requested by the application. This behavior doesn't > benefit from PMD ops and doesn't provide the benefit the > API was created in the first place for. APIs are also unnecessarily > complicated. Both APIs have different returns for the same params. > > In this fix, instead of reading all the stats and finding the > requested value, drivers can provide ops to get selected xstats. > API no longer crashes with certain params, > > rte_eth_get_by_id returned seg fault with > "ids = NULL && values != NULL && n rte_eth_get_names_by_id returned seg fault with > "ids = NULL && values != NULL && n=0" > These now return max number of stats available, matching the other API. > > rte_eth_get_by_id returned seg fault with > "ids != NULL && values = NULL && n This now returns -22,(EINVAL). > > Standardized variable/parameter names between the 2 APIs. > > Overall code complexity reduced. > > Fixes: 79c913a42f0e ("ethdev: retrieve xstats by ID") > Cc: kubax.kozak@intel.com > > Signed-off-by: Lee Daly > Reviewed-by: Ferruh Yigit I have a serious concern regarding the patch. There is a common scenario of rte_eth_xstats_get_names_by_id() usage, and the patch breaks it. Typically, the function is called with 'ids = NULL' and 'xstats_names = NULL' in order to get the number of figures. Then the function is called one more time with appropriate storage for 'xstats_names'. According to the patch, on the former step get_xstats_count() is called to count the figures available. The resulting number counts for PMD statistics + RTE_NB_STATS + some amount of per-queue figures. However, on the latter step the following may take place: > + if (dev->dev_ops->xstats_get_names_by_id != NULL) > + return (*dev->dev_ops->xstats_get_names_by_id)( > + dev, xstats_names, ids, size); This obviously means that in the case when 'xstats_get_names_by_id' is present, it will be called directly, and RTE statistics will not be filled in the storage before the PMD figures. Accordingly, the value returned by the function in this case will count for PMD figures only (i.e. will be less than the value obtained on the first step). This is an obvious malfunction, and it would be desirable to provide a fix for that. I hope for your understanding. -- Best regards, Ivan