From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 6DF763237 for ; Wed, 20 Jul 2016 10:51:16 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga101.jf.intel.com with ESMTP; 20 Jul 2016 01:51:15 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,393,1464678000"; d="scan'208";a="998877593" Received: from irsmsx101.ger.corp.intel.com ([163.33.3.153]) by orsmga001.jf.intel.com with ESMTP; 20 Jul 2016 01:51:14 -0700 Received: from irsmsx102.ger.corp.intel.com ([169.254.2.123]) by IRSMSX101.ger.corp.intel.com ([169.254.1.155]) with mapi id 14.03.0248.002; Wed, 20 Jul 2016 09:51:13 +0100 From: "Kerlin, MarcinX" To: "Tootoonchian, Amin" CC: "dev@dpdk.org" , "thomas.monjalon@6wind.com" Thread-Topic: [PATCH] ethdev: ensure consistent port id assignment Thread-Index: AdHb4OxXvCXtsLkTSc+EWpRn1GpjVgGftbBw Date: Wed, 20 Jul 2016 08:51:12 +0000 Message-ID: <68D830D942438745AD09BAFA99E33E812746D0@IRSMSX102.ger.corp.intel.com> References: <5905C8E33883CA46A8878E2D7724E21516DF5D@ORSMSX109.amr.corp.intel.com> In-Reply-To: <5905C8E33883CA46A8878E2D7724E21516DF5D@ORSMSX109.amr.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] ethdev: ensure consistent port id assignment 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, 20 Jul 2016 08:51:17 -0000 Hi Amin, > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tootoonchian, Amin > Sent: Tuesday, July 12, 2016 4:01 AM > To: thomas.monjalon@6wind.com > Cc: dev@dpdk.org > Subject: [dpdk-dev] [PATCH] ethdev: ensure consistent port id assignment >=20 > The rte_eth_dev_allocate() code has an implicit assumption that the port = id > assignment in the secondary process is consistent with that of the primar= y. The > current code breaks if the enumeration of ethdevs in primary and secondar= y > processes are not identical (e.g., when the black/whitelist and vdev args= of the > primary and secondary do not match, or when the primary dynamically > adds/removes ethdevs). >=20 > To fix this problem, rte_eth_dev_allocate() now looks up port id by name = in a > secondary process (making it explicit that ethdevs can only be created an= d > initialized by the primary process). Upon releasing a port, the primary p= rocess > zeros out eth_dev->data to avoid false positives in port id lookup by > rte_eth_dev_get_port_by_name(). >=20 > Signed-off-by: Amin Tootoonchian > --- > lib/librte_ether/rte_ethdev.c | 44 +++++++++++++++++++++++++++++--------= ---- > -- > 1 file changed, 30 insertions(+), 14 deletions(-) >=20 > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.= c index > 0a6e3f1..1801f57 100644 > --- a/lib/librte_ether/rte_ethdev.c > +++ b/lib/librte_ether/rte_ethdev.c > @@ -195,25 +195,37 @@ rte_eth_dev_allocate(const char *name, enum > rte_eth_dev_type type) > uint8_t port_id; > struct rte_eth_dev *eth_dev; >=20 > - port_id =3D rte_eth_dev_find_free_port(); > - if (port_id =3D=3D RTE_MAX_ETHPORTS) { > - RTE_PMD_DEBUG_TRACE("Reached maximum number of > Ethernet ports\n"); > - return NULL; > - } > - > if (rte_eth_dev_data =3D=3D NULL) > rte_eth_dev_data_alloc(); >=20 > - if (rte_eth_dev_allocated(name) !=3D NULL) { > - RTE_PMD_DEBUG_TRACE("Ethernet Device with name %s > already allocated!\n", > - name); > - return NULL; > + if (rte_eal_process_type() =3D=3D RTE_PROC_PRIMARY) { > + port_id =3D rte_eth_dev_find_free_port(); > + if (port_id =3D=3D RTE_MAX_ETHPORTS) { > + RTE_PMD_DEBUG_TRACE("Reached maximum number > of Ethernet ports\n"); > + return NULL; > + } > + > + if (rte_eth_dev_allocated(name) !=3D NULL) { > + RTE_PMD_DEBUG_TRACE("Ethernet Device with name > %s already allocated!\n", > + name); > + return NULL; > + } > + } else { > + if (rte_eth_dev_get_port_by_name(name, &port_id) !=3D 0) { I was working also on this problem but I didn't send patch yet, so I did re= view of your code. Condition (rte_eth_dev_get_port_by_name(name, &port_id) !=3D 0) will always= fail. Secondary process enter here and he will be looking for him name but has no= t yet added and the application return NULL here e.g. we will run app with device name = pcap1 but this=20 device is not on list and function return null. > + RTE_PMD_DEBUG_TRACE("Ethernet Device with name > %s could not be found!\n", > + name); > + return NULL; > + } > } >=20 > eth_dev =3D &rte_eth_devices[port_id]; > eth_dev->data =3D &rte_eth_dev_data[port_id]; > - snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", > name); > - eth_dev->data->port_id =3D port_id; > + > + if (rte_eal_process_type() =3D=3D RTE_PROC_PRIMARY) { > + snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), > "%s", name); > + eth_dev->data->port_id =3D port_id; > + } > + 1. rte_eth_dev_data[port_id] -> port id should be shifted because secondary= process=20 overwrite e.g. first position which is common with primary process, so shou= ld be add at the end 2. If this condition is true only for primary it means that secondary proce= ss can't add own name. So this excludes with above line: "if (rte_eth_dev_get_port_by_name(name, &= port_id) !=3D 0)"? I will send also my patch soon and we can compare and prepare a common vers= ion. We should=20 keep in mind also the hot plugging. > eth_dev->attached =3D DEV_ATTACHED; > eth_dev->dev_type =3D type; > nb_ports++; > @@ -293,8 +305,10 @@ rte_eth_dev_init(struct rte_pci_driver *pci_drv, > pci_drv->name, > (unsigned) pci_dev->id.vendor_id, > (unsigned) pci_dev->id.device_id); > - if (rte_eal_process_type() =3D=3D RTE_PROC_PRIMARY) > + if (rte_eal_process_type() =3D=3D RTE_PROC_PRIMARY) { > rte_free(eth_dev->data->dev_private); > + memset(eth_dev->data, 0, sizeof(*rte_eth_dev_data)); > + } > rte_eth_dev_release_port(eth_dev); > return diag; > } > @@ -330,8 +344,10 @@ rte_eth_dev_uninit(struct rte_pci_device *pci_dev) > /* free ether device */ > rte_eth_dev_release_port(eth_dev); >=20 > - if (rte_eal_process_type() =3D=3D RTE_PROC_PRIMARY) > + if (rte_eal_process_type() =3D=3D RTE_PROC_PRIMARY) { > rte_free(eth_dev->data->dev_private); > + memset(eth_dev->data, 0, sizeof(*rte_eth_dev_data)); > + } >=20 > eth_dev->pci_dev =3D NULL; > eth_dev->driver =3D NULL; > -- > 2.8.1 Regards, Marcin