Re: [dpdk-dev] [PATCH 2/4] ethdev: expose extended error stats

["Tahhan, Maryam" <maryam.tahhan@intel.com>]

<snip>
> >>> + packets. */
> >>>  };
> >>
> >> You are extending the generic stats. This is not the idea behind xstats.
> >> 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?
> 
> As Thomas explained, I think we should avoid adding fields in struct
> rte_eth_stats. This structure already contains statistics that are specific 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_errors).
> 
> The xstats framework allows you to add any driver or hardware specific stats
> and I think it's the proper place to add them for ixgbe.
> 


Alright that sounds good.

> By the way, something could be modified in xstats framework. Today, the
> behavior is:
> - if ethdev->dev_ops->xstats_get != 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/ 
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.

> 
> A better behavior could be:
> - Always dump the generic stats in xstats format
> - and if thdev->dev_ops->xstats_get != NULL, add driver-specific stats
> 
> This would avoid to duplicate code that dumps generic stats in all drivers.
> 
> 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
> 
> Bonus:
> 3. identify all the specific stats that could be removed from
>    rte_eth_stats and start to mark them as deprecated.
> 

Will do.

> 
> Regards,
> Olivier

Thanks
Maryam
<snip>