Message ID | 1551698315-2611-1-git-send-email-david.marchand@redhat.com (mailing list archive) |
---|---|
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 940CF2C18; Mon, 4 Mar 2019 12:19:00 +0100 (CET) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 39A772BA3 for <dev@dpdk.org>; Mon, 4 Mar 2019 12:18:58 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 255D330821FF for <dev@dpdk.org>; Mon, 4 Mar 2019 11:18:58 +0000 (UTC) Received: from dmarchan.remote.csb (ovpn-204-16.brq.redhat.com [10.40.204.16]) by smtp.corp.redhat.com (Postfix) with ESMTP id 585795DD63 for <dev@dpdk.org>; Mon, 4 Mar 2019 11:18:57 +0000 (UTC) From: David Marchand <david.marchand@redhat.com> To: dev@dpdk.org Date: Mon, 4 Mar 2019 12:18:23 +0100 Message-Id: <1551698315-2611-1-git-send-email-david.marchand@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.47]); Mon, 04 Mar 2019 11:18:58 +0000 (UTC) Subject: [dpdk-dev] [PATCH 00/12] rxq q_errors[] statistics fixes X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Series |
rxq q_errors[] statistics fixes
|
|
Message
David Marchand
March 4, 2019, 11:18 a.m. UTC
According to the api, the q_errors[] per queue statistic is for reception errors not transmit errors. This is a first cleanup on statistics before looking at oerrors.
Comments
On 3/4/2019 11:18 AM, David Marchand wrote: > According to the api, the q_errors[] per queue statistic is for reception > errors not transmit errors. > This is a first cleanup on statistics before looking at oerrors. > Yes, the patchset looks aligned with the API documentation [1]. What can be the solution after cleanup? We can merge this cleanup and solution next to each-other to not leave a gap? 1- Different variables for Rx and Tx errors? 2- Combine Rx & Tx into this single variable? It can be good to find a solution because new PMDs doing same mistake because of copy/paste... [1] https://git.dpdk.org/dpdk/tree/lib/librte_ethdev/rte_ethdev.h?h=v19.02#n263
On Mon, Mar 11, 2019 at 6:22 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote: > On 3/4/2019 11:18 AM, David Marchand wrote: > > According to the api, the q_errors[] per queue statistic is for reception > > errors not transmit errors. > > This is a first cleanup on statistics before looking at oerrors. > > > > Yes, the patchset looks aligned with the API documentation [1]. > > What can be the solution after cleanup? We can merge this cleanup and > solution > next to each-other to not leave a gap? > 1- Different variables for Rx and Tx errors? > 2- Combine Rx & Tx into this single variable? > > It can be good to find a solution because new PMDs doing same mistake > because of > copy/paste... > Might not be feasible but how about we could introduce an internal stats structure containing the needed field for tx. pmd would use it but ethdev would translate it to the current exposed api rte_eth_dev_stats_get ? The additional field would be formatted by ethdev to be provided through the xstats api.
On Mon, Mar 11, 2019 at 7:09 PM David Marchand <david.marchand@redhat.com> wrote: > On Mon, Mar 11, 2019 at 6:22 PM Ferruh Yigit <ferruh.yigit@intel.com> > wrote: > >> On 3/4/2019 11:18 AM, David Marchand wrote: >> > According to the api, the q_errors[] per queue statistic is for >> reception >> > errors not transmit errors. >> > This is a first cleanup on statistics before looking at oerrors. >> > >> >> Yes, the patchset looks aligned with the API documentation [1]. >> >> What can be the solution after cleanup? We can merge this cleanup and >> solution >> next to each-other to not leave a gap? >> 1- Different variables for Rx and Tx errors? >> 2- Combine Rx & Tx into this single variable? >> >> It can be good to find a solution because new PMDs doing same mistake >> because of >> copy/paste... >> > > Might not be feasible but how about we could introduce an internal stats > structure containing the needed field for tx. > pmd would use it but ethdev would translate it to the current exposed api > rte_eth_dev_stats_get ? > The additional field would be formatted by ethdev to be provided through > the xstats api. > Sending RFC patches to have something to discuss on.
11/03/2019 18:22, Ferruh Yigit: > On 3/4/2019 11:18 AM, David Marchand wrote: > > According to the api, the q_errors[] per queue statistic is for reception > > errors not transmit errors. > > This is a first cleanup on statistics before looking at oerrors. > > > > Yes, the patchset looks aligned with the API documentation [1]. > > What can be the solution after cleanup? We can merge this cleanup and solution > next to each-other to not leave a gap? I think we should merge those fixes in 19.05-rc2. It seems there is a lot more work to achieve on stats, so better to start without waiting for the full picture.
On 4/12/2019 4:07 PM, Thomas Monjalon wrote: > 11/03/2019 18:22, Ferruh Yigit: >> On 3/4/2019 11:18 AM, David Marchand wrote: >>> According to the api, the q_errors[] per queue statistic is for reception >>> errors not transmit errors. >>> This is a first cleanup on statistics before looking at oerrors. >>> >> >> Yes, the patchset looks aligned with the API documentation [1]. >> >> What can be the solution after cleanup? We can merge this cleanup and solution >> next to each-other to not leave a gap? > > I think we should merge those fixes in 19.05-rc2. > > It seems there is a lot more work to achieve on stats, so better > to start without waiting for the full picture. > The problem is "q_errors" is available only for Rx queues, and David's patch is preventing drivers to put Tx error stats into "q_errors" field. But it is clear that there is a need for a field for Tx queues errors. David has another patch to using xstats for this. But I believe xstats is making solution confusing, and now approach is unbalanced for Rx and Tx queues. I am for adding a new field for Tx queues "q_errors", and this will make getting stats and David's patch very simple. The problem with the new fields is it breaks the ABI, but we already increased the ABIVER for ethdev this release, I believe this is very good timing for this fix.
12/04/2019 17:38, Ferruh Yigit: > On 4/12/2019 4:07 PM, Thomas Monjalon wrote: > > 11/03/2019 18:22, Ferruh Yigit: > >> On 3/4/2019 11:18 AM, David Marchand wrote: > >>> According to the api, the q_errors[] per queue statistic is for reception > >>> errors not transmit errors. > >>> This is a first cleanup on statistics before looking at oerrors. > >>> > >> > >> Yes, the patchset looks aligned with the API documentation [1]. > >> > >> What can be the solution after cleanup? We can merge this cleanup and solution > >> next to each-other to not leave a gap? > > > > I think we should merge those fixes in 19.05-rc2. > > > > It seems there is a lot more work to achieve on stats, so better > > to start without waiting for the full picture. > > > > The problem is "q_errors" is available only for Rx queues, and David's patch is > preventing drivers to put Tx error stats into "q_errors" field. > > But it is clear that there is a need for a field for Tx queues errors. David has > another patch to using xstats for this. But I believe xstats is making solution > confusing, and now approach is unbalanced for Rx and Tx queues. > > I am for adding a new field for Tx queues "q_errors", and this will make getting > stats and David's patch very simple. > > The problem with the new fields is it breaks the ABI, but we already increased > the ABIVER for ethdev this release, I believe this is very good timing for this fix. If changing the stats API, we should increase the number of stats per queue: #define RTE_ETHDEV_QUEUE_STAT_CNTRS 16 What about 128 queues per port? 256?
On 4/12/2019 4:45 PM, Thomas Monjalon wrote: > 12/04/2019 17:38, Ferruh Yigit: >> On 4/12/2019 4:07 PM, Thomas Monjalon wrote: >>> 11/03/2019 18:22, Ferruh Yigit: >>>> On 3/4/2019 11:18 AM, David Marchand wrote: >>>>> According to the api, the q_errors[] per queue statistic is for reception >>>>> errors not transmit errors. >>>>> This is a first cleanup on statistics before looking at oerrors. >>>>> >>>> >>>> Yes, the patchset looks aligned with the API documentation [1]. >>>> >>>> What can be the solution after cleanup? We can merge this cleanup and solution >>>> next to each-other to not leave a gap? >>> >>> I think we should merge those fixes in 19.05-rc2. >>> >>> It seems there is a lot more work to achieve on stats, so better >>> to start without waiting for the full picture. >>> >> >> The problem is "q_errors" is available only for Rx queues, and David's patch is >> preventing drivers to put Tx error stats into "q_errors" field. >> >> But it is clear that there is a need for a field for Tx queues errors. David has >> another patch to using xstats for this. But I believe xstats is making solution >> confusing, and now approach is unbalanced for Rx and Tx queues. >> >> I am for adding a new field for Tx queues "q_errors", and this will make getting >> stats and David's patch very simple. >> >> The problem with the new fields is it breaks the ABI, but we already increased >> the ABIVER for ethdev this release, I believe this is very good timing for this fix. > > If changing the stats API, we should increase the number of stats per queue: > #define RTE_ETHDEV_QUEUE_STAT_CNTRS 16 > What about 128 queues per port? 256? We have 5 uint64_t using this [1], it will be 6 if we add new one. So having 256 queues, will cost 12K memory, this is not a big number. Is there any other concern of having large array other than possible memory waste? [1] uint64_t q_ipackets[RTE_ETHDEV_QUEUE_STAT_CNTRS]; uint64_t q_opackets[RTE_ETHDEV_QUEUE_STAT_CNTRS]; uint64_t q_ibytes[RTE_ETHDEV_QUEUE_STAT_CNTRS]; uint64_t q_obytes[RTE_ETHDEV_QUEUE_STAT_CNTRS]; uint64_t q_errors[RTE_ETHDEV_QUEUE_STAT_CNTRS];
On 3/4/2019 11:18 AM, David Marchand wrote: > According to the api, the q_errors[] per queue statistic is for reception > errors not transmit errors. > This is a first cleanup on statistics before looking at oerrors. > The fix is correct according the documentation but it was waiting for a solution to capture the Tx queue errors which we are removing this support from some PMDs because they were storing this information into wrong field. Since the affected stats are Tx queue error stats, and all Tx errors still can be stored on 'oerrors' I am for getting the fix, although we need to figure out how to store Tx queue error stats. For series, Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com> Series applied to dpdk-next-net/master, thanks.