From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id EAE4B7E8E for ; Wed, 29 Oct 2014 16:05:54 +0100 (CET) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga103.fm.intel.com with ESMTP; 29 Oct 2014 08:08:24 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.97,862,1389772800"; d="scan'208";a="407922574" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.17]) by FMSMGA003.fm.intel.com with SMTP; 29 Oct 2014 08:06:16 -0700 Received: by (sSMTP sendmail emulation); Wed, 29 Oct 2014 15:14:21 +0100 Date: Wed, 29 Oct 2014 15:14:20 +0000 From: Bruce Richardson To: Tetsuya Mukawa Message-ID: <20141029145207.GA7084@bricha3-MOBL3> References: <1414572576-21371-1-git-send-email-mukawa@igel.co.jp> <1414572576-21371-3-git-send-email-mukawa@igel.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1414572576-21371-3-git-send-email-mukawa@igel.co.jp> Organization: Intel Shannon Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) Cc: dev@dpdk.org, nakajima.yoshihiro@lab.ntt.co.jp, masutani.hitoshi@lab.ntt.co.jp Subject: Re: [dpdk-dev] [RFC PATCH 02/25] ethdev: Remove assumption that port will not be detached X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Oct 2014 15:05:56 -0000 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 > --- > 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(ð_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