From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Tahhan, Maryam" Subject: Re: [PATCH 2/4] ethdev: expose extended error stats Date: Mon, 22 Jun 2015 15:35:51 +0000 Message-ID: <1A27633A6DA49C4A92FCD5D4312DBF536A4404C3@IRSMSX109.ger.corp.intel.com> References: <1433525705-17041-1-git-send-email-maryam.tahhan@intel.com> <1433525705-17041-3-git-send-email-maryam.tahhan@intel.com> <6388992.ny7qEppTdV@xps13> <1A27633A6DA49C4A92FCD5D4312DBF536A43EB82@IRSMSX109.ger.corp.intel.com> <558825D3.2090106@6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" To: Olivier MATZ , Thomas Monjalon Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id BF358C6B6 for ; Mon, 22 Jun 2015 17:35:58 +0200 (CEST) In-Reply-To: <558825D3.2090106@6wind.com> Content-Language: en-US List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > >>> + packets. */ > >>> }; > >> > >> You are extending the generic stats. This is not the idea behind xstat= s. > >> The xstats are specific to the driver. > > > > I'd followed the example of: > > http://patchwork.dpdk.org/dev/patchwork/patch/85/ > > to added generic extended stats (at least what I thought were > > generic). I think that dropped packets should fall under struct > > rte_eth_stats, and should perhaps be left there, as most NICs/drivers > > should be able to provide that number. Would this be an agreeable > solution? > > > > I have no other way to expose the total MAC errors and the total PHY > > errors without Adding counters into struct ixgbe_hw_stats, but I wasn't= sure > if this was allowable, is it? > > > > The only other option is to round up all the errors into ierrors, > > without having the granularity of what errors fall under. Is the > > latter option to sum up the values under one umbrella preferred? >=20 > As Thomas explained, I think we should avoid adding fields in struct > rte_eth_stats. This structure already contains statistics that are specif= ic to > some hardware drivers. We should try to go in the opposite direction and > remove all the fields that do not apply to all nics (physical or virtual)= . For me, it > would be only something like (rx, tx, rx_bytes, tx_bytes, rx_errors, tx_e= rrors). >=20 > The xstats framework allows you to add any driver or hardware specific st= ats > and I think it's the proper place to add them for ixgbe. >=20 Alright that sounds good. > By the way, something could be modified in xstats framework. Today, the > behavior is: > - if ethdev->dev_ops->xstats_get !=3D NULL, call it > - else, dump the generic stats in xstats format As it happens I did do this in: http://dpdk.org/dev/patchwork/patch/5324/=20 but I will reverse the order as what happens now is xstats from the driver are displayed first and will look at always dumping generic stats in xstats= format as you mention below. >=20 > A better behavior could be: > - Always dump the generic stats in xstats format > - and if thdev->dev_ops->xstats_get !=3D NULL, add driver-specific stats >=20 > This would avoid to duplicate code that dumps generic stats in all driver= s. >=20 > So, to summarize what I think should be done for stats/xstats: > 1. modify xstats behavior as described above 2. add the xstats dev_ops in > ixgbe, it will dump the new stats >=20 > Bonus: > 3. identify all the specific stats that could be removed from > rte_eth_stats and start to mark them as deprecated. >=20 Will do. >=20 > Regards, > Olivier Thanks Maryam