Re: [dpdk-dev] [PATCH 6/9] qede: add support for xstats

["Van Haaren, Harry" <harry.van.haaren@intel.com>]

Hi Rasesh,

Some comments about xstats strings below, please refer to the docs here:
http://dpdk.org/doc/guides/prog_guide/poll_mode_drv.html#extended-statistics-api
Due to my familiarity with ixgbe, I will use it as an example: other PMDs should
be identical. 

I understand these may seem trivial and pointless changes, but ensuring the xstats strings are consistent is vital for apps to find the metadata contained.

Regards, -Harry

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Rasesh Mody
> Subject: [dpdk-dev] [PATCH 6/9] qede: add support for xstats
> 
> This patch adds support for extended statistics for QEDE PMD.
> 	
> +static const struct rte_eth_xstats qede_eth_stats[] = {
> +	{"no_buff_discards",
> +		offsetof(struct ecore_eth_stats, no_buff_discards)},

Is provided by ethdev as rx_mbuf_allocation_errors
http://dpdk.org/browse/dpdk/tree/lib/librte_ether/rte_ethdev.c#n93

> +	{"packet_too_big_discard",
> +		offsetof(struct ecore_eth_stats, packet_too_big_discard)},

Is this not the same as rx_oversize below? It reads like it is - perhaps try be more descriptive (provide a direction, and differentiate it from the counter below) 

> +	{"ttl0_discard", offsetof(struct ecore_eth_stats, ttl0_discard)},

This needs a direction, "rx", I suggest "rx_ttl_zero_dropped" 

> +	{"rx_ucast_bytes", offsetof(struct ecore_eth_stats, rx_ucast_bytes)},
> +	{"rx_mcast_bytes", offsetof(struct ecore_eth_stats, rx_mcast_bytes)},
> +	{"rx_bcast_bytes", offsetof(struct ecore_eth_stats, rx_bcast_bytes)},
> +	{"rx_ucast_pkts", offsetof(struct ecore_eth_stats, rx_ucast_pkts)},
> +	{"rx_mcast_pkts", offsetof(struct ecore_eth_stats, rx_mcast_pkts)},
> +	{"rx_bcast_pkts", offsetof(struct ecore_eth_stats, rx_bcast_pkts)},

expand these names - unicast, packets. Otherwise formatting is good.

> +	{"mftag_filter_discards",
> +		offsetof(struct ecore_eth_stats, mftag_filter_discards)},
> +	{"mac_filter_discards",
> +		offsetof(struct ecore_eth_stats, mac_filter_discards)},

Direction, "mftag"?

> +	{"tx_ucast_bytes", offsetof(struct ecore_eth_stats, tx_ucast_bytes)},
> +	{"tx_mcast_bytes", offsetof(struct ecore_eth_stats, tx_mcast_bytes)},
> +	{"tx_bcast_bytes", offsetof(struct ecore_eth_stats, tx_bcast_bytes)},
> +	{"tx_ucast_pkts", offsetof(struct ecore_eth_stats, tx_ucast_pkts)},
> +	{"tx_mcast_pkts", offsetof(struct ecore_eth_stats, tx_mcast_pkts)},
> +	{"tx_bcast_pkts", offsetof(struct ecore_eth_stats, tx_bcast_pkts)},

Same as RX (expand to unicast etc, packets)

> +	{"tx_err_drop_pkts",
> +		offsetof(struct ecore_eth_stats, tx_err_drop_pkts)},
> +	{"tpa_coalesced_pkts",
> +		offsetof(struct ecore_eth_stats, tpa_coalesced_pkts)},
> +	{"tpa_coalesced_events",
> +		offsetof(struct ecore_eth_stats, tpa_coalesced_events)},
> +	{"tpa_aborts_num", offsetof(struct ecore_eth_stats, tpa_aborts_num)},
> +	{"tpa_not_coalesced_pkts",
> +		offsetof(struct ecore_eth_stats, tpa_not_coalesced_pkts)},
> +	{"tpa_coalesced_bytes",
> +		offsetof(struct ecore_eth_stats, tpa_coalesced_bytes)},

"tpa" should be human-readable, and these need a direction (unless this counter explicitly has no affinity to rx/tx)

> +	{"rx_64_byte_packets",
> +		offsetof(struct ecore_eth_stats, rx_64_byte_packets)},
<<until>>
> +	{"rx_9217_to_16383_byte_packets", offsetof(struct ecore_eth_stats,
> +						rx_9217_to_16383_byte_packets)},

Formatting not consistent, see docs linked at top.		

> +	{"rx_mac_crtl_frames",
> +		offsetof(struct ecore_eth_stats, rx_mac_crtl_frames)},

and 

> +	{"rx_prio_flow_ctrl_frames",
> +		offsetof(struct ecore_eth_stats, rx_pfc_frames)},

Expand crtl

> +	{"rx_oversize_packets",
> +		offsetof(struct ecore_eth_stats, rx_oversize_packets)},

Does this packet get received properly? If so, packets is the right counter. Otherwise this should be errors:
http://dpdk.org/browse/dpdk/tree/drivers/net/ixgbe/ixgbe_ethdev.c#n617

> +	{"rx_jabbers", offsetof(struct ecore_eth_stats, rx_jabbers)},

rx_jabber_errors

> +	{"rx_undersize_packets",
> +		offsetof(struct ecore_eth_stats, rx_undersize_packets)},

Same as oversize.

> +	{"rx_fragments", offsetof(struct ecore_eth_stats, rx_fragments)},

rx_fragment_packets?
http://dpdk.org/browse/dpdk/tree/drivers/net/ixgbe/ixgbe_ethdev.c#n615

> +	{"tx_64_byte_packets",
> +		offsetof(struct ecore_eth_stats, tx_64_byte_packets)},

Same as RX size comment.