diff mbox

[API-NEXT,1/5] linux-generic: sockets: implement pktio statistics counters

Message ID 1447068113-3733-2-git-send-email-maxim.uvarov@linaro.org
State New
Headers show

Commit Message

Maxim Uvarov Nov. 9, 2015, 11:21 a.m. UTC 
Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
---
 platform/linux-generic/Makefile.am                 |  1 +
 .../linux-generic/include/odp_packet_io_internal.h |  9 +++
 platform/linux-generic/odp_packet_io.c             | 53 +++++++++++++++++
 platform/linux-generic/pktio/socket.c              | 59 ++++++++++++++++++
 platform/linux-generic/pktio/socket_mmap.c         |  8 +++
 platform/linux-generic/pktio/stats_sysfs.c         | 69 ++++++++++++++++++++++
 6 files changed, 199 insertions(+)
 create mode 100644 platform/linux-generic/pktio/stats_sysfs.c

Comments

Stuart Haslam Nov. 10, 2015, 6:29 p.m. UTC | #1 
On Mon, Nov 09, 2015 at 02:21:49PM +0300, Maxim Uvarov wrote:
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> ---
>  platform/linux-generic/Makefile.am                 |  1 +
>  .../linux-generic/include/odp_packet_io_internal.h |  9 +++
>  platform/linux-generic/odp_packet_io.c             | 53 +++++++++++++++++
>  platform/linux-generic/pktio/socket.c              | 59 ++++++++++++++++++
>  platform/linux-generic/pktio/socket_mmap.c         |  8 +++
>  platform/linux-generic/pktio/stats_sysfs.c         | 69 ++++++++++++++++++++++
>  6 files changed, 199 insertions(+)
>  create mode 100644 platform/linux-generic/pktio/stats_sysfs.c
> 
> diff --git a/platform/linux-generic/Makefile.am b/platform/linux-generic/Makefile.am
> index 610c79c..e3e4954 100644
> --- a/platform/linux-generic/Makefile.am
> +++ b/platform/linux-generic/Makefile.am
> @@ -170,6 +170,7 @@ __LIB__libodp_la_SOURCES = \
>  			   pktio/netmap.c \
>  			   pktio/socket.c \
>  			   pktio/socket_mmap.c \
> +			   pktio/stats_sysfs.c \
>  			   odp_pool.c \
>  			   odp_queue.c \
>  			   odp_rwlock.c \
> diff --git a/platform/linux-generic/include/odp_packet_io_internal.h b/platform/linux-generic/include/odp_packet_io_internal.h
> index 1a1118c..ec43e87 100644
> --- a/platform/linux-generic/include/odp_packet_io_internal.h
> +++ b/platform/linux-generic/include/odp_packet_io_internal.h
> @@ -84,6 +84,7 @@ struct pktio_entry {
>  		STATE_STOP
>  	} state;
>  	classifier_t cls;		/**< classifier linked with this pktio*/
> +	odp_pktio_stats_t stats;	/**< statistic counters for pktio */
>  	char name[PKTIO_NAME_LEN];	/**< name of pktio provided to
>  					   pktio_open() */
>  	odp_pktio_param_t param;
> @@ -107,6 +108,8 @@ typedef struct pktio_if_ops {
>  	int (*close)(pktio_entry_t *pktio_entry);
>  	int (*start)(pktio_entry_t *pktio_entry);
>  	int (*stop)(pktio_entry_t *pktio_entry);
> +	int (*stats)(pktio_entry_t *pktio_entry, odp_pktio_stats_t *stats);
> +	int (*stats_reset)(pktio_entry_t *pktio_entry);
>  	int (*recv)(pktio_entry_t *pktio_entry, odp_packet_t pkt_table[],
>  		    unsigned len);
>  	int (*send)(pktio_entry_t *pktio_entry, odp_packet_t pkt_table[],
> @@ -159,6 +162,12 @@ extern const pktio_if_ops_t pcap_pktio_ops;
>  #endif
>  extern const pktio_if_ops_t * const pktio_if_ops[];
>  
> +int sysfs_stats(pktio_entry_t *pktio_entry,
> +		odp_pktio_stats_t *stats);
> +int sock_stats_reset(pktio_entry_t *pktio_entry);
> +int sock_stats(pktio_entry_t *pktio_entry,
> +	       odp_pktio_stats_t *stats);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c
> index 3ef400f..ba97629 100644
> --- a/platform/linux-generic/odp_packet_io.c
> +++ b/platform/linux-generic/odp_packet_io.c
> @@ -862,3 +862,56 @@ void odp_pktio_print(odp_pktio_t id)
>  
>  	ODP_PRINT("\n%s\n", str);
>  }
> +
> +int odp_pktio_stats(odp_pktio_t pktio,
> +		    odp_pktio_stats_t *stats)
> +{
> +	pktio_entry_t *entry;
> +	int ret = -1;
> +
> +	entry = get_pktio_entry(pktio);
> +	if (entry == NULL) {
> +		ODP_DBG("pktio entry %d does not exist\n", pktio);
> +		return -1;
> +	}
> +
> +	lock_entry(entry);
> +
> +	if (odp_unlikely(is_free(entry))) {
> +		unlock_entry(entry);
> +		ODP_DBG("already freed pktio\n");
> +		return -1;
> +	}
> +
> +	if (entry->s.ops->stats)
> +		ret = entry->s.ops->stats(entry, stats);
> +	unlock_entry(entry);
> +
> +	return ret;
> +}
> +
> +int odp_pktio_stats_reset(odp_pktio_t pktio)
> +{
> +	pktio_entry_t *entry;
> +	int ret = -1;
> +
> +	entry = get_pktio_entry(pktio);
> +	if (entry == NULL) {
> +		ODP_DBG("pktio entry %d does not exist\n", pktio);
> +		return -1;
> +	}
> +
> +	lock_entry(entry);
> +
> +	if (odp_unlikely(is_free(entry))) {
> +		unlock_entry(entry);
> +		ODP_DBG("already freed pktio\n");
> +		return -1;
> +	}
> +
> +	if (entry->s.ops->stats)
> +		ret = entry->s.ops->stats_reset(entry);
> +	unlock_entry(entry);
> +
> +	return ret;
> +}
> diff --git a/platform/linux-generic/pktio/socket.c b/platform/linux-generic/pktio/socket.c
> index 5f5e0ae..0bda57b 100644
> --- a/platform/linux-generic/pktio/socket.c
> +++ b/platform/linux-generic/pktio/socket.c
> @@ -254,6 +254,12 @@ static int sock_setup_pkt(pktio_entry_t *pktio_entry, const char *netdev,
>  		goto error;
>  	}
>  
> +	err = sock_stats_reset(pktio_entry);
> +	if (err != 0) {
> +		ODP_ERR("reset stats\n");
> +		goto error;
> +	}
> +
>  	return 0;
>  
>  error:
> @@ -467,6 +473,57 @@ static int sock_promisc_mode_get(pktio_entry_t *pktio_entry)
>  				   pktio_entry->s.name);
>  }
>  
> +int sock_stats(pktio_entry_t *pktio_entry,
> +	       odp_pktio_stats_t *stats)
> +{
> +	odp_pktio_stats_t cur_stats;
> +	int ret;
> +
> +	memset(&cur_stats, 0, sizeof(odp_pktio_stats_t));
> +	ret = sysfs_stats(pktio_entry, &cur_stats);
> +	if (ret)
> +		ODP_ABORT("sysfs stats error\n");
> +
> +	stats->in_octets = cur_stats.in_octets -
> +				pktio_entry->s.stats.in_octets;
> +	stats->in_ucast_pkts = cur_stats.in_ucast_pkts -
> +				pktio_entry->s.stats.in_ucast_pkts;
> +	stats->in_discards = cur_stats.in_discards -
> +				pktio_entry->s.stats.in_discards;
> +	stats->in_errors = cur_stats.in_errors -
> +				pktio_entry->s.stats.in_errors;
> +	stats->in_unknown_protos = cur_stats.in_unknown_protos -
> +				pktio_entry->s.stats.in_unknown_protos;
> +
> +	stats->out_octets = cur_stats.out_octets -
> +				pktio_entry->s.stats.out_octets;
> +	stats->out_ucast_pkts = cur_stats.out_ucast_pkts -
> +				pktio_entry->s.stats.out_ucast_pkts;
> +	stats->out_discards = cur_stats.out_discards -
> +				pktio_entry->s.stats.out_discards;
> +	stats->out_errors = cur_stats.out_errors -
> +				pktio_entry->s.stats.out_errors;
> +
> +	return 0;
> +}
> +
> +int sock_stats_reset(pktio_entry_t *pktio_entry)
> +{
> +	int err = 0;
> +	odp_pktio_stats_t cur;
> +
> +	memset(&cur, 0, sizeof(odp_pktio_stats_t));
> +	err = sysfs_stats(pktio_entry, &cur);
> +	if (err != 0) {
> +		__odp_errno = errno;
> +		ODP_ERR("sysfs stats error\n");
> +	} else {
> +		memcpy(&pktio_entry->s.stats, &cur, sizeof(odp_pktio_stats_t));
> +	}
> +
> +	return err;
> +}
> +
>  const pktio_if_ops_t sock_mmsg_pktio_ops = {
>  	.init = NULL,
>  	.term = NULL,
> @@ -474,6 +531,8 @@ const pktio_if_ops_t sock_mmsg_pktio_ops = {
>  	.close = sock_close,
>  	.start = NULL,
>  	.stop = NULL,
> +	.stats = sock_stats,
> +	.stats_reset = sock_stats_reset,
>  	.recv = sock_mmsg_recv,
>  	.send = sock_mmsg_send,
>  	.mtu_get = sock_mtu_get,
> diff --git a/platform/linux-generic/pktio/socket_mmap.c b/platform/linux-generic/pktio/socket_mmap.c
> index 79ff82d..2031485 100644
> --- a/platform/linux-generic/pktio/socket_mmap.c
> +++ b/platform/linux-generic/pktio/socket_mmap.c
> @@ -467,6 +467,12 @@ static int sock_mmap_open(odp_pktio_t id ODP_UNUSED,
>  			goto error;
>  	}
>  
> +	ret = sock_stats_reset(pktio_entry);
> +	if (ret != 0) {
> +		ODP_ERR("reset stats\n");
> +		goto error;
> +	}
> +
>  	return 0;
>  
>  error:
> @@ -525,6 +531,8 @@ const pktio_if_ops_t sock_mmap_pktio_ops = {
>  	.close = sock_mmap_close,
>  	.start = NULL,
>  	.stop = NULL,
> +	.stats = sock_stats,
> +	.stats_reset = sock_stats_reset,
>  	.recv = sock_mmap_recv,
>  	.send = sock_mmap_send,
>  	.mtu_get = sock_mmap_mtu_get,
> diff --git a/platform/linux-generic/pktio/stats_sysfs.c b/platform/linux-generic/pktio/stats_sysfs.c
> new file mode 100644
> index 0000000..5033b7e
> --- /dev/null
> +++ b/platform/linux-generic/pktio/stats_sysfs.c
> @@ -0,0 +1,69 @@
> +/* Copyright (c) 2015, Linaro Limited
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier:     BSD-3-Clause
> + */
> +
> +#include <odp.h>
> +#include <odp_packet_io_internal.h>
> +#include <stdio.h>
> +
> +static uint64_t sysfs_get_val(const char *fname)
> +{
> +	FILE  *file;
> +	char str[128];
> +	uint64_t ret = -1;
> +
> +	file = fopen(fname, "rt");
> +	if (file == NULL) {
> +		/* File not found */
> +		return 0;
> +	}
> +
> +	if (fgets(str, sizeof(str), file) != NULL) {
> +		/* Read cache line size */

??

> +		if (sscanf(str, "%" SCNx64, &ret) != 1) {
> +			ODP_ERR("read %s\n", fname);
> +			ret = 0;
> +		}
> +	}
> +
> +	(void)fclose(file);
> +	return ret;
> +}

Error handling is a bit broken here, 0 isn't a good failure indication.
It would be better to return an int and take a uint64_t* parameter.

Although I think it would actually be better to use ETHTOOL_GSTATS as
that should work for netmap too, in which case this helper wouldn't be
needed.

> +
> +int sysfs_stats(pktio_entry_t *pktio_entry,
> +		odp_pktio_stats_t *stats)
> +{
> +	char fname[256];
> +	const char *dev = pktio_entry->s.name;
> +
> +	sprintf(fname, "/sys/class/net/%s/statistics/rx_bytes", dev);
> +	stats->in_octets = sysfs_get_val(fname);
> +
> +	sprintf(fname, "/sys/class/net/%s/statistics/rx_packets", dev);
> +	stats->in_ucast_pkts = sysfs_get_val(fname);
> +
> +	sprintf(fname, "/sys/class/net/%s/statistics/rx_droppped", dev);
> +	stats->in_discards = sysfs_get_val(fname);
> +
> +	sprintf(fname, "/sys/class/net/%s/statistics/rx_errors", dev);
> +	stats->in_errors = sysfs_get_val(fname);
> +
> +	stats->in_unknown_protos = 0;

I presume there's no easy way of getting this information, could do with
a comment noting that it's not supported.

> +
> +	sprintf(fname, "/sys/class/net/%s/statistics/tx_bytes", dev);
> +	stats->out_octets = sysfs_get_val(fname);
> +
> +	sprintf(fname, "/sys/class/net/%s/statistics/tx_packets", dev);
> +	stats->out_ucast_pkts = sysfs_get_val(fname);
> +
> +	sprintf(fname, "/sys/class/net/%s/statistics/tx_dropped", dev);
> +	stats->out_discards = sysfs_get_val(fname);
> +
> +	sprintf(fname, "/sys/class/net/%s/statistics/tx_errors", dev);
> +	stats->out_errors = sysfs_get_val(fname);
> +
> +	return 0;
> +}
> +
> -- 
> 1.9.1
>
Maxim Uvarov Nov. 12, 2015, 2:14 p.m. UTC | #2 
On 11/10/2015 21:29, Stuart Haslam wrote:
> On Mon, Nov 09, 2015 at 02:21:49PM +0300, Maxim Uvarov wrote:
>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
>> ---
>>   platform/linux-generic/Makefile.am                 |  1 +
>>   .../linux-generic/include/odp_packet_io_internal.h |  9 +++
>>   platform/linux-generic/odp_packet_io.c             | 53 +++++++++++++++++
>>   platform/linux-generic/pktio/socket.c              | 59 ++++++++++++++++++
>>   platform/linux-generic/pktio/socket_mmap.c         |  8 +++
>>   platform/linux-generic/pktio/stats_sysfs.c         | 69 ++++++++++++++++++++++
>>   6 files changed, 199 insertions(+)
>>   create mode 100644 platform/linux-generic/pktio/stats_sysfs.c
>>
>> diff --git a/platform/linux-generic/Makefile.am b/platform/linux-generic/Makefile.am
>> index 610c79c..e3e4954 100644
>> --- a/platform/linux-generic/Makefile.am
>> +++ b/platform/linux-generic/Makefile.am
>> @@ -170,6 +170,7 @@ __LIB__libodp_la_SOURCES = \
>>   			   pktio/netmap.c \
>>   			   pktio/socket.c \
>>   			   pktio/socket_mmap.c \
>> +			   pktio/stats_sysfs.c \
>>   			   odp_pool.c \
>>   			   odp_queue.c \
>>   			   odp_rwlock.c \
>> diff --git a/platform/linux-generic/include/odp_packet_io_internal.h b/platform/linux-generic/include/odp_packet_io_internal.h
>> index 1a1118c..ec43e87 100644
>> --- a/platform/linux-generic/include/odp_packet_io_internal.h
>> +++ b/platform/linux-generic/include/odp_packet_io_internal.h
>> @@ -84,6 +84,7 @@ struct pktio_entry {
>>   		STATE_STOP
>>   	} state;
>>   	classifier_t cls;		/**< classifier linked with this pktio*/
>> +	odp_pktio_stats_t stats;	/**< statistic counters for pktio */
>>   	char name[PKTIO_NAME_LEN];	/**< name of pktio provided to
>>   					   pktio_open() */
>>   	odp_pktio_param_t param;
>> @@ -107,6 +108,8 @@ typedef struct pktio_if_ops {
>>   	int (*close)(pktio_entry_t *pktio_entry);
>>   	int (*start)(pktio_entry_t *pktio_entry);
>>   	int (*stop)(pktio_entry_t *pktio_entry);
>> +	int (*stats)(pktio_entry_t *pktio_entry, odp_pktio_stats_t *stats);
>> +	int (*stats_reset)(pktio_entry_t *pktio_entry);
>>   	int (*recv)(pktio_entry_t *pktio_entry, odp_packet_t pkt_table[],
>>   		    unsigned len);
>>   	int (*send)(pktio_entry_t *pktio_entry, odp_packet_t pkt_table[],
>> @@ -159,6 +162,12 @@ extern const pktio_if_ops_t pcap_pktio_ops;
>>   #endif
>>   extern const pktio_if_ops_t * const pktio_if_ops[];
>>   
>> +int sysfs_stats(pktio_entry_t *pktio_entry,
>> +		odp_pktio_stats_t *stats);
>> +int sock_stats_reset(pktio_entry_t *pktio_entry);
>> +int sock_stats(pktio_entry_t *pktio_entry,
>> +	       odp_pktio_stats_t *stats);
>> +
>>   #ifdef __cplusplus
>>   }
>>   #endif
>> diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c
>> index 3ef400f..ba97629 100644
>> --- a/platform/linux-generic/odp_packet_io.c
>> +++ b/platform/linux-generic/odp_packet_io.c
>> @@ -862,3 +862,56 @@ void odp_pktio_print(odp_pktio_t id)
>>   
>>   	ODP_PRINT("\n%s\n", str);
>>   }
>> +
>> +int odp_pktio_stats(odp_pktio_t pktio,
>> +		    odp_pktio_stats_t *stats)
>> +{
>> +	pktio_entry_t *entry;
>> +	int ret = -1;
>> +
>> +	entry = get_pktio_entry(pktio);
>> +	if (entry == NULL) {
>> +		ODP_DBG("pktio entry %d does not exist\n", pktio);
>> +		return -1;
>> +	}
>> +
>> +	lock_entry(entry);
>> +
>> +	if (odp_unlikely(is_free(entry))) {
>> +		unlock_entry(entry);
>> +		ODP_DBG("already freed pktio\n");
>> +		return -1;
>> +	}
>> +
>> +	if (entry->s.ops->stats)
>> +		ret = entry->s.ops->stats(entry, stats);
>> +	unlock_entry(entry);
>> +
>> +	return ret;
>> +}
>> +
>> +int odp_pktio_stats_reset(odp_pktio_t pktio)
>> +{
>> +	pktio_entry_t *entry;
>> +	int ret = -1;
>> +
>> +	entry = get_pktio_entry(pktio);
>> +	if (entry == NULL) {
>> +		ODP_DBG("pktio entry %d does not exist\n", pktio);
>> +		return -1;
>> +	}
>> +
>> +	lock_entry(entry);
>> +
>> +	if (odp_unlikely(is_free(entry))) {
>> +		unlock_entry(entry);
>> +		ODP_DBG("already freed pktio\n");
>> +		return -1;
>> +	}
>> +
>> +	if (entry->s.ops->stats)
>> +		ret = entry->s.ops->stats_reset(entry);
>> +	unlock_entry(entry);
>> +
>> +	return ret;
>> +}
>> diff --git a/platform/linux-generic/pktio/socket.c b/platform/linux-generic/pktio/socket.c
>> index 5f5e0ae..0bda57b 100644
>> --- a/platform/linux-generic/pktio/socket.c
>> +++ b/platform/linux-generic/pktio/socket.c
>> @@ -254,6 +254,12 @@ static int sock_setup_pkt(pktio_entry_t *pktio_entry, const char *netdev,
>>   		goto error;
>>   	}
>>   
>> +	err = sock_stats_reset(pktio_entry);
>> +	if (err != 0) {
>> +		ODP_ERR("reset stats\n");
>> +		goto error;
>> +	}
>> +
>>   	return 0;
>>   
>>   error:
>> @@ -467,6 +473,57 @@ static int sock_promisc_mode_get(pktio_entry_t *pktio_entry)
>>   				   pktio_entry->s.name);
>>   }
>>   
>> +int sock_stats(pktio_entry_t *pktio_entry,
>> +	       odp_pktio_stats_t *stats)
>> +{
>> +	odp_pktio_stats_t cur_stats;
>> +	int ret;
>> +
>> +	memset(&cur_stats, 0, sizeof(odp_pktio_stats_t));
>> +	ret = sysfs_stats(pktio_entry, &cur_stats);
>> +	if (ret)
>> +		ODP_ABORT("sysfs stats error\n");
>> +
>> +	stats->in_octets = cur_stats.in_octets -
>> +				pktio_entry->s.stats.in_octets;
>> +	stats->in_ucast_pkts = cur_stats.in_ucast_pkts -
>> +				pktio_entry->s.stats.in_ucast_pkts;
>> +	stats->in_discards = cur_stats.in_discards -
>> +				pktio_entry->s.stats.in_discards;
>> +	stats->in_errors = cur_stats.in_errors -
>> +				pktio_entry->s.stats.in_errors;
>> +	stats->in_unknown_protos = cur_stats.in_unknown_protos -
>> +				pktio_entry->s.stats.in_unknown_protos;
>> +
>> +	stats->out_octets = cur_stats.out_octets -
>> +				pktio_entry->s.stats.out_octets;
>> +	stats->out_ucast_pkts = cur_stats.out_ucast_pkts -
>> +				pktio_entry->s.stats.out_ucast_pkts;
>> +	stats->out_discards = cur_stats.out_discards -
>> +				pktio_entry->s.stats.out_discards;
>> +	stats->out_errors = cur_stats.out_errors -
>> +				pktio_entry->s.stats.out_errors;
>> +
>> +	return 0;
>> +}
>> +
>> +int sock_stats_reset(pktio_entry_t *pktio_entry)
>> +{
>> +	int err = 0;
>> +	odp_pktio_stats_t cur;
>> +
>> +	memset(&cur, 0, sizeof(odp_pktio_stats_t));
>> +	err = sysfs_stats(pktio_entry, &cur);
>> +	if (err != 0) {
>> +		__odp_errno = errno;
>> +		ODP_ERR("sysfs stats error\n");
>> +	} else {
>> +		memcpy(&pktio_entry->s.stats, &cur, sizeof(odp_pktio_stats_t));
>> +	}
>> +
>> +	return err;
>> +}
>> +
>>   const pktio_if_ops_t sock_mmsg_pktio_ops = {
>>   	.init = NULL,
>>   	.term = NULL,
>> @@ -474,6 +531,8 @@ const pktio_if_ops_t sock_mmsg_pktio_ops = {
>>   	.close = sock_close,
>>   	.start = NULL,
>>   	.stop = NULL,
>> +	.stats = sock_stats,
>> +	.stats_reset = sock_stats_reset,
>>   	.recv = sock_mmsg_recv,
>>   	.send = sock_mmsg_send,
>>   	.mtu_get = sock_mtu_get,
>> diff --git a/platform/linux-generic/pktio/socket_mmap.c b/platform/linux-generic/pktio/socket_mmap.c
>> index 79ff82d..2031485 100644
>> --- a/platform/linux-generic/pktio/socket_mmap.c
>> +++ b/platform/linux-generic/pktio/socket_mmap.c
>> @@ -467,6 +467,12 @@ static int sock_mmap_open(odp_pktio_t id ODP_UNUSED,
>>   			goto error;
>>   	}
>>   
>> +	ret = sock_stats_reset(pktio_entry);
>> +	if (ret != 0) {
>> +		ODP_ERR("reset stats\n");
>> +		goto error;
>> +	}
>> +
>>   	return 0;
>>   
>>   error:
>> @@ -525,6 +531,8 @@ const pktio_if_ops_t sock_mmap_pktio_ops = {
>>   	.close = sock_mmap_close,
>>   	.start = NULL,
>>   	.stop = NULL,
>> +	.stats = sock_stats,
>> +	.stats_reset = sock_stats_reset,
>>   	.recv = sock_mmap_recv,
>>   	.send = sock_mmap_send,
>>   	.mtu_get = sock_mmap_mtu_get,
>> diff --git a/platform/linux-generic/pktio/stats_sysfs.c b/platform/linux-generic/pktio/stats_sysfs.c
>> new file mode 100644
>> index 0000000..5033b7e
>> --- /dev/null
>> +++ b/platform/linux-generic/pktio/stats_sysfs.c
>> @@ -0,0 +1,69 @@
>> +/* Copyright (c) 2015, Linaro Limited
>> + * All rights reserved.
>> + *
>> + * SPDX-License-Identifier:     BSD-3-Clause
>> + */
>> +
>> +#include <odp.h>
>> +#include <odp_packet_io_internal.h>
>> +#include <stdio.h>
>> +
>> +static uint64_t sysfs_get_val(const char *fname)
>> +{
>> +	FILE  *file;
>> +	char str[128];
>> +	uint64_t ret = -1;
>> +
>> +	file = fopen(fname, "rt");
>> +	if (file == NULL) {
>> +		/* File not found */
>> +		return 0;
>> +	}
>> +
>> +	if (fgets(str, sizeof(str), file) != NULL) {
>> +		/* Read cache line size */
> ??

thanks, copy paste error :)
>
>> +		if (sscanf(str, "%" SCNx64, &ret) != 1) {
>> +			ODP_ERR("read %s\n", fname);
>> +			ret = 0;
>> +		}
>> +	}
>> +
>> +	(void)fclose(file);
>> +	return ret;
>> +}
> Error handling is a bit broken here, 0 isn't a good failure indication.
> It would be better to return an int and take a uint64_t* parameter.

For unsupported counters we just have 0. So 0 here is not an error,
it's just zero.

> Although I think it would actually be better to use ETHTOOL_GSTATS as
> that should work for netmap too, in which case this helper wouldn't be
> needed.

By doing that we should be depend on ethtool headers. And quite tricky
to rewrite eththool GPLv2 functions to code compatible BSD. Simple copy
paste will not work and I'm know how to rewrite it in different manner.

Sysfs was the most easy solution to read counters for me. Btw are there
any reasons to not export counters to /sysfs in netmap?

>> +
>> +int sysfs_stats(pktio_entry_t *pktio_entry,
>> +		odp_pktio_stats_t *stats)
>> +{
>> +	char fname[256];
>> +	const char *dev = pktio_entry->s.name;
>> +
>> +	sprintf(fname, "/sys/class/net/%s/statistics/rx_bytes", dev);
>> +	stats->in_octets = sysfs_get_val(fname);
>> +
>> +	sprintf(fname, "/sys/class/net/%s/statistics/rx_packets", dev);
>> +	stats->in_ucast_pkts = sysfs_get_val(fname);
>> +
>> +	sprintf(fname, "/sys/class/net/%s/statistics/rx_droppped", dev);
>> +	stats->in_discards = sysfs_get_val(fname);
>> +
>> +	sprintf(fname, "/sys/class/net/%s/statistics/rx_errors", dev);
>> +	stats->in_errors = sysfs_get_val(fname);
>> +
>> +	stats->in_unknown_protos = 0;
> I presume there's no easy way of getting this information, could do with
> a comment noting that it's not supported.

In linux-generic we have software classifier. If it's unable to parse 
packet
and detect protocol, then that counter have to be increased. I wanted to
send separate patch for it.
>
>> +
>> +	sprintf(fname, "/sys/class/net/%s/statistics/tx_bytes", dev);
>> +	stats->out_octets = sysfs_get_val(fname);
>> +
>> +	sprintf(fname, "/sys/class/net/%s/statistics/tx_packets", dev);
>> +	stats->out_ucast_pkts = sysfs_get_val(fname);
>> +
>> +	sprintf(fname, "/sys/class/net/%s/statistics/tx_dropped", dev);
>> +	stats->out_discards = sysfs_get_val(fname);
>> +
>> +	sprintf(fname, "/sys/class/net/%s/statistics/tx_errors", dev);
>> +	stats->out_errors = sysfs_get_val(fname);
>> +
>> +	return 0;
>> +}
>> +
>> -- 
>> 1.9.1
>>
Stuart Haslam Nov. 13, 2015, 1:45 p.m. UTC | #3 
On Thu, Nov 12, 2015 at 05:14:22PM +0300, Maxim Uvarov wrote:
> On 11/10/2015 21:29, Stuart Haslam wrote:
> >On Mon, Nov 09, 2015 at 02:21:49PM +0300, Maxim Uvarov wrote:
> >>Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> >>---
> >>  platform/linux-generic/Makefile.am                 |  1 +
> >>  .../linux-generic/include/odp_packet_io_internal.h |  9 +++
> >>  platform/linux-generic/odp_packet_io.c             | 53 +++++++++++++++++
> >>  platform/linux-generic/pktio/socket.c              | 59 ++++++++++++++++++
> >>  platform/linux-generic/pktio/socket_mmap.c         |  8 +++
> >>  platform/linux-generic/pktio/stats_sysfs.c         | 69 ++++++++++++++++++++++
> >>  6 files changed, 199 insertions(+)
> >>  create mode 100644 platform/linux-generic/pktio/stats_sysfs.c
> >>
> >>diff --git a/platform/linux-generic/Makefile.am b/platform/linux-generic/Makefile.am
> >>index 610c79c..e3e4954 100644
> >>--- a/platform/linux-generic/Makefile.am
> >>+++ b/platform/linux-generic/Makefile.am
> >>@@ -170,6 +170,7 @@ __LIB__libodp_la_SOURCES = \
> >>  			   pktio/netmap.c \
> >>  			   pktio/socket.c \
> >>  			   pktio/socket_mmap.c \
> >>+			   pktio/stats_sysfs.c \
> >>  			   odp_pool.c \
> >>  			   odp_queue.c \
> >>  			   odp_rwlock.c \
> >>diff --git a/platform/linux-generic/include/odp_packet_io_internal.h b/platform/linux-generic/include/odp_packet_io_internal.h
> >>index 1a1118c..ec43e87 100644
> >>--- a/platform/linux-generic/include/odp_packet_io_internal.h
> >>+++ b/platform/linux-generic/include/odp_packet_io_internal.h
> >>@@ -84,6 +84,7 @@ struct pktio_entry {
> >>  		STATE_STOP
> >>  	} state;
> >>  	classifier_t cls;		/**< classifier linked with this pktio*/
> >>+	odp_pktio_stats_t stats;	/**< statistic counters for pktio */
> >>  	char name[PKTIO_NAME_LEN];	/**< name of pktio provided to
> >>  					   pktio_open() */
> >>  	odp_pktio_param_t param;
> >>@@ -107,6 +108,8 @@ typedef struct pktio_if_ops {
> >>  	int (*close)(pktio_entry_t *pktio_entry);
> >>  	int (*start)(pktio_entry_t *pktio_entry);
> >>  	int (*stop)(pktio_entry_t *pktio_entry);
> >>+	int (*stats)(pktio_entry_t *pktio_entry, odp_pktio_stats_t *stats);
> >>+	int (*stats_reset)(pktio_entry_t *pktio_entry);
> >>  	int (*recv)(pktio_entry_t *pktio_entry, odp_packet_t pkt_table[],
> >>  		    unsigned len);
> >>  	int (*send)(pktio_entry_t *pktio_entry, odp_packet_t pkt_table[],
> >>@@ -159,6 +162,12 @@ extern const pktio_if_ops_t pcap_pktio_ops;
> >>  #endif
> >>  extern const pktio_if_ops_t * const pktio_if_ops[];
> >>+int sysfs_stats(pktio_entry_t *pktio_entry,
> >>+		odp_pktio_stats_t *stats);
> >>+int sock_stats_reset(pktio_entry_t *pktio_entry);
> >>+int sock_stats(pktio_entry_t *pktio_entry,
> >>+	       odp_pktio_stats_t *stats);
> >>+
> >>  #ifdef __cplusplus
> >>  }
> >>  #endif
> >>diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c
> >>index 3ef400f..ba97629 100644
> >>--- a/platform/linux-generic/odp_packet_io.c
> >>+++ b/platform/linux-generic/odp_packet_io.c
> >>@@ -862,3 +862,56 @@ void odp_pktio_print(odp_pktio_t id)
> >>  	ODP_PRINT("\n%s\n", str);
> >>  }
> >>+
> >>+int odp_pktio_stats(odp_pktio_t pktio,
> >>+		    odp_pktio_stats_t *stats)
> >>+{
> >>+	pktio_entry_t *entry;
> >>+	int ret = -1;
> >>+
> >>+	entry = get_pktio_entry(pktio);
> >>+	if (entry == NULL) {
> >>+		ODP_DBG("pktio entry %d does not exist\n", pktio);
> >>+		return -1;
> >>+	}
> >>+
> >>+	lock_entry(entry);
> >>+
> >>+	if (odp_unlikely(is_free(entry))) {
> >>+		unlock_entry(entry);
> >>+		ODP_DBG("already freed pktio\n");
> >>+		return -1;
> >>+	}
> >>+
> >>+	if (entry->s.ops->stats)
> >>+		ret = entry->s.ops->stats(entry, stats);
> >>+	unlock_entry(entry);
> >>+
> >>+	return ret;
> >>+}
> >>+
> >>+int odp_pktio_stats_reset(odp_pktio_t pktio)
> >>+{
> >>+	pktio_entry_t *entry;
> >>+	int ret = -1;
> >>+
> >>+	entry = get_pktio_entry(pktio);
> >>+	if (entry == NULL) {
> >>+		ODP_DBG("pktio entry %d does not exist\n", pktio);
> >>+		return -1;
> >>+	}
> >>+
> >>+	lock_entry(entry);
> >>+
> >>+	if (odp_unlikely(is_free(entry))) {
> >>+		unlock_entry(entry);
> >>+		ODP_DBG("already freed pktio\n");
> >>+		return -1;
> >>+	}
> >>+
> >>+	if (entry->s.ops->stats)
> >>+		ret = entry->s.ops->stats_reset(entry);
> >>+	unlock_entry(entry);
> >>+
> >>+	return ret;
> >>+}
> >>diff --git a/platform/linux-generic/pktio/socket.c b/platform/linux-generic/pktio/socket.c
> >>index 5f5e0ae..0bda57b 100644
> >>--- a/platform/linux-generic/pktio/socket.c
> >>+++ b/platform/linux-generic/pktio/socket.c
> >>@@ -254,6 +254,12 @@ static int sock_setup_pkt(pktio_entry_t *pktio_entry, const char *netdev,
> >>  		goto error;
> >>  	}
> >>+	err = sock_stats_reset(pktio_entry);
> >>+	if (err != 0) {
> >>+		ODP_ERR("reset stats\n");
> >>+		goto error;
> >>+	}
> >>+
> >>  	return 0;
> >>  error:
> >>@@ -467,6 +473,57 @@ static int sock_promisc_mode_get(pktio_entry_t *pktio_entry)
> >>  				   pktio_entry->s.name);
> >>  }
> >>+int sock_stats(pktio_entry_t *pktio_entry,
> >>+	       odp_pktio_stats_t *stats)
> >>+{
> >>+	odp_pktio_stats_t cur_stats;
> >>+	int ret;
> >>+
> >>+	memset(&cur_stats, 0, sizeof(odp_pktio_stats_t));
> >>+	ret = sysfs_stats(pktio_entry, &cur_stats);
> >>+	if (ret)
> >>+		ODP_ABORT("sysfs stats error\n");
> >>+
> >>+	stats->in_octets = cur_stats.in_octets -
> >>+				pktio_entry->s.stats.in_octets;
> >>+	stats->in_ucast_pkts = cur_stats.in_ucast_pkts -
> >>+				pktio_entry->s.stats.in_ucast_pkts;
> >>+	stats->in_discards = cur_stats.in_discards -
> >>+				pktio_entry->s.stats.in_discards;
> >>+	stats->in_errors = cur_stats.in_errors -
> >>+				pktio_entry->s.stats.in_errors;
> >>+	stats->in_unknown_protos = cur_stats.in_unknown_protos -
> >>+				pktio_entry->s.stats.in_unknown_protos;
> >>+
> >>+	stats->out_octets = cur_stats.out_octets -
> >>+				pktio_entry->s.stats.out_octets;
> >>+	stats->out_ucast_pkts = cur_stats.out_ucast_pkts -
> >>+				pktio_entry->s.stats.out_ucast_pkts;
> >>+	stats->out_discards = cur_stats.out_discards -
> >>+				pktio_entry->s.stats.out_discards;
> >>+	stats->out_errors = cur_stats.out_errors -
> >>+				pktio_entry->s.stats.out_errors;
> >>+
> >>+	return 0;
> >>+}
> >>+
> >>+int sock_stats_reset(pktio_entry_t *pktio_entry)
> >>+{
> >>+	int err = 0;
> >>+	odp_pktio_stats_t cur;
> >>+
> >>+	memset(&cur, 0, sizeof(odp_pktio_stats_t));
> >>+	err = sysfs_stats(pktio_entry, &cur);
> >>+	if (err != 0) {
> >>+		__odp_errno = errno;
> >>+		ODP_ERR("sysfs stats error\n");
> >>+	} else {
> >>+		memcpy(&pktio_entry->s.stats, &cur, sizeof(odp_pktio_stats_t));
> >>+	}
> >>+
> >>+	return err;
> >>+}
> >>+
> >>  const pktio_if_ops_t sock_mmsg_pktio_ops = {
> >>  	.init = NULL,
> >>  	.term = NULL,
> >>@@ -474,6 +531,8 @@ const pktio_if_ops_t sock_mmsg_pktio_ops = {
> >>  	.close = sock_close,
> >>  	.start = NULL,
> >>  	.stop = NULL,
> >>+	.stats = sock_stats,
> >>+	.stats_reset = sock_stats_reset,
> >>  	.recv = sock_mmsg_recv,
> >>  	.send = sock_mmsg_send,
> >>  	.mtu_get = sock_mtu_get,
> >>diff --git a/platform/linux-generic/pktio/socket_mmap.c b/platform/linux-generic/pktio/socket_mmap.c
> >>index 79ff82d..2031485 100644
> >>--- a/platform/linux-generic/pktio/socket_mmap.c
> >>+++ b/platform/linux-generic/pktio/socket_mmap.c
> >>@@ -467,6 +467,12 @@ static int sock_mmap_open(odp_pktio_t id ODP_UNUSED,
> >>  			goto error;
> >>  	}
> >>+	ret = sock_stats_reset(pktio_entry);
> >>+	if (ret != 0) {
> >>+		ODP_ERR("reset stats\n");
> >>+		goto error;
> >>+	}
> >>+
> >>  	return 0;
> >>  error:
> >>@@ -525,6 +531,8 @@ const pktio_if_ops_t sock_mmap_pktio_ops = {
> >>  	.close = sock_mmap_close,
> >>  	.start = NULL,
> >>  	.stop = NULL,
> >>+	.stats = sock_stats,
> >>+	.stats_reset = sock_stats_reset,
> >>  	.recv = sock_mmap_recv,
> >>  	.send = sock_mmap_send,
> >>  	.mtu_get = sock_mmap_mtu_get,
> >>diff --git a/platform/linux-generic/pktio/stats_sysfs.c b/platform/linux-generic/pktio/stats_sysfs.c
> >>new file mode 100644
> >>index 0000000..5033b7e
> >>--- /dev/null
> >>+++ b/platform/linux-generic/pktio/stats_sysfs.c
> >>@@ -0,0 +1,69 @@
> >>+/* Copyright (c) 2015, Linaro Limited
> >>+ * All rights reserved.
> >>+ *
> >>+ * SPDX-License-Identifier:     BSD-3-Clause
> >>+ */
> >>+
> >>+#include <odp.h>
> >>+#include <odp_packet_io_internal.h>
> >>+#include <stdio.h>
> >>+
> >>+static uint64_t sysfs_get_val(const char *fname)
> >>+{
> >>+	FILE  *file;
> >>+	char str[128];
> >>+	uint64_t ret = -1;
> >>+
> >>+	file = fopen(fname, "rt");
> >>+	if (file == NULL) {
> >>+		/* File not found */
> >>+		return 0;
> >>+	}
> >>+
> >>+	if (fgets(str, sizeof(str), file) != NULL) {
> >>+		/* Read cache line size */
> >??
> 
> thanks, copy paste error :)
> >
> >>+		if (sscanf(str, "%" SCNx64, &ret) != 1) {
> >>+			ODP_ERR("read %s\n", fname);
> >>+			ret = 0;
> >>+		}
> >>+	}
> >>+
> >>+	(void)fclose(file);
> >>+	return ret;
> >>+}
> >Error handling is a bit broken here, 0 isn't a good failure indication.
> >It would be better to return an int and take a uint64_t* parameter.
> 
> For unsupported counters we just have 0. So 0 here is not an error,
> it's just zero.
> 

But things may fail for a reason other than the counter being
unsupported, e.g. out of file descriptors. The odp_pktio_stats() API
is supposed to return <0 on failure and it can't do properly if failures
are squashed here.

> >Although I think it would actually be better to use ETHTOOL_GSTATS as
> >that should work for netmap too, in which case this helper wouldn't be
> >needed.
> 
> By doing that we should be depend on ethtool headers. And quite tricky
> to rewrite eththool GPLv2 functions to code compatible BSD. Simple copy
> paste will not work and I'm know how to rewrite it in different manner.
> 

Hrm, but we already include other kernel headers. I *think* this is OK,
but a quick google search only reveals a bunch of noise about bionic's
use of kernel headers without any real conclusion that I could see.

> Sysfs was the most easy solution to read counters for me. Btw are there
> any reasons to not export counters to /sysfs in netmap?
> 

The counters are present, they just don't get updated while the
interface is in netmap mode (i.e. while an application has it open with
nm_open()). I imagine it's down to the fact netmap detaches the interface
from the kernel's stack so the code that manages those counters doesn't
see the packets.

> >>+
> >>+int sysfs_stats(pktio_entry_t *pktio_entry,
> >>+		odp_pktio_stats_t *stats)
> >>+{
> >>+	char fname[256];
> >>+	const char *dev = pktio_entry->s.name;
> >>+
> >>+	sprintf(fname, "/sys/class/net/%s/statistics/rx_bytes", dev);
> >>+	stats->in_octets = sysfs_get_val(fname);
> >>+
> >>+	sprintf(fname, "/sys/class/net/%s/statistics/rx_packets", dev);
> >>+	stats->in_ucast_pkts = sysfs_get_val(fname);
> >>+
> >>+	sprintf(fname, "/sys/class/net/%s/statistics/rx_droppped", dev);
> >>+	stats->in_discards = sysfs_get_val(fname);
> >>+
> >>+	sprintf(fname, "/sys/class/net/%s/statistics/rx_errors", dev);
> >>+	stats->in_errors = sysfs_get_val(fname);
> >>+
> >>+	stats->in_unknown_protos = 0;
> >I presume there's no easy way of getting this information, could do with
> >a comment noting that it's not supported.
> 
> In linux-generic we have software classifier. If it's unable to
> parse packet
> and detect protocol, then that counter have to be increased. I wanted to
> send separate patch for it.
> >

OK, I was just suggesting adding a "not supported" comment for now.

This counter seems a bit odd actually, what layer is it referring to? I
don't think it makes sense to count unknown L3 protocols given that
they may be implemented above the API.

> >>+
> >>+	sprintf(fname, "/sys/class/net/%s/statistics/tx_bytes", dev);
> >>+	stats->out_octets = sysfs_get_val(fname);
> >>+
> >>+	sprintf(fname, "/sys/class/net/%s/statistics/tx_packets", dev);
> >>+	stats->out_ucast_pkts = sysfs_get_val(fname);
> >>+
> >>+	sprintf(fname, "/sys/class/net/%s/statistics/tx_dropped", dev);
> >>+	stats->out_discards = sysfs_get_val(fname);
> >>+
> >>+	sprintf(fname, "/sys/class/net/%s/statistics/tx_errors", dev);
> >>+	stats->out_errors = sysfs_get_val(fname);
> >>+
> >>+	return 0;
> >>+}
> >>+
> >>-- 
> >>1.9.1
> >>
>
Maxim Uvarov Nov. 13, 2015, 1:56 p.m. UTC | #4 
On 11/13/2015 16:45, Stuart Haslam wrote:
> On Thu, Nov 12, 2015 at 05:14:22PM +0300, Maxim Uvarov wrote:
>> On 11/10/2015 21:29, Stuart Haslam wrote:
>>> On Mon, Nov 09, 2015 at 02:21:49PM +0300, Maxim Uvarov wrote:
>>>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
>>>> ---
>>>>   platform/linux-generic/Makefile.am                 |  1 +
>>>>   .../linux-generic/include/odp_packet_io_internal.h |  9 +++
>>>>   platform/linux-generic/odp_packet_io.c             | 53 +++++++++++++++++
>>>>   platform/linux-generic/pktio/socket.c              | 59 ++++++++++++++++++
>>>>   platform/linux-generic/pktio/socket_mmap.c         |  8 +++
>>>>   platform/linux-generic/pktio/stats_sysfs.c         | 69 ++++++++++++++++++++++
>>>>   6 files changed, 199 insertions(+)
>>>>   create mode 100644 platform/linux-generic/pktio/stats_sysfs.c
>>>>
>>>> diff --git a/platform/linux-generic/Makefile.am b/platform/linux-generic/Makefile.am
>>>> index 610c79c..e3e4954 100644
>>>> --- a/platform/linux-generic/Makefile.am
>>>> +++ b/platform/linux-generic/Makefile.am
>>>> @@ -170,6 +170,7 @@ __LIB__libodp_la_SOURCES = \
>>>>   			   pktio/netmap.c \
>>>>   			   pktio/socket.c \
>>>>   			   pktio/socket_mmap.c \
>>>> +			   pktio/stats_sysfs.c \
>>>>   			   odp_pool.c \
>>>>   			   odp_queue.c \
>>>>   			   odp_rwlock.c \
>>>> diff --git a/platform/linux-generic/include/odp_packet_io_internal.h b/platform/linux-generic/include/odp_packet_io_internal.h
>>>> index 1a1118c..ec43e87 100644
>>>> --- a/platform/linux-generic/include/odp_packet_io_internal.h
>>>> +++ b/platform/linux-generic/include/odp_packet_io_internal.h
>>>> @@ -84,6 +84,7 @@ struct pktio_entry {
>>>>   		STATE_STOP
>>>>   	} state;
>>>>   	classifier_t cls;		/**< classifier linked with this pktio*/
>>>> +	odp_pktio_stats_t stats;	/**< statistic counters for pktio */
>>>>   	char name[PKTIO_NAME_LEN];	/**< name of pktio provided to
>>>>   					   pktio_open() */
>>>>   	odp_pktio_param_t param;
>>>> @@ -107,6 +108,8 @@ typedef struct pktio_if_ops {
>>>>   	int (*close)(pktio_entry_t *pktio_entry);
>>>>   	int (*start)(pktio_entry_t *pktio_entry);
>>>>   	int (*stop)(pktio_entry_t *pktio_entry);
>>>> +	int (*stats)(pktio_entry_t *pktio_entry, odp_pktio_stats_t *stats);
>>>> +	int (*stats_reset)(pktio_entry_t *pktio_entry);
>>>>   	int (*recv)(pktio_entry_t *pktio_entry, odp_packet_t pkt_table[],
>>>>   		    unsigned len);
>>>>   	int (*send)(pktio_entry_t *pktio_entry, odp_packet_t pkt_table[],
>>>> @@ -159,6 +162,12 @@ extern const pktio_if_ops_t pcap_pktio_ops;
>>>>   #endif
>>>>   extern const pktio_if_ops_t * const pktio_if_ops[];
>>>> +int sysfs_stats(pktio_entry_t *pktio_entry,
>>>> +		odp_pktio_stats_t *stats);
>>>> +int sock_stats_reset(pktio_entry_t *pktio_entry);
>>>> +int sock_stats(pktio_entry_t *pktio_entry,
>>>> +	       odp_pktio_stats_t *stats);
>>>> +
>>>>   #ifdef __cplusplus
>>>>   }
>>>>   #endif
>>>> diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c
>>>> index 3ef400f..ba97629 100644
>>>> --- a/platform/linux-generic/odp_packet_io.c
>>>> +++ b/platform/linux-generic/odp_packet_io.c
>>>> @@ -862,3 +862,56 @@ void odp_pktio_print(odp_pktio_t id)
>>>>   	ODP_PRINT("\n%s\n", str);
>>>>   }
>>>> +
>>>> +int odp_pktio_stats(odp_pktio_t pktio,
>>>> +		    odp_pktio_stats_t *stats)
>>>> +{
>>>> +	pktio_entry_t *entry;
>>>> +	int ret = -1;
>>>> +
>>>> +	entry = get_pktio_entry(pktio);
>>>> +	if (entry == NULL) {
>>>> +		ODP_DBG("pktio entry %d does not exist\n", pktio);
>>>> +		return -1;
>>>> +	}
>>>> +
>>>> +	lock_entry(entry);
>>>> +
>>>> +	if (odp_unlikely(is_free(entry))) {
>>>> +		unlock_entry(entry);
>>>> +		ODP_DBG("already freed pktio\n");
>>>> +		return -1;
>>>> +	}
>>>> +
>>>> +	if (entry->s.ops->stats)
>>>> +		ret = entry->s.ops->stats(entry, stats);
>>>> +	unlock_entry(entry);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +int odp_pktio_stats_reset(odp_pktio_t pktio)
>>>> +{
>>>> +	pktio_entry_t *entry;
>>>> +	int ret = -1;
>>>> +
>>>> +	entry = get_pktio_entry(pktio);
>>>> +	if (entry == NULL) {
>>>> +		ODP_DBG("pktio entry %d does not exist\n", pktio);
>>>> +		return -1;
>>>> +	}
>>>> +
>>>> +	lock_entry(entry);
>>>> +
>>>> +	if (odp_unlikely(is_free(entry))) {
>>>> +		unlock_entry(entry);
>>>> +		ODP_DBG("already freed pktio\n");
>>>> +		return -1;
>>>> +	}
>>>> +
>>>> +	if (entry->s.ops->stats)
>>>> +		ret = entry->s.ops->stats_reset(entry);
>>>> +	unlock_entry(entry);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> diff --git a/platform/linux-generic/pktio/socket.c b/platform/linux-generic/pktio/socket.c
>>>> index 5f5e0ae..0bda57b 100644
>>>> --- a/platform/linux-generic/pktio/socket.c
>>>> +++ b/platform/linux-generic/pktio/socket.c
>>>> @@ -254,6 +254,12 @@ static int sock_setup_pkt(pktio_entry_t *pktio_entry, const char *netdev,
>>>>   		goto error;
>>>>   	}
>>>> +	err = sock_stats_reset(pktio_entry);
>>>> +	if (err != 0) {
>>>> +		ODP_ERR("reset stats\n");
>>>> +		goto error;
>>>> +	}
>>>> +
>>>>   	return 0;
>>>>   error:
>>>> @@ -467,6 +473,57 @@ static int sock_promisc_mode_get(pktio_entry_t *pktio_entry)
>>>>   				   pktio_entry->s.name);
>>>>   }
>>>> +int sock_stats(pktio_entry_t *pktio_entry,
>>>> +	       odp_pktio_stats_t *stats)
>>>> +{
>>>> +	odp_pktio_stats_t cur_stats;
>>>> +	int ret;
>>>> +
>>>> +	memset(&cur_stats, 0, sizeof(odp_pktio_stats_t));
>>>> +	ret = sysfs_stats(pktio_entry, &cur_stats);
>>>> +	if (ret)
>>>> +		ODP_ABORT("sysfs stats error\n");
>>>> +
>>>> +	stats->in_octets = cur_stats.in_octets -
>>>> +				pktio_entry->s.stats.in_octets;
>>>> +	stats->in_ucast_pkts = cur_stats.in_ucast_pkts -
>>>> +				pktio_entry->s.stats.in_ucast_pkts;
>>>> +	stats->in_discards = cur_stats.in_discards -
>>>> +				pktio_entry->s.stats.in_discards;
>>>> +	stats->in_errors = cur_stats.in_errors -
>>>> +				pktio_entry->s.stats.in_errors;
>>>> +	stats->in_unknown_protos = cur_stats.in_unknown_protos -
>>>> +				pktio_entry->s.stats.in_unknown_protos;
>>>> +
>>>> +	stats->out_octets = cur_stats.out_octets -
>>>> +				pktio_entry->s.stats.out_octets;
>>>> +	stats->out_ucast_pkts = cur_stats.out_ucast_pkts -
>>>> +				pktio_entry->s.stats.out_ucast_pkts;
>>>> +	stats->out_discards = cur_stats.out_discards -
>>>> +				pktio_entry->s.stats.out_discards;
>>>> +	stats->out_errors = cur_stats.out_errors -
>>>> +				pktio_entry->s.stats.out_errors;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +int sock_stats_reset(pktio_entry_t *pktio_entry)
>>>> +{
>>>> +	int err = 0;
>>>> +	odp_pktio_stats_t cur;
>>>> +
>>>> +	memset(&cur, 0, sizeof(odp_pktio_stats_t));
>>>> +	err = sysfs_stats(pktio_entry, &cur);
>>>> +	if (err != 0) {
>>>> +		__odp_errno = errno;
>>>> +		ODP_ERR("sysfs stats error\n");
>>>> +	} else {
>>>> +		memcpy(&pktio_entry->s.stats, &cur, sizeof(odp_pktio_stats_t));
>>>> +	}
>>>> +
>>>> +	return err;
>>>> +}
>>>> +
>>>>   const pktio_if_ops_t sock_mmsg_pktio_ops = {
>>>>   	.init = NULL,
>>>>   	.term = NULL,
>>>> @@ -474,6 +531,8 @@ const pktio_if_ops_t sock_mmsg_pktio_ops = {
>>>>   	.close = sock_close,
>>>>   	.start = NULL,
>>>>   	.stop = NULL,
>>>> +	.stats = sock_stats,
>>>> +	.stats_reset = sock_stats_reset,
>>>>   	.recv = sock_mmsg_recv,
>>>>   	.send = sock_mmsg_send,
>>>>   	.mtu_get = sock_mtu_get,
>>>> diff --git a/platform/linux-generic/pktio/socket_mmap.c b/platform/linux-generic/pktio/socket_mmap.c
>>>> index 79ff82d..2031485 100644
>>>> --- a/platform/linux-generic/pktio/socket_mmap.c
>>>> +++ b/platform/linux-generic/pktio/socket_mmap.c
>>>> @@ -467,6 +467,12 @@ static int sock_mmap_open(odp_pktio_t id ODP_UNUSED,
>>>>   			goto error;
>>>>   	}
>>>> +	ret = sock_stats_reset(pktio_entry);
>>>> +	if (ret != 0) {
>>>> +		ODP_ERR("reset stats\n");
>>>> +		goto error;
>>>> +	}
>>>> +
>>>>   	return 0;
>>>>   error:
>>>> @@ -525,6 +531,8 @@ const pktio_if_ops_t sock_mmap_pktio_ops = {
>>>>   	.close = sock_mmap_close,
>>>>   	.start = NULL,
>>>>   	.stop = NULL,
>>>> +	.stats = sock_stats,
>>>> +	.stats_reset = sock_stats_reset,
>>>>   	.recv = sock_mmap_recv,
>>>>   	.send = sock_mmap_send,
>>>>   	.mtu_get = sock_mmap_mtu_get,
>>>> diff --git a/platform/linux-generic/pktio/stats_sysfs.c b/platform/linux-generic/pktio/stats_sysfs.c
>>>> new file mode 100644
>>>> index 0000000..5033b7e
>>>> --- /dev/null
>>>> +++ b/platform/linux-generic/pktio/stats_sysfs.c
>>>> @@ -0,0 +1,69 @@
>>>> +/* Copyright (c) 2015, Linaro Limited
>>>> + * All rights reserved.
>>>> + *
>>>> + * SPDX-License-Identifier:     BSD-3-Clause
>>>> + */
>>>> +
>>>> +#include <odp.h>
>>>> +#include <odp_packet_io_internal.h>
>>>> +#include <stdio.h>
>>>> +
>>>> +static uint64_t sysfs_get_val(const char *fname)
>>>> +{
>>>> +	FILE  *file;
>>>> +	char str[128];
>>>> +	uint64_t ret = -1;
>>>> +
>>>> +	file = fopen(fname, "rt");
>>>> +	if (file == NULL) {
>>>> +		/* File not found */
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	if (fgets(str, sizeof(str), file) != NULL) {
>>>> +		/* Read cache line size */
>>> ??
>> thanks, copy paste error :)
>>>> +		if (sscanf(str, "%" SCNx64, &ret) != 1) {
>>>> +			ODP_ERR("read %s\n", fname);
>>>> +			ret = 0;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	(void)fclose(file);
>>>> +	return ret;
>>>> +}
>>> Error handling is a bit broken here, 0 isn't a good failure indication.
>>> It would be better to return an int and take a uint64_t* parameter.
>> For unsupported counters we just have 0. So 0 here is not an error,
>> it's just zero.
>>
> But things may fail for a reason other than the counter being
> unsupported, e.g. out of file descriptors. The odp_pktio_stats() API
> is supposed to return <0 on failure and it can't do properly if failures
> are squashed here.
>
>>> Although I think it would actually be better to use ETHTOOL_GSTATS as
>>> that should work for netmap too, in which case this helper wouldn't be
>>> needed.
>> By doing that we should be depend on ethtool headers. And quite tricky
>> to rewrite eththool GPLv2 functions to code compatible BSD. Simple copy
>> paste will not work and I'm know how to rewrite it in different manner.
>>
> Hrm, but we already include other kernel headers. I *think* this is OK,
> but a quick google search only reveals a bunch of noise about bionic's
> use of kernel headers without any real conclusion that I could see.
>
>> Sysfs was the most easy solution to read counters for me. Btw are there
>> any reasons to not export counters to /sysfs in netmap?
>>
> The counters are present, they just don't get updated while the
> interface is in netmap mode (i.e. while an application has it open with
> nm_open()). I imagine it's down to the fact netmap detaches the interface
> from the kernel's stack so the code that manages those counters doesn't
> see the packets.

Then ethtool is kernel nic driver also. Are you sure that netmap updates it?
It should not for the same reason. I looked at dpdk and they read stats from
nic directly (from pci).

Maxim.

>>>> +
>>>> +int sysfs_stats(pktio_entry_t *pktio_entry,
>>>> +		odp_pktio_stats_t *stats)
>>>> +{
>>>> +	char fname[256];
>>>> +	const char *dev = pktio_entry->s.name;
>>>> +
>>>> +	sprintf(fname, "/sys/class/net/%s/statistics/rx_bytes", dev);
>>>> +	stats->in_octets = sysfs_get_val(fname);
>>>> +
>>>> +	sprintf(fname, "/sys/class/net/%s/statistics/rx_packets", dev);
>>>> +	stats->in_ucast_pkts = sysfs_get_val(fname);
>>>> +
>>>> +	sprintf(fname, "/sys/class/net/%s/statistics/rx_droppped", dev);
>>>> +	stats->in_discards = sysfs_get_val(fname);
>>>> +
>>>> +	sprintf(fname, "/sys/class/net/%s/statistics/rx_errors", dev);
>>>> +	stats->in_errors = sysfs_get_val(fname);
>>>> +
>>>> +	stats->in_unknown_protos = 0;
>>> I presume there's no easy way of getting this information, could do with
>>> a comment noting that it's not supported.
>> In linux-generic we have software classifier. If it's unable to
>> parse packet
>> and detect protocol, then that counter have to be increased. I wanted to
>> send separate patch for it.
> OK, I was just suggesting adding a "not supported" comment for now.
>
> This counter seems a bit odd actually, what layer is it referring to? I
> don't think it makes sense to count unknown L3 protocols given that
> they may be implemented above the API.
>
>>>> +
>>>> +	sprintf(fname, "/sys/class/net/%s/statistics/tx_bytes", dev);
>>>> +	stats->out_octets = sysfs_get_val(fname);
>>>> +
>>>> +	sprintf(fname, "/sys/class/net/%s/statistics/tx_packets", dev);
>>>> +	stats->out_ucast_pkts = sysfs_get_val(fname);
>>>> +
>>>> +	sprintf(fname, "/sys/class/net/%s/statistics/tx_dropped", dev);
>>>> +	stats->out_discards = sysfs_get_val(fname);
>>>> +
>>>> +	sprintf(fname, "/sys/class/net/%s/statistics/tx_errors", dev);
>>>> +	stats->out_errors = sysfs_get_val(fname);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> -- 
>>>> 1.9.1
>>>>
Stuart Haslam Nov. 13, 2015, 3:06 p.m. UTC | #5 
On Fri, Nov 13, 2015 at 04:56:58PM +0300, Maxim Uvarov wrote:
> On 11/13/2015 16:45, Stuart Haslam wrote:
> >On Thu, Nov 12, 2015 at 05:14:22PM +0300, Maxim Uvarov wrote:
> >>On 11/10/2015 21:29, Stuart Haslam wrote:
> >>>On Mon, Nov 09, 2015 at 02:21:49PM +0300, Maxim Uvarov wrote:
> >>>>Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> >>>>---
> >>>>  platform/linux-generic/Makefile.am                 |  1 +
> >>>>  .../linux-generic/include/odp_packet_io_internal.h |  9 +++
> >>>>  platform/linux-generic/odp_packet_io.c             | 53 +++++++++++++++++
> >>>>  platform/linux-generic/pktio/socket.c              | 59 ++++++++++++++++++
> >>>>  platform/linux-generic/pktio/socket_mmap.c         |  8 +++
> >>>>  platform/linux-generic/pktio/stats_sysfs.c         | 69 ++++++++++++++++++++++
> >>>>  6 files changed, 199 insertions(+)
> >>>>  create mode 100644 platform/linux-generic/pktio/stats_sysfs.c
> >>>>
> >>>>diff --git a/platform/linux-generic/Makefile.am b/platform/linux-generic/Makefile.am
> >>>>index 610c79c..e3e4954 100644
> >>>>--- a/platform/linux-generic/Makefile.am
> >>>>+++ b/platform/linux-generic/Makefile.am
> >>>>@@ -170,6 +170,7 @@ __LIB__libodp_la_SOURCES = \
> >>>>  			   pktio/netmap.c \
> >>>>  			   pktio/socket.c \
> >>>>  			   pktio/socket_mmap.c \
> >>>>+			   pktio/stats_sysfs.c \
> >>>>  			   odp_pool.c \
> >>>>  			   odp_queue.c \
> >>>>  			   odp_rwlock.c \
> >>>>diff --git a/platform/linux-generic/include/odp_packet_io_internal.h b/platform/linux-generic/include/odp_packet_io_internal.h
> >>>>index 1a1118c..ec43e87 100644
> >>>>--- a/platform/linux-generic/include/odp_packet_io_internal.h
> >>>>+++ b/platform/linux-generic/include/odp_packet_io_internal.h
> >>>>@@ -84,6 +84,7 @@ struct pktio_entry {
> >>>>  		STATE_STOP
> >>>>  	} state;
> >>>>  	classifier_t cls;		/**< classifier linked with this pktio*/
> >>>>+	odp_pktio_stats_t stats;	/**< statistic counters for pktio */
> >>>>  	char name[PKTIO_NAME_LEN];	/**< name of pktio provided to
> >>>>  					   pktio_open() */
> >>>>  	odp_pktio_param_t param;
> >>>>@@ -107,6 +108,8 @@ typedef struct pktio_if_ops {
> >>>>  	int (*close)(pktio_entry_t *pktio_entry);
> >>>>  	int (*start)(pktio_entry_t *pktio_entry);
> >>>>  	int (*stop)(pktio_entry_t *pktio_entry);
> >>>>+	int (*stats)(pktio_entry_t *pktio_entry, odp_pktio_stats_t *stats);
> >>>>+	int (*stats_reset)(pktio_entry_t *pktio_entry);
> >>>>  	int (*recv)(pktio_entry_t *pktio_entry, odp_packet_t pkt_table[],
> >>>>  		    unsigned len);
> >>>>  	int (*send)(pktio_entry_t *pktio_entry, odp_packet_t pkt_table[],
> >>>>@@ -159,6 +162,12 @@ extern const pktio_if_ops_t pcap_pktio_ops;
> >>>>  #endif
> >>>>  extern const pktio_if_ops_t * const pktio_if_ops[];
> >>>>+int sysfs_stats(pktio_entry_t *pktio_entry,
> >>>>+		odp_pktio_stats_t *stats);
> >>>>+int sock_stats_reset(pktio_entry_t *pktio_entry);
> >>>>+int sock_stats(pktio_entry_t *pktio_entry,
> >>>>+	       odp_pktio_stats_t *stats);
> >>>>+
> >>>>  #ifdef __cplusplus
> >>>>  }
> >>>>  #endif
> >>>>diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c
> >>>>index 3ef400f..ba97629 100644
> >>>>--- a/platform/linux-generic/odp_packet_io.c
> >>>>+++ b/platform/linux-generic/odp_packet_io.c
> >>>>@@ -862,3 +862,56 @@ void odp_pktio_print(odp_pktio_t id)
> >>>>  	ODP_PRINT("\n%s\n", str);
> >>>>  }
> >>>>+
> >>>>+int odp_pktio_stats(odp_pktio_t pktio,
> >>>>+		    odp_pktio_stats_t *stats)
> >>>>+{
> >>>>+	pktio_entry_t *entry;
> >>>>+	int ret = -1;
> >>>>+
> >>>>+	entry = get_pktio_entry(pktio);
> >>>>+	if (entry == NULL) {
> >>>>+		ODP_DBG("pktio entry %d does not exist\n", pktio);
> >>>>+		return -1;
> >>>>+	}
> >>>>+
> >>>>+	lock_entry(entry);
> >>>>+
> >>>>+	if (odp_unlikely(is_free(entry))) {
> >>>>+		unlock_entry(entry);
> >>>>+		ODP_DBG("already freed pktio\n");
> >>>>+		return -1;
> >>>>+	}
> >>>>+
> >>>>+	if (entry->s.ops->stats)
> >>>>+		ret = entry->s.ops->stats(entry, stats);
> >>>>+	unlock_entry(entry);
> >>>>+
> >>>>+	return ret;
> >>>>+}
> >>>>+
> >>>>+int odp_pktio_stats_reset(odp_pktio_t pktio)
> >>>>+{
> >>>>+	pktio_entry_t *entry;
> >>>>+	int ret = -1;
> >>>>+
> >>>>+	entry = get_pktio_entry(pktio);
> >>>>+	if (entry == NULL) {
> >>>>+		ODP_DBG("pktio entry %d does not exist\n", pktio);
> >>>>+		return -1;
> >>>>+	}
> >>>>+
> >>>>+	lock_entry(entry);
> >>>>+
> >>>>+	if (odp_unlikely(is_free(entry))) {
> >>>>+		unlock_entry(entry);
> >>>>+		ODP_DBG("already freed pktio\n");
> >>>>+		return -1;
> >>>>+	}
> >>>>+
> >>>>+	if (entry->s.ops->stats)
> >>>>+		ret = entry->s.ops->stats_reset(entry);
> >>>>+	unlock_entry(entry);
> >>>>+
> >>>>+	return ret;
> >>>>+}
> >>>>diff --git a/platform/linux-generic/pktio/socket.c b/platform/linux-generic/pktio/socket.c
> >>>>index 5f5e0ae..0bda57b 100644
> >>>>--- a/platform/linux-generic/pktio/socket.c
> >>>>+++ b/platform/linux-generic/pktio/socket.c
> >>>>@@ -254,6 +254,12 @@ static int sock_setup_pkt(pktio_entry_t *pktio_entry, const char *netdev,
> >>>>  		goto error;
> >>>>  	}
> >>>>+	err = sock_stats_reset(pktio_entry);
> >>>>+	if (err != 0) {
> >>>>+		ODP_ERR("reset stats\n");
> >>>>+		goto error;
> >>>>+	}
> >>>>+
> >>>>  	return 0;
> >>>>  error:
> >>>>@@ -467,6 +473,57 @@ static int sock_promisc_mode_get(pktio_entry_t *pktio_entry)
> >>>>  				   pktio_entry->s.name);
> >>>>  }
> >>>>+int sock_stats(pktio_entry_t *pktio_entry,
> >>>>+	       odp_pktio_stats_t *stats)
> >>>>+{
> >>>>+	odp_pktio_stats_t cur_stats;
> >>>>+	int ret;
> >>>>+
> >>>>+	memset(&cur_stats, 0, sizeof(odp_pktio_stats_t));
> >>>>+	ret = sysfs_stats(pktio_entry, &cur_stats);
> >>>>+	if (ret)
> >>>>+		ODP_ABORT("sysfs stats error\n");
> >>>>+
> >>>>+	stats->in_octets = cur_stats.in_octets -
> >>>>+				pktio_entry->s.stats.in_octets;
> >>>>+	stats->in_ucast_pkts = cur_stats.in_ucast_pkts -
> >>>>+				pktio_entry->s.stats.in_ucast_pkts;
> >>>>+	stats->in_discards = cur_stats.in_discards -
> >>>>+				pktio_entry->s.stats.in_discards;
> >>>>+	stats->in_errors = cur_stats.in_errors -
> >>>>+				pktio_entry->s.stats.in_errors;
> >>>>+	stats->in_unknown_protos = cur_stats.in_unknown_protos -
> >>>>+				pktio_entry->s.stats.in_unknown_protos;
> >>>>+
> >>>>+	stats->out_octets = cur_stats.out_octets -
> >>>>+				pktio_entry->s.stats.out_octets;
> >>>>+	stats->out_ucast_pkts = cur_stats.out_ucast_pkts -
> >>>>+				pktio_entry->s.stats.out_ucast_pkts;
> >>>>+	stats->out_discards = cur_stats.out_discards -
> >>>>+				pktio_entry->s.stats.out_discards;
> >>>>+	stats->out_errors = cur_stats.out_errors -
> >>>>+				pktio_entry->s.stats.out_errors;
> >>>>+
> >>>>+	return 0;
> >>>>+}
> >>>>+
> >>>>+int sock_stats_reset(pktio_entry_t *pktio_entry)
> >>>>+{
> >>>>+	int err = 0;
> >>>>+	odp_pktio_stats_t cur;
> >>>>+
> >>>>+	memset(&cur, 0, sizeof(odp_pktio_stats_t));
> >>>>+	err = sysfs_stats(pktio_entry, &cur);
> >>>>+	if (err != 0) {
> >>>>+		__odp_errno = errno;
> >>>>+		ODP_ERR("sysfs stats error\n");
> >>>>+	} else {
> >>>>+		memcpy(&pktio_entry->s.stats, &cur, sizeof(odp_pktio_stats_t));
> >>>>+	}
> >>>>+
> >>>>+	return err;
> >>>>+}
> >>>>+
> >>>>  const pktio_if_ops_t sock_mmsg_pktio_ops = {
> >>>>  	.init = NULL,
> >>>>  	.term = NULL,
> >>>>@@ -474,6 +531,8 @@ const pktio_if_ops_t sock_mmsg_pktio_ops = {
> >>>>  	.close = sock_close,
> >>>>  	.start = NULL,
> >>>>  	.stop = NULL,
> >>>>+	.stats = sock_stats,
> >>>>+	.stats_reset = sock_stats_reset,
> >>>>  	.recv = sock_mmsg_recv,
> >>>>  	.send = sock_mmsg_send,
> >>>>  	.mtu_get = sock_mtu_get,
> >>>>diff --git a/platform/linux-generic/pktio/socket_mmap.c b/platform/linux-generic/pktio/socket_mmap.c
> >>>>index 79ff82d..2031485 100644
> >>>>--- a/platform/linux-generic/pktio/socket_mmap.c
> >>>>+++ b/platform/linux-generic/pktio/socket_mmap.c
> >>>>@@ -467,6 +467,12 @@ static int sock_mmap_open(odp_pktio_t id ODP_UNUSED,
> >>>>  			goto error;
> >>>>  	}
> >>>>+	ret = sock_stats_reset(pktio_entry);
> >>>>+	if (ret != 0) {
> >>>>+		ODP_ERR("reset stats\n");
> >>>>+		goto error;
> >>>>+	}
> >>>>+
> >>>>  	return 0;
> >>>>  error:
> >>>>@@ -525,6 +531,8 @@ const pktio_if_ops_t sock_mmap_pktio_ops = {
> >>>>  	.close = sock_mmap_close,
> >>>>  	.start = NULL,
> >>>>  	.stop = NULL,
> >>>>+	.stats = sock_stats,
> >>>>+	.stats_reset = sock_stats_reset,
> >>>>  	.recv = sock_mmap_recv,
> >>>>  	.send = sock_mmap_send,
> >>>>  	.mtu_get = sock_mmap_mtu_get,
> >>>>diff --git a/platform/linux-generic/pktio/stats_sysfs.c b/platform/linux-generic/pktio/stats_sysfs.c
> >>>>new file mode 100644
> >>>>index 0000000..5033b7e
> >>>>--- /dev/null
> >>>>+++ b/platform/linux-generic/pktio/stats_sysfs.c
> >>>>@@ -0,0 +1,69 @@
> >>>>+/* Copyright (c) 2015, Linaro Limited
> >>>>+ * All rights reserved.
> >>>>+ *
> >>>>+ * SPDX-License-Identifier:     BSD-3-Clause
> >>>>+ */
> >>>>+
> >>>>+#include <odp.h>
> >>>>+#include <odp_packet_io_internal.h>
> >>>>+#include <stdio.h>
> >>>>+
> >>>>+static uint64_t sysfs_get_val(const char *fname)
> >>>>+{
> >>>>+	FILE  *file;
> >>>>+	char str[128];
> >>>>+	uint64_t ret = -1;
> >>>>+
> >>>>+	file = fopen(fname, "rt");
> >>>>+	if (file == NULL) {
> >>>>+		/* File not found */
> >>>>+		return 0;
> >>>>+	}
> >>>>+
> >>>>+	if (fgets(str, sizeof(str), file) != NULL) {
> >>>>+		/* Read cache line size */
> >>>??
> >>thanks, copy paste error :)
> >>>>+		if (sscanf(str, "%" SCNx64, &ret) != 1) {
> >>>>+			ODP_ERR("read %s\n", fname);
> >>>>+			ret = 0;
> >>>>+		}
> >>>>+	}
> >>>>+
> >>>>+	(void)fclose(file);
> >>>>+	return ret;
> >>>>+}
> >>>Error handling is a bit broken here, 0 isn't a good failure indication.
> >>>It would be better to return an int and take a uint64_t* parameter.
> >>For unsupported counters we just have 0. So 0 here is not an error,
> >>it's just zero.
> >>
> >But things may fail for a reason other than the counter being
> >unsupported, e.g. out of file descriptors. The odp_pktio_stats() API
> >is supposed to return <0 on failure and it can't do properly if failures
> >are squashed here.
> >
> >>>Although I think it would actually be better to use ETHTOOL_GSTATS as
> >>>that should work for netmap too, in which case this helper wouldn't be
> >>>needed.
> >>By doing that we should be depend on ethtool headers. And quite tricky
> >>to rewrite eththool GPLv2 functions to code compatible BSD. Simple copy
> >>paste will not work and I'm know how to rewrite it in different manner.
> >>
> >Hrm, but we already include other kernel headers. I *think* this is OK,
> >but a quick google search only reveals a bunch of noise about bionic's
> >use of kernel headers without any real conclusion that I could see.
> >
> >>Sysfs was the most easy solution to read counters for me. Btw are there
> >>any reasons to not export counters to /sysfs in netmap?
> >>
> >The counters are present, they just don't get updated while the
> >interface is in netmap mode (i.e. while an application has it open with
> >nm_open()). I imagine it's down to the fact netmap detaches the interface
> >from the kernel's stack so the code that manages those counters doesn't
> >see the packets.
> 
> Then ethtool is kernel nic driver also. Are you sure that netmap updates it?

Yes.. and no. It actually seems to only update some of the ethtool stats
while in netmap mode, at least that's what happens on ixgbe -

# ethtool -S eth5 | head
NIC statistics:
     rx_packets: 9182003108
     tx_packets: 22060137
     rx_bytes: 562584746236
     tx_bytes: 1610397009
     rx_pkts_nic: 123315757495
     tx_pkts_nic: 9970128059534
     rx_bytes_nic: 12260214089032
     tx_bytes_nic: 638088481758517

The first 4 don't get updated, the next 4 do. Looking at the driver source

http://lxr.free-electrons.com/source/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c#L64

The IXGBE_NETDEV_STATS()s are the ones reported via sysfs and they don't
get updated. The IXGBE_STAT()s do get updated, but they seem to use
device specific names so are not that useful, unless I'm missing something.

> It should not for the same reason. I looked at dpdk and they read stats from
> nic directly (from pci).

DPDK typically takes full control of the device so they have no choice.
netmap still uses the kernel driver but just redirects some/all of the
queues to userspace, so it can't go poking directly at PCI.

So I'm not sure how to get the stats out for netmap.
diff mbox

Patch

diff --git a/platform/linux-generic/Makefile.am b/platform/linux-generic/Makefile.am
index 610c79c..e3e4954 100644
--- a/platform/linux-generic/Makefile.am
+++ b/platform/linux-generic/Makefile.am
@@ -170,6 +170,7 @@  __LIB__libodp_la_SOURCES = \
 			   pktio/netmap.c \
 			   pktio/socket.c \
 			   pktio/socket_mmap.c \
+			   pktio/stats_sysfs.c \
 			   odp_pool.c \
 			   odp_queue.c \
 			   odp_rwlock.c \
diff --git a/platform/linux-generic/include/odp_packet_io_internal.h b/platform/linux-generic/include/odp_packet_io_internal.h
index 1a1118c..ec43e87 100644
--- a/platform/linux-generic/include/odp_packet_io_internal.h
+++ b/platform/linux-generic/include/odp_packet_io_internal.h
@@ -84,6 +84,7 @@  struct pktio_entry {
 		STATE_STOP
 	} state;
 	classifier_t cls;		/**< classifier linked with this pktio*/
+	odp_pktio_stats_t stats;	/**< statistic counters for pktio */
 	char name[PKTIO_NAME_LEN];	/**< name of pktio provided to
 					   pktio_open() */
 	odp_pktio_param_t param;
@@ -107,6 +108,8 @@  typedef struct pktio_if_ops {
 	int (*close)(pktio_entry_t *pktio_entry);
 	int (*start)(pktio_entry_t *pktio_entry);
 	int (*stop)(pktio_entry_t *pktio_entry);
+	int (*stats)(pktio_entry_t *pktio_entry, odp_pktio_stats_t *stats);
+	int (*stats_reset)(pktio_entry_t *pktio_entry);
 	int (*recv)(pktio_entry_t *pktio_entry, odp_packet_t pkt_table[],
 		    unsigned len);
 	int (*send)(pktio_entry_t *pktio_entry, odp_packet_t pkt_table[],
@@ -159,6 +162,12 @@  extern const pktio_if_ops_t pcap_pktio_ops;
 #endif
 extern const pktio_if_ops_t * const pktio_if_ops[];
 
+int sysfs_stats(pktio_entry_t *pktio_entry,
+		odp_pktio_stats_t *stats);
+int sock_stats_reset(pktio_entry_t *pktio_entry);
+int sock_stats(pktio_entry_t *pktio_entry,
+	       odp_pktio_stats_t *stats);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c
index 3ef400f..ba97629 100644
--- a/platform/linux-generic/odp_packet_io.c
+++ b/platform/linux-generic/odp_packet_io.c
@@ -862,3 +862,56 @@  void odp_pktio_print(odp_pktio_t id)
 
 	ODP_PRINT("\n%s\n", str);
 }
+
+int odp_pktio_stats(odp_pktio_t pktio,
+		    odp_pktio_stats_t *stats)
+{
+	pktio_entry_t *entry;
+	int ret = -1;
+
+	entry = get_pktio_entry(pktio);
+	if (entry == NULL) {
+		ODP_DBG("pktio entry %d does not exist\n", pktio);
+		return -1;
+	}
+
+	lock_entry(entry);
+
+	if (odp_unlikely(is_free(entry))) {
+		unlock_entry(entry);
+		ODP_DBG("already freed pktio\n");
+		return -1;
+	}
+
+	if (entry->s.ops->stats)
+		ret = entry->s.ops->stats(entry, stats);
+	unlock_entry(entry);
+
+	return ret;
+}
+
+int odp_pktio_stats_reset(odp_pktio_t pktio)
+{
+	pktio_entry_t *entry;
+	int ret = -1;
+
+	entry = get_pktio_entry(pktio);
+	if (entry == NULL) {
+		ODP_DBG("pktio entry %d does not exist\n", pktio);
+		return -1;
+	}
+
+	lock_entry(entry);
+
+	if (odp_unlikely(is_free(entry))) {
+		unlock_entry(entry);
+		ODP_DBG("already freed pktio\n");
+		return -1;
+	}
+
+	if (entry->s.ops->stats)
+		ret = entry->s.ops->stats_reset(entry);
+	unlock_entry(entry);
+
+	return ret;
+}
diff --git a/platform/linux-generic/pktio/socket.c b/platform/linux-generic/pktio/socket.c
index 5f5e0ae..0bda57b 100644
--- a/platform/linux-generic/pktio/socket.c
+++ b/platform/linux-generic/pktio/socket.c
@@ -254,6 +254,12 @@  static int sock_setup_pkt(pktio_entry_t *pktio_entry, const char *netdev,
 		goto error;
 	}
 
+	err = sock_stats_reset(pktio_entry);
+	if (err != 0) {
+		ODP_ERR("reset stats\n");
+		goto error;
+	}
+
 	return 0;
 
 error:
@@ -467,6 +473,57 @@  static int sock_promisc_mode_get(pktio_entry_t *pktio_entry)
 				   pktio_entry->s.name);
 }
 
+int sock_stats(pktio_entry_t *pktio_entry,
+	       odp_pktio_stats_t *stats)
+{
+	odp_pktio_stats_t cur_stats;
+	int ret;
+
+	memset(&cur_stats, 0, sizeof(odp_pktio_stats_t));
+	ret = sysfs_stats(pktio_entry, &cur_stats);
+	if (ret)
+		ODP_ABORT("sysfs stats error\n");
+
+	stats->in_octets = cur_stats.in_octets -
+				pktio_entry->s.stats.in_octets;
+	stats->in_ucast_pkts = cur_stats.in_ucast_pkts -
+				pktio_entry->s.stats.in_ucast_pkts;
+	stats->in_discards = cur_stats.in_discards -
+				pktio_entry->s.stats.in_discards;
+	stats->in_errors = cur_stats.in_errors -
+				pktio_entry->s.stats.in_errors;
+	stats->in_unknown_protos = cur_stats.in_unknown_protos -
+				pktio_entry->s.stats.in_unknown_protos;
+
+	stats->out_octets = cur_stats.out_octets -
+				pktio_entry->s.stats.out_octets;
+	stats->out_ucast_pkts = cur_stats.out_ucast_pkts -
+				pktio_entry->s.stats.out_ucast_pkts;
+	stats->out_discards = cur_stats.out_discards -
+				pktio_entry->s.stats.out_discards;
+	stats->out_errors = cur_stats.out_errors -
+				pktio_entry->s.stats.out_errors;
+
+	return 0;
+}
+
+int sock_stats_reset(pktio_entry_t *pktio_entry)
+{
+	int err = 0;
+	odp_pktio_stats_t cur;
+
+	memset(&cur, 0, sizeof(odp_pktio_stats_t));
+	err = sysfs_stats(pktio_entry, &cur);
+	if (err != 0) {
+		__odp_errno = errno;
+		ODP_ERR("sysfs stats error\n");
+	} else {
+		memcpy(&pktio_entry->s.stats, &cur, sizeof(odp_pktio_stats_t));
+	}
+
+	return err;
+}
+
 const pktio_if_ops_t sock_mmsg_pktio_ops = {
 	.init = NULL,
 	.term = NULL,
@@ -474,6 +531,8 @@  const pktio_if_ops_t sock_mmsg_pktio_ops = {
 	.close = sock_close,
 	.start = NULL,
 	.stop = NULL,
+	.stats = sock_stats,
+	.stats_reset = sock_stats_reset,
 	.recv = sock_mmsg_recv,
 	.send = sock_mmsg_send,
 	.mtu_get = sock_mtu_get,
diff --git a/platform/linux-generic/pktio/socket_mmap.c b/platform/linux-generic/pktio/socket_mmap.c
index 79ff82d..2031485 100644
--- a/platform/linux-generic/pktio/socket_mmap.c
+++ b/platform/linux-generic/pktio/socket_mmap.c
@@ -467,6 +467,12 @@  static int sock_mmap_open(odp_pktio_t id ODP_UNUSED,
 			goto error;
 	}
 
+	ret = sock_stats_reset(pktio_entry);
+	if (ret != 0) {
+		ODP_ERR("reset stats\n");
+		goto error;
+	}
+
 	return 0;
 
 error:
@@ -525,6 +531,8 @@  const pktio_if_ops_t sock_mmap_pktio_ops = {
 	.close = sock_mmap_close,
 	.start = NULL,
 	.stop = NULL,
+	.stats = sock_stats,
+	.stats_reset = sock_stats_reset,
 	.recv = sock_mmap_recv,
 	.send = sock_mmap_send,
 	.mtu_get = sock_mmap_mtu_get,
diff --git a/platform/linux-generic/pktio/stats_sysfs.c b/platform/linux-generic/pktio/stats_sysfs.c
new file mode 100644
index 0000000..5033b7e
--- /dev/null
+++ b/platform/linux-generic/pktio/stats_sysfs.c
@@ -0,0 +1,69 @@ 
+/* Copyright (c) 2015, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier:     BSD-3-Clause
+ */
+
+#include <odp.h>
+#include <odp_packet_io_internal.h>
+#include <stdio.h>
+
+static uint64_t sysfs_get_val(const char *fname)
+{
+	FILE  *file;
+	char str[128];
+	uint64_t ret = -1;
+
+	file = fopen(fname, "rt");
+	if (file == NULL) {
+		/* File not found */
+		return 0;
+	}
+
+	if (fgets(str, sizeof(str), file) != NULL) {
+		/* Read cache line size */
+		if (sscanf(str, "%" SCNx64, &ret) != 1) {
+			ODP_ERR("read %s\n", fname);
+			ret = 0;
+		}
+	}
+
+	(void)fclose(file);
+	return ret;
+}
+
+int sysfs_stats(pktio_entry_t *pktio_entry,
+		odp_pktio_stats_t *stats)
+{
+	char fname[256];
+	const char *dev = pktio_entry->s.name;
+
+	sprintf(fname, "/sys/class/net/%s/statistics/rx_bytes", dev);
+	stats->in_octets = sysfs_get_val(fname);
+
+	sprintf(fname, "/sys/class/net/%s/statistics/rx_packets", dev);
+	stats->in_ucast_pkts = sysfs_get_val(fname);
+
+	sprintf(fname, "/sys/class/net/%s/statistics/rx_droppped", dev);
+	stats->in_discards = sysfs_get_val(fname);
+
+	sprintf(fname, "/sys/class/net/%s/statistics/rx_errors", dev);
+	stats->in_errors = sysfs_get_val(fname);
+
+	stats->in_unknown_protos = 0;
+
+	sprintf(fname, "/sys/class/net/%s/statistics/tx_bytes", dev);
+	stats->out_octets = sysfs_get_val(fname);
+
+	sprintf(fname, "/sys/class/net/%s/statistics/tx_packets", dev);
+	stats->out_ucast_pkts = sysfs_get_val(fname);
+
+	sprintf(fname, "/sys/class/net/%s/statistics/tx_dropped", dev);
+	stats->out_discards = sysfs_get_val(fname);
+
+	sprintf(fname, "/sys/class/net/%s/statistics/tx_errors", dev);
+	stats->out_errors = sysfs_get_val(fname);
+
+	return 0;
+}
+