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

[Olivier MATZ]

Hello Maryam,

On 06/22/2015 04:12 PM, Tahhan, Maryam wrote:
> 
>> 2015-06-05 18:35, Maryam Tahhan:
>>> Extend rte_eth_xstats_get to retrieve additional stats from the device
>>> driver as well the top level extended stats. Add additional drop
>>> counters to the extended stats.
>>>
>>> Signed-off-by: Maryam Tahhan <maryam.tahhan at intel.com>
>> [..]
>> Patch 1/4 doesn't compile without patch 2/4.
> 
> The rebased patches should fix this issue.
>>
>>> --- a/lib/librte_ether/rte_ethdev.c
>>> +++ b/lib/librte_ether/rte_ethdev.c
>>> @@ -129,6 +129,8 @@ static const struct rte_eth_xstats_name_off
>> rte_stats_strings[] = {
>>>  	{"rx_crc_errors", offsetof(struct rte_eth_stats, ibadcrc)},
>>>  	{"rx_bad_length_errors", offsetof(struct rte_eth_stats, ibadlen)},
>>>  	{"rx_errors", offsetof(struct rte_eth_stats, ierrors)},
>>> +	{"rx_mac_err", offsetof(struct rte_eth_stats, imacerr)},
>>> +	{"rx_phy_err", offsetof(struct rte_eth_stats, iphyerr)},
>>>  	{"alloc_rx_buff_failed", offsetof(struct rte_eth_stats, rx_nombuf)},
>>>  	{"fdir_match", offsetof(struct rte_eth_stats, fdirmatch)},
>>>  	{"fdir_miss", offsetof(struct rte_eth_stats, fdirmiss)}, @@ -136,6
>>> +138,8 @@ static const struct rte_eth_xstats_name_off rte_stats_strings[]
>> = {
>>>  	{"rx_flow_control_xon", offsetof(struct rte_eth_stats,
>> rx_pause_xon)},
>>>  	{"tx_flow_control_xoff", offsetof(struct rte_eth_stats,
>> tx_pause_xoff)},
>>>  	{"rx_flow_control_xoff", offsetof(struct rte_eth_stats,
>>> rx_pause_xoff)},
>>> +	{"tx_drops", offsetof(struct rte_eth_stats, odrop)},
>>> +	{"rx_drops", offsetof(struct rte_eth_stats, idrop)},
>>>  };
>> [...]
>>> --- a/lib/librte_ether/rte_ethdev.h
>>> +++ b/lib/librte_ether/rte_ethdev.h
>>> @@ -224,6 +224,10 @@ struct rte_eth_stats {
>>>
>>>         /**< Total number of good bytes received from loopback,VF Only */
>>>         uint64_t olbbytes;
>>>         /**< Total number of good bytes transmitted to loopback,VF
>>> Only */
>>>
>>> +       uint64_t imacerr;   /**< Total of RX packets with MAC Errors. */
>>> +       uint64_t iphyerr;   /**< Total of RX packets with PHY Errors. */
>>> +       uint64_t idrop;  /**< Total number of dropped received packets. */
>>> +       uint64_t odrop;  /**< Total number of dropped transmitted
>>> + 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.

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

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.


Regards,
Olivier


> 
>> Furthermore we should migrate some "not really generic stats" to xstats in
>> order to keep only the really basic and common stats in rte_eth_stats.
>> By the way, in order to avoid duplicated code when getting generic stats
>> through xstats API, we need to change the implementation of
>> rte_eth_xstats_get() to add generic stats automatically, even if the driver
>> provide some xstats.