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 981092C56 for ; Fri, 7 Oct 2016 14:23:31 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga103.fm.intel.com with ESMTP; 07 Oct 2016 05:23:31 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,308,1473145200"; d="scan'208";a="1041632347" Received: from irsmsx103.ger.corp.intel.com ([163.33.3.157]) by orsmga001.jf.intel.com with ESMTP; 07 Oct 2016 05:23:29 -0700 Received: from irsmsx156.ger.corp.intel.com (10.108.20.68) by IRSMSX103.ger.corp.intel.com (163.33.3.157) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 7 Oct 2016 13:23:28 +0100 Received: from irsmsx102.ger.corp.intel.com ([169.254.2.198]) by IRSMSX156.ger.corp.intel.com ([169.254.3.80]) with mapi id 14.03.0248.002; Fri, 7 Oct 2016 13:23:28 +0100 From: "Kerlin, MarcinX" To: Thomas Monjalon CC: "dev@dpdk.org" , "De Lara Guarch, Pablo" Thread-Topic: [PATCH v5 1/2] librte_ether: add protection against overwrite device data Thread-Index: AQHSGyRENUzbhZI1jkOj78RFQIWo3aCbe40AgAFud4A= Date: Fri, 7 Oct 2016 12:23:27 +0000 Message-ID: <68D830D942438745AD09BAFA99E33E812BE66F@IRSMSX102.ger.corp.intel.com> References: <1474974783-4861-2-git-send-email-marcinx.kerlin@intel.com> <1475244055-6309-1-git-send-email-marcinx.kerlin@intel.com> <1475244055-6309-2-git-send-email-marcinx.kerlin@intel.com> <4741418.sMAp9bqNYx@xps13> In-Reply-To: <4741418.sMAp9bqNYx@xps13> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v5 1/2] librte_ether: add protection against overwrite device data 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: Fri, 07 Oct 2016 12:23:32 -0000 Hi Thomas, > -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > Sent: Thursday, October 06, 2016 4:53 PM > To: Kerlin, MarcinX > Cc: dev@dpdk.org; De Lara Guarch, Pablo > Subject: Re: [PATCH v5 1/2] librte_ether: add protection against overwrit= e > device data >=20 > 2016-09-30 16:00, Marcin Kerlin: > > Added protection against overwrite device data in array > > rte_eth_dev_data[] for the next secondary applications. Secondary > > process appends in the first free place rather than at the beginning. > > This behavior prevents overwriting devices data of primary process by > secondary process. >=20 > It would be good to state what is a secondary process. > You are trying to extend its capabilities to be able to initialize device= s. > So primary and secondary processes are almost equivalent? > What happens if we do not create any device in the primary? > Answer from code review: "Cannot allocate memzone for ethernet port data\= n" >=20 > The secondary process is a hack to me. > But it is fine to have such hack for debug or monitoring purpose. > I would like to understand what are the other use cases? It's true, it is fine for debug or monitoring but If DPDK allow run seconda= ry app with=20 devices then it should be safe or completely not allowed.=20 This bug has been observed while running secondary testpmd with virtual dev= ices. I will adapt to the decision of maintainers regards to design of secondary = process. >=20 > By the way, the code managing the shared data of a device should be at th= e > EAL level in order to be used by other interfaces like crypto. >=20 > > @@ -631,6 +692,8 @@ int > > rte_eth_dev_detach(uint8_t port_id, char *name) { > > struct rte_pci_addr addr; > > + struct rte_eth_dev_data *eth_dev_data =3D NULL; > > + char device[RTE_ETH_NAME_MAX_LEN]; > > int ret =3D -1; > > > > if (name =3D=3D NULL) { > > @@ -642,6 +705,15 @@ rte_eth_dev_detach(uint8_t port_id, char *name) > > if (rte_eth_dev_is_detachable(port_id)) > > goto err; > > > > + /* get device name by port id */ > > + if (rte_eth_dev_get_name_by_port(port_id, device)) > > + goto err; > > + > > + /* look for an entry in the shared device data */ > > + eth_dev_data =3D rte_eth_dev_get_dev_data_by_name(device); > > + if (eth_dev_data =3D=3D NULL) > > + goto err; >=20 > Why not getting eth_dev_data from rte_eth_devices[port_id].data ? because rte_eth_devices[port_id].data for some drivers (mainly virtual devi= ces) is pointer to local eth_dev_data (e.g rte_eth_pcap.c:816 and also other dri= vers). This causes that local eth_dev_data is clearing rather than shared in memzo= ne.=20 Naming is unique so if device was added then there (shared memzone) has to = be.=20 >=20 > > --- a/lib/librte_ether/rte_ethdev.h > > +++ b/lib/librte_ether/rte_ethdev.h > > /** > > * @internal > > + * Release device data kept in shared memory for all processes. > > + * > > + * @param port_id > > + * The port identifier of the device to release device data. > > + * @return > > + * - 0 on success, negative on error > > + */ > > +int rte_eth_dev_release_dev_data(uint8_t port_id); >=20 > Why this function? It is not used. > You already have done the job in the detach function. This function is using in testpmd.c:1640, basic wrapper for clean up. Detach function is working only for detachable devices, release_dev_data()= =20 no matter just clean up shared array before next run secondary e.g testpmd. Regards, Marcin