Re: [dpdk-dev] [RFC PATCH 02/25] ethdev: Remove assumption that port will not be detached

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Oct 29, 2014 at 05:49:13PM +0900, Tetsuya Mukawa wrote:
> To remove assumption, do like followings.
> 
> - Add 'attached' member to rte_eth_dev structure.
>   This member is used for indicating the port is attached, or not.
> - Delete nb_ports, and fix rte_eth_dev_count().
>   The value was used for counting attached ports and also used for indicating
>   maximum attached port number. But if some ports are detached, these 2 values
>   may not be equal. So delete nb_ports, and fix rte_eth_dev_count() to count
>   how many ports are attached actually.
> - Add rte_eth_dev_allocate_new_port().
>   This function is used for allocating new port.
> - Add rte_eth_dev_validate_port().
>   This function is used for check whether the port number is valid and the
>   port is attached.
> - Replace port validation codes to rte_eth_dev_validate_port().
>   nb_ports is deleted in this patch, so use rte_eth_dev_validate_port() to
>   validate a port.
> 
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> ---
>  lib/librte_ether/rte_ethdev.c | 247 +++++++++++++++++++++++-------------------
>  lib/librte_ether/rte_ethdev.h |   5 +
>  2 files changed, 143 insertions(+), 109 deletions(-)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index ff1c769..93d5a42 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -109,7 +109,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 = NULL;
> -static uint8_t nb_ports = 0;
>  
>  /* spinlock for eth device callbacks */
>  static rte_spinlock_t rte_eth_dev_cb_lock = RTE_SPINLOCK_INITIALIZER;
> @@ -201,19 +200,33 @@ rte_eth_dev_allocated(const char *name)
>  {
>  	unsigned i;
>  
> -	for (i = 0; i < nb_ports; i++) {
> -		if (strcmp(rte_eth_devices[i].data->name, name) == 0)
> +	for (i = 0; i < RTE_MAX_ETHPORTS; i++)
> +		if (rte_eth_devices[i].attached && strcmp(
> +				rte_eth_dev_data[i].name, name) == 0)
>  			return &rte_eth_devices[i];
> -	}
> +
>  	return NULL;
>  }
>  
> +static uint8_t
> +rte_eth_dev_allocate_new_port(void)
> +{
> +	unsigned i;
> +
> +	for (i = 0; i < RTE_MAX_ETHPORTS; i++)
> +		if (!rte_eth_devices[i].attached)
> +			return i;
> +	return RTE_MAX_ETHPORTS;
> +}
> +
>  struct rte_eth_dev *
>  rte_eth_dev_allocate(const char *name)
>  {
> +	uint8_t port_id;
>  	struct rte_eth_dev *eth_dev;
>  
> -	if (nb_ports == RTE_MAX_ETHPORTS) {
> +	port_id = rte_eth_dev_allocate_new_port();
> +	if (port_id >= RTE_MAX_ETHPORTS) {
>  		PMD_DEBUG_TRACE("Reached maximum number of Ethernet ports\n");
>  		return NULL;
>  	}
> @@ -226,10 +239,11 @@ rte_eth_dev_allocate(const char *name)
>  		return NULL;
>  	}
>  
> -	eth_dev = &rte_eth_devices[nb_ports];
> -	eth_dev->data = &rte_eth_dev_data[nb_ports];
> +	eth_dev = &rte_eth_devices[port_id];
> +	eth_dev->data = &rte_eth_dev_data[port_id];
>  	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
> -	eth_dev->data->port_id = nb_ports++;
> +	eth_dev->data->port_id = port_id;
> +	eth_dev->attached = 1;
>  	return eth_dev;
>  }
>  
> @@ -283,7 +297,7 @@ rte_eth_dev_init(struct rte_pci_driver *pci_drv,
>  			(unsigned) pci_dev->id.device_id);
>  	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
>  		rte_free(eth_dev->data->dev_private);
> -	nb_ports--;
> +	eth_dev->attached = 0;
>  	return diag;
>  }
>  
> @@ -308,10 +322,19 @@ rte_eth_driver_register(struct eth_driver *eth_drv)
>  	rte_eal_pci_register(&eth_drv->pci_drv);
>  }
>  
> +static int
> +rte_eth_dev_validate_port(uint8_t port_id)
> +{
> +	if (port_id >= RTE_MAX_ETHPORTS)
> +		return 1;
> +
> +	return !rte_eth_devices[port_id].attached;
> +}
> +
>  int
>  rte_eth_dev_socket_id(uint8_t port_id)
>  {
> -	if (port_id >= nb_ports)
> +	if (rte_eth_dev_validate_port(port_id))
>  		return -1;
>  	return rte_eth_devices[port_id].pci_dev->numa_node;
>  }
> @@ -319,7 +342,13 @@ rte_eth_dev_socket_id(uint8_t port_id)
>  uint8_t
>  rte_eth_dev_count(void)
>  {
> -	return (nb_ports);
> +	unsigned i;
> +	uint8_t nb_ports = 0;
> +
> +	for (i = 0; i < RTE_MAX_ETHPORTS; i++)
> +		if (rte_eth_devices[i].attached)
> +			nb_ports++;
> +	return nb_ports;
>  }
>  

Given that apps may regularly use eth_dev_count, might it not be worthwhile to retain the nb_ports local variable to make this API call just be a variable read. The other APIs to enable/disable individual ports could easily just inc/dec this var as they do so.

/Bruce