Messages in this thread |  | | Date | Wed, 13 Nov 2013 13:13:08 +0000 | From | Stefano Stabellini <> | Subject | Re: [PATCH] xen-gnttab: do not add m2p override entries for blkback mappings |
| |
On Wed, 13 Nov 2013, David Vrabel wrote: > On 13/11/13 01:48, Anthony Liguori wrote: > > From: Anthony Liguori <aliguori@amazon.com> > > > > Commit 5dc03639 switched blkback to also add m2p override entries > > when mapping grant pages but history seems to have forgotten why > > this is useful if it ever was. > > > > The blkback driver does not need m2p override entries to exist > > and there is significant overhead due to the locking in the m2p > > override table. We see about a 10% improvement in IOP rate with > > this patch applied running FIO in the guest. > > > > See http://article.gmane.org/gmane.linux.kernel/1590932 for a full > > analysis of current users. > > I think it would be better if it was made clearer what > m2p_add/remove_override() is for (i.e., allowing get_user_pages_fast() > to work) so there isn't this confusion in the future. Please add some > comments for this and move the calls into the gntdev driver.
Even though get_user_pages_fast is obviously the most important use case for the m2p_override, it is not the only one, see below.
> As a prerequiste, the call to dma_mark_clean() in xen_swiotlb_unmap() > needs to be removed and replaced with a comment explaining why this now > differs from the generic implementation. i.e., the necessary > phys_to_virt() will not work for foreign pages
Actually it is machine_to_phys that is not going to work and only on x86. machine_to_phys is going to be fine for foreign pages on ARM.
Let's be clear about this: without the m2p_override on x86 xen_swiotlb_unmap is going to fail and that undeniably is a mistake. However it is a mistake that happens not to cause any problems today.
In fact the following comment is not right:
+/* override is used to add m2p override table entries when mapping the + * grant references. Currently there are only two callers to this function, + * blkback and gntdev. gntdev needs all grant mappings to have corresponding + * m2p override table entries but blkback currently doesn't. This is because + * blkback can gracefully handle the case where m2p(p2m(pfn)) fails with + * foreign pfns. If you cannot handle this correctly, you need to set + * override 1 when calling the map and unmap functions. + */
I would add the following comment to xen_unmap_single instead:
/* On x86 xen_bus_to_phys is going to fail for foreign pages unless they * have been added to the m2p_override. Only gntdev currently adds them to * the m2p_override, while blkback does not. As a consequence any foreign * pages mapped by blkback and used for DMA are not going to be unmapped * correctly here. Fortunately this is not a problem because this function * is actually a nop for non-swiotlb pages on x86. */
> and dma_mark_clean() is a nop on all Xen supported architectures.
That's true, it should be removed. It is just confusing.
|  |