diff mbox

[dpdk-dev,3/4] ethdev: count devices consistently

Message ID 08811b2c92fa8c802a13000186aaebd5db5ee2ca.1488550937.git.gaetan.rivet@6wind.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail apply patch file failure

Commit Message

Gaëtan Rivet March 3, 2017, 3:40 p.m. UTC 
  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 <gaetan.rivet@6wind.com>
---
 lib/librte_ether/rte_ethdev.c | 20 ++++++++++----------
 lib/librte_ether/rte_ethdev.h | 14 ++++++++------
 2 files changed, 18 insertions(+), 16 deletions(-)

Comments

Thomas Monjalon March 30, 2017, 7:26 p.m. UTC | #1 
2017-03-03 16:40, Gaetan Rivet:
> 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.

> 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.
[...]
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> -#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 March 31, 2017, 9:13 a.m. UTC | #2 
On Thu, Mar 30, 2017 at 09:26:12PM +0200, Thomas Monjalon wrote:
>2017-03-03 16:40, Gaetan Rivet:
>> 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.

>> 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.
>[...]
>> --- a/lib/librte_ether/rte_ethdev.h
>> +++ b/lib/librte_ether/rte_ethdev.h
>> -#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.
diff mbox

Patch

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index fcb9933..3a52d0a 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -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))) {
 
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 59c4123..bdad81b 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -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.