clarify eth_dev state management

PATCH Superseded Gaetan Rivet 2 participants 13 messages Mar 3, 2017 – Apr 5, 2017

Share & tools

Gaetan Rivet

author

2017-03-03 15:40 UTC #0 lore raw

cover clarify eth_dev state management

1. The hotplug API currently available in DPDK introduces the notion of device states (DEV_DETACHED, DEV_ATTACHED). These states are implicit and internal to the ethdev layer.

2. Device enumeration and access is done directly, without regard to the underlying state of a device. Applications are currently expected to cycle through the static rte_eth_devices array. Those using the hotplug API (rte_eth_dev_attach(), rte_eth_dev_detach()), are thus expected to deal themselves with possible discrepancies internal to the ethdev layer, i.e. avoid detached devices if necessary, without the state of the devices having been explicitly defined or exposed.

3. The hotplug API itself is not complete and cannot be used without introducing some bugs. Detaching a device will introduce inconsistencies in the device count, as explained in the related API.
No function is exposed in the ethdev layer to permit applications to deal with it.

This series addresses these issues so that:

1. Applications are not expected to manage the states of their devices, those should be kept internal to the ethdev layer.

2. The hotplug API is cleaner, with specific operations from the ethdev layer to be used if necessary.

3. Applications that are not interested in hotplug functionality are not concerned and do not require any change.

Gaetan Rivet (4):
ethdev: expose device states
ethdev: add device iterator
ethdev: count devices consistently app/testpmd: use ethdev iterator to list devices

 app/test-pmd/cmdline.c        | 31 ++++++++++++++---------------
 app/test-pmd/cmdline_flow.c   |  2 +-
 app/test-pmd/config.c         | 12 +++++------
 app/test-pmd/parameters.c     |  4 ++--
 app/test-pmd/testpmd.c        | 44 +++++++++++------------------------------
 app/test-pmd/testpmd.h        |  9 ---------
 lib/librte_ether/rte_ethdev.c | 46 +++++++++++++++++++++++++------------------
 lib/librte_ether/rte_ethdev.h | 37 ++++++++++++++++++++++++++++++----
 8 files changed, 95 insertions(+), 90 deletions(-)

Signature · 1 line

2.1.4

Gaetan Rivet

author

2017-03-03 15:40 UTC #1 lore raw

PATCH 1/4 ethdev: expose device states

The hotplug API introduced multiple states for a device with possible values defined internally, while the related field in struct rte_eth_dev was made public.

Exposing those states improves consistency because applications have to deal with the device list directly.

"DEV_DETACHED" is renamed "RTE_ETH_DEV_UNUSED" to better reflect that the emptiness of a slot is not necessarily the result of detaching a device.

Signed-off-by Gaetan Rivet <[email protected]>

---
 lib/librte_ether/rte_ethdev.c | 15 +++++----------
 lib/librte_ether/rte_ethdev.h | 10 +++++++++-
 2 files changed, 14 insertions(+), 11 deletions(-)

lib/librte_ether/rte_ethdev.c +5 ~~-10~~

index f3c32c4..d5d1b52 100644
@@ -138,11 +138,6 @@ enum {
 	STAT_QMAP_RX
 };
 
-enum {
-	DEV_DETACHED = 0,
-	DEV_ATTACHED
-};
-
 static void
 rte_eth_dev_data_alloc(void)
 {

@@ -170,7 +165,7 @@ rte_eth_dev_allocated(const char *name)
 	unsigned i;
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
-		if ((rte_eth_devices[i].attached == DEV_ATTACHED) &&
+		if ((rte_eth_devices[i].state == RTE_ETH_DEV_ATTACHED) &&
 		    strcmp(rte_eth_devices[i].data->name, name) == 0)
 			return &rte_eth_devices[i];
 	}

@@ -183,7 +178,7 @@ rte_eth_dev_find_free_port(void)
 	unsigned i;
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
-		if (rte_eth_devices[i].attached == DEV_DETACHED)
+		if (rte_eth_devices[i].state == RTE_ETH_DEV_UNUSED)
 			return i;
 	}
 	return RTE_MAX_ETHPORTS;

@@ -195,7 +190,7 @@ eth_dev_get(uint8_t port_id)
 	struct rte_eth_dev *eth_dev = &rte_eth_devices[port_id];
 
 	eth_dev->data = &rte_eth_dev_data[port_id];
-	eth_dev->attached = DEV_ATTACHED;
+	eth_dev->state = RTE_ETH_DEV_ATTACHED;
 	TAILQ_INIT(&(eth_dev->link_intr_cbs));
 
 	eth_dev_last_created_port = port_id;

@@ -271,7 +266,7 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 	if (eth_dev == NULL)
 		return -EINVAL;
 
-	eth_dev->attached = DEV_DETACHED;
+	eth_dev->state = RTE_ETH_DEV_UNUSED;
 	nb_ports--;
 	return 0;
 }

@@ -377,7 +372,7 @@ int
 rte_eth_dev_is_valid_port(uint8_t port_id)
 {
 	if (port_id >= RTE_MAX_ETHPORTS ||
-	    rte_eth_devices[port_id].attached != DEV_ATTACHED)
+	    rte_eth_devices[port_id].state != RTE_ETH_DEV_ATTACHED)
 		return 0;
 	else
 		return 1;

lib/librte_ether/rte_ethdev.h +9 -1

index 97f3e2d..d51b8e9 100644
@@ -1614,6 +1614,14 @@ struct rte_eth_rxtx_callback {
 };
 
 /**
+ * A set of values to describe the possible states of an eth device.
+ */
+enum rte_eth_dev_state {
+	RTE_ETH_DEV_UNUSED = 0,
+	RTE_ETH_DEV_ATTACHED,
+};
+
+/**
  * @internal
  * The generic data structure associated with each ethernet device.
  *

@@ -1644,7 +1652,7 @@ struct rte_eth_dev {
 	 * received packets before passing them to the driver for transmission.
 	 */
 	struct rte_eth_rxtx_callback *pre_tx_burst_cbs[RTE_MAX_QUEUES_PER_PORT];
-	uint8_t attached; /**< Flag indicating the port is attached */
+	enum rte_eth_dev_state state:8; /**< Flag indicating the port state */
 } __rte_cache_aligned;
 
 struct rte_eth_dev_sriov {

Signature · 1 line

2.1.4

Gaetan Rivet

author

2017-03-03 15:40 UTC #2 lore raw

PATCH 2/4 ethdev: add device iterator

In reply to Gaetan Rivet

This iterator helps applications iterate over the device list and skip holes caused by invalid or detached devices.

Signed-off-by Gaetan Rivet <[email protected]>

---
 lib/librte_ether/rte_ethdev.c | 13 +++++++++++++
 lib/librte_ether/rte_ethdev.h | 19 +++++++++++++++++++
 2 files changed, 32 insertions(+)

lib/librte_ether/rte_ethdev.c +13 -0

index d5d1b52..fcb9933 100644
@@ -138,6 +138,19 @@ enum {
 	STAT_QMAP_RX
 };
 
+uint8_t
+rte_eth_find_next(uint8_t port_id)
+{
+	while (port_id < RTE_MAX_ETHPORTS &&
+	       rte_eth_devices[port_id].state != RTE_ETH_DEV_ATTACHED)
+		port_id++;
+
+	if (port_id >= RTE_MAX_ETHPORTS)
+		return RTE_MAX_ETHPORTS;
+
+	return port_id;
+}
+
 static void
 rte_eth_dev_data_alloc(void)
 {

lib/librte_ether/rte_ethdev.h +19 -0

index d51b8e9..59c4123 100644
@@ -1731,6 +1731,25 @@ struct rte_eth_dev_data {
 extern struct rte_eth_dev rte_eth_devices[];
 
 /**
+ * Iterates over valid ethdev ports.
+ *
+ * @param: port_id
+ *   The id of the next possible valid port.
+ * @return
+ *   Next valid port id, RTE_MAX_ETHPORTS is there is none.
+ */
+uint8_t rte_eth_find_next(uint8_t port_id);
+
+/**
+ * Macro to iterate over all enabled ethdev ports.
+ */
+#define RTE_ETH_FOREACH_DEV(p)			\
+	for (p = rte_eth_find_next(0);		\
+	     p < RTE_MAX_ETHPORTS;		\
+	     p = rte_eth_find_next(p + 1))
+
+
+/**
  * Get the total number of Ethernet devices that have been successfully
  * initialized by the [matching] Ethernet driver during the PCI probing phase.
  * All devices whose port identifier is in the range

Signature · 1 line

2.1.4

Gaetan Rivet

author

2017-03-03 15:40 UTC #3 lore raw

PATCH 3/4 ethdev: count devices consistently

In reply to Gaetan Rivet

Make the rte_eth_dev_count() return the correct number of devices even after some are detached by the hotplug API.

This change does not affect existing applications that do not use hotplug API calls. Those that do are already aware that port IDs are not necessarily contiguous.

Signed-off-by Gaetan Rivet <[email protected]>

---
 lib/librte_ether/rte_ethdev.c | 20 ++++++++++----------
 lib/librte_ether/rte_ethdev.h | 14 ++++++++------
 2 files changed, 18 insertions(+), 16 deletions(-)

lib/librte_ether/rte_ethdev.c +10 ~~-10~~

index fcb9933..3a52d0a 100644
@@ -72,7 +72,6 @@ static const char *MZ_RTE_ETH_DEV_DATA = "rte_eth_dev_data";
 struct rte_eth_dev rte_eth_devices[RTE_MAX_ETHPORTS];
 static struct rte_eth_dev_data *rte_eth_dev_data;
 static uint8_t eth_dev_last_created_port;
-static uint8_t nb_ports;
 
 /* spinlock for eth device callbacks */
 static rte_spinlock_t rte_eth_dev_cb_lock = RTE_SPINLOCK_INITIALIZER;

@@ -207,7 +206,6 @@ eth_dev_get(uint8_t port_id)
 	TAILQ_INIT(&(eth_dev->link_intr_cbs));
 
 	eth_dev_last_created_port = port_id;
-	nb_ports++;
 
 	return eth_dev;
 }

@@ -280,7 +278,6 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 		return -EINVAL;
 
 	eth_dev->state = RTE_ETH_DEV_UNUSED;
-	nb_ports--;
 	return 0;
 }

@@ -401,7 +398,15 @@ rte_eth_dev_socket_id(uint8_t port_id)
 uint8_t
 rte_eth_dev_count(void)
 {
-	return nb_ports;
+	uint8_t p;
+	uint8_t count;
+
+	count = 0;
+
+	RTE_ETH_FOREACH_DEV(p)
+		count++;
+
+	return count;
 }
 
 int

@@ -433,13 +438,8 @@ rte_eth_dev_get_port_by_name(const char *name, uint8_t *port_id)
 		return -EINVAL;
 	}
 
-	if (!nb_ports)
-		return -ENODEV;
-
 	*port_id = RTE_MAX_ETHPORTS;
-
-	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
-
+	RTE_ETH_FOREACH_DEV(i) {
 		if (!strncmp(name,
 			rte_eth_dev_data[i].name, strlen(name))) {

lib/librte_ether/rte_ethdev.h +8 -6

index 59c4123..bdad81b 100644
@@ -1743,9 +1743,9 @@ uint8_t rte_eth_find_next(uint8_t port_id);
 /**
  * Macro to iterate over all enabled ethdev ports.
  */
-#define RTE_ETH_FOREACH_DEV(p)			\
-	for (p = rte_eth_find_next(0);		\
-	     p < RTE_MAX_ETHPORTS;		\
+#define RTE_ETH_FOREACH_DEV(p)					\
+	for (p = rte_eth_find_next(0);				\
+	     (unsigned int)p < (unsigned int)RTE_MAX_ETHPORTS;	\
 	     p = rte_eth_find_next(p + 1))

@@ -1755,9 +1755,11 @@ uint8_t rte_eth_find_next(uint8_t port_id);
  * All devices whose port identifier is in the range
  * [0,  rte_eth_dev_count() - 1] can be operated on by network applications
  * immediately after invoking rte_eal_init().
- * If the application unplugs a port using hotplug function, The enabled port
- * numbers may be noncontiguous. In the case, the applications need to manage
- * enabled port by themselves.
+ * If the application unplugs a port using a hotplug function, the range of
+ * enabled ports may be non-contiguous. In this case, this function returns
+ * the actual number of enabled ports and the application must keep track
+ * of possible gaps in the enabled range, or use the ``RTE_ETH_FOREACH_DEV()``
+ * macro.
  *
  * @return
  *   - The total number of usable Ethernet devices.

Signature · 1 line

2.1.4

Thomas Monjalon

2017-03-30 19:26 UTC #4 lore raw

Re: [PATCH 3/4] ethdev: count devices consistently

2017-03-03 16:40, Gaetan Rivet:

Quoted 2 lines

Make the rte_eth_dev_count() return the correct number of devices even after some are detached by the hotplug API.

Please explain what is the correct number,
or that the wrong number was a max id.

Quoted 3 lines

This change does not affect existing applications that do not use hotplug API calls. Those that do are already aware that port IDs are not necessarily contiguous.

[...]

lib/librte_ether/rte_ethdev.h uint8_t rte_eth_find_next(uint8_t port_id); · line 1749

Quoted 9 lines

lib/librte_ether/rte_ethdev.h +3 -3

-#define RTE_ETH_FOREACH_DEV(p)			\
-	for (p = rte_eth_find_next(0);		\
-	     p < RTE_MAX_ETHPORTS;		\
+#define RTE_ETH_FOREACH_DEV(p)					\
+	for (p = rte_eth_find_next(0);				\
+	     (unsigned int)p < (unsigned int)RTE_MAX_ETHPORTS;	\
 	     p = rte_eth_find_next(p + 1))

This macro was introduced in previous patch.
Why adding the cast here?

Gaëtan Rivet

author

2017-03-31 09:13 UTC #5 lore raw

Re: [PATCH 3/4] ethdev: count devices consistently

Quoted 7 lines · Thomas Monjalon

2017-03-03 16:40, Gaetan Rivet:
Quoted 2 lines
Make the rte_eth_dev_count() return the correct number of devices even after some are detached by the hotplug API.
Please explain what is the correct number,
or that the wrong number was a max id.

will do.

Quoted 4 lines

Quoted 3 lines
This change does not affect existing applications that do not use hotplug API calls. Those that do are already aware that port IDs are not necessarily contiguous.
[...]

Quoted 12 lines

lib/librte_ether/rte_ethdev.h uint8_t rte_eth_find_next(uint8_t port_id); · line 1749
Quoted 9 lines
lib/librte_ether/rte_ethdev.h +3 -3
-#define RTE_ETH_FOREACH_DEV(p)			\
-	for (p = rte_eth_find_next(0);		\
-	     p < RTE_MAX_ETHPORTS;		\
+#define RTE_ETH_FOREACH_DEV(p)					\
+	for (p = rte_eth_find_next(0);				\
+	     (unsigned int)p < (unsigned int)RTE_MAX_ETHPORTS;	\
 	     p = rte_eth_find_next(p + 1))
This macro was introduced in previous patch.
Why adding the cast here?

In the function rte_eth_dev_get_port_by_name(), the iterator is an int.
When I introduced the use of the iterator there, I then realized that it would be better to allow users to use signed ints as well, to avoid unnecessary edits on their part.

In retrospective, I agree that it would have been better in the previous patch.

Signature · 2 lines

Gaëtan Rivet
6WIND

Gaetan Rivet

author

2017-03-03 15:40 UTC #6 lore raw

PATCH 4/4 app/testpmd: use ethdev iterator to list devices

In reply to Gaetan Rivet

This commit replaces redundant code with public ethdev layer calls.

Signed-off-by Gaetan Rivet <[email protected]>

---
 app/test-pmd/cmdline.c      | 31 +++++++++++++++----------------
 app/test-pmd/cmdline_flow.c |  2 +-
 app/test-pmd/config.c       | 12 ++++++------
 app/test-pmd/parameters.c   |  4 ++--
 app/test-pmd/testpmd.c      | 44 +++++++++++---------------------------------
 app/test-pmd/testpmd.h      |  9 ---------
 6 files changed, 35 insertions(+), 67 deletions(-)

app/test-pmd/cmdline.c +15 ~~-16~~

index 43fc636..871ab19 100644
@@ -1139,7 +1139,7 @@ cmd_config_speed_all_parsed(void *parsed_result,
 			&link_speed) < 0)
 		return;
 
-	FOREACH_PORT(pid, ports) {
+	RTE_ETH_FOREACH_DEV(pid) {
 		ports[pid].dev_conf.link_speeds = link_speed;
 	}

@@ -4641,7 +4641,6 @@ static void cmd_create_bonded_device_parsed(void *parsed_result,
 		nb_ports = rte_eth_dev_count();
 		reconfig(port_id, res->socket);
 		rte_eth_promiscuous_enable(port_id);
-		ports[port_id].enabled = 1;
 	}
 
 }

@@ -5000,7 +4999,7 @@ static void cmd_set_promisc_mode_parsed(void *parsed_result,
 
 	/* all ports */
 	if (allports) {
-		FOREACH_PORT(i, ports) {
+		RTE_ETH_FOREACH_DEV(i) {
 			if (enable)
 				rte_eth_promiscuous_enable(i);
 			else

@@ -5080,7 +5079,7 @@ static void cmd_set_allmulti_mode_parsed(void *parsed_result,
 
 	/* all ports */
 	if (allports) {
-		FOREACH_PORT(i, ports) {
+		RTE_ETH_FOREACH_DEV(i) {
 			if (enable)
 				rte_eth_allmulticast_enable(i);
 			else

@@ -5814,31 +5813,31 @@ static void cmd_showportall_parsed(void *parsed_result,
 	struct cmd_showportall_result *res = parsed_result;
 	if (!strcmp(res->show, "clear")) {
 		if (!strcmp(res->what, "stats"))
-			FOREACH_PORT(i, ports)
+			RTE_ETH_FOREACH_DEV(i)
 				nic_stats_clear(i);
 		else if (!strcmp(res->what, "xstats"))
-			FOREACH_PORT(i, ports)
+			RTE_ETH_FOREACH_DEV(i)
 				nic_xstats_clear(i);
 	} else if (!strcmp(res->what, "info"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			port_infos_display(i);
 	else if (!strcmp(res->what, "stats"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			nic_stats_display(i);
 	else if (!strcmp(res->what, "xstats"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			nic_xstats_display(i);
 	else if (!strcmp(res->what, "fdir"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			fdir_get_infos(i);
 	else if (!strcmp(res->what, "stat_qmap"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			nic_stats_mapping_display(i);
 	else if (!strcmp(res->what, "dcb_tc"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			port_dcb_info_display(i);
 	else if (!strcmp(res->what, "cap"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			port_offload_cap_display(i);
 }

@@ -10315,7 +10314,7 @@ cmd_config_l2_tunnel_eth_type_all_parsed
 	entry.l2_tunnel_type = str2fdir_l2_tunnel_type(res->l2_tunnel_type);
 	entry.ether_type = res->eth_type_val;
 
-	FOREACH_PORT(pid, ports) {
+	RTE_ETH_FOREACH_DEV(pid) {
 		rte_eth_dev_l2_tunnel_eth_type_conf(pid, &entry);
 	}
 }

@@ -10431,7 +10430,7 @@ cmd_config_l2_tunnel_en_dis_all_parsed(
 	else
 		en = 0;
 
-	FOREACH_PORT(pid, ports) {
+	RTE_ETH_FOREACH_DEV(pid) {
 		rte_eth_dev_l2_tunnel_offload_set(pid,
 						  &entry,
 						  ETH_L2_TUNNEL_ENABLE_MASK,

@@ -12601,7 +12600,7 @@ cmd_reconfig_device_queue(portid_t id, uint8_t dev, uint8_t queue)
 	if (id == (portid_t)RTE_PORT_ALL) {
 		portid_t pid;
 
-		FOREACH_PORT(pid, ports) {
+		RTE_ETH_FOREACH_DEV(pid) {
 			/* check if need_reconfig has been set to 1 */
 			if (ports[pid].need_reconfig == 0)
 				ports[pid].need_reconfig = dev;

app/test-pmd/cmdline_flow.c +1 -1

index ff98690..b7d2c41 100644
@@ -2369,7 +2369,7 @@ comp_port(struct context *ctx, const struct token *token,
 
 	(void)ctx;
 	(void)token;
-	FOREACH_PORT(p, ports) {
+	RTE_ETH_FOREACH_DEV(p) {
 		if (buf && i == ent)
 			return snprintf(buf, size, "%u", p);
 		++i;

app/test-pmd/config.c +6 -6

index 80491fc..54e38c4 100644
@@ -174,7 +174,7 @@ nic_stats_display(portid_t port_id)
 
 	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 		printf("Valid port range is [0");
-		FOREACH_PORT(pid, ports)
+		RTE_ETH_FOREACH_DEV(pid)
 			printf(", %d", pid);
 		printf("]\n");
 		return;

@@ -252,7 +252,7 @@ nic_stats_clear(portid_t port_id)
 
 	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 		printf("Valid port range is [0");
-		FOREACH_PORT(pid, ports)
+		RTE_ETH_FOREACH_DEV(pid)
 			printf(", %d", pid);
 		printf("]\n");
 		return;

@@ -334,7 +334,7 @@ nic_stats_mapping_display(portid_t port_id)
 
 	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 		printf("Valid port range is [0");
-		FOREACH_PORT(pid, ports)
+		RTE_ETH_FOREACH_DEV(pid)
 			printf(", %d", pid);
 		printf("]\n");
 		return;

@@ -452,7 +452,7 @@ port_infos_display(portid_t port_id)
 
 	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 		printf("Valid port range is [0");
-		FOREACH_PORT(pid, ports)
+		RTE_ETH_FOREACH_DEV(pid)
 			printf(", %d", pid);
 		printf("]\n");
 		return;

@@ -725,7 +725,7 @@ port_id_is_invalid(portid_t port_id, enum print_warning warning)
 	if (port_id == (portid_t)RTE_PORT_ALL)
 		return 0;
 
-	if (port_id < RTE_MAX_ETHPORTS && ports[port_id].enabled)
+	if (rte_eth_dev_is_valid_port(port_id))
 		return 0;
 
 	if (warning == ENABLED_WARN)

@@ -2279,7 +2279,7 @@ set_fwd_ports_mask(uint64_t portmask)
 		return;
 	}
 	nb_pt = 0;
-	for (i = 0; i < (unsigned)RTE_MIN(64, RTE_MAX_ETHPORTS); i++) {
+	RTE_ETH_FOREACH_DEV(i) {
 		if (! ((uint64_t)(1ULL << i) & portmask))
 			continue;
 		portlist[nb_pt++] = i;

app/test-pmd/parameters.c +2 -2

index 28db8cd..67d8bf2 100644
@@ -394,7 +394,7 @@ parse_portnuma_config(const char *q_arg)
 		port_id = (uint8_t)int_fld[FLD_PORT];
 		if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 			printf("Valid port range is [0");
-			FOREACH_PORT(pid, ports)
+			RTE_ETH_FOREACH_DEV(pid)
 				printf(", %d", pid);
 			printf("]\n");
 			return -1;

@@ -454,7 +454,7 @@ parse_ringnuma_config(const char *q_arg)
 		port_id = (uint8_t)int_fld[FLD_PORT];
 		if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 			printf("Valid port range is [0");
-			FOREACH_PORT(pid, ports)
+			RTE_ETH_FOREACH_DEV(pid)
 				printf(", %d", pid);
 			printf("]\n");
 			return -1;

app/test-pmd/testpmd.c +11 ~~-33~~

140 lines not on this page · raw message

app/test-pmd/testpmd.h +0 -9

index 8cf2860..e1e4939 100644
@@ -163,7 +163,6 @@ struct port_flow {
  * The data structure associated with each port.
  */
 struct rte_port {
-	uint8_t                 enabled;    /**< Port enabled or not */
 	struct rte_eth_dev_info dev_info;   /**< PCI info + driver name */
 	struct rte_eth_conf     dev_conf;   /**< Port configuration. */
 	struct ether_addr       eth_addr;   /**< Port ethernet address */

@@ -195,14 +194,6 @@ struct rte_port {
 	struct port_flow        *flow_list; /**< Associated flows. */
 };
 
-extern portid_t __rte_unused
-find_next_port(portid_t p, struct rte_port *ports, int size);
-
-#define FOREACH_PORT(p, ports) \
-	for (p = find_next_port(0, ports, RTE_MAX_ETHPORTS); \
-	    p < RTE_MAX_ETHPORTS; \
-	    p = find_next_port(p + 1, ports, RTE_MAX_ETHPORTS))
-
 /**
  * The data structure associated with each forwarding logical core.
  * The logical cores are internally numbered by a core index from 0 to

Signature · 1 line

2.1.4

Gaetan Rivet

author

2017-03-31 12:04 UTC #7 lore raw

v2 cover clarify eth_dev state management

In reply to Gaetan Rivet

1. The hotplug API currently available in DPDK introduces the notion of device states (DEV_DETACHED, DEV_ATTACHED). These states are implicit and internal to the ethdev layer.

This series addresses these issues so that:

1. Applications are not expected to manage the states of their devices, those should be kept internal to the ethdev layer.

2. The hotplug API is cleaner, with specific operations from the ethdev layer to be used if necessary.

3. Applications that are not interested in hotplug functionality are not concerned and do not require any change.

v1 --> v2:

* Improved patches consistency, iterator definition in one patch only

* Removed device count changes, not needed without additional device states.

Gaetan Rivet (3):
ethdev: expose device states
ethdev: add device iterator
app/testpmd: use ethdev iterator to list devices

 app/test-pmd/cmdline.c        | 31 +++++++++++++++---------------
 app/test-pmd/cmdline_flow.c   |  2 +-
 app/test-pmd/config.c         | 12 ++++++------
 app/test-pmd/parameters.c     |  4 ++--
 app/test-pmd/testpmd.c        | 44 +++++++++++--------------------------------
 app/test-pmd/testpmd.h        |  9 ---------
 lib/librte_ether/rte_ethdev.c | 30 +++++++++++++++++------------
 lib/librte_ether/rte_ethdev.h | 31 ++++++++++++++++++++++++++++--
 8 files changed, 82 insertions(+), 81 deletions(-)

Signature · 1 line

2.1.4

Thomas Monjalon

Applied

2017-04-05 20:47 UTC #8 lore raw

2017-03-31 14:04, Gaetan Rivet:

Quoted 20 lines

1. The hotplug API currently available in DPDK introduces the notion of device states (DEV_DETACHED, DEV_ATTACHED). These states are implicit and internal to the ethdev layer.
2. Device enumeration and access is done directly, without regard to the underlying state of a device. Applications are currently expected to cycle through the static rte_eth_devices array. Those using the hotplug API (rte_eth_dev_attach(), rte_eth_dev_detach()), are thus expected to deal themselves with possible discrepancies internal to the ethdev layer, i.e. avoid detached devices if necessary, without the state of the devices having been explicitly defined or exposed.
3. The hotplug API itself is not complete and cannot be used without introducing some bugs. Detaching a device will introduce inconsistencies in the device count, as explained in the related API.
No function is exposed in the ethdev layer to permit applications to deal with it.
This series addresses these issues so that:

Quoted 8 lines

1. Applications are not expected to manage the states of their devices, those should be kept internal to the ethdev layer.
2. The hotplug API is cleaner, with specific operations from the ethdev layer to be used if necessary.
3. Applications that are not interested in hotplug functionality are not concerned and do not require any change.

Applied, thanks

Gaetan Rivet

author

2017-03-31 12:04 UTC #9 lore raw

PATCH v2 1/3 ethdev: expose device states

In reply to Gaetan Rivet

The hotplug API introduced multiple states for a device with possible values defined internally, while the related field in struct rte_eth_dev was made public.

Exposing those states improves consistency because applications have to deal with the device list directly.

"DEV_DETACHED" is renamed "RTE_ETH_DEV_UNUSED" to better reflect that the emptiness of a slot is not necessarily the result of detaching a device.

Signed-off-by Gaetan Rivet <[email protected]>

---
 lib/librte_ether/rte_ethdev.c | 15 +++++----------
 lib/librte_ether/rte_ethdev.h | 10 +++++++++-
 2 files changed, 14 insertions(+), 11 deletions(-)

lib/librte_ether/rte_ethdev.c +5 ~~-10~~

index b796e7d..90d0713 100644
@@ -138,11 +138,6 @@ enum {
 	STAT_QMAP_RX
 };
 
-enum {
-	DEV_DETACHED = 0,
-	DEV_ATTACHED
-};
-
 static void
 rte_eth_dev_data_alloc(void)
 {

@@ -170,7 +165,7 @@ rte_eth_dev_allocated(const char *name)
 	unsigned i;
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
-		if ((rte_eth_devices[i].attached == DEV_ATTACHED) &&
+		if ((rte_eth_devices[i].state == RTE_ETH_DEV_ATTACHED) &&
 		    strcmp(rte_eth_devices[i].data->name, name) == 0)
 			return &rte_eth_devices[i];
 	}

@@ -183,7 +178,7 @@ rte_eth_dev_find_free_port(void)
 	unsigned i;
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
-		if (rte_eth_devices[i].attached == DEV_DETACHED)
+		if (rte_eth_devices[i].state == RTE_ETH_DEV_UNUSED)
 			return i;
 	}
 	return RTE_MAX_ETHPORTS;

@@ -195,7 +190,7 @@ eth_dev_get(uint8_t port_id)
 	struct rte_eth_dev *eth_dev = &rte_eth_devices[port_id];
 
 	eth_dev->data = &rte_eth_dev_data[port_id];
-	eth_dev->attached = DEV_ATTACHED;
+	eth_dev->state = RTE_ETH_DEV_ATTACHED;
 	TAILQ_INIT(&(eth_dev->link_intr_cbs));
 
 	eth_dev_last_created_port = port_id;

@@ -271,7 +266,7 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 	if (eth_dev == NULL)
 		return -EINVAL;
 
-	eth_dev->attached = DEV_DETACHED;
+	eth_dev->state = RTE_ETH_DEV_UNUSED;
 	nb_ports--;
 	return 0;
 }

@@ -377,7 +372,7 @@ int
 rte_eth_dev_is_valid_port(uint8_t port_id)
 {
 	if (port_id >= RTE_MAX_ETHPORTS ||
-	    rte_eth_devices[port_id].attached != DEV_ATTACHED)
+	    rte_eth_devices[port_id].state != RTE_ETH_DEV_ATTACHED)
 		return 0;
 	else
 		return 1;

lib/librte_ether/rte_ethdev.h +9 -1

index b3ee872..b9531f3 100644
@@ -1619,6 +1619,14 @@ struct rte_eth_rxtx_callback {
 };
 
 /**
+ * A set of values to describe the possible states of an eth device.
+ */
+enum rte_eth_dev_state {
+	RTE_ETH_DEV_UNUSED = 0,
+	RTE_ETH_DEV_ATTACHED,
+};
+
+/**
  * @internal
  * The generic data structure associated with each ethernet device.
  *

@@ -1649,7 +1657,7 @@ struct rte_eth_dev {
 	 * received packets before passing them to the driver for transmission.
 	 */
 	struct rte_eth_rxtx_callback *pre_tx_burst_cbs[RTE_MAX_QUEUES_PER_PORT];
-	uint8_t attached; /**< Flag indicating the port is attached */
+	enum rte_eth_dev_state state:8; /**< Flag indicating the port state */
 } __rte_cache_aligned;
 
 struct rte_eth_dev_sriov {

Signature · 1 line

2.1.4

Gaetan Rivet

author

2017-03-31 12:04 UTC #10 lore raw

PATCH v2 2/3 ethdev: add device iterator

In reply to Gaetan Rivet

This iterator helps applications iterate over the device list and skip holes caused by invalid or detached devices.

Signed-off-by Gaetan Rivet <[email protected]>

---
 lib/librte_ether/rte_ethdev.c | 17 ++++++++++++++---
 lib/librte_ether/rte_ethdev.h | 21 ++++++++++++++++++++-
 2 files changed, 34 insertions(+), 4 deletions(-)

lib/librte_ether/rte_ethdev.c +14 -3

index 90d0713..ca7af35 100644
@@ -138,6 +138,19 @@ enum {
 	STAT_QMAP_RX
 };
 
+uint8_t
+rte_eth_find_next(uint8_t port_id)
+{
+	while (port_id < RTE_MAX_ETHPORTS &&
+	       rte_eth_devices[port_id].state != RTE_ETH_DEV_ATTACHED)
+		port_id++;
+
+	if (port_id >= RTE_MAX_ETHPORTS)
+		return RTE_MAX_ETHPORTS;
+
+	return port_id;
+}
+
 static void
 rte_eth_dev_data_alloc(void)
 {

@@ -424,9 +437,7 @@ rte_eth_dev_get_port_by_name(const char *name, uint8_t *port_id)
 		return -ENODEV;
 
 	*port_id = RTE_MAX_ETHPORTS;
-
-	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
-
+	RTE_ETH_FOREACH_DEV(i) {
 		if (!strncmp(name,
 			rte_eth_dev_data[i].name, strlen(name))) {

lib/librte_ether/rte_ethdev.h +20 -1

index b9531f3..bed7dff 100644
@@ -1736,6 +1736,25 @@ struct rte_eth_dev_data {
 extern struct rte_eth_dev rte_eth_devices[];
 
 /**
+ * Iterates over valid ethdev ports.
+ *
+ * @param: port_id
+ *   The id of the next possible valid port.
+ * @return
+ *   Next valid port id, RTE_MAX_ETHPORTS if there is none.
+ */
+uint8_t rte_eth_find_next(uint8_t port_id);
+
+/**
+ * Macro to iterate over all enabled ethdev ports.
+ */
+#define RTE_ETH_FOREACH_DEV(p)					\
+	for (p = rte_eth_find_next(0);				\
+	     (unsigned int)p < (unsigned int)RTE_MAX_ETHPORTS;	\
+	     p = rte_eth_find_next(p + 1))
+
+
+/**
  * Get the total number of Ethernet devices that have been successfully
  * initialized by the [matching] Ethernet driver during the PCI probing phase.
  * All devices whose port identifier is in the range

@@ -1743,7 +1762,7 @@ extern struct rte_eth_dev rte_eth_devices[];
  * immediately after invoking rte_eal_init().
  * If the application unplugs a port using hotplug function, The enabled port
  * numbers may be noncontiguous. In the case, the applications need to manage
- * enabled port by themselves.
+ * enabled port by using the ``RTE_ETH_FOREACH_DEV()`` macro.
  *
  * @return
  *   - The total number of usable Ethernet devices.

Signature · 1 line

2.1.4

Thomas Monjalon

2017-04-05 20:42 UTC #11 lore raw

Re: [PATCH v2 2/3] ethdev: add device iterator

2017-03-31 14:04, Gaetan Rivet:

Quoted 4 lines

/**
+ * Iterates over valid ethdev ports.
+ *
+ * @param: port_id

warning: expected whitespace after : command

Will fix it when applying

Quoted 6 lines

+ * The id of the next possible valid port.
+ * @return
+ * Next valid port id, RTE_MAX_ETHPORTS if there is none.
+ */
+uint8_t rte_eth_find_next(uint8_t port_id);

Gaetan Rivet

author

2017-03-31 12:04 UTC #12 lore raw

PATCH v2 3/3 app/testpmd: use ethdev iterator to list devices

In reply to Gaetan Rivet

This commit replaces redundant code with public ethdev layer calls.

Signed-off-by Gaetan Rivet <[email protected]>

---
 app/test-pmd/cmdline.c      | 31 +++++++++++++++----------------
 app/test-pmd/cmdline_flow.c |  2 +-
 app/test-pmd/config.c       | 12 ++++++------
 app/test-pmd/parameters.c   |  4 ++--
 app/test-pmd/testpmd.c      | 44 +++++++++++---------------------------------
 app/test-pmd/testpmd.h      |  9 ---------
 6 files changed, 35 insertions(+), 67 deletions(-)

app/test-pmd/cmdline.c +15 ~~-16~~

index aac4efb..cf5da4b 100644
@@ -1160,7 +1160,7 @@ cmd_config_speed_all_parsed(void *parsed_result,
 			&link_speed) < 0)
 		return;
 
-	FOREACH_PORT(pid, ports) {
+	RTE_ETH_FOREACH_DEV(pid) {
 		ports[pid].dev_conf.link_speeds = link_speed;
 	}

@@ -4662,7 +4662,6 @@ static void cmd_create_bonded_device_parsed(void *parsed_result,
 		nb_ports = rte_eth_dev_count();
 		reconfig(port_id, res->socket);
 		rte_eth_promiscuous_enable(port_id);
-		ports[port_id].enabled = 1;
 	}
 
 }

@@ -5021,7 +5020,7 @@ static void cmd_set_promisc_mode_parsed(void *parsed_result,
 
 	/* all ports */
 	if (allports) {
-		FOREACH_PORT(i, ports) {
+		RTE_ETH_FOREACH_DEV(i) {
 			if (enable)
 				rte_eth_promiscuous_enable(i);
 			else

@@ -5101,7 +5100,7 @@ static void cmd_set_allmulti_mode_parsed(void *parsed_result,
 
 	/* all ports */
 	if (allports) {
-		FOREACH_PORT(i, ports) {
+		RTE_ETH_FOREACH_DEV(i) {
 			if (enable)
 				rte_eth_allmulticast_enable(i);
 			else

@@ -5835,31 +5834,31 @@ static void cmd_showportall_parsed(void *parsed_result,
 	struct cmd_showportall_result *res = parsed_result;
 	if (!strcmp(res->show, "clear")) {
 		if (!strcmp(res->what, "stats"))
-			FOREACH_PORT(i, ports)
+			RTE_ETH_FOREACH_DEV(i)
 				nic_stats_clear(i);
 		else if (!strcmp(res->what, "xstats"))
-			FOREACH_PORT(i, ports)
+			RTE_ETH_FOREACH_DEV(i)
 				nic_xstats_clear(i);
 	} else if (!strcmp(res->what, "info"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			port_infos_display(i);
 	else if (!strcmp(res->what, "stats"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			nic_stats_display(i);
 	else if (!strcmp(res->what, "xstats"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			nic_xstats_display(i);
 	else if (!strcmp(res->what, "fdir"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			fdir_get_infos(i);
 	else if (!strcmp(res->what, "stat_qmap"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			nic_stats_mapping_display(i);
 	else if (!strcmp(res->what, "dcb_tc"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			port_dcb_info_display(i);
 	else if (!strcmp(res->what, "cap"))
-		FOREACH_PORT(i, ports)
+		RTE_ETH_FOREACH_DEV(i)
 			port_offload_cap_display(i);
 }

@@ -10339,7 +10338,7 @@ cmd_config_l2_tunnel_eth_type_all_parsed
 	entry.l2_tunnel_type = str2fdir_l2_tunnel_type(res->l2_tunnel_type);
 	entry.ether_type = res->eth_type_val;
 
-	FOREACH_PORT(pid, ports) {
+	RTE_ETH_FOREACH_DEV(pid) {
 		rte_eth_dev_l2_tunnel_eth_type_conf(pid, &entry);
 	}
 }

@@ -10455,7 +10454,7 @@ cmd_config_l2_tunnel_en_dis_all_parsed(
 	else
 		en = 0;
 
-	FOREACH_PORT(pid, ports) {
+	RTE_ETH_FOREACH_DEV(pid) {
 		rte_eth_dev_l2_tunnel_offload_set(pid,
 						  &entry,
 						  ETH_L2_TUNNEL_ENABLE_MASK,

@@ -13114,7 +13113,7 @@ cmd_reconfig_device_queue(portid_t id, uint8_t dev, uint8_t queue)
 	if (id == (portid_t)RTE_PORT_ALL) {
 		portid_t pid;
 
-		FOREACH_PORT(pid, ports) {
+		RTE_ETH_FOREACH_DEV(pid) {
 			/* check if need_reconfig has been set to 1 */
 			if (ports[pid].need_reconfig == 0)
 				ports[pid].need_reconfig = dev;

app/test-pmd/cmdline_flow.c +1 -1

index 2149df4..4e99f0f 100644
@@ -2416,7 +2416,7 @@ comp_port(struct context *ctx, const struct token *token,
 
 	(void)ctx;
 	(void)token;
-	FOREACH_PORT(p, ports) {
+	RTE_ETH_FOREACH_DEV(p) {
 		if (buf && i == ent)
 			return snprintf(buf, size, "%u", p);
 		++i;

app/test-pmd/config.c +6 -6

index 780ce6b..9d05e01 100644
@@ -174,7 +174,7 @@ nic_stats_display(portid_t port_id)
 
 	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 		printf("Valid port range is [0");
-		FOREACH_PORT(pid, ports)
+		RTE_ETH_FOREACH_DEV(pid)
 			printf(", %d", pid);
 		printf("]\n");
 		return;

@@ -252,7 +252,7 @@ nic_stats_clear(portid_t port_id)
 
 	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 		printf("Valid port range is [0");
-		FOREACH_PORT(pid, ports)
+		RTE_ETH_FOREACH_DEV(pid)
 			printf(", %d", pid);
 		printf("]\n");
 		return;

@@ -334,7 +334,7 @@ nic_stats_mapping_display(portid_t port_id)
 
 	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 		printf("Valid port range is [0");
-		FOREACH_PORT(pid, ports)
+		RTE_ETH_FOREACH_DEV(pid)
 			printf(", %d", pid);
 		printf("]\n");
 		return;

@@ -452,7 +452,7 @@ port_infos_display(portid_t port_id)
 
 	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 		printf("Valid port range is [0");
-		FOREACH_PORT(pid, ports)
+		RTE_ETH_FOREACH_DEV(pid)
 			printf(", %d", pid);
 		printf("]\n");
 		return;

@@ -725,7 +725,7 @@ port_id_is_invalid(portid_t port_id, enum print_warning warning)
 	if (port_id == (portid_t)RTE_PORT_ALL)
 		return 0;
 
-	if (port_id < RTE_MAX_ETHPORTS && ports[port_id].enabled)
+	if (rte_eth_dev_is_valid_port(port_id))
 		return 0;
 
 	if (warning == ENABLED_WARN)

@@ -2281,7 +2281,7 @@ set_fwd_ports_mask(uint64_t portmask)
 		return;
 	}
 	nb_pt = 0;
-	for (i = 0; i < (unsigned)RTE_MIN(64, RTE_MAX_ETHPORTS); i++) {
+	RTE_ETH_FOREACH_DEV(i) {
 		if (! ((uint64_t)(1ULL << i) & portmask))
 			continue;
 		portlist[nb_pt++] = i;

app/test-pmd/parameters.c +2 -2

index 28db8cd..67d8bf2 100644
@@ -394,7 +394,7 @@ parse_portnuma_config(const char *q_arg)
 		port_id = (uint8_t)int_fld[FLD_PORT];
 		if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 			printf("Valid port range is [0");
-			FOREACH_PORT(pid, ports)
+			RTE_ETH_FOREACH_DEV(pid)
 				printf(", %d", pid);
 			printf("]\n");
 			return -1;

@@ -454,7 +454,7 @@ parse_ringnuma_config(const char *q_arg)
 		port_id = (uint8_t)int_fld[FLD_PORT];
 		if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 			printf("Valid port range is [0");
-			FOREACH_PORT(pid, ports)
+			RTE_ETH_FOREACH_DEV(pid)
 				printf(", %d", pid);
 			printf("]\n");
 			return -1;

app/test-pmd/testpmd.c +11 ~~-33~~

140 lines not on this page · raw message

app/test-pmd/testpmd.h +0 -9

index 5c18151..011f4b5 100644
@@ -163,7 +163,6 @@ struct port_flow {
  * The data structure associated with each port.
  */
 struct rte_port {
-	uint8_t                 enabled;    /**< Port enabled or not */
 	struct rte_eth_dev_info dev_info;   /**< PCI info + driver name */
 	struct rte_eth_conf     dev_conf;   /**< Port configuration. */
 	struct ether_addr       eth_addr;   /**< Port ethernet address */

@@ -195,14 +194,6 @@ struct rte_port {
 	struct port_flow        *flow_list; /**< Associated flows. */
 };
 
-extern portid_t __rte_unused
-find_next_port(portid_t p, struct rte_port *ports, int size);
-
-#define FOREACH_PORT(p, ports) \
-	for (p = find_next_port(0, ports, RTE_MAX_ETHPORTS); \
-	    p < RTE_MAX_ETHPORTS; \
-	    p = find_next_port(p + 1, ports, RTE_MAX_ETHPORTS))
-
 /**
  * The data structure associated with each forwarding logical core.
  * The logical cores are internally numbered by a core index from 0 to

Signature · 1 line

2.1.4