From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 61EB5530A for ; Thu, 21 Jul 2016 15:56:03 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga101.fm.intel.com with ESMTP; 21 Jul 2016 06:55:49 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,399,1464678000"; d="scan'208";a="1011264883" Received: from irsmsx106.ger.corp.intel.com ([163.33.3.31]) by fmsmga001.fm.intel.com with ESMTP; 21 Jul 2016 06:55:50 -0700 Received: from irsmsx112.ger.corp.intel.com (10.108.20.5) by IRSMSX106.ger.corp.intel.com (163.33.3.31) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 21 Jul 2016 14:54:51 +0100 Received: from irsmsx102.ger.corp.intel.com ([169.254.2.123]) by irsmsx112.ger.corp.intel.com ([169.254.1.33]) with mapi id 14.03.0248.002; Thu, 21 Jul 2016 14:54:51 +0100 From: "Kerlin, MarcinX" To: "Tootoonchian, Amin" CC: "dev@dpdk.org" , "Gonzalez Monroy, Sergio" Thread-Topic: [PATCH] ethdev: ensure consistent port id assignment Thread-Index: AdHb4OxXvCXtsLkTSc+EWpRn1GpjVgGftbBwAA3FvRAAL2NkgA== Date: Thu, 21 Jul 2016 13:54:50 +0000 Message-ID: <68D830D942438745AD09BAFA99E33E81275B25@IRSMSX102.ger.corp.intel.com> References: <5905C8E33883CA46A8878E2D7724E21516DF5D@ORSMSX109.amr.corp.intel.com> <68D830D942438745AD09BAFA99E33E812746D0@IRSMSX102.ger.corp.intel.com> <5905C8E33883CA46A8878E2D7724E215172927@ORSMSX109.amr.corp.intel.com> In-Reply-To: <5905C8E33883CA46A8878E2D7724E215172927@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: Thu, 21 Jul 2016 13:56:04 -0000 Hi Amin, > -----Original Message----- > From: Tootoonchian, Amin > Sent: Wednesday, July 20, 2016 5:08 PM > To: Kerlin, MarcinX > Cc: dev@dpdk.org; thomas.monjalon@6wind.com > Subject: RE: [PATCH] ethdev: ensure consistent port id assignment >=20 > Hi Marcin, >=20 > Comments inline: >=20 > > -----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 > > > > 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 > > > > > > 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_na= me(). > > > > > > 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) { > > > > > > I was working also on this problem but I didn't send patch yet, so I > > did review 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 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 funct= ion > return null. >=20 > This is the intended behavior with this patch. Ports are to be created on= ly by the > primary process. This is required for correct operation IMO, because if w= e > allow secondary processes to create ports dynamically (and locally use > conflicting port ids) without any synchronization mechanism, they're > guaranteed to overwrite each other's rte_eth_dev_data. Thanks Amin for clarification, I had another approach, that rte_eth_devices and rte_eth_dev_data should ha= ve=20 different offset of port_id and secondary process can also add devices. as I now understand with this patch we will not be able do something like: Primary: ./test-pmd -c 0xf -n 4 --socket-mem=3D'512,0' -w 03:00.1 -w 03:00.0 =20 --proc-type=3Dprimary --file-prefix=3Dxz1 -- -i Secondary:=20 ./test-pmd -c 0xf0 --socket-mem=3D'512,0' -n 4 -v -b 03:00.1 -b 03:00.0=20 --vdev 'eth_pcap0,rx_pcap=3D/var/log/device1.pcap,tx_pcap=3D/var/log/device= 2.pcap' --proc-type=3Dsecondary --file-prefix=3Dxz1 -- -i=20 Because secondary processes "Ports are to be created only by the primary pr= ocess"? I tried run and secondary process failed here: else { if (rte_eth_dev_get_port_by_name(name, &port_id) !=3D 0) { RTE_PMD_DEBUG_TRACE("Ethernet Device with name %s could not be found!\n"= , name); return NULL; } } because he did not find the name "eth_pcap0" + Sergio, just like you added also in the next mail >=20 > > > + 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; > > > + } > > > + > > > > > > 1. rte_eth_dev_data[port_id] -> port id should be shifted because > > secondary process overwrite e.g. first position which is common with > > primary process, so should be add at the end > > > > 2. If this condition is true only for primary it means that secondary > > process can't add own name. > > So this excludes with above line: "if > > (rte_eth_dev_get_port_by_name(name, > > &port_id) !=3D 0)"? >=20 > No, with this patch, secondary processes cannot add their own ports, but > instead, through some mechanism, should ask the primary process to create > the port for them. >=20 > > 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. >=20 > Thanks Marcin! Indeed, I have been using this patch in an environment whi= ch > we use hotplug very aggressively and I confirm that the patch works flawl= essly. > Of course, all processes using an about-to-be-removed port need to close = and > detach. For attaching, the primary attaches first and any secondary can a= ttach > next. >=20 > Thomas, your thoughts? >=20 > Thanks, > Amin