lkml.org 
[lkml]   [2013]   [Nov]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.


\
 
 \ /
  Last update: 2013-11-13 14:41    [W:0.061 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site