From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 7D0D2377C for ; Wed, 20 Jul 2016 17:07:56 +0200 (CEST) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga102.fm.intel.com with ESMTP; 20 Jul 2016 08:07:55 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,394,1464678000"; d="scan'208";a="142599956" Received: from orsmsx108.amr.corp.intel.com ([10.22.240.6]) by fmsmga004.fm.intel.com with ESMTP; 20 Jul 2016 08:07:55 -0700 Received: from orsmsx160.amr.corp.intel.com (10.22.226.43) by ORSMSX108.amr.corp.intel.com (10.22.240.6) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 20 Jul 2016 08:07:55 -0700 Received: from orsmsx109.amr.corp.intel.com ([169.254.11.206]) by ORSMSX160.amr.corp.intel.com ([169.254.13.231]) with mapi id 14.03.0248.002; Wed, 20 Jul 2016 08:07:54 -0700 From: "Tootoonchian, Amin" To: "Kerlin, MarcinX" CC: "dev@dpdk.org" , "thomas.monjalon@6wind.com" Thread-Topic: [PATCH] ethdev: ensure consistent port id assignment Thread-Index: AdHb4OxXvCXtsLkTSc+EWpRn1GpjVgGftbBwAA3FvRA= Date: Wed, 20 Jul 2016 15:07:54 +0000 Message-ID: <5905C8E33883CA46A8878E2D7724E215172927@ORSMSX109.amr.corp.intel.com> References: <5905C8E33883CA46A8878E2D7724E21516DF5D@ORSMSX109.amr.corp.intel.com> <68D830D942438745AD09BAFA99E33E812746D0@IRSMSX102.ger.corp.intel.com> In-Reply-To: <68D830D942438745AD09BAFA99E33E812746D0@IRSMSX102.ger.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.22.254.139] 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 15:07:57 -0000 Hi Marcin, Comments inline: > -----Original Message----- > From: Kerlin, MarcinX > Sent: Wednesday, July 20, 2016 1:51 AM > To: Tootoonchian, Amin > Cc: dev@dpdk.org; thomas.monjalon@6wind.com > Subject: RE: [PATCH] ethdev: ensure consistent port id assignment >=20 > Hi Amin, >=20 > > -----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 > > > > 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 primary. The current code breaks if the enumeration of ethdevs in > > primary and secondary 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). > > > > 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 and initialized by the primary process). Upon releasing a > > port, the primary process zeros out eth_dev->data to avoid false > > positives in port id lookup by rte_eth_dev_get_port_by_name(). > > > > Signed-off-by: Amin Tootoonchian > > --- > > lib/librte_ether/rte_ethdev.c | 44 > > +++++++++++++++++++++++++++++------------ > > -- > > 1 file changed, 30 insertions(+), 14 deletions(-) > > > > 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; > > > > - 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(); > > > > - 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) { >=20 >=20 > I was working also on this problem but I didn't send patch yet, so I did = review of > your code. >=20 > Condition (rte_eth_dev_get_port_by_name(name, &port_id) !=3D 0) will alwa= ys > fail. > Secondary process enter here and he will be looking for him name but has = not > yet added and the application return NULL here e.g. we will run app with = device > name pcap1 but this device is not on list and function return null. This is the intended behavior with this patch. Ports are to be created only= by the primary process. This is required for correct operation IMO, becaus= e if we allow secondary processes to create ports dynamically (and locally = use conflicting port ids) without any synchronization mechanism, they're gu= aranteed to overwrite each other's rte_eth_dev_data. > > + RTE_PMD_DEBUG_TRACE("Ethernet Device with name > > %s could not be found!\n", > > + name); > > + return NULL; > > + } > > } > > > > 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; > > + } > > + >=20 >=20 > 1. rte_eth_dev_data[port_id] -> port id should be shifted because seconda= ry > process > overwrite e.g. first position which is common with primary process, so sh= ould be > add at the end > > 2. If this condition is true only for primary it means that secondary pro= cess can't > add own name. > So this excludes with above line: "if (rte_eth_dev_get_port_by_name(name, > &port_id) !=3D 0)"? No, with this patch, secondary processes cannot add their own ports, but in= stead, through some mechanism, should ask the primary process to create the= port for them. > I will send also my patch soon and we can compare and prepare a common > version. We should > keep in mind also the hot plugging. Thanks Marcin! Indeed, I have been using this patch in an environment which= we use hotplug very aggressively and I confirm that the patch works flawle= ssly. Of course, all processes using an about-to-be-removed port need to cl= ose and detach. For attaching, the primary attaches first and any secondary= can attach next. Thomas, your thoughts? Thanks, Amin