From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754158Ab3KGURZ (ORCPT ); Thu, 7 Nov 2013 15:17:25 -0500 Received: from smtp02.citrix.com ([66.165.176.63]:8468 "EHLO SMTP02.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751701Ab3KGURQ (ORCPT ); Thu, 7 Nov 2013 15:17:16 -0500 X-IronPort-AV: E=Sophos;i="4.93,653,1378857600"; d="scan'208";a="69233065" Date: Thu, 7 Nov 2013 20:15:57 +0000 From: Stefano Stabellini X-X-Sender: sstabellini@kaball.uk.xensource.com To: Konrad Rzeszutek Wilk CC: Matt Wilson , Stefano Stabellini , David Vrabel , Anthony Liguori , =?UTF-8?Q?Roger_Pau_Monn=C3=A9?= , Ian Campbell , , , Matt Wilson Subject: Re: [Xen-devel] [PATCH] grant-table: don't set m2p override if kmap_ops is not set In-Reply-To: <20131106185233.GI17101@phenom.dumpdata.com> Message-ID: References: <5278E64C.40200@citrix.com> <527904EC.9090402@citrix.com> <20131105145607.GB25836@phenom.dumpdata.com> <5279085E.7070703@citrix.com> <1383664090.13961.88.camel@kazak.uk.xensource.com> <527916EE.6060104@citrix.com> <20131105200616.GA23362@u109add4315675089e695.ant.amazon.com> <878ux2vade.fsf@codemonkey.ws> <527A2943.7050400@citrix.com> <20131106175934.GA18002@u109add4315675089e695.ant.amazon.com> <20131106185233.GI17101@phenom.dumpdata.com> User-Agent: Alpine 2.02 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" X-DLP: MIA1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 6 Nov 2013, Konrad Rzeszutek Wilk wrote: > On Wed, Nov 06, 2013 at 09:59:34AM -0800, Matt Wilson wrote: > > On Wed, Nov 06, 2013 at 11:34:27AM +0000, David Vrabel wrote: > > [...] > > > > > > Matt, Anthony, I presume you have profiling results or performance data > > > that support this proposed change? Can you provide them? > > > > I've measured 10-20% performance improvement in configurations where: > > > > 1) dom0 has a moderate number of vCPUs doing blkback work > > 2) domU has 32 vCPUs > > 3) 24 configured VBDs without persistent grant support > > 4) some lock contention in grant table hypercalls has been alleviated > > > > More specific results are still in the works. > > > > > > It's perfectly fine to store a foreign pfn in the m2p table. The m2p > > > > override table is used by the grant device to allow a reverse lookup of > > > > the real mfn to a pfn even if it's foreign. > > > > > > > > blkback doesn't actually need this though. This was introduced in: > > > > > > > > commit 5dc03639cc903f887931831d69895facb5260f4b > > > > Author: Konrad Rzeszutek Wilk > > > > Date: Tue Mar 1 16:46:45 2011 -0500 > > > > > > > > xen/blkback: Utilize the M2P override mechanism for GNTMAP_host_map > > > > > > > > Purely as an optimization. In practice though due to lock contention it > > > > slows things down. > > > > > > The full changeset description for this change doesn't make sense to me. > > > > > > xen/blkback: Utilize the M2P override mechanism for GNTMAP_host_map > > > > > > Instead of doing copy grants lets do mapping grants using > > > the M2P(and P2M) override mechanism. > > > > > > As all it is doing is replacing set_phys_to_machine() calls with > > > m2p_add_override(). > > > > Indeed, since this had nothing to do with copying. We were confused > > also. Konrad? > > > > 2011? Hm, I really don't remember. It does not look to be needed. > > I think that back in 2011 the m2p override mechanism was much simpler > and was just a wrapper around set_phys_to_machine with a non-lock > write in the m2p override. > > The only concern I had, which David had looked at and I did here > too was the DMA unmap operation, but as you can see from the > writeup - it is not warranted. > > I think that the set_phys_to_machine is the way to go then > and ditching the m2p_override. Two questions remain: > > - How does this work when block back is running in an HVM domain? HVM guests don't use the m2p_override, so no problems. > - Stefano, do we need to worry about the crazy scenario of > dom0 using xen-blkfront and xen-blkback inside itself to fetch > bits of data from QEMU? It's not a crazy scenario, in fact it is enabled by default on modern systems :-) In any case it wouldn't be affected by changes that only impact the "no kmap_ops" case, because QEMU uses the gntdev device, therefore kmap_ops would be set. > ? > Here is how it interacts with SWIOTLB: > > > 1) Backend gets the foreign MFN from the grant call. Calls m2p_add_override > which calls set_phys_to_machine and also adds the MFN on the m2p_overrides. > Takes a lock. > 2). Backend submit_bio, it ends up in AHCI driver. Said driver does dma_map_sg > 3). We call xen_swiotlb_map_sg_attrs, which does: > a) pfn_to_mfn, gets from the P2M the MFN | FOREIGN_FRAME_BIT. Strips > the FOREIGN_FRAME_BIT. Returns MFN. > b). Setups up the sg->dma_address > > But it also might setup an bounce buffer in case the AHCI can't reach > the MFN >> PAGE_SHIFT. At which point we just save the virtual > address of the page handed to us in the IOTLB. > > 4). .. ahci driver does it cmd, once it is done it calls 'dma_unmap_sg' > which ends up in xen_swiotlb_unmap_sg_attrs and we call xen_unmap_single > which calls: > > 393 phys_addr_t paddr = xen_bus_to_phys(dev_addr); > which ends up doing (in mfn_to_pfn): > > 112 pfn = mfn_to_pfn_no_overrides(mfn); > > lookup in the M2P array. We find an MFN and then we check the P2M: > > 113 if (get_phys_to_machine(pfn) != mfn) { > > Since the MFN hasn't changed (it is still the 'local' one instead > of the forign one - since we can't modify the M2P) we end up > with p2m(pfn) != mfn. As the p2m(pfn) ends up giving us the 'foreign' > MFN value and the m2p(pfn) ends up giving us the 'local' MFN. > > So then we consult the override: > 114 /* > 115 * If this appears to be a foreign mfn (because the pfn > 116 * doesn't map back to the mfn), then check the local override > 117 * table to see if there's a better pfn to use. > 118 * > 119 * m2p_find_override_pfn returns ~0 if it doesn't find anything. > 120 */ > 121 pfn = m2p_find_override_pfn(mfn, ~0); > > And we find it the MFN of the foreign domain, return it back to the SWIOTLB > as physical address (so mfn << PAGE_SHIFT) so it can a) either ignore it, > or b) if the bounce buffer ended up being used - ignore it too. As the > bounce buffer mechanism ends up using the original stashed virtual address > and bounces between the bounce buffer and foreign owned virtual address. > > The dma_mark_clean is a nop. Removing the m2p_add_override from blkback would cause xen_bus_to_phys to return ~0 on x86 if the swiotlb is not bouncing the dma request. In that case all the following operations in xen_unmap_single would fail, but nothing actually needs to be done on x86. If the request is bounced everything works as usual. The situation is different on ARM: when dma requests are not bounced we need to call the native ARM unmap_page dma_op for cache coherency reasons. But on ARM we don't use the m2p_override, we opt instead for a couple of rbtrees updated at every call of set_phys_to_machine. Effectively we would still have something equivalent to the m2p_override. Therefore I think that we can avoid the m2p_add_override call from blkback but I would like a very clear comment in the code to explain the situation.