Re: [Xen-devel] [PATCH] grant-table: don't set m2p override ifkmap_ops is not set

[Stefano Stabellini]

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 <konrad.wilk@xxxxxxxxxx>
> > > > 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?
> 
> <confused as well>
> 
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/