From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id C7BC64CA1; Fri, 5 Oct 2018 10:53:09 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Oct 2018 01:53:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,343,1534834800"; d="scan'208";a="94850276" Received: from aburakov-mobl1.ger.corp.intel.com (HELO [10.252.17.112]) ([10.252.17.112]) by fmsmga004.fm.intel.com with ESMTP; 05 Oct 2018 01:52:28 -0700 To: Darek Stojaczyk , dev@dpdk.org Cc: wuzhouhui250@gmail.com, pawelx.wodkowski@intel.com, gowrishankar.m@linux.vnet.ibm.com, stable@dpdk.org References: <20181003123925.6343-1-dariusz.stojaczyk@intel.com> <254f2fd2-832b-0722-cb1b-bd0a5050bd00@intel.com> From: "Burakov, Anatoly" Message-ID: <2ec447ba-fa37-1b95-fcb6-c6d45a007853@intel.com> Date: Fri, 5 Oct 2018 09:52:28 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <254f2fd2-832b-0722-cb1b-bd0a5050bd00@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] eal/vfio: share the default container in multi process X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 05 Oct 2018 08:53:10 -0000 On 04-Oct-18 4:52 PM, Burakov, Anatoly wrote: > On 03-Oct-18 1:39 PM, Darek Stojaczyk wrote: >> So far each process in MP used to have a separate container >> and relied on the primary process to register all memsegs. >> >> Mapping external memory via rte_vfio_container_dma_map() >> in secondary processes was broken, because the default >> (process-local) container had no groups bound. There was >> even no way to bind any groups to it, because the container >> fd was deeply encapsulated within EAL. >> >> This patch introduces a new SOCKET_REQ_DEFAULT_CONTAINER >> message type for MP synchronization, makes all processes >> within a MP party use a single default container, and hence >> fixes rte_vfio_container_dma_map() for secondary processes. >> >>> From what I checked this behavior was always the same, but >> started to be invalid/insufficient once mapping external >> memory was allowed. >> >> Fixes: 73a639085938 ("vfio: allow to map other memory regions") >> Cc: anatoly.burakov@intel.com >> Cc: pawelx.wodkowski@intel.com >> Cc: gowrishankar.m@linux.vnet.ibm.com >> Cc: stable@dpdk.org >> >> While here, fix up the comment on rte_vfio_get_container_fd(). >> This function always opens a new container, never reuses >> an old one. >> >> Signed-off-by: Darek Stojaczyk >> --- > > This makes things a bit funky when we consider external memory support. > The same address space may or may not be mapped into the process, so we > may or may not need to map things for DMA depending on whether we're > mapping shared memory for DMA, or not. > > The previous behavior may be counter-intuitive, but it's IMO the correct > one - each process has its own container and performs its own DMA mappings. > Re-reading the commit message, i can see that i've misinterpreted the commit a bit (or rather i missed the fact that we can't even map anything for DMA in the secondaries due to container being not the same container we use devices for). However, my above comment regarding external memory still applies - if we share DMA mappings between processes, it will be possible for secondary to map something else into space occupied by external memory, and potentially overwrite the DMA mappings. Unfortunately, we cannot do anything about it, as that memory isn't under our control. I will have to update my external memory patchset to account for this and map memory for DMA only once, in whatever process that creates the memory in the first place. So, on the concept of this i have no objections here, as i think this fix is necessary. I'll have to review the code some more to make sure i'm not missing anything. -- Thanks, Anatoly