diff mbox

[dpdk-dev,RFC,v2] ethdev: add IF-MIB attributes implementation

Message ID 1499963067-32504-1-git-send-email-reshma.pattan@intel.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Pattan, Reshma July 13, 2017, 4:24 p.m. UTC 
  The RFC shows implementation of IF-MIB attributes in ethdev layer.
These attributes are implemented from the information available from
rte_eth_dev_data struct.

The new APIs are added, to allow applications to trigger IF-MIB attribute
initialization and the implementation based on the port id.
IF-MIB attributes are allocated in shared memory to support multi process
access.

For not to expose IF-MIB attributes structs to user, the structs are kept
internal. Making use of the metrics library to register attributes names to
metrics library and update their values to metrics library by calling
rte_metrics_reg_names() and rte_metrics_update_values() respectively.
Applications should call rte_metrics_get_values() and
rte_metrics_get_names() to view IF-MIB info.

The above approach need, making ethdev librray dependent on metrics
library. Is this acceptable? Comments are welcome.

If not, we can move all the implementation to new library similar to
bitrate and latency libraries. With this approach new library needs
the access to rte_eth_devices[] to access rte_eth_dev_data. But AFAIK
rte_eth_devices[] is not allowed to be accessed outside of PMDs and
ethdev layer. Any comments here?

Looking forward to hear opinions.

Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
---
v2: Corrected typos and description of the commit message.
---
 lib/Makefile                      |   6 +-
 lib/librte_ether/rte_ethdev.c     | 130 +++++++++++++++++++++++++++++++++++++-
 lib/librte_ether/rte_ethdev.h     |   7 ++
 lib/librte_ether/rte_ethdev_pci.h |   4 ++
 mk/rte.app.mk                     |   2 +-
 5 files changed, 144 insertions(+), 5 deletions(-)

Comments

Stephen Hemminger July 14, 2017, 12:07 a.m. UTC | #1 
On Thu, 13 Jul 2017 17:24:27 +0100
Reshma Pattan <reshma.pattan@intel.com> wrote:

> diff --git a/lib/librte_ether/rte_ethdev_pci.h b/lib/librte_ether/rte_ethdev_pci.h
> index 69aab03..f829407 100644
> --- a/lib/librte_ether/rte_ethdev_pci.h
> +++ b/lib/librte_ether/rte_ethdev_pci.h
> @@ -37,6 +37,7 @@
>  #include <rte_malloc.h>
>  #include <rte_pci.h>
>  #include <rte_ethdev.h>
> +#include <rte_cycles.h>
>  
>  /**
>   * Copy pci device info to the Ethernet device data.
> @@ -157,6 +158,9 @@ rte_eth_dev_pci_generic_probe(struct rte_pci_device *pci_dev,
>  
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev_init, -EINVAL);
>  	ret = dev_init(eth_dev);
> +	eth_dev->data->sys_up_time_start = rte_rdtsc();
> +	eth_dev->data->if_counter_discontinuity_time = 0;
> +	eth_dev->data->if_last_change = 0;

Shouldn't this be in base eth_dev layer rather than the  PCI specific code.

If you look in rte_eth_dev_data_alloc, the dev_data is already zeroed.

Also, DPDK in general does BSD style structure names (ie always putting prefix
on structure tags). In BSD, this is a leftover from ancient V6 UNIX where
compiler did no real checking of structure tags. My preference is that
variables and structure names be as short as possible.

The TSC counter is not a good value to use on this anyway. You don't care
about sub-microsecond accuracy and it wraps around. Better to figure out
a better clock source. Is there a DPDK wrapper of clock_gettime(CLOCK_MONOTONIC)?
Thomas Monjalon July 14, 2017, 9:33 a.m. UTC | #2 
14/07/2017 02:07, Stephen Hemminger:
> On Thu, 13 Jul 2017 17:24:27 +0100
> Reshma Pattan <reshma.pattan@intel.com> wrote:
> 
> > diff --git a/lib/librte_ether/rte_ethdev_pci.h b/lib/librte_ether/rte_ethdev_pci.h
> > index 69aab03..f829407 100644
> > --- a/lib/librte_ether/rte_ethdev_pci.h
> > +++ b/lib/librte_ether/rte_ethdev_pci.h
> > @@ -37,6 +37,7 @@
> >  #include <rte_malloc.h>
> >  #include <rte_pci.h>
> >  #include <rte_ethdev.h>
> > +#include <rte_cycles.h>
> >  
> >  /**
> >   * Copy pci device info to the Ethernet device data.
> > @@ -157,6 +158,9 @@ rte_eth_dev_pci_generic_probe(struct rte_pci_device *pci_dev,
> >  
> >  	RTE_FUNC_PTR_OR_ERR_RET(*dev_init, -EINVAL);
> >  	ret = dev_init(eth_dev);
> > +	eth_dev->data->sys_up_time_start = rte_rdtsc();
> > +	eth_dev->data->if_counter_discontinuity_time = 0;
> > +	eth_dev->data->if_last_change = 0;
> 
> Shouldn't this be in base eth_dev layer rather than the  PCI specific code.
> 
> If you look in rte_eth_dev_data_alloc, the dev_data is already zeroed.
> 
> Also, DPDK in general does BSD style structure names (ie always putting prefix
> on structure tags). In BSD, this is a leftover from ancient V6 UNIX where
> compiler did no real checking of structure tags. My preference is that
> variables and structure names be as short as possible.
> 
> The TSC counter is not a good value to use on this anyway. You don't care
> about sub-microsecond accuracy and it wraps around. Better to figure out
> a better clock source. Is there a DPDK wrapper of clock_gettime(CLOCK_MONOTONIC)?

In rte_cycles.h, the TSC is wrapped into rte_get_timer_cycles().
About clock_gettime(CLOCK_MONOTONIC), I think we may need to create
a file rte_clock.h.
diff mbox

Patch

diff --git a/lib/Makefile b/lib/Makefile
index 07e1fd0..aa2be5b 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -35,6 +35,8 @@  DIRS-y += librte_compat
 DIRS-$(CONFIG_RTE_LIBRTE_EAL) += librte_eal
 DIRS-$(CONFIG_RTE_LIBRTE_RING) += librte_ring
 DEPDIRS-librte_ring := librte_eal
+DIRS-$(CONFIG_RTE_LIBRTE_METRICS) += librte_metrics
+DEPDIRS-librte_metrics := librte_eal
 DIRS-$(CONFIG_RTE_LIBRTE_MEMPOOL) += librte_mempool
 DEPDIRS-librte_mempool := librte_eal librte_ring
 DIRS-$(CONFIG_RTE_LIBRTE_MBUF) += librte_mbuf
@@ -46,7 +48,7 @@  DEPDIRS-librte_cfgfile := librte_eal
 DIRS-$(CONFIG_RTE_LIBRTE_CMDLINE) += librte_cmdline
 DEPDIRS-librte_cmdline := librte_eal
 DIRS-$(CONFIG_RTE_LIBRTE_ETHER) += librte_ether
-DEPDIRS-librte_ether := librte_net librte_eal librte_mempool librte_ring
+DEPDIRS-librte_ether := librte_net librte_eal librte_mempool librte_ring librte_metrics
 DEPDIRS-librte_ether += librte_mbuf
 DIRS-$(CONFIG_RTE_LIBRTE_CRYPTODEV) += librte_cryptodev
 DEPDIRS-librte_cryptodev := librte_eal librte_mempool librte_ring librte_mbuf
@@ -70,8 +72,6 @@  DEPDIRS-librte_ip_frag := librte_eal librte_mempool librte_mbuf librte_ether
 DEPDIRS-librte_ip_frag += librte_hash
 DIRS-$(CONFIG_RTE_LIBRTE_JOBSTATS) += librte_jobstats
 DEPDIRS-librte_jobstats := librte_eal
-DIRS-$(CONFIG_RTE_LIBRTE_METRICS) += librte_metrics
-DEPDIRS-librte_metrics := librte_eal
 DIRS-$(CONFIG_RTE_LIBRTE_BITRATE) += librte_bitratestats
 DEPDIRS-librte_bitratestats := librte_eal librte_metrics librte_ether
 DIRS-$(CONFIG_RTE_LIBRTE_LATENCY_STATS) += librte_latencystats
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 71a576c..966b929 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -64,6 +64,8 @@ 
 #include <rte_errno.h>
 #include <rte_spinlock.h>
 #include <rte_string_fns.h>
+#include <rte_cycles.h>
+#include <rte_metrics.h>
 
 #include "rte_ether.h"
 #include "rte_ethdev.h"
@@ -118,7 +120,8 @@  static const struct rte_eth_xstats_name_off rte_txq_stats_strings[] = {
 #define RTE_NB_TXQ_STATS (sizeof(rte_txq_stats_strings) /	\
 		sizeof(rte_txq_stats_strings[0]))
 
-
+#define RTE_MIB_ATTR_NAME_SIZE 32
+#define RTE_IF_TYPE_ETHERNETCSMACD 6
 /**
  * The user application callback description.
  *
@@ -139,6 +142,11 @@  enum {
 	STAT_QMAP_RX
 };
 
+enum {
+	OPER_STATUS_UP = 1,
+	OPER_STATUS_DOWN
+};
+
 uint8_t
 rte_eth_find_next(uint8_t port_id)
 {
@@ -923,6 +931,8 @@  rte_eth_dev_start(uint8_t port_id)
 	if (dev->data->dev_conf.intr_conf.lsc == 0) {
 		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->link_update, -ENOTSUP);
 		(*dev->dev_ops->link_update)(dev, 0);
+		dev->data->if_last_change =
+			rte_rdtsc() - dev->data->sys_up_time_start;
 	}
 	return 0;
 }
@@ -1351,6 +1361,8 @@  rte_eth_stats_reset(uint8_t port_id)
 
 	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->stats_reset);
 	(*dev->dev_ops->stats_reset)(dev);
+	dev->data->if_counter_discontinuity_time =
+		rte_rdtsc() - dev->data->sys_up_time_start;
 	dev->data->rx_mbuf_alloc_failed = 0;
 }
 
@@ -1829,6 +1841,8 @@  rte_eth_xstats_reset(uint8_t port_id)
 	/* implemented by the driver */
 	if (dev->dev_ops->xstats_reset != NULL) {
 		(*dev->dev_ops->xstats_reset)(dev);
+		dev->data->if_counter_discontinuity_time =
+			rte_rdtsc() - dev->data->sys_up_time_start;
 		return;
 	}
 
@@ -3354,3 +3368,117 @@  rte_eth_dev_l2_tunnel_offload_set(uint8_t port_id,
 				-ENOTSUP);
 	return (*dev->dev_ops->l2_tunnel_offload_set)(dev, l2_tunnel, mask, en);
 }
+
+/* IF-MIB attributes*/
+struct rte_if_mib_attrs {
+	uint64_t if_number;			/* ifNumber */
+	uint64_t if_index;			/* ifIndex */
+	uint64_t if_type;			/* ifType */
+	uint64_t if_mtu;			/* ifMtu */
+	uint64_t if_speed;			/* ifSpeed */
+	uint64_t if_phys_address;		/* ifPhysAddress */
+	uint64_t if_oper_status;		/* ifOperStatus */
+	uint64_t if_last_change;		/* ifLastChange */
+	uint64_t if_high_speed;			/* ifHighSpeed */
+	uint64_t if_connector_present;		/* ifConnectorPresent */
+	uint64_t if_counter_discontinuity_time;	/* ifCounterDiscontinuityTime */
+};
+
+static struct rte_if_mib_attrs *if_mib_attrs;
+
+struct if_mib_attrs_nameoff {
+	char name[RTE_MIB_ATTR_NAME_SIZE];
+	unsigned int offset;
+};
+
+static const struct if_mib_attrs_nameoff if_mib_attrs_strings[] = {
+	{"ifNumber", offsetof(struct rte_if_mib_attrs, if_number)},
+	{"ifIndex", offsetof(struct rte_if_mib_attrs, if_index)},
+	{"ifType", offsetof(struct rte_if_mib_attrs, if_type)},
+	{"ifMtu", offsetof(struct rte_if_mib_attrs, if_mtu)},
+	{"ifSpeed", offsetof(struct rte_if_mib_attrs, if_speed)},
+	{"ifPhysAddress", offsetof(struct rte_if_mib_attrs, if_phys_address)},
+	{"ifOperStatus", offsetof(struct rte_if_mib_attrs, if_oper_status)},
+	{"ifLastChange", offsetof(struct rte_if_mib_attrs, if_last_change)},
+	{"ifHighSpeed", offsetof(struct rte_if_mib_attrs, if_high_speed)},
+	{"ifConnectorPresent", offsetof(struct rte_if_mib_attrs,
+		if_connector_present)},
+	{"ifCounterDiscontinuityTime", offsetof(struct rte_if_mib_attrs,
+		if_counter_discontinuity_time)},
+};
+
+#define NUM_IF_MIB_ATTRS (sizeof(if_mib_attrs_strings) / \
+	sizeof(if_mib_attrs_strings[0]))
+
+int
+rte_eth_ifmib_attr_init(void) {
+	if_mib_attrs = rte_zmalloc(NULL, sizeof(struct rte_if_mib_attrs),
+		RTE_CACHE_LINE_SIZE);
+
+	return 0;
+}
+
+int
+rte_eth_ifmib_attr_reg(uint8_t port_id) {
+
+	int idx;
+	unsigned int i;
+	uint64_t *attr_ptr = NULL;
+	const char *ptr_strings[NUM_IF_MIB_ATTRS] = {0};
+	struct rte_eth_dev *eth_dev;
+	struct rte_device *device;
+	struct rte_eth_dev_data *data;
+	uint64_t values[NUM_IF_MIB_ATTRS] = {0};
+
+	for (i = 0; i < NUM_IF_MIB_ATTRS; i++)
+		ptr_strings[i] = if_mib_attrs_strings[i].name;
+
+	/* register names with metrics library */
+	idx = rte_metrics_reg_names(ptr_strings, NUM_IF_MIB_ATTRS);
+
+	eth_dev = eth_dev_get(port_id);
+	data = eth_dev->data;
+	device = eth_dev->device;
+
+	/* implement if mib attributes */
+	if_mib_attrs->if_number = rte_eth_dev_count();
+
+	if_mib_attrs->if_index = data->port_id + 1;
+
+	if_mib_attrs->if_type = RTE_IF_TYPE_ETHERNETCSMACD;
+
+	if_mib_attrs->if_mtu = data->mtu;
+
+	if_mib_attrs->if_speed =
+			(data->dev_link.link_speed < (UINT32_MAX / 1000000)) ?
+			(data->dev_link.link_speed * 1000000) : UINT32_MAX;
+
+	if_mib_attrs->if_phys_address = 0;
+	ether_addr_copy(data->mac_addrs,
+		(struct ether_addr *)if_mib_attrs->if_phys_address);
+
+	if_mib_attrs->if_oper_status = data->dev_link.link_status ?
+					OPER_STATUS_UP : OPER_STATUS_DOWN;
+
+	if_mib_attrs->if_last_change =
+			data->if_last_change / (rte_get_tsc_hz() * 100);
+
+	if_mib_attrs->if_high_speed = data->dev_link.link_speed;
+
+	if_mib_attrs->if_connector_present =
+		(device->devargs->type == RTE_DEVTYPE_VIRTUAL) ?
+					OPER_STATUS_UP : OPER_STATUS_DOWN;
+
+	if_mib_attrs->if_counter_discontinuity_time =
+		data->if_counter_discontinuity_time / (rte_get_tsc_hz() * 100);
+
+	for (i = 0; i < NUM_IF_MIB_ATTRS; i++) {
+		attr_ptr = RTE_PTR_ADD(if_mib_attrs,
+					if_mib_attrs_strings[i].offset);
+		values[i] = *attr_ptr;
+	}
+	/* update values to metrics library */
+	rte_metrics_update_values(port_id, idx, values, NUM_IF_MIB_ATTRS);
+
+	return 0;
+}
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 1446540..47c6cf6 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1674,6 +1674,10 @@  struct rte_eth_dev_data {
 	uint32_t dev_flags; /**< Capabilities */
 	enum rte_kernel_driver kdrv;    /**< Kernel driver passthrough */
 	int numa_node;  /**< NUMA node connection */
+	/** IF-MIB attributes ifLastChange and ifCounterDiscontinuityTime */
+	uint64_t sys_up_time_start;
+	uint64_t if_counter_discontinuity_time;
+	uint64_t if_last_change;
 };
 
 /** Device supports hotplug detach */
@@ -4375,6 +4379,9 @@  rte_eth_dev_get_port_by_name(const char *name, uint8_t *port_id);
 int
 rte_eth_dev_get_name_by_port(uint8_t port_id, char *name);
 
+int rte_eth_ifmib_attr_init(void);
+int rte_eth_ifmib_attr_reg(uint8_t port_id);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/librte_ether/rte_ethdev_pci.h b/lib/librte_ether/rte_ethdev_pci.h
index 69aab03..f829407 100644
--- a/lib/librte_ether/rte_ethdev_pci.h
+++ b/lib/librte_ether/rte_ethdev_pci.h
@@ -37,6 +37,7 @@ 
 #include <rte_malloc.h>
 #include <rte_pci.h>
 #include <rte_ethdev.h>
+#include <rte_cycles.h>
 
 /**
  * Copy pci device info to the Ethernet device data.
@@ -157,6 +158,9 @@  rte_eth_dev_pci_generic_probe(struct rte_pci_device *pci_dev,
 
 	RTE_FUNC_PTR_OR_ERR_RET(*dev_init, -EINVAL);
 	ret = dev_init(eth_dev);
+	eth_dev->data->sys_up_time_start = rte_rdtsc();
+	eth_dev->data->if_counter_discontinuity_time = 0;
+	eth_dev->data->if_last_change = 0;
 	if (ret)
 		rte_eth_dev_pci_release(eth_dev);
 
diff --git a/mk/rte.app.mk b/mk/rte.app.mk
index 7d71a49..9dc9597 100644
--- a/mk/rte.app.mk
+++ b/mk/rte.app.mk
@@ -73,7 +73,6 @@  _LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += --whole-archive
 _LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += -lrte_acl
 _LDLIBS-$(CONFIG_RTE_LIBRTE_ACL)            += --no-whole-archive
 _LDLIBS-$(CONFIG_RTE_LIBRTE_JOBSTATS)       += -lrte_jobstats
-_LDLIBS-$(CONFIG_RTE_LIBRTE_METRICS)        += -lrte_metrics
 _LDLIBS-$(CONFIG_RTE_LIBRTE_BITRATE)        += -lrte_bitratestats
 _LDLIBS-$(CONFIG_RTE_LIBRTE_LATENCY_STATS)  += -lrte_latencystats
 _LDLIBS-$(CONFIG_RTE_LIBRTE_POWER)          += -lrte_power
@@ -95,6 +94,7 @@  _LDLIBS-$(CONFIG_RTE_LIBRTE_EVENTDEV)       += -lrte_eventdev
 _LDLIBS-$(CONFIG_RTE_LIBRTE_MEMPOOL)        += -lrte_mempool
 _LDLIBS-$(CONFIG_RTE_DRIVER_MEMPOOL_RING)   += -lrte_mempool_ring
 _LDLIBS-$(CONFIG_RTE_LIBRTE_RING)           += -lrte_ring
+_LDLIBS-$(CONFIG_RTE_LIBRTE_METRICS)        += -lrte_metrics
 _LDLIBS-$(CONFIG_RTE_LIBRTE_EAL)            += -lrte_eal
 _LDLIBS-$(CONFIG_RTE_LIBRTE_CMDLINE)        += -lrte_cmdline
 _LDLIBS-$(CONFIG_RTE_LIBRTE_REORDER)        += -lrte_reorder