From mboxrd@z Thu Jan 1 00:00:00 1970 From: "John Daley (johndale)" Subject: Re: [PATCH] net/enic: fix multi-process operation Date: Tue, 19 Sep 2017 05:31:37 +0000 Message-ID: References: <20170911185833.11458-1-johndale@cisco.com> <488ca130-7a8e-223f-5b9e-50bdab9b93f2@intel.com> <4171800.AgbnscgTn2@xps> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: Ferruh Yigit , "dev@dpdk.org" , Sergio Gonzalez Monroy To: Thomas Monjalon Return-path: Received: from rcdn-iport-6.cisco.com (rcdn-iport-6.cisco.com [173.37.86.77]) by dpdk.org (Postfix) with ESMTP id 5CD471AEF0 for ; Tue, 19 Sep 2017 07:31:39 +0200 (CEST) In-Reply-To: <4171800.AgbnscgTn2@xps> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Monday, September 18, 2017 3:25 PM > To: John Daley (johndale) > Cc: Ferruh Yigit ; dev@dpdk.org; Sergio Gonzalez > Monroy > Subject: Re: [PATCH] net/enic: fix multi-process operation >=20 > 18/09/2017 23:27, Ferruh Yigit: > > On 9/11/2017 7:58 PM, John Daley wrote: > > > - Use rte_malloc() instead of malloc() for the per device 'vdev' stru= cture > > > so that it can be shared across processes. > > > - Only initialize the device if the process type is RTE_PROC_PRIMARY > > > - Only allow the primary process to do queue setup, start/stop, promi= sc > > > allmulticast, mac add/del, mtu. > [...] > > > --- a/drivers/net/enic/enic_ethdev.c > > > +++ b/drivers/net/enic/enic_ethdev.c > > > @@ -142,6 +142,10 @@ enicpmd_dev_filter_ctrl(struct rte_eth_dev > > > *dev, static void enicpmd_dev_tx_queue_release(void *txq) { > > > ENICPMD_FUNC_TRACE(); > > > + > > > + if (rte_eal_process_type() !=3D RTE_PROC_PRIMARY) > > > + return; > > > + > > > > Hi John, > > > > I am not sure about these updates. Agree that these functions should > > know process type, but all others PMDs don't do this. I looked at mlx5 and it does something similar with its mlx5_is_secondary()= in functions that modify fields in its shared private structure. > > > > Added a few more people for comment, but as far I understand its > > application responsibility to NOT call these functions if it is > > secondary process. > > > > For device init/uninit, that is part of eal_init() and have to be > > called both for primary and secondary process and PMD needs to protect > > it, for other functions application's responsibility. I put the checks into the PMD because I didn't want to trust the app and I = didn't see checks in the library functions. I assumed that is why it was do= ne in mlx5. I was afraid that the secondary may try to write fields in the = shared structure and cause some silent bad behavior, like if secondary sets= the MTU then the primary does, then secondary assumes it was different tha= n what it is, or something like that. >=20 > Yes for now it is the policy. > But it is a gray area and it could be clearer with my "ownership proposal= ": > http://dpdk.org/ml/archives/dev/2017-September/074656.html > A secondary process could manage the ports it owns. I think the ownership concept would help make DPDK more flexible and potent= ially cleaner. Perhaps ownership checks could be pushed the lib functions, = like rte_eth_dev_set_mtu(), and remove all such checks in the PMD(s).=20 >=20 > Feel free to comment the proposal.