Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1390375 > unrolled thread

[BUG] vfio device assignment regression with THP ref counting redesign

Back to article view | Back to linux.kernel

Contents

  [BUG] vfio device assignment regression with THP ref counting  redesign Alex Williamson <alex.williamson@redhat.com> - 2016-04-28 18:30 +0200
    Re: [BUG] vfio device assignment regression with THP ref counting  redesign "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-04-28 20:20 +0200
      Re: [BUG] vfio device assignment regression with THP ref counting  redesign Alex Williamson <alex.williamson@redhat.com> - 2016-04-28 21:00 +0200
        Re: [BUG] vfio device assignment regression with THP ref counting  redesign Andrea Arcangeli <aarcange@redhat.com> - 2016-04-29 01:30 +0200
          Re: [BUG] vfio device assignment regression with THP ref counting  redesign Alex Williamson <alex.williamson@redhat.com> - 2016-04-29 02:50 +0200
          Re: [BUG] vfio device assignment regression with THP ref counting  redesign "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-04-29 03:00 +0200
            Re: [BUG] vfio device assignment regression with THP ref counting  redesign Alex Williamson <alex.williamson@redhat.com> - 2016-04-29 04:50 +0200
              Re: [BUG] vfio device assignment regression with THP ref counting  redesign "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-04-29 09:10 +0200
                Re: [BUG] vfio device assignment regression with THP ref counting  redesign Alex Williamson <alex.williamson@redhat.com> - 2016-04-29 17:20 +0200
                Re: [BUG] vfio device assignment regression with THP ref counting  redesign Andrea Arcangeli <aarcange@redhat.com> - 2016-04-29 18:40 +0200
                  Re: [BUG] vfio device assignment regression with THP ref counting  redesign Alex Williamson <alex.williamson@redhat.com> - 2016-04-30 00:40 +0200
                  Re: [BUG] vfio device assignment regression with THP ref counting  redesign "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-05-02 12:50 +0200
                    Re: [BUG] vfio device assignment regression with THP ref counting  redesign Jerome Glisse <j.glisse@gmail.com> - 2016-05-02 13:20 +0200
                      GUP guarantees wrt to userspace mappings redesign "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-05-02 14:20 +0200
                        Re: GUP guarantees wrt to userspace mappings redesign Jerome Glisse <j.glisse@gmail.com> - 2016-05-02 15:40 +0200
                          Re: GUP guarantees wrt to userspace mappings "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-05-02 17:10 +0200
                            Re: GUP guarantees wrt to userspace mappings Jerome Glisse <j.glisse@gmail.com> - 2016-05-02 17:30 +0200
                              Re: GUP guarantees wrt to userspace mappings "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-05-02 18:20 +0200
                                Re: GUP guarantees wrt to userspace mappings Andrea Arcangeli <aarcange@redhat.com> - 2016-05-02 21:20 +0200
                              Re: GUP guarantees wrt to userspace mappings Andrea Arcangeli <aarcange@redhat.com> - 2016-05-02 21:20 +0200
                            Re: GUP guarantees wrt to userspace mappings Andrea Arcangeli <aarcange@redhat.com> - 2016-05-02 21:10 +0200
                        Re: GUP guarantees wrt to userspace mappings redesign Oleg Nesterov <oleg@redhat.com> - 2016-05-02 17:20 +0200
                          Re: GUP guarantees wrt to userspace mappings redesign "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-05-02 18:30 +0200
                            Re: GUP guarantees wrt to userspace mappings redesign Oleg Nesterov <oleg@redhat.com> - 2016-05-02 19:30 +0200
                              Re: GUP guarantees wrt to userspace mappings redesign "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-05-02 20:10 +0200
                                Re: GUP guarantees wrt to userspace mappings redesign Oleg Nesterov <oleg@redhat.com> - 2016-05-02 20:50 +0200
                        Re: GUP guarantees wrt to userspace mappings redesign Andrea Arcangeli <aarcange@redhat.com> - 2016-05-02 21:00 +0200
                    Re: [BUG] vfio device assignment regression with THP ref counting  redesign Andrea Arcangeli <aarcange@redhat.com> - 2016-05-02 17:30 +0200
                      Re: [BUG] vfio device assignment regression with THP ref counting  redesign "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-05-02 18:10 +0200
                        Re: [BUG] vfio device assignment regression with THP ref counting  redesign Andrea Arcangeli <aarcange@redhat.com> - 2016-05-02 20:10 +0200
                          Re: [BUG] vfio device assignment regression with THP ref counting  redesign Alex Williamson <alex.williamson@redhat.com> - 2016-05-05 03:20 +0200
                            Re: [BUG] vfio device assignment regression with THP ref counting  redesign Andrea Arcangeli <aarcange@redhat.com> - 2016-05-05 16:40 +0200
                              Re: [BUG] vfio device assignment regression with THP ref counting  redesign Andrea Arcangeli <aarcange@redhat.com> - 2016-05-05 17:10 +0200
                              Re: [BUG] vfio device assignment regression with THP ref counting  redesign "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-05-05 17:20 +0200
                                Re: [BUG] vfio device assignment regression with THP ref counting  redesign Andrea Arcangeli <aarcange@redhat.com> - 2016-05-05 17:30 +0200
                                  Re: [BUG] vfio device assignment regression with THP ref counting  redesign "Kirill A. Shutemov" <kirill@shutemov.name> - 2016-05-06 09:30 +0200

Page 1 of 2  [1] 2  Next page →

#1390375 — [BUG] vfio device assignment regression with THP ref counting redesign

Hi,

vfio-based device assignment makes use of get_user_pages_fast() in order
to pin pages for mapping through the iommu for userspace drivers.
Until the recent redesign of THP reference counting in the v4.5 kernel,
this all worked well.  Now we're seeing cases where a sanity test
before we release our "pinned" mapping results in a different page
address than what we programmed into the iommu.  So something is
occurring which pretty much negates the pinning we're trying to do.

The test program I'm using is here:

https://github.com/awilliam/tests/blob/master/vfio-iommu-map-unmap.c

Apologies for lack of makefile, simply build with gcc -o <out> <in.c>.

To run this, enable the IOMMU on your system - enable in BIOS plus add
intel_iommu=on to the kernel commandline (only Intel x86_64 tested).

Pick a target PCI device, it doesn't matter what it is, the test only
needs a device for the purpose of creating an iommu domain, the device
is never actually touched.  In my case I use a spare NIC at 00:19.0.
libvirt tools are useful for setting this up, simply run 'virsh
nodedev-detach pci_0000_00_19_0'.  Otherwise bind the device manually
to vfio-pci using the standard new_id bind (ask, I can provide
instructions).

I also tweak THP scanning to make sure it is actively trying to
collapse pages:

echo always > /sys/kernel/mm/transparent_hugepage/defrag
echo 0 > /sys/kernel/mm/transparent_hugepage/khugepaged/scan_sleep_millisecs
echo 65536 > /sys/kernel/mm/transparent_hugepage/khugepaged/pages_to_scan

Run the test with 'vfio-iommu-map-unmap 0000:00:19.0', or your chosen
target device.

Of course to see that the mappings are moving, we need additional
sanity testing in the vfio iommu driver.  For that:

https://github.com/awilliam/linux-vfio/commit/379f324e3629349a7486018ad1cc5d4877228d1e

When we map memory for vfio, we use get_user_pages_fast() on the
process vaddr to give us a page.  page_to_pfn() then gives us the
physical memory address which we program into the iommu.  Obviously we
expect this mapping to be stable so long as we hold the page
reference.  On unmap we generally retrieve the physical memory address
from the iommu, convert it back to a page, and release our reference to
it.  The debug code above adds an additional sanity test where on unmap
we also call get_user_pages_fast() again before we're released the
mapping reference and compare whether the physical page address still
matches what we previously stored in the iommu.  On a v4.4 kernel this
works every time.  On v4.5+, we get mismatches in dmesg within a few
lines of output from the test program.

It's difficult to bisect around the THP reference counting redesign
since THP becomes disabled for much of it.  I have discovered that this
commit is a significant contributor:

1f25fe2 mm, thp: adjust conditions when we can reuse the page on WP fault

Particularly the middle chunk in huge_memory.c.  Reverting this change
alone significantly improves the problem, but does not lead to a stable
system.

I'm not an mm expert, so I'm looking for help debugging this.  As shown
above this issue is reproducible without KVM, so Andrea's previous KVM
specific fix to this code is not applicable.  It also still occurs on
kernels as recent as v4.6-rc5, so the issue hasn't been silently fixed
yet.  I'm able to reproduce this fairly quickly with the above test,
but it's not hard to imagine a test w/o any iommu dependencies which
simply does a user directed get_user_pages_fast() on a set of userspace
addresses, retains the reference, and at some point later rechecks that
a new get_user_pages_fast() results in the same page address.  It
appears that any sort of device assignment, either vfio or legacy kvm,
should be susceptible to this issue and therefore unsafe to use on v4.5+
kernels without using explicit hugepages or disabling THP.  Thanks,

Alex

[toc] | [next] | [standalone]

#1390437

On Thu, Apr 28, 2016 at 10:20:51AM -0600, Alex Williamson wrote:
> Hi,
> 
> vfio-based device assignment makes use of get_user_pages_fast() in order
> to pin pages for mapping through the iommu for userspace drivers.
> Until the recent redesign of THP reference counting in the v4.5 kernel,
> this all worked well.  Now we're seeing cases where a sanity test
> before we release our "pinned" mapping results in a different page
> address than what we programmed into the iommu.  So something is
> occurring which pretty much negates the pinning we're trying to do.
> 
> The test program I'm using is here:
> 
> https://github.com/awilliam/tests/blob/master/vfio-iommu-map-unmap.c
> 
> Apologies for lack of makefile, simply build with gcc -o <out> <in.c>.
> 
> To run this, enable the IOMMU on your system - enable in BIOS plus add
> intel_iommu=on to the kernel commandline (only Intel x86_64 tested).
> 
> Pick a target PCI device, it doesn't matter what it is, the test only
> needs a device for the purpose of creating an iommu domain, the device
> is never actually touched.  In my case I use a spare NIC at 00:19.0.
> libvirt tools are useful for setting this up, simply run 'virsh
> nodedev-detach pci_0000_00_19_0'.  Otherwise bind the device manually
> to vfio-pci using the standard new_id bind (ask, I can provide
> instructions).
> 
> I also tweak THP scanning to make sure it is actively trying to
> collapse pages:
> 
> echo always > /sys/kernel/mm/transparent_hugepage/defrag
> echo 0 > /sys/kernel/mm/transparent_hugepage/khugepaged/scan_sleep_millisecs
> echo 65536 > /sys/kernel/mm/transparent_hugepage/khugepaged/pages_to_scan
> 
> Run the test with 'vfio-iommu-map-unmap 0000:00:19.0', or your chosen
> target device.
> 
> Of course to see that the mappings are moving, we need additional
> sanity testing in the vfio iommu driver.  For that:
> 
> https://github.com/awilliam/linux-vfio/commit/379f324e3629349a7486018ad1cc5d4877228d1e
> 
> When we map memory for vfio, we use get_user_pages_fast() on the
> process vaddr to give us a page.  page_to_pfn() then gives us the
> physical memory address which we program into the iommu.  Obviously we
> expect this mapping to be stable so long as we hold the page
> reference.  On unmap we generally retrieve the physical memory address
> from the iommu, convert it back to a page, and release our reference to
> it.  The debug code above adds an additional sanity test where on unmap
> we also call get_user_pages_fast() again before we're released the
> mapping reference and compare whether the physical page address still
> matches what we previously stored in the iommu.  On a v4.4 kernel this
> works every time.  On v4.5+, we get mismatches in dmesg within a few
> lines of output from the test program.
> 
> It's difficult to bisect around the THP reference counting redesign
> since THP becomes disabled for much of it.  I have discovered that this
> commit is a significant contributor:
> 
> 1f25fe2 mm, thp: adjust conditions when we can reuse the page on WP fault
> 
> Particularly the middle chunk in huge_memory.c.  Reverting this change
> alone significantly improves the problem, but does not lead to a stable
> system.
> 
> I'm not an mm expert, so I'm looking for help debugging this.  As shown
> above this issue is reproducible without KVM, so Andrea's previous KVM
> specific fix to this code is not applicable.  It also still occurs on
> kernels as recent as v4.6-rc5, so the issue hasn't been silently fixed
> yet.  I'm able to reproduce this fairly quickly with the above test,
> but it's not hard to imagine a test w/o any iommu dependencies which
> simply does a user directed get_user_pages_fast() on a set of userspace
> addresses, retains the reference, and at some point later rechecks that
> a new get_user_pages_fast() results in the same page address.  It
> appears that any sort of device assignment, either vfio or legacy kvm,
> should be susceptible to this issue and therefore unsafe to use on v4.5+
> kernels without using explicit hugepages or disabling THP.  Thanks,

I'm not able to reproduce it so far. How long does it usually take?

How much memory your system has? Could you share your kernel config?

I've modified your instrumentation slightly to provide more info.
Could you try this:

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 75b24e93cedb..434954841d19 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -59,6 +59,7 @@ struct vfio_iommu {
 	struct rb_root		dma_list;
 	bool			v2;
 	bool			nesting;
+	bool			dying;
 };
 
 struct vfio_domain {
@@ -336,8 +337,10 @@ static long vfio_unpin_pages(unsigned long pfn, long npage,
 static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma)
 {
 	dma_addr_t iova = dma->iova, end = dma->iova + dma->size;
+	unsigned long vaddr = dma->vaddr;
 	struct vfio_domain *domain, *d;
 	long unlocked = 0;
+	struct page *page;
 
 	if (!dma->size)
 		return;
@@ -363,9 +366,20 @@ static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma)
 		phys = iommu_iova_to_phys(domain->domain, iova);
 		if (WARN_ON(!phys)) {
 			iova += PAGE_SIZE;
+			vaddr += PAGE_SIZE;
 			continue;
 		}
 
+		if (!iommu->dying && get_user_pages_fast(vaddr, 1, !!(dma->prot & IOMMU_WRITE), &page) == 1) {
+			if (phys >> PAGE_SHIFT != page_to_pfn(page)) {
+				dump_page(page, NULL);
+				if (PageTail(page))
+					dump_page(compound_head(page), NULL);
+				dump_page(pfn_to_page(phys >> PAGE_SHIFT), "1");
+			}
+			put_page(page);
+		}
+
 		/*
 		 * To optimize for fewer iommu_unmap() calls, each of which
 		 * may require hardware cache flushing, try to find the
@@ -374,6 +388,17 @@ static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma)
 		for (len = PAGE_SIZE;
 		     !domain->fgsp && iova + len < end; len += PAGE_SIZE) {
 			next = iommu_iova_to_phys(domain->domain, iova + len);
+
+			if (!iommu->dying && get_user_pages_fast(vaddr + len, 1, !!(dma->prot & IOMMU_WRITE), &page) == 1) {
+				if (next >> PAGE_SHIFT != page_to_pfn(page)) {
+					dump_page(page, NULL);
+					if (PageTail(page))
+						dump_page(compound_head(page), NULL);
+					dump_page(pfn_to_page(next >> PAGE_SHIFT), "2");
+				}
+				put_page(page);
+			}
+
 			if (next != phys + len)
 				break;
 		}
@@ -386,6 +411,7 @@ static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma)
 					     unmapped >> PAGE_SHIFT,
 					     dma->prot, false);
 		iova += unmapped;
+		vaddr += unmapped;
 
 		cond_resched();
 	}
@@ -855,6 +881,8 @@ static void vfio_iommu_unmap_unpin_all(struct vfio_iommu *iommu)
 {
 	struct rb_node *node;
 
+	iommu->dying = true;
+
 	while ((node = rb_first(&iommu->dma_list)))
 		vfio_remove_dma(iommu, rb_entry(node, struct vfio_dma, node));
 }
-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1390464

On Thu, 28 Apr 2016 21:17:26 +0300
"Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> On Thu, Apr 28, 2016 at 10:20:51AM -0600, Alex Williamson wrote:
> > Hi,
> > 
> > vfio-based device assignment makes use of get_user_pages_fast() in order
> > to pin pages for mapping through the iommu for userspace drivers.
> > Until the recent redesign of THP reference counting in the v4.5 kernel,
> > this all worked well.  Now we're seeing cases where a sanity test
> > before we release our "pinned" mapping results in a different page
> > address than what we programmed into the iommu.  So something is
> > occurring which pretty much negates the pinning we're trying to do.
> > 
> > The test program I'm using is here:
> > 
> > https://github.com/awilliam/tests/blob/master/vfio-iommu-map-unmap.c
> > 
> > Apologies for lack of makefile, simply build with gcc -o <out> <in.c>.
> > 
> > To run this, enable the IOMMU on your system - enable in BIOS plus add
> > intel_iommu=on to the kernel commandline (only Intel x86_64 tested).
> > 
> > Pick a target PCI device, it doesn't matter what it is, the test only
> > needs a device for the purpose of creating an iommu domain, the device
> > is never actually touched.  In my case I use a spare NIC at 00:19.0.
> > libvirt tools are useful for setting this up, simply run 'virsh
> > nodedev-detach pci_0000_00_19_0'.  Otherwise bind the device manually
> > to vfio-pci using the standard new_id bind (ask, I can provide
> > instructions).
> > 
> > I also tweak THP scanning to make sure it is actively trying to
> > collapse pages:
> > 
> > echo always > /sys/kernel/mm/transparent_hugepage/defrag
> > echo 0 > /sys/kernel/mm/transparent_hugepage/khugepaged/scan_sleep_millisecs
> > echo 65536 > /sys/kernel/mm/transparent_hugepage/khugepaged/pages_to_scan
> > 
> > Run the test with 'vfio-iommu-map-unmap 0000:00:19.0', or your chosen
> > target device.
> > 
> > Of course to see that the mappings are moving, we need additional
> > sanity testing in the vfio iommu driver.  For that:
> > 
> > https://github.com/awilliam/linux-vfio/commit/379f324e3629349a7486018ad1cc5d4877228d1e
> > 
> > When we map memory for vfio, we use get_user_pages_fast() on the
> > process vaddr to give us a page.  page_to_pfn() then gives us the
> > physical memory address which we program into the iommu.  Obviously we
> > expect this mapping to be stable so long as we hold the page
> > reference.  On unmap we generally retrieve the physical memory address
> > from the iommu, convert it back to a page, and release our reference to
> > it.  The debug code above adds an additional sanity test where on unmap
> > we also call get_user_pages_fast() again before we're released the
> > mapping reference and compare whether the physical page address still
> > matches what we previously stored in the iommu.  On a v4.4 kernel this
> > works every time.  On v4.5+, we get mismatches in dmesg within a few
> > lines of output from the test program.
> > 
> > It's difficult to bisect around the THP reference counting redesign
> > since THP becomes disabled for much of it.  I have discovered that this
> > commit is a significant contributor:
> > 
> > 1f25fe2 mm, thp: adjust conditions when we can reuse the page on WP fault
> > 
> > Particularly the middle chunk in huge_memory.c.  Reverting this change
> > alone significantly improves the problem, but does not lead to a stable
> > system.
> > 
> > I'm not an mm expert, so I'm looking for help debugging this.  As shown
> > above this issue is reproducible without KVM, so Andrea's previous KVM
> > specific fix to this code is not applicable.  It also still occurs on
> > kernels as recent as v4.6-rc5, so the issue hasn't been silently fixed
> > yet.  I'm able to reproduce this fairly quickly with the above test,
> > but it's not hard to imagine a test w/o any iommu dependencies which
> > simply does a user directed get_user_pages_fast() on a set of userspace
> > addresses, retains the reference, and at some point later rechecks that
> > a new get_user_pages_fast() results in the same page address.  It
> > appears that any sort of device assignment, either vfio or legacy kvm,
> > should be susceptible to this issue and therefore unsafe to use on v4.5+
> > kernels without using explicit hugepages or disabling THP.  Thanks,  
> 
> I'm not able to reproduce it so far. How long does it usually take?

Generally within the first line of output from the test program.
 
> How much memory your system has? Could you share your kernel config?

24G, dual-socket Ivy Brdige EP.

Config:
https://paste.fedoraproject.org/360803/14618689/
 
> I've modified your instrumentation slightly to provide more info.
> Could you try this:

Thanks!  Results in:

[   83.429809] page:ffffea0010e57fc0 count:0 mapcount:1 mapping:dead000000000400 index:0x1 compound_mapcount: 1
[   83.439696] flags: 0x2fffff80000000()
[   83.443408] page:ffffea0010e50000 count:3 mapcount:1 mapping:ffff88044c0fa8a1 index:0x7f8ae1400 compound_mapcount: 1
[   83.454001] flags: 0x2fffff80044048(uptodate|active|head|swapbacked)
[   83.460456] page:ffffea0018a67fc0 count:0 mapcount:0 mapping:dead000000000400 index:0x0 compound_mapcount: 0
[   83.470298] flags: 0x6fffff80000000()
[   83.473973] page dumped because: 1
[   83.477412] page:ffffea0010e57f80 count:0 mapcount:1 mapping:dead000000000400 index:0x1 compound_mapcount: 1
[   83.487283] flags: 0x2fffff80000000()
[   83.490969] page:ffffea0010e50000 count:3 mapcount:1 mapping:ffff88044c0fa8a1 index:0x7f8ae1400 compound_mapcount: 1
[   83.501502] flags: 0x2fffff8004404c(referenced|uptodate|active|head|swapbacked)
[   83.508915] page:ffffea0018a67f80 count:0 mapcount:0 mapping:dead000000000400 index:0x0 compound_mapcount: 0
[   83.518758] flags: 0x6fffff80000000()
[   83.522443] page dumped because: 1
[   83.525874] page:ffffea0010e57f40 count:0 mapcount:1 mapping:dead000000000400 index:0x1 compound_mapcount: 1
[   83.535737] flags: 0x2fffff80000000()
[   83.539434] page:ffffea0010e50000 count:3 mapcount:1 mapping:ffff88044c0fa8a1 index:0x7f8ae1400 compound_mapcount: 1
[   83.549979] flags: 0x2fffff8004404c(referenced|uptodate|active|head|swapbacked)
[   83.557412] page:ffffea0018a67f40 count:0 mapcount:0 mapping:dead000000000400 index:0x0 compound_mapcount: 0
[   83.567260] flags: 0x6fffff80000000()
[   83.570943] page dumped because: 1
[   83.574366] page:ffffea0010e57f00 count:0 mapcount:1 mapping:dead000000000400 index:0x1 compound_mapcount: 1
[   83.584211] flags: 0x2fffff80000000()
[   83.587878] page:ffffea0010e50000 count:3 mapcount:1 mapping:ffff88044c0fa8a1 index:0x7f8ae1400 compound_mapcount: 1
[   83.598413] flags: 0x2fffff8004404c(referenced|uptodate|active|head|swapbacked)
[   83.605862] page:ffffea0018a67f00 count:0 mapcount:0 mapping:dead000000000400 index:0x0 compound_mapcount: 0
[   83.615722] flags: 0x6fffff80000000()
[   83.619399] page dumped because: 1
[   83.622835] page:ffffea0010e57ec0 count:0 mapcount:1 mapping:dead000000000400 index:0x1 compound_mapcount: 1
[   83.632673] flags: 0x2fffff80000000()
[   83.636363] page:ffffea0010e50000 count:3 mapcount:1 mapping:ffff88044c0fa8a1 index:0x7f8ae1400 compound_mapcount: 1
[   83.646893] flags: 0x2fffff8004404c(referenced|uptodate|active|head|swapbacked)
[   83.654302] page:ffffea0018a67ec0 count:0 mapcount:0 mapping:dead000000000400 index:0x0 compound_mapcount: 0
[   83.664150] flags: 0x6fffff80000000()
[   83.667840] page dumped because: 1
[   83.671255] page:ffffea0010e57e80 count:0 mapcount:1 mapping:dead000000000400 index:0x1 compound_mapcount: 1
[   83.681108] flags: 0x2fffff80000000()
[   83.684783] page:ffffea0010e50000 count:3 mapcount:1 mapping:ffff88044c0fa8a1 index:0x7f8ae1400 compound_mapcount: 1
[   83.695335] flags: 0x2fffff8004404c(referenced|uptodate|active|head|swapbacked)
[   83.702773] page:ffffea0018a67e80 count:0 mapcount:0 mapping:dead000000000400 index:0x0 compound_mapcount: 0
[   83.712640] flags: 0x6fffff80000000()
[   83.716335] page dumped because: 1
[   83.719746] page:ffffea0010e57e40 count:0 mapcount:1 mapping:dead000000000400 index:0x1 compound_mapcount: 1
[   83.729591] flags: 0x2fffff80000000()
[   83.733279] page:ffffea0010e50000 count:3 mapcount:1 mapping:ffff88044c0fa8a1 index:0x7f8ae1400 compound_mapcount: 1
[   83.743843] flags: 0x2fffff8004404c(referenced|uptodate|active|head|swapbacked)
[   83.751268] page:ffffea0018a67e40 count:0 mapcount:0 mapping:dead000000000400 index:0x0 compound_mapcount: 0
[   83.761108] flags: 0x6fffff80000000()
[   83.764784] page dumped because: 1
[   83.768206] page:ffffea0010e57e00 count:0 mapcount:1 mapping:dead000000000400 index:0x1 compound_mapcount: 1
[   83.778076] flags: 0x2fffff80000000()
[   83.781754] page:ffffea0010e50000 count:3 mapcount:1 mapping:ffff88044c0fa8a1 index:0x7f8ae1400 compound_mapcount: 1
[   83.792283] flags: 0x2fffff8004404c(referenced|uptodate|active|head|swapbacked)
[   83.799712] page:ffffea0018a67e00 count:0 mapcount:0 mapping:dead000000000400 index:0x0 compound_mapcount: 0
[   83.809559] flags: 0x6fffff80000000()
[   83.813257] page dumped because: 1
[   83.816722] page:ffffea0010e57dc0 count:0 mapcount:1 mapping:dead000000000400 index:0x1 compound_mapcount: 1
[   83.826605] flags: 0x2fffff80000000()
[   83.830285] page:ffffea0010e50000 count:3 mapcount:1 mapping:ffff88044c0fa8a1 index:0x7f8ae1400 compound_mapcount: 1
[   83.840877] flags: 0x2fffff8004404c(referenced|uptodate|active|head|swapbacked)
[   83.848321] page:ffffea0018a67dc0 count:0 mapcount:0 mapping:dead000000000400 index:0x0 compound_mapcount: 0
[   83.858214] flags: 0x6fffff80000000()
[   83.861899] page dumped because: 1
[   83.865355] page:ffffea0010e57d80 count:0 mapcount:1 mapping:dead000000000400 index:0x1 compound_mapcount: 1
[   83.875246] flags: 0x2fffff80000000()
[   83.878930] page:ffffea0010e50000 count:3 mapcount:1 mapping:ffff88044c0fa8a1 index:0x7f8ae1400 compound_mapcount: 1
[   83.889525] flags: 0x2fffff8004404c(referenced|uptodate|active|head|swapbacked)
[   83.896970] page:ffffea0018a67d80 count:0 mapcount:0 mapping:dead000000000400 index:0x0 compound_mapcount: 0
[   83.906883] flags: 0x6fffff80000000()
[   83.910563] page dumped because: 1
[   83.914018] page:ffffea0010e57d40 count:0 mapcount:1 mapping:dead000000000400 index:0x1 compound_mapcount: 1
[   83.923862] flags: 0x2fffff80000000()
[   83.927540] page:ffffea0010e50000 count:3 mapcount:1 mapping:ffff88044c0fa8a1 index:0x7f8ae1400 compound_mapcount: 1
[   83.938079] flags: 0x2fffff8004404c(referenced|uptodate|active|head|swapbacked)
[   83.945493] page:ffffea0018a67d40 count:0 mapcount:0 mapping:dead000000000400 index:0x0 compound_mapcount: 0
[   83.955341] flags: 0x6fffff80000000()
[   83.959022] page dumped because: 1
[   83.962446] page:ffffea0010e57d00 count:0 mapcount:1 mapping:dead000000000400 index:0x1 compound_mapcount: 1
[   83.972296] flags: 0x2fffff80000000()
[   83.975980] page:ffffea0010e50000 count:3 mapcount:1 mapping:ffff88044c0fa8a1 index:0x7f8ae1400 compound_mapcount: 1
[   83.986516] flags: 0x2fffff8004404c(referenced|uptodate|active|head|swapbacked)
[   83.993932] page:ffffea0018a67d00 count:0 mapcount:0 mapping:dead000000000400 index:0x0 compound_mapcount: 0
[   84.003778] flags: 0x6fffff80000000()
[   84.007456] page dumped because: 1
...

As you can see by the kernel timestamp, this happened almost
immediately for me.  Thanks for taking a look at this,

Alex

[toc] | [prev] | [next] | [standalone]

#1390643

Hello Alex and Kirill,

On Thu, Apr 28, 2016 at 12:58:08PM -0600, Alex Williamson wrote:
> > > specific fix to this code is not applicable.  It also still occurs on
> > > kernels as recent as v4.6-rc5, so the issue hasn't been silently fixed
> > > yet.  I'm able to reproduce this fairly quickly with the above test,
> > > but it's not hard to imagine a test w/o any iommu dependencies which
> > > simply does a user directed get_user_pages_fast() on a set of userspace
> > > addresses, retains the reference, and at some point later rechecks that
> > > a new get_user_pages_fast() results in the same page address.  It

Can you try to "git revert 1f25fe20a76af0d960172fb104d4b13697cafa84"
and then apply the below patch on top of the revert?

Totally untested... if I missed something and it isn't correct, I hope
this brings us in the right direction faster at least.

Overall the problem I think is that we need to restore full accuracy
and we can't deal with false positive COWs (which aren't entirely
cheap either... reading 512 cachelines should be much faster than
copying 2MB and using 4MB of CPU cache). 32k vs 4MB. The problem of
course is when we really need a COW, we'll waste an additional 32k,
but then it doesn't matter that much as we'd be forced to load 4MB of
cache anyway in such case. There's room for optimizations but even the
simple below patch would be ok for now.

From 09e3d1ff10b49fb9c3ab77f0b96a862848e30067 Mon Sep 17 00:00:00 2001
From: Andrea Arcangeli <aarcange@redhat.com>
Date: Fri, 29 Apr 2016 01:05:06 +0200
Subject: [PATCH 1/1] mm: thp: calculate page_mapcount() correctly for THP
 pages

This allows to revert commit 1f25fe20a76af0d960172fb104d4b13697cafa84
and it provides fully accuracy with wrprotect faults so page pinning
will stop causing false positive copy-on-writes.

Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
 mm/util.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/util.c b/mm/util.c
index 6cc81e7..a0b9f63 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -383,9 +383,10 @@ struct address_space *page_mapping(struct page *page)
 /* Slow path of page_mapcount() for compound pages */
 int __page_mapcount(struct page *page)
 {
-	int ret;
+	int ret = 0, i;
 
-	ret = atomic_read(&page->_mapcount) + 1;
+	for (i = 0; i < HPAGE_PMD_NR; i++)
+		ret = max(ret, atomic_read(&page->_mapcount) + 1);
 	page = compound_head(page);
 	ret += atomic_read(compound_mapcount_ptr(page)) + 1;
 	if (PageDoubleMap(page))

[toc] | [prev] | [next] | [standalone]

#1390677

On Fri, 29 Apr 2016 01:21:27 +0200
Andrea Arcangeli <aarcange@redhat.com> wrote:

> Hello Alex and Kirill,
> 
> On Thu, Apr 28, 2016 at 12:58:08PM -0600, Alex Williamson wrote:
> > > > specific fix to this code is not applicable.  It also still occurs on
> > > > kernels as recent as v4.6-rc5, so the issue hasn't been silently fixed
> > > > yet.  I'm able to reproduce this fairly quickly with the above test,
> > > > but it's not hard to imagine a test w/o any iommu dependencies which
> > > > simply does a user directed get_user_pages_fast() on a set of userspace
> > > > addresses, retains the reference, and at some point later rechecks that
> > > > a new get_user_pages_fast() results in the same page address.  It  
> 
> Can you try to "git revert 1f25fe20a76af0d960172fb104d4b13697cafa84"
> and then apply the below patch on top of the revert?

Looking good so far!  I haven't seen any errors yet with this
combination of v4.5, 1f25fe20a reverted, and your patch applied on
top.  I'll keep testing since reverting 1f25fe20a alone already made
the bug much more elusive.  Thanks Andrea!

Alex

> Totally untested... if I missed something and it isn't correct, I hope
> this brings us in the right direction faster at least.
> 
> Overall the problem I think is that we need to restore full accuracy
> and we can't deal with false positive COWs (which aren't entirely
> cheap either... reading 512 cachelines should be much faster than
> copying 2MB and using 4MB of CPU cache). 32k vs 4MB. The problem of
> course is when we really need a COW, we'll waste an additional 32k,
> but then it doesn't matter that much as we'd be forced to load 4MB of
> cache anyway in such case. There's room for optimizations but even the
> simple below patch would be ok for now.
> 
> From 09e3d1ff10b49fb9c3ab77f0b96a862848e30067 Mon Sep 17 00:00:00 2001
> From: Andrea Arcangeli <aarcange@redhat.com>
> Date: Fri, 29 Apr 2016 01:05:06 +0200
> Subject: [PATCH 1/1] mm: thp: calculate page_mapcount() correctly for THP
>  pages
> 
> This allows to revert commit 1f25fe20a76af0d960172fb104d4b13697cafa84
> and it provides fully accuracy with wrprotect faults so page pinning
> will stop causing false positive copy-on-writes.
> 
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> ---
>  mm/util.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/util.c b/mm/util.c
> index 6cc81e7..a0b9f63 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -383,9 +383,10 @@ struct address_space *page_mapping(struct page *page)
>  /* Slow path of page_mapcount() for compound pages */
>  int __page_mapcount(struct page *page)
>  {
> -	int ret;
> +	int ret = 0, i;
>  
> -	ret = atomic_read(&page->_mapcount) + 1;
> +	for (i = 0; i < HPAGE_PMD_NR; i++)
> +		ret = max(ret, atomic_read(&page->_mapcount) + 1);
>  	page = compound_head(page);
>  	ret += atomic_read(compound_mapcount_ptr(page)) + 1;
>  	if (PageDoubleMap(page))

[toc] | [prev] | [next] | [standalone]

#1390682

On Fri, Apr 29, 2016 at 01:21:27AM +0200, Andrea Arcangeli wrote:
> Hello Alex and Kirill,
> 
> On Thu, Apr 28, 2016 at 12:58:08PM -0600, Alex Williamson wrote:
> > > > specific fix to this code is not applicable.  It also still occurs on
> > > > kernels as recent as v4.6-rc5, so the issue hasn't been silently fixed
> > > > yet.  I'm able to reproduce this fairly quickly with the above test,
> > > > but it's not hard to imagine a test w/o any iommu dependencies which
> > > > simply does a user directed get_user_pages_fast() on a set of userspace
> > > > addresses, retains the reference, and at some point later rechecks that
> > > > a new get_user_pages_fast() results in the same page address.  It
> 
> Can you try to "git revert 1f25fe20a76af0d960172fb104d4b13697cafa84"
> and then apply the below patch on top of the revert?
> 
> Totally untested... if I missed something and it isn't correct, I hope
> this brings us in the right direction faster at least.
> 
> Overall the problem I think is that we need to restore full accuracy
> and we can't deal with false positive COWs (which aren't entirely
> cheap either... reading 512 cachelines should be much faster than
> copying 2MB and using 4MB of CPU cache). 32k vs 4MB. The problem of
> course is when we really need a COW, we'll waste an additional 32k,
> but then it doesn't matter that much as we'd be forced to load 4MB of
> cache anyway in such case. There's room for optimizations but even the
> simple below patch would be ok for now.
> 
> From 09e3d1ff10b49fb9c3ab77f0b96a862848e30067 Mon Sep 17 00:00:00 2001
> From: Andrea Arcangeli <aarcange@redhat.com>
> Date: Fri, 29 Apr 2016 01:05:06 +0200
> Subject: [PATCH 1/1] mm: thp: calculate page_mapcount() correctly for THP
>  pages
> 
> This allows to revert commit 1f25fe20a76af0d960172fb104d4b13697cafa84
> and it provides fully accuracy with wrprotect faults so page pinning
> will stop causing false positive copy-on-writes.
> 
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> ---
>  mm/util.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/util.c b/mm/util.c
> index 6cc81e7..a0b9f63 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -383,9 +383,10 @@ struct address_space *page_mapping(struct page *page)
>  /* Slow path of page_mapcount() for compound pages */
>  int __page_mapcount(struct page *page)
>  {
> -	int ret;
> +	int ret = 0, i;
>  
> -	ret = atomic_read(&page->_mapcount) + 1;
> +	for (i = 0; i < HPAGE_PMD_NR; i++)
> +		ret = max(ret, atomic_read(&page->_mapcount) + 1);
>  	page = compound_head(page);
>  	ret += atomic_read(compound_mapcount_ptr(page)) + 1;
>  	if (PageDoubleMap(page))

You are right about the cause. I spend some time on wrong path: I was only
able to trigger the bug with numa balancing enabled, so I assumed
something is wrong in that code...

I would like to preserve current page_mapcount() behaviouts.
I think this fix is better:

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 86f9f8b82f8e..163c10f48e1b 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1298,15 +1298,9 @@ int do_huge_pmd_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,
        VM_BUG_ON_PAGE(!PageCompound(page) || !PageHead(page), page);
        /*
         * We can only reuse the page if nobody else maps the huge page or it's
-        * part. We can do it by checking page_mapcount() on each sub-page, but
-        * it's expensive.
-        * The cheaper way is to check page_count() to be equal 1: every
-        * mapcount takes page reference reference, so this way we can
-        * guarantee, that the PMD is the only mapping.
-        * This can give false negative if somebody pinned the page, but that's
-        * fine.
+        * part.
         */
-       if (page_mapcount(page) == 1 && page_count(page) == 1) {
+       if (total_mapcount(page) == 1) {
                pmd_t entry;
                entry = pmd_mkyoung(orig_pmd);
                entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1390707

On Fri, 29 Apr 2016 03:51:06 +0300
"Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> On Fri, Apr 29, 2016 at 01:21:27AM +0200, Andrea Arcangeli wrote:
> > Hello Alex and Kirill,
> > 
> > On Thu, Apr 28, 2016 at 12:58:08PM -0600, Alex Williamson wrote:  
> > > > > specific fix to this code is not applicable.  It also still occurs on
> > > > > kernels as recent as v4.6-rc5, so the issue hasn't been silently fixed
> > > > > yet.  I'm able to reproduce this fairly quickly with the above test,
> > > > > but it's not hard to imagine a test w/o any iommu dependencies which
> > > > > simply does a user directed get_user_pages_fast() on a set of userspace
> > > > > addresses, retains the reference, and at some point later rechecks that
> > > > > a new get_user_pages_fast() results in the same page address.  It  
> > 
> > Can you try to "git revert 1f25fe20a76af0d960172fb104d4b13697cafa84"
> > and then apply the below patch on top of the revert?
> > 
> > Totally untested... if I missed something and it isn't correct, I hope
> > this brings us in the right direction faster at least.
> > 
> > Overall the problem I think is that we need to restore full accuracy
> > and we can't deal with false positive COWs (which aren't entirely
> > cheap either... reading 512 cachelines should be much faster than
> > copying 2MB and using 4MB of CPU cache). 32k vs 4MB. The problem of
> > course is when we really need a COW, we'll waste an additional 32k,
> > but then it doesn't matter that much as we'd be forced to load 4MB of
> > cache anyway in such case. There's room for optimizations but even the
> > simple below patch would be ok for now.
> > 
> > From 09e3d1ff10b49fb9c3ab77f0b96a862848e30067 Mon Sep 17 00:00:00 2001
> > From: Andrea Arcangeli <aarcange@redhat.com>
> > Date: Fri, 29 Apr 2016 01:05:06 +0200
> > Subject: [PATCH 1/1] mm: thp: calculate page_mapcount() correctly for THP
> >  pages
> > 
> > This allows to revert commit 1f25fe20a76af0d960172fb104d4b13697cafa84
> > and it provides fully accuracy with wrprotect faults so page pinning
> > will stop causing false positive copy-on-writes.
> > 
> > Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> > ---
> >  mm/util.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/mm/util.c b/mm/util.c
> > index 6cc81e7..a0b9f63 100644
> > --- a/mm/util.c
> > +++ b/mm/util.c
> > @@ -383,9 +383,10 @@ struct address_space *page_mapping(struct page *page)
> >  /* Slow path of page_mapcount() for compound pages */
> >  int __page_mapcount(struct page *page)
> >  {
> > -	int ret;
> > +	int ret = 0, i;
> >  
> > -	ret = atomic_read(&page->_mapcount) + 1;
> > +	for (i = 0; i < HPAGE_PMD_NR; i++)
> > +		ret = max(ret, atomic_read(&page->_mapcount) + 1);
> >  	page = compound_head(page);
> >  	ret += atomic_read(compound_mapcount_ptr(page)) + 1;
> >  	if (PageDoubleMap(page))  
> 
> You are right about the cause. I spend some time on wrong path: I was only
> able to trigger the bug with numa balancing enabled, so I assumed
> something is wrong in that code...
> 
> I would like to preserve current page_mapcount() behaviouts.
> I think this fix is better:

This also seems to work in my testing, but assuming all else being
equal, there is a performance difference between the two for this test
case in favor of Andrea's solution.  Modifying the test to exit after
the first set of iterations, my system takes on average 107s to complete
with the solution below or 103.5s with the other approach.  Please note
that I have every mm debugging option I could find enabled and THP
scanning full speed on the system, so I don't know how this would play
out in a more tuned configuration.

The only reason I noticed is that I added a side test to sleep a random
number of seconds and kill the test program because sometimes killing
the test triggers errors.  I didn't see any errors with either of these
solutions, but suspected the first solution was completing more
iterations for similar intervals.  Modifying the test to exit seems to
prove that true.

I can't speak to which is the more architecturally correct solution,
but there may be a measurable performance difference to consider.
Thanks,

Alex

> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 86f9f8b82f8e..163c10f48e1b 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1298,15 +1298,9 @@ int do_huge_pmd_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,
>         VM_BUG_ON_PAGE(!PageCompound(page) || !PageHead(page), page);
>         /*
>          * We can only reuse the page if nobody else maps the huge page or it's
> -        * part. We can do it by checking page_mapcount() on each sub-page, but
> -        * it's expensive.
> -        * The cheaper way is to check page_count() to be equal 1: every
> -        * mapcount takes page reference reference, so this way we can
> -        * guarantee, that the PMD is the only mapping.
> -        * This can give false negative if somebody pinned the page, but that's
> -        * fine.
> +        * part.
>          */
> -       if (page_mapcount(page) == 1 && page_count(page) == 1) {
> +       if (total_mapcount(page) == 1) {
>                 pmd_t entry;
>                 entry = pmd_mkyoung(orig_pmd);
>                 entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);

[toc] | [prev] | [next] | [standalone]

#1390811

On Thu, Apr 28, 2016 at 08:45:42PM -0600, Alex Williamson wrote:
> On Fri, 29 Apr 2016 03:51:06 +0300
> "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> 
> > On Fri, Apr 29, 2016 at 01:21:27AM +0200, Andrea Arcangeli wrote:
> > > Hello Alex and Kirill,
> > > 
> > > On Thu, Apr 28, 2016 at 12:58:08PM -0600, Alex Williamson wrote:  
> > > > > > specific fix to this code is not applicable.  It also still occurs on
> > > > > > kernels as recent as v4.6-rc5, so the issue hasn't been silently fixed
> > > > > > yet.  I'm able to reproduce this fairly quickly with the above test,
> > > > > > but it's not hard to imagine a test w/o any iommu dependencies which
> > > > > > simply does a user directed get_user_pages_fast() on a set of userspace
> > > > > > addresses, retains the reference, and at some point later rechecks that
> > > > > > a new get_user_pages_fast() results in the same page address.  It  
> > > 
> > > Can you try to "git revert 1f25fe20a76af0d960172fb104d4b13697cafa84"
> > > and then apply the below patch on top of the revert?
> > > 
> > > Totally untested... if I missed something and it isn't correct, I hope
> > > this brings us in the right direction faster at least.
> > > 
> > > Overall the problem I think is that we need to restore full accuracy
> > > and we can't deal with false positive COWs (which aren't entirely
> > > cheap either... reading 512 cachelines should be much faster than
> > > copying 2MB and using 4MB of CPU cache). 32k vs 4MB. The problem of
> > > course is when we really need a COW, we'll waste an additional 32k,
> > > but then it doesn't matter that much as we'd be forced to load 4MB of
> > > cache anyway in such case. There's room for optimizations but even the
> > > simple below patch would be ok for now.
> > > 
> > > From 09e3d1ff10b49fb9c3ab77f0b96a862848e30067 Mon Sep 17 00:00:00 2001
> > > From: Andrea Arcangeli <aarcange@redhat.com>
> > > Date: Fri, 29 Apr 2016 01:05:06 +0200
> > > Subject: [PATCH 1/1] mm: thp: calculate page_mapcount() correctly for THP
> > >  pages
> > > 
> > > This allows to revert commit 1f25fe20a76af0d960172fb104d4b13697cafa84
> > > and it provides fully accuracy with wrprotect faults so page pinning
> > > will stop causing false positive copy-on-writes.
> > > 
> > > Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> > > ---
> > >  mm/util.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/mm/util.c b/mm/util.c
> > > index 6cc81e7..a0b9f63 100644
> > > --- a/mm/util.c
> > > +++ b/mm/util.c
> > > @@ -383,9 +383,10 @@ struct address_space *page_mapping(struct page *page)
> > >  /* Slow path of page_mapcount() for compound pages */
> > >  int __page_mapcount(struct page *page)
> > >  {
> > > -	int ret;
> > > +	int ret = 0, i;
> > >  
> > > -	ret = atomic_read(&page->_mapcount) + 1;
> > > +	for (i = 0; i < HPAGE_PMD_NR; i++)
> > > +		ret = max(ret, atomic_read(&page->_mapcount) + 1);
> > >  	page = compound_head(page);
> > >  	ret += atomic_read(compound_mapcount_ptr(page)) + 1;
> > >  	if (PageDoubleMap(page))  
> > 
> > You are right about the cause. I spend some time on wrong path: I was only
> > able to trigger the bug with numa balancing enabled, so I assumed
> > something is wrong in that code...
> > 
> > I would like to preserve current page_mapcount() behaviouts.
> > I think this fix is better:
> 
> This also seems to work in my testing, but assuming all else being
> equal, there is a performance difference between the two for this test
> case in favor of Andrea's solution.  Modifying the test to exit after
> the first set of iterations, my system takes on average 107s to complete
> with the solution below or 103.5s with the other approach.  Please note
> that I have every mm debugging option I could find enabled and THP
> scanning full speed on the system, so I don't know how this would play
> out in a more tuned configuration.
> 
> The only reason I noticed is that I added a side test to sleep a random
> number of seconds and kill the test program because sometimes killing
> the test triggers errors.  I didn't see any errors with either of these
> solutions, but suspected the first solution was completing more
> iterations for similar intervals.  Modifying the test to exit seems to
> prove that true.
> 
> I can't speak to which is the more architecturally correct solution,
> but there may be a measurable performance difference to consider.

Hm. I just woke up and haven't got any coffee yet, but I don't why my
approach would be worse for performance. Both have the same algorithmic
complexity.

> Thanks,
> 
> Alex
> 
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 86f9f8b82f8e..163c10f48e1b 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -1298,15 +1298,9 @@ int do_huge_pmd_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,
> >         VM_BUG_ON_PAGE(!PageCompound(page) || !PageHead(page), page);
> >         /*
> >          * We can only reuse the page if nobody else maps the huge page or it's
> > -        * part. We can do it by checking page_mapcount() on each sub-page, but
> > -        * it's expensive.
> > -        * The cheaper way is to check page_count() to be equal 1: every
> > -        * mapcount takes page reference reference, so this way we can
> > -        * guarantee, that the PMD is the only mapping.
> > -        * This can give false negative if somebody pinned the page, but that's
> > -        * fine.
> > +        * part.
> >          */
> > -       if (page_mapcount(page) == 1 && page_count(page) == 1) {
> > +       if (total_mapcount(page) == 1) {
> >                 pmd_t entry;
> >                 entry = pmd_mkyoung(orig_pmd);
> >                 entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
> 

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1391239

On Fri, 29 Apr 2016 10:06:11 +0300
"Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> On Thu, Apr 28, 2016 at 08:45:42PM -0600, Alex Williamson wrote:
> > On Fri, 29 Apr 2016 03:51:06 +0300
> > "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> >   
> > > On Fri, Apr 29, 2016 at 01:21:27AM +0200, Andrea Arcangeli wrote:  
> > > > Hello Alex and Kirill,
> > > > 
> > > > On Thu, Apr 28, 2016 at 12:58:08PM -0600, Alex Williamson wrote:    
> > > > > > > specific fix to this code is not applicable.  It also still occurs on
> > > > > > > kernels as recent as v4.6-rc5, so the issue hasn't been silently fixed
> > > > > > > yet.  I'm able to reproduce this fairly quickly with the above test,
> > > > > > > but it's not hard to imagine a test w/o any iommu dependencies which
> > > > > > > simply does a user directed get_user_pages_fast() on a set of userspace
> > > > > > > addresses, retains the reference, and at some point later rechecks that
> > > > > > > a new get_user_pages_fast() results in the same page address.  It    
> > > > 
> > > > Can you try to "git revert 1f25fe20a76af0d960172fb104d4b13697cafa84"
> > > > and then apply the below patch on top of the revert?
> > > > 
> > > > Totally untested... if I missed something and it isn't correct, I hope
> > > > this brings us in the right direction faster at least.
> > > > 
> > > > Overall the problem I think is that we need to restore full accuracy
> > > > and we can't deal with false positive COWs (which aren't entirely
> > > > cheap either... reading 512 cachelines should be much faster than
> > > > copying 2MB and using 4MB of CPU cache). 32k vs 4MB. The problem of
> > > > course is when we really need a COW, we'll waste an additional 32k,
> > > > but then it doesn't matter that much as we'd be forced to load 4MB of
> > > > cache anyway in such case. There's room for optimizations but even the
> > > > simple below patch would be ok for now.
> > > > 
> > > > From 09e3d1ff10b49fb9c3ab77f0b96a862848e30067 Mon Sep 17 00:00:00 2001
> > > > From: Andrea Arcangeli <aarcange@redhat.com>
> > > > Date: Fri, 29 Apr 2016 01:05:06 +0200
> > > > Subject: [PATCH 1/1] mm: thp: calculate page_mapcount() correctly for THP
> > > >  pages
> > > > 
> > > > This allows to revert commit 1f25fe20a76af0d960172fb104d4b13697cafa84
> > > > and it provides fully accuracy with wrprotect faults so page pinning
> > > > will stop causing false positive copy-on-writes.
> > > > 
> > > > Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> > > > ---
> > > >  mm/util.c | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/mm/util.c b/mm/util.c
> > > > index 6cc81e7..a0b9f63 100644
> > > > --- a/mm/util.c
> > > > +++ b/mm/util.c
> > > > @@ -383,9 +383,10 @@ struct address_space *page_mapping(struct page *page)
> > > >  /* Slow path of page_mapcount() for compound pages */
> > > >  int __page_mapcount(struct page *page)
> > > >  {
> > > > -	int ret;
> > > > +	int ret = 0, i;
> > > >  
> > > > -	ret = atomic_read(&page->_mapcount) + 1;
> > > > +	for (i = 0; i < HPAGE_PMD_NR; i++)
> > > > +		ret = max(ret, atomic_read(&page->_mapcount) + 1);
> > > >  	page = compound_head(page);
> > > >  	ret += atomic_read(compound_mapcount_ptr(page)) + 1;
> > > >  	if (PageDoubleMap(page))    
> > > 
> > > You are right about the cause. I spend some time on wrong path: I was only
> > > able to trigger the bug with numa balancing enabled, so I assumed
> > > something is wrong in that code...
> > > 
> > > I would like to preserve current page_mapcount() behaviouts.
> > > I think this fix is better:  
> > 
> > This also seems to work in my testing, but assuming all else being
> > equal, there is a performance difference between the two for this test
> > case in favor of Andrea's solution.  Modifying the test to exit after
> > the first set of iterations, my system takes on average 107s to complete
> > with the solution below or 103.5s with the other approach.  Please note
> > that I have every mm debugging option I could find enabled and THP
> > scanning full speed on the system, so I don't know how this would play
> > out in a more tuned configuration.
> > 
> > The only reason I noticed is that I added a side test to sleep a random
> > number of seconds and kill the test program because sometimes killing
> > the test triggers errors.  I didn't see any errors with either of these
> > solutions, but suspected the first solution was completing more
> > iterations for similar intervals.  Modifying the test to exit seems to
> > prove that true.
> > 
> > I can't speak to which is the more architecturally correct solution,
> > but there may be a measurable performance difference to consider.  
> 
> Hm. I just woke up and haven't got any coffee yet, but I don't why my
> approach would be worse for performance. Both have the same algorithmic
> complexity.

I can't explain it either, I won't claim to understand either solution,
but with all the kernel hacking vm debug/sanity options disabled, there
still appears to be a very slight advantage to Andrea's proposal.  I
expect the test program should show this even if you're having a
difficult time using it to reproduce the bug.  I ran Andrea's patch
overnight, no issues reported.  Running your patch for an extended test
now.  Thanks,

Alex

[toc] | [prev] | [next] | [standalone]

#1391284

On Fri, Apr 29, 2016 at 10:06:11AM +0300, Kirill A. Shutemov wrote:
> Hm. I just woke up and haven't got any coffee yet, but I don't why my
> approach would be worse for performance. Both have the same algorithmic
> complexity.

Even before looking at the overall performance, I'm not sure your
patch is really fixing it all: you didn't touch reuse_swap_page which
is used by do_wp_page to know if it can call do_wp_page_reuse. Your
patch would still trigger a COW instead of calling do_wp_page_reuse,
but it would only happen if the page was pinned after the pmd split,
which is probably not what the testcase is triggering. My patch
instead fixed that too.

total_mapcount returns the wrong value for reuse_swap_page, which is
probably why you didn't try to use it there.

The main issue of my patch is that it has a performance downside that
is page_mapcount becomes expensive for all other usages, which is
better than breaking vfio but I couldn't use total_mapcount again
because it counts things wrong in reuse_swap_page.

Like I said there's room for optimizations so today I tried to
optimize more stuff...

From 74f1fd7fab71a2cce0d1796fb38241acde2c1224 Mon Sep 17 00:00:00 2001
From: Andrea Arcangeli <aarcange@redhat.com>
Date: Fri, 29 Apr 2016 01:05:06 +0200
Subject: [PATCH 1/1] mm: thp: calculate the mapcount correctly for THP pages
 during WP faults

This will provide fully accuracy to the mapcount calculation in the
write protect faults, so page pinning will not get broken by false
positive copy-on-writes.

total_mapcount() isn't the right calculation needed in
reuse_swap_page, so this introduces a page_trans_huge_mapcount() that
is effectively the full accurate return value for page_mapcount() if
dealing with Transparent Hugepages, however we only use the
page_trans_huge_mapcount() during COW faults where it strictly needed,
due to its higher runtime cost.

Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
 include/linux/mm.h   |  5 +++++
 include/linux/swap.h |  3 +--
 mm/huge_memory.c     | 44 ++++++++++++++++++++++++++++++++++++--------
 mm/swapfile.c        |  5 +----
 4 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 8fb3604..c2026a1 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -501,11 +501,16 @@ static inline int page_mapcount(struct page *page)
 
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 int total_mapcount(struct page *page);
+int page_trans_huge_mapcount(struct page *page);
 #else
 static inline int total_mapcount(struct page *page)
 {
 	return page_mapcount(page);
 }
+static inline int page_trans_huge_mapcount(struct page *page)
+{
+	return page_mapcount(page);
+}
 #endif
 
 static inline struct page *virt_to_head_page(const void *x)
diff --git a/include/linux/swap.h b/include/linux/swap.h
index 2f6478f..905bf8e 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -517,8 +517,7 @@ static inline int swp_swapcount(swp_entry_t entry)
 	return 0;
 }
 
-#define reuse_swap_page(page) \
-	(!PageTransCompound(page) && page_mapcount(page) == 1)
+#define reuse_swap_page(page) (page_trans_huge_mapcount(page) == 1)
 
 static inline int try_to_free_swap(struct page *page)
 {
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 06bce0f..6a6d9c0 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1298,15 +1298,9 @@ int do_huge_pmd_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,
 	VM_BUG_ON_PAGE(!PageCompound(page) || !PageHead(page), page);
 	/*
 	 * We can only reuse the page if nobody else maps the huge page or it's
-	 * part. We can do it by checking page_mapcount() on each sub-page, but
-	 * it's expensive.
-	 * The cheaper way is to check page_count() to be equal 1: every
-	 * mapcount takes page reference reference, so this way we can
-	 * guarantee, that the PMD is the only mapping.
-	 * This can give false negative if somebody pinned the page, but that's
-	 * fine.
+	 * part.
 	 */
-	if (page_mapcount(page) == 1 && page_count(page) == 1) {
+	if (page_trans_huge_mapcount(page) == 1) {
 		pmd_t entry;
 		entry = pmd_mkyoung(orig_pmd);
 		entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
@@ -3226,6 +3220,40 @@ int total_mapcount(struct page *page)
 }
 
 /*
+ * This calculates accurately how many mappings a transparent hugepage
+ * has (unlike page_mapcount() which isn't fully accurate). This full
+ * accuracy is primarily needed to know if copy-on-write faults can
+ * takeover the page and change the mapping to read-write instead of
+ * copying them. This is different from total_mapcount() too: we must
+ * not count all mappings on the subpages individually, but instead we
+ * must check the highest mapcount any one of the subpages has.
+ *
+ * It would be entirely safe and even more correct to replace
+ * page_mapcount() with page_trans_huge_mapcount(), however we only
+ * use page_trans_huge_mapcount() in the copy-on-write faults where we
+ * need full accuracy to avoid breaking page pinning.
+ */
+int page_trans_huge_mapcount(struct page *page)
+{
+	int i, ret;
+
+	VM_BUG_ON_PAGE(PageTail(page), page);
+
+	if (likely(!PageCompound(page)))
+		return atomic_read(&page->_mapcount) + 1;
+
+	ret = 0;
+	if (likely(!PageHuge(page))) {
+		for (i = 0; i < HPAGE_PMD_NR; i++)
+			ret = max(ret, atomic_read(&page[i]._mapcount) + 1);
+		if (PageDoubleMap(page))
+			ret -= 1;
+	}
+	ret += compound_mapcount(page);
+	return ret;
+}
+
+/*
  * This function splits huge page into normal pages. @page can point to any
  * subpage of huge page to split. Split doesn't change the position of @page.
  *
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 83874ec..984470a 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -930,10 +930,7 @@ int reuse_swap_page(struct page *page)
 	VM_BUG_ON_PAGE(!PageLocked(page), page);
 	if (unlikely(PageKsm(page)))
 		return 0;
-	/* The page is part of THP and cannot be reused */
-	if (PageTransCompound(page))
-		return 0;
-	count = page_mapcount(page);
+	count = page_trans_huge_mapcount(page);
 	if (count <= 1 && PageSwapCache(page)) {
 		count += page_swapcount(page);
 		if (count == 1 && !PageWriteback(page)) {

[toc] | [prev] | [next] | [standalone]

#1391500

On Fri, 29 Apr 2016 18:34:44 +0200
Andrea Arcangeli <aarcange@redhat.com> wrote:

> On Fri, Apr 29, 2016 at 10:06:11AM +0300, Kirill A. Shutemov wrote:
> > Hm. I just woke up and haven't got any coffee yet, but I don't why my
> > approach would be worse for performance. Both have the same algorithmic
> > complexity.  
> 
> Even before looking at the overall performance, I'm not sure your
> patch is really fixing it all: you didn't touch reuse_swap_page which
> is used by do_wp_page to know if it can call do_wp_page_reuse. Your
> patch would still trigger a COW instead of calling do_wp_page_reuse,
> but it would only happen if the page was pinned after the pmd split,
> which is probably not what the testcase is triggering. My patch
> instead fixed that too.
> 
> total_mapcount returns the wrong value for reuse_swap_page, which is
> probably why you didn't try to use it there.
> 
> The main issue of my patch is that it has a performance downside that
> is page_mapcount becomes expensive for all other usages, which is
> better than breaking vfio but I couldn't use total_mapcount again
> because it counts things wrong in reuse_swap_page.
> 
> Like I said there's room for optimizations so today I tried to
> optimize more stuff...

I've had this under test for several hours without error.  Thanks!

Alex


> From 74f1fd7fab71a2cce0d1796fb38241acde2c1224 Mon Sep 17 00:00:00 2001
> From: Andrea Arcangeli <aarcange@redhat.com>
> Date: Fri, 29 Apr 2016 01:05:06 +0200
> Subject: [PATCH 1/1] mm: thp: calculate the mapcount correctly for THP pages
>  during WP faults
> 
> This will provide fully accuracy to the mapcount calculation in the
> write protect faults, so page pinning will not get broken by false
> positive copy-on-writes.
> 
> total_mapcount() isn't the right calculation needed in
> reuse_swap_page, so this introduces a page_trans_huge_mapcount() that
> is effectively the full accurate return value for page_mapcount() if
> dealing with Transparent Hugepages, however we only use the
> page_trans_huge_mapcount() during COW faults where it strictly needed,
> due to its higher runtime cost.
> 
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> ---
>  include/linux/mm.h   |  5 +++++
>  include/linux/swap.h |  3 +--
>  mm/huge_memory.c     | 44 ++++++++++++++++++++++++++++++++++++--------
>  mm/swapfile.c        |  5 +----
>  4 files changed, 43 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 8fb3604..c2026a1 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -501,11 +501,16 @@ static inline int page_mapcount(struct page *page)
>  
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>  int total_mapcount(struct page *page);
> +int page_trans_huge_mapcount(struct page *page);
>  #else
>  static inline int total_mapcount(struct page *page)
>  {
>  	return page_mapcount(page);
>  }
> +static inline int page_trans_huge_mapcount(struct page *page)
> +{
> +	return page_mapcount(page);
> +}
>  #endif
>  
>  static inline struct page *virt_to_head_page(const void *x)
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index 2f6478f..905bf8e 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -517,8 +517,7 @@ static inline int swp_swapcount(swp_entry_t entry)
>  	return 0;
>  }
>  
> -#define reuse_swap_page(page) \
> -	(!PageTransCompound(page) && page_mapcount(page) == 1)
> +#define reuse_swap_page(page) (page_trans_huge_mapcount(page) == 1)
>  
>  static inline int try_to_free_swap(struct page *page)
>  {
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 06bce0f..6a6d9c0 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1298,15 +1298,9 @@ int do_huge_pmd_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,
>  	VM_BUG_ON_PAGE(!PageCompound(page) || !PageHead(page), page);
>  	/*
>  	 * We can only reuse the page if nobody else maps the huge page or it's
> -	 * part. We can do it by checking page_mapcount() on each sub-page, but
> -	 * it's expensive.
> -	 * The cheaper way is to check page_count() to be equal 1: every
> -	 * mapcount takes page reference reference, so this way we can
> -	 * guarantee, that the PMD is the only mapping.
> -	 * This can give false negative if somebody pinned the page, but that's
> -	 * fine.
> +	 * part.
>  	 */
> -	if (page_mapcount(page) == 1 && page_count(page) == 1) {
> +	if (page_trans_huge_mapcount(page) == 1) {
>  		pmd_t entry;
>  		entry = pmd_mkyoung(orig_pmd);
>  		entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
> @@ -3226,6 +3220,40 @@ int total_mapcount(struct page *page)
>  }
>  
>  /*
> + * This calculates accurately how many mappings a transparent hugepage
> + * has (unlike page_mapcount() which isn't fully accurate). This full
> + * accuracy is primarily needed to know if copy-on-write faults can
> + * takeover the page and change the mapping to read-write instead of
> + * copying them. This is different from total_mapcount() too: we must
> + * not count all mappings on the subpages individually, but instead we
> + * must check the highest mapcount any one of the subpages has.
> + *
> + * It would be entirely safe and even more correct to replace
> + * page_mapcount() with page_trans_huge_mapcount(), however we only
> + * use page_trans_huge_mapcount() in the copy-on-write faults where we
> + * need full accuracy to avoid breaking page pinning.
> + */
> +int page_trans_huge_mapcount(struct page *page)
> +{
> +	int i, ret;
> +
> +	VM_BUG_ON_PAGE(PageTail(page), page);
> +
> +	if (likely(!PageCompound(page)))
> +		return atomic_read(&page->_mapcount) + 1;
> +
> +	ret = 0;
> +	if (likely(!PageHuge(page))) {
> +		for (i = 0; i < HPAGE_PMD_NR; i++)
> +			ret = max(ret, atomic_read(&page[i]._mapcount) + 1);
> +		if (PageDoubleMap(page))
> +			ret -= 1;
> +	}
> +	ret += compound_mapcount(page);
> +	return ret;
> +}
> +
> +/*
>   * This function splits huge page into normal pages. @page can point to any
>   * subpage of huge page to split. Split doesn't change the position of @page.
>   *
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 83874ec..984470a 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -930,10 +930,7 @@ int reuse_swap_page(struct page *page)
>  	VM_BUG_ON_PAGE(!PageLocked(page), page);
>  	if (unlikely(PageKsm(page)))
>  		return 0;
> -	/* The page is part of THP and cannot be reused */
> -	if (PageTransCompound(page))
> -		return 0;
> -	count = page_mapcount(page);
> +	count = page_trans_huge_mapcount(page);
>  	if (count <= 1 && PageSwapCache(page)) {
>  		count += page_swapcount(page);
>  		if (count == 1 && !PageWriteback(page)) {

[toc] | [prev] | [next] | [standalone]

#1392115

On Fri, Apr 29, 2016 at 06:34:44PM +0200, Andrea Arcangeli wrote:
> On Fri, Apr 29, 2016 at 10:06:11AM +0300, Kirill A. Shutemov wrote:
> > Hm. I just woke up and haven't got any coffee yet, but I don't why my
> > approach would be worse for performance. Both have the same algorithmic
> > complexity.
> 
> Even before looking at the overall performance, I'm not sure your
> patch is really fixing it all: you didn't touch reuse_swap_page which
> is used by do_wp_page to know if it can call do_wp_page_reuse. Your
> patch would still trigger a COW instead of calling do_wp_page_reuse,
> but it would only happen if the page was pinned after the pmd split,
> which is probably not what the testcase is triggering. My patch
> instead fixed that too.
> 
> total_mapcount returns the wrong value for reuse_swap_page, which is
> probably why you didn't try to use it there.
> 
> The main issue of my patch is that it has a performance downside that
> is page_mapcount becomes expensive for all other usages, which is
> better than breaking vfio but I couldn't use total_mapcount again
> because it counts things wrong in reuse_swap_page.
> 
> Like I said there's room for optimizations so today I tried to
> optimize more stuff...
> 
> From 74f1fd7fab71a2cce0d1796fb38241acde2c1224 Mon Sep 17 00:00:00 2001
> From: Andrea Arcangeli <aarcange@redhat.com>
> Date: Fri, 29 Apr 2016 01:05:06 +0200
> Subject: [PATCH 1/1] mm: thp: calculate the mapcount correctly for THP pages
>  during WP faults
> 
> This will provide fully accuracy to the mapcount calculation in the
> write protect faults, so page pinning will not get broken by false
> positive copy-on-writes.
> 
> total_mapcount() isn't the right calculation needed in
> reuse_swap_page, so this introduces a page_trans_huge_mapcount() that
> is effectively the full accurate return value for page_mapcount() if
> dealing with Transparent Hugepages, however we only use the
> page_trans_huge_mapcount() during COW faults where it strictly needed,
> due to its higher runtime cost.
> 
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> ---
>  include/linux/mm.h   |  5 +++++
>  include/linux/swap.h |  3 +--
>  mm/huge_memory.c     | 44 ++++++++++++++++++++++++++++++++++++--------
>  mm/swapfile.c        |  5 +----
>  4 files changed, 43 insertions(+), 14 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 8fb3604..c2026a1 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -501,11 +501,16 @@ static inline int page_mapcount(struct page *page)
>  
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>  int total_mapcount(struct page *page);
> +int page_trans_huge_mapcount(struct page *page);
>  #else
>  static inline int total_mapcount(struct page *page)
>  {
>  	return page_mapcount(page);
>  }
> +static inline int page_trans_huge_mapcount(struct page *page)
> +{
> +	return page_mapcount(page);
> +}
>  #endif
>  
>  static inline struct page *virt_to_head_page(const void *x)
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index 2f6478f..905bf8e 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -517,8 +517,7 @@ static inline int swp_swapcount(swp_entry_t entry)
>  	return 0;
>  }
>  
> -#define reuse_swap_page(page) \
> -	(!PageTransCompound(page) && page_mapcount(page) == 1)
> +#define reuse_swap_page(page) (page_trans_huge_mapcount(page) == 1)
>  
>  static inline int try_to_free_swap(struct page *page)
>  {
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 06bce0f..6a6d9c0 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1298,15 +1298,9 @@ int do_huge_pmd_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,
>  	VM_BUG_ON_PAGE(!PageCompound(page) || !PageHead(page), page);
>  	/*
>  	 * We can only reuse the page if nobody else maps the huge page or it's
> -	 * part. We can do it by checking page_mapcount() on each sub-page, but
> -	 * it's expensive.
> -	 * The cheaper way is to check page_count() to be equal 1: every
> -	 * mapcount takes page reference reference, so this way we can
> -	 * guarantee, that the PMD is the only mapping.
> -	 * This can give false negative if somebody pinned the page, but that's
> -	 * fine.
> +	 * part.
>  	 */
> -	if (page_mapcount(page) == 1 && page_count(page) == 1) {
> +	if (page_trans_huge_mapcount(page) == 1) {
>  		pmd_t entry;
>  		entry = pmd_mkyoung(orig_pmd);
>  		entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
> @@ -3226,6 +3220,40 @@ int total_mapcount(struct page *page)
>  }
>  
>  /*
> + * This calculates accurately how many mappings a transparent hugepage
> + * has (unlike page_mapcount() which isn't fully accurate). This full
> + * accuracy is primarily needed to know if copy-on-write faults can
> + * takeover the page and change the mapping to read-write instead of
> + * copying them. This is different from total_mapcount() too: we must
> + * not count all mappings on the subpages individually, but instead we
> + * must check the highest mapcount any one of the subpages has.
> + *
> + * It would be entirely safe and even more correct to replace
> + * page_mapcount() with page_trans_huge_mapcount(), however we only
> + * use page_trans_huge_mapcount() in the copy-on-write faults where we
> + * need full accuracy to avoid breaking page pinning.
> + */
> +int page_trans_huge_mapcount(struct page *page)
> +{
> +	int i, ret;
> +
> +	VM_BUG_ON_PAGE(PageTail(page), page);
> +
> +	if (likely(!PageCompound(page)))
> +		return atomic_read(&page->_mapcount) + 1;
> +
> +	ret = 0;
> +	if (likely(!PageHuge(page))) {
> +		for (i = 0; i < HPAGE_PMD_NR; i++)
> +			ret = max(ret, atomic_read(&page[i]._mapcount) + 1);
> +		if (PageDoubleMap(page))
> +			ret -= 1;
> +	}
> +	ret += compound_mapcount(page);
> +	return ret;
> +}
> +
> +/*
>   * This function splits huge page into normal pages. @page can point to any
>   * subpage of huge page to split. Split doesn't change the position of @page.
>   *
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 83874ec..984470a 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -930,10 +930,7 @@ int reuse_swap_page(struct page *page)
>  	VM_BUG_ON_PAGE(!PageLocked(page), page);
>  	if (unlikely(PageKsm(page)))
>  		return 0;
> -	/* The page is part of THP and cannot be reused */
> -	if (PageTransCompound(page))
> -		return 0;
> -	count = page_mapcount(page);
> +	count = page_trans_huge_mapcount(page);
>  	if (count <= 1 && PageSwapCache(page)) {
>  		count += page_swapcount(page);
>  		if (count == 1 && !PageWriteback(page)) {

I don't think this would work correctly. Let's check one of callers:

static int do_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,
		unsigned long address, pte_t *page_table, pmd_t *pmd,
		spinlock_t *ptl, pte_t orig_pte)
	__releases(ptl)
{
...
		if (reuse_swap_page(old_page)) {
			/*
			 * The page is all ours.  Move it to our anon_vma so
			 * the rmap code will not search our parent or siblings.
			 * Protected against the rmap code by the page lock.
			 */
			page_move_anon_rmap(old_page, vma, address);
			unlock_page(old_page);
			return wp_page_reuse(mm, vma, address, page_table, ptl,
					     orig_pte, old_page, 0, 0);
		}

The first thing to notice is that old_page can be a tail page here
therefore page_move_anon_rmap() should be able to handle this after you
patch, which it doesn't.

But I think there's a bigger problem.

Consider the following situation: after split_huge_pmd() we have
pte-mapped THP, fork() comes and now the pages is shared between two
processes. Child process munmap()s one half of the THP page, parent
munmap()s the other half.

IIUC, afther that page_trans_huge_mapcount() would give us 1 as all 4k
subpages have mapcount exactly one. Fault in the child would trigger
do_wp_page() and reuse_swap_page() returns true, which would lead to
page_move_anon_rmap() tranferring the whole compound page to child's
anon_vma. That's not correct.

We should at least avoid page_move_anon_rmap() for compound pages there.


Other thing I would like to discuss is if there's a problem on vfio side.
To me it looks like vfio expects guarantee from get_user_pages() which it
doesn't provide: obtaining pin on the page doesn't guarantee that the page
is going to remain mapped into userspace until the pin is gone.

Even with THP COW regressing fixed, vfio would stay fragile: any
MADV_DONTNEED/fork()/mremap()/whatever what would make vfio expectation
broken.

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1392135

On Mon, May 02, 2016 at 01:41:19PM +0300, Kirill A. Shutemov wrote:
> On Fri, Apr 29, 2016 at 06:34:44PM +0200, Andrea Arcangeli wrote:
> > On Fri, Apr 29, 2016 at 10:06:11AM +0300, Kirill A. Shutemov wrote:
> > > Hm. I just woke up and haven't got any coffee yet, but I don't why my
> > > approach would be worse for performance. Both have the same algorithmic
> > > complexity.
> > 
> > Even before looking at the overall performance, I'm not sure your
> > patch is really fixing it all: you didn't touch reuse_swap_page which
> > is used by do_wp_page to know if it can call do_wp_page_reuse. Your
> > patch would still trigger a COW instead of calling do_wp_page_reuse,
> > but it would only happen if the page was pinned after the pmd split,
> > which is probably not what the testcase is triggering. My patch
> > instead fixed that too.
> > 
> > total_mapcount returns the wrong value for reuse_swap_page, which is
> > probably why you didn't try to use it there.
> > 
> > The main issue of my patch is that it has a performance downside that
> > is page_mapcount becomes expensive for all other usages, which is
> > better than breaking vfio but I couldn't use total_mapcount again
> > because it counts things wrong in reuse_swap_page.
> > 
> > Like I said there's room for optimizations so today I tried to
> > optimize more stuff...
> > 
> > From 74f1fd7fab71a2cce0d1796fb38241acde2c1224 Mon Sep 17 00:00:00 2001
> > From: Andrea Arcangeli <aarcange@redhat.com>
> > Date: Fri, 29 Apr 2016 01:05:06 +0200
> > Subject: [PATCH 1/1] mm: thp: calculate the mapcount correctly for THP pages
> >  during WP faults
> > 
> > This will provide fully accuracy to the mapcount calculation in the
> > write protect faults, so page pinning will not get broken by false
> > positive copy-on-writes.
> > 
> > total_mapcount() isn't the right calculation needed in
> > reuse_swap_page, so this introduces a page_trans_huge_mapcount() that
> > is effectively the full accurate return value for page_mapcount() if
> > dealing with Transparent Hugepages, however we only use the
> > page_trans_huge_mapcount() during COW faults where it strictly needed,
> > due to its higher runtime cost.
> > 
> > Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> > ---
> >  include/linux/mm.h   |  5 +++++
> >  include/linux/swap.h |  3 +--
> >  mm/huge_memory.c     | 44 ++++++++++++++++++++++++++++++++++++--------
> >  mm/swapfile.c        |  5 +----
> >  4 files changed, 43 insertions(+), 14 deletions(-)
> > 
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 8fb3604..c2026a1 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -501,11 +501,16 @@ static inline int page_mapcount(struct page *page)
> >  
> >  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> >  int total_mapcount(struct page *page);
> > +int page_trans_huge_mapcount(struct page *page);
> >  #else
> >  static inline int total_mapcount(struct page *page)
> >  {
> >  	return page_mapcount(page);
> >  }
> > +static inline int page_trans_huge_mapcount(struct page *page)
> > +{
> > +	return page_mapcount(page);
> > +}
> >  #endif
> >  
> >  static inline struct page *virt_to_head_page(const void *x)
> > diff --git a/include/linux/swap.h b/include/linux/swap.h
> > index 2f6478f..905bf8e 100644
> > --- a/include/linux/swap.h
> > +++ b/include/linux/swap.h
> > @@ -517,8 +517,7 @@ static inline int swp_swapcount(swp_entry_t entry)
> >  	return 0;
> >  }
> >  
> > -#define reuse_swap_page(page) \
> > -	(!PageTransCompound(page) && page_mapcount(page) == 1)
> > +#define reuse_swap_page(page) (page_trans_huge_mapcount(page) == 1)
> >  
> >  static inline int try_to_free_swap(struct page *page)
> >  {
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 06bce0f..6a6d9c0 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -1298,15 +1298,9 @@ int do_huge_pmd_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,
> >  	VM_BUG_ON_PAGE(!PageCompound(page) || !PageHead(page), page);
> >  	/*
> >  	 * We can only reuse the page if nobody else maps the huge page or it's
> > -	 * part. We can do it by checking page_mapcount() on each sub-page, but
> > -	 * it's expensive.
> > -	 * The cheaper way is to check page_count() to be equal 1: every
> > -	 * mapcount takes page reference reference, so this way we can
> > -	 * guarantee, that the PMD is the only mapping.
> > -	 * This can give false negative if somebody pinned the page, but that's
> > -	 * fine.
> > +	 * part.
> >  	 */
> > -	if (page_mapcount(page) == 1 && page_count(page) == 1) {
> > +	if (page_trans_huge_mapcount(page) == 1) {
> >  		pmd_t entry;
> >  		entry = pmd_mkyoung(orig_pmd);
> >  		entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
> > @@ -3226,6 +3220,40 @@ int total_mapcount(struct page *page)
> >  }
> >  
> >  /*
> > + * This calculates accurately how many mappings a transparent hugepage
> > + * has (unlike page_mapcount() which isn't fully accurate). This full
> > + * accuracy is primarily needed to know if copy-on-write faults can
> > + * takeover the page and change the mapping to read-write instead of
> > + * copying them. This is different from total_mapcount() too: we must
> > + * not count all mappings on the subpages individually, but instead we
> > + * must check the highest mapcount any one of the subpages has.
> > + *
> > + * It would be entirely safe and even more correct to replace
> > + * page_mapcount() with page_trans_huge_mapcount(), however we only
> > + * use page_trans_huge_mapcount() in the copy-on-write faults where we
> > + * need full accuracy to avoid breaking page pinning.
> > + */
> > +int page_trans_huge_mapcount(struct page *page)
> > +{
> > +	int i, ret;
> > +
> > +	VM_BUG_ON_PAGE(PageTail(page), page);
> > +
> > +	if (likely(!PageCompound(page)))
> > +		return atomic_read(&page->_mapcount) + 1;
> > +
> > +	ret = 0;
> > +	if (likely(!PageHuge(page))) {
> > +		for (i = 0; i < HPAGE_PMD_NR; i++)
> > +			ret = max(ret, atomic_read(&page[i]._mapcount) + 1);
> > +		if (PageDoubleMap(page))
> > +			ret -= 1;
> > +	}
> > +	ret += compound_mapcount(page);
> > +	return ret;
> > +}
> > +
> > +/*
> >   * This function splits huge page into normal pages. @page can point to any
> >   * subpage of huge page to split. Split doesn't change the position of @page.
> >   *
> > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > index 83874ec..984470a 100644
> > --- a/mm/swapfile.c
> > +++ b/mm/swapfile.c
> > @@ -930,10 +930,7 @@ int reuse_swap_page(struct page *page)
> >  	VM_BUG_ON_PAGE(!PageLocked(page), page);
> >  	if (unlikely(PageKsm(page)))
> >  		return 0;
> > -	/* The page is part of THP and cannot be reused */
> > -	if (PageTransCompound(page))
> > -		return 0;
> > -	count = page_mapcount(page);
> > +	count = page_trans_huge_mapcount(page);
> >  	if (count <= 1 && PageSwapCache(page)) {
> >  		count += page_swapcount(page);
> >  		if (count == 1 && !PageWriteback(page)) {
> 
> I don't think this would work correctly. Let's check one of callers:
> 
> static int do_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,
> 		unsigned long address, pte_t *page_table, pmd_t *pmd,
> 		spinlock_t *ptl, pte_t orig_pte)
> 	__releases(ptl)
> {
> ...
> 		if (reuse_swap_page(old_page)) {
> 			/*
> 			 * The page is all ours.  Move it to our anon_vma so
> 			 * the rmap code will not search our parent or siblings.
> 			 * Protected against the rmap code by the page lock.
> 			 */
> 			page_move_anon_rmap(old_page, vma, address);
> 			unlock_page(old_page);
> 			return wp_page_reuse(mm, vma, address, page_table, ptl,
> 					     orig_pte, old_page, 0, 0);
> 		}
> 
> The first thing to notice is that old_page can be a tail page here
> therefore page_move_anon_rmap() should be able to handle this after you
> patch, which it doesn't.
> 
> But I think there's a bigger problem.
> 
> Consider the following situation: after split_huge_pmd() we have
> pte-mapped THP, fork() comes and now the pages is shared between two
> processes. Child process munmap()s one half of the THP page, parent
> munmap()s the other half.
> 
> IIUC, afther that page_trans_huge_mapcount() would give us 1 as all 4k
> subpages have mapcount exactly one. Fault in the child would trigger
> do_wp_page() and reuse_swap_page() returns true, which would lead to
> page_move_anon_rmap() tranferring the whole compound page to child's
> anon_vma. That's not correct.
> 
> We should at least avoid page_move_anon_rmap() for compound pages there.
> 
> 
> Other thing I would like to discuss is if there's a problem on vfio side.
> To me it looks like vfio expects guarantee from get_user_pages() which it
> doesn't provide: obtaining pin on the page doesn't guarantee that the page
> is going to remain mapped into userspace until the pin is gone.
> 
> Even with THP COW regressing fixed, vfio would stay fragile: any
> MADV_DONTNEED/fork()/mremap()/whatever what would make vfio expectation
> broken.
> 

Well i don't think it is fair/accurate assessment of get_user_pages(), page
must remain mapped to same virtual address until pin is gone. I am ignoring
mremap() as it is a scient decision from userspace and while virtual address
change in that case, the pined page behind should move with the mapping.
Same of MADV_DONTNEED. I agree that get_user_pages() is broken after fork()
but this have been the case since dawn of time, so it is something expected.

If not vfio, then direct-io, have been expecting this kind of behavior for
long time, so i see this as part of get_user_pages() guarantee.

Concerning vfio, not providing this guarantee will break countless number of
workload. Thing like qemu/kvm allocate anonymous memory and hand it over to
the guest kernel which presents it as memory. Now a device driver inside the
guest kernel need to get bus mapping for a given (guest) page, which from
host point of view means a mapping from anonymous page to bus mapping but
for guest to keep accessing the same page the anonymous mapping (ie a
specific virtual address on the host side) must keep pointing to the same
page. This have been the case with get_user_pages() until now, so whether
we like it or not we must keep that guarantee.

This kind of workload knows that they can't do mremap()/fork()/... and keep
that guarantee but they at expect existing guarantee and i don't think we
can break that.

Cheers,
Jérôme

[toc] | [prev] | [next] | [standalone]

#1392165 — GUP guarantees wrt to userspace mappings redesign

On Mon, May 02, 2016 at 01:15:13PM +0200, Jerome Glisse wrote:
> On Mon, May 02, 2016 at 01:41:19PM +0300, Kirill A. Shutemov wrote:
> > Other thing I would like to discuss is if there's a problem on vfio side.
> > To me it looks like vfio expects guarantee from get_user_pages() which it
> > doesn't provide: obtaining pin on the page doesn't guarantee that the page
> > is going to remain mapped into userspace until the pin is gone.
> > 
> > Even with THP COW regressing fixed, vfio would stay fragile: any
> > MADV_DONTNEED/fork()/mremap()/whatever what would make vfio expectation
> > broken.
> > 
> 
> Well i don't think it is fair/accurate assessment of get_user_pages(), page
> must remain mapped to same virtual address until pin is gone. I am ignoring
> mremap() as it is a scient decision from userspace and while virtual address
> change in that case, the pined page behind should move with the mapping.
> Same of MADV_DONTNEED. I agree that get_user_pages() is broken after fork()
> but this have been the case since dawn of time, so it is something expected.
> 
> If not vfio, then direct-io, have been expecting this kind of behavior for
> long time, so i see this as part of get_user_pages() guarantee.
> 
> Concerning vfio, not providing this guarantee will break countless number of
> workload. Thing like qemu/kvm allocate anonymous memory and hand it over to
> the guest kernel which presents it as memory. Now a device driver inside the
> guest kernel need to get bus mapping for a given (guest) page, which from
> host point of view means a mapping from anonymous page to bus mapping but
> for guest to keep accessing the same page the anonymous mapping (ie a
> specific virtual address on the host side) must keep pointing to the same
> page. This have been the case with get_user_pages() until now, so whether
> we like it or not we must keep that guarantee.
> 
> This kind of workload knows that they can't do mremap()/fork()/... and keep
> that guarantee but they at expect existing guarantee and i don't think we
> can break that.

Quick look around:

 - I don't see any check page_count() around __replace_page() in uprobes,
   so it can easily replace pinned page.

 - KSM has the page_count() check, there's still race wrt GUP_fast: it can
   take the pin between the check and establishing new pte entry.

 - khugepaged: the same story as with KSM.

I don't see how we can deliver on the guarantee, especially with lockless
GUP_fast.

Or am I missing something important?

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1392213 — Re: GUP guarantees wrt to userspace mappings redesign

On Mon, May 02, 2016 at 03:14:02PM +0300, Kirill A. Shutemov wrote:
> On Mon, May 02, 2016 at 01:15:13PM +0200, Jerome Glisse wrote:
> > On Mon, May 02, 2016 at 01:41:19PM +0300, Kirill A. Shutemov wrote:
> > > Other thing I would like to discuss is if there's a problem on vfio side.
> > > To me it looks like vfio expects guarantee from get_user_pages() which it
> > > doesn't provide: obtaining pin on the page doesn't guarantee that the page
> > > is going to remain mapped into userspace until the pin is gone.
> > > 
> > > Even with THP COW regressing fixed, vfio would stay fragile: any
> > > MADV_DONTNEED/fork()/mremap()/whatever what would make vfio expectation
> > > broken.
> > > 
> > 
> > Well i don't think it is fair/accurate assessment of get_user_pages(), page
> > must remain mapped to same virtual address until pin is gone. I am ignoring
> > mremap() as it is a scient decision from userspace and while virtual address
> > change in that case, the pined page behind should move with the mapping.
> > Same of MADV_DONTNEED. I agree that get_user_pages() is broken after fork()
> > but this have been the case since dawn of time, so it is something expected.
> > 
> > If not vfio, then direct-io, have been expecting this kind of behavior for
> > long time, so i see this as part of get_user_pages() guarantee.
> > 
> > Concerning vfio, not providing this guarantee will break countless number of
> > workload. Thing like qemu/kvm allocate anonymous memory and hand it over to
> > the guest kernel which presents it as memory. Now a device driver inside the
> > guest kernel need to get bus mapping for a given (guest) page, which from
> > host point of view means a mapping from anonymous page to bus mapping but
> > for guest to keep accessing the same page the anonymous mapping (ie a
> > specific virtual address on the host side) must keep pointing to the same
> > page. This have been the case with get_user_pages() until now, so whether
> > we like it or not we must keep that guarantee.
> > 
> > This kind of workload knows that they can't do mremap()/fork()/... and keep
> > that guarantee but they at expect existing guarantee and i don't think we
> > can break that.
> 
> Quick look around:
> 
>  - I don't see any check page_count() around __replace_page() in uprobes,
>    so it can easily replace pinned page.

Not an issue for existing user as this is only use to instrument code, existing
user do not execute code from virtual address for which they have done a GUP.

> 
>  - KSM has the page_count() check, there's still race wrt GUP_fast: it can
>    take the pin between the check and establishing new pte entry.

KSM is not an issue for existing user as they all do get_user_pages() with
write = 1 and the KSM first map page read only before considering to replace
them and check page refcount. So there can be no race with gup_fast there.

> 
>  - khugepaged: the same story as with KSM.

I am assuming you are talking about collapse_huge_page() here, if you look in
that function there is a comment about GUP_fast. Noneless i believe the comment
is wrong as i believe there is an existing race window btw pmdp_collapse_flush()
and __collapse_huge_page_isolate() :

  get_user_pages_fast()          | collapse_huge_page()
   gup_pmd_range() -> valid pmd  | ...
                                 | pmdp_collapse_flush() clear pmd
                                 | ...
                                 | __collapse_huge_page_isolate()
                                 | [Above check page count and see no GUP]
   gup_pte_range() -> ref page   |

This is a very unlikely race because get_user_pages_fast() can not be preempted
while collapse_huge_page() can be preempted btw pmdp_collapse_flush() and
__collapse_huge_page_isolate(), more over collapse_huge_page() has lot more
instructions to chew on than get_user_pages_fast() btw gup_pmd_range() and
gup_pte_range().

So i think this is an unlikely race. I am not sure how to forbid it from
happening, except maybe in get_user_pages_fast() by checking pmd is still
valid after gup_pte_range().

> 
> I don't see how we can deliver on the guarantee, especially with lockless
> GUP_fast.
> 
> Or am I missing something important?

So as said above, i think existing user of get_user_pages() are not sensitive
to the races you pointed above. I am sure there are some corner case where
the guarantee that GUP pin a page against a virtual address is violated but
i do not think they apply to any existing user of GUP.

Note that i would personaly like that this existing assumption about GUP did
not exist. I hate it, but fact is that it does exist and nobody can remember
where the Doc did park the Delorean

Cheers,
Jérôme

[toc] | [prev] | [next] | [standalone]

#1392272 — Re: GUP guarantees wrt to userspace mappings

On Mon, May 02, 2016 at 03:39:20PM +0200, Jerome Glisse wrote:
> On Mon, May 02, 2016 at 03:14:02PM +0300, Kirill A. Shutemov wrote:
> > On Mon, May 02, 2016 at 01:15:13PM +0200, Jerome Glisse wrote:
> > > On Mon, May 02, 2016 at 01:41:19PM +0300, Kirill A. Shutemov wrote:
> > > > Other thing I would like to discuss is if there's a problem on vfio side.
> > > > To me it looks like vfio expects guarantee from get_user_pages() which it
> > > > doesn't provide: obtaining pin on the page doesn't guarantee that the page
> > > > is going to remain mapped into userspace until the pin is gone.
> > > > 
> > > > Even with THP COW regressing fixed, vfio would stay fragile: any
> > > > MADV_DONTNEED/fork()/mremap()/whatever what would make vfio expectation
> > > > broken.
> > > > 
> > > 
> > > Well i don't think it is fair/accurate assessment of get_user_pages(), page
> > > must remain mapped to same virtual address until pin is gone. I am ignoring
> > > mremap() as it is a scient decision from userspace and while virtual address
> > > change in that case, the pined page behind should move with the mapping.
> > > Same of MADV_DONTNEED. I agree that get_user_pages() is broken after fork()
> > > but this have been the case since dawn of time, so it is something expected.
> > > 
> > > If not vfio, then direct-io, have been expecting this kind of behavior for
> > > long time, so i see this as part of get_user_pages() guarantee.
> > > 
> > > Concerning vfio, not providing this guarantee will break countless number of
> > > workload. Thing like qemu/kvm allocate anonymous memory and hand it over to
> > > the guest kernel which presents it as memory. Now a device driver inside the
> > > guest kernel need to get bus mapping for a given (guest) page, which from
> > > host point of view means a mapping from anonymous page to bus mapping but
> > > for guest to keep accessing the same page the anonymous mapping (ie a
> > > specific virtual address on the host side) must keep pointing to the same
> > > page. This have been the case with get_user_pages() until now, so whether
> > > we like it or not we must keep that guarantee.
> > > 
> > > This kind of workload knows that they can't do mremap()/fork()/... and keep
> > > that guarantee but they at expect existing guarantee and i don't think we
> > > can break that.
> > 
> > Quick look around:
> > 
> >  - I don't see any check page_count() around __replace_page() in uprobes,
> >    so it can easily replace pinned page.
> 
> Not an issue for existing user as this is only use to instrument code, existing
> user do not execute code from virtual address for which they have done a GUP.

Okay, so we can establish that GUP doesn't provide the guarantee in some
cases.

> >  - KSM has the page_count() check, there's still race wrt GUP_fast: it can
> >    take the pin between the check and establishing new pte entry.
> 
> KSM is not an issue for existing user as they all do get_user_pages() with
> write = 1 and the KSM first map page read only before considering to replace
> them and check page refcount. So there can be no race with gup_fast there.

In vfio case, 'write' is conditional on IOMMU_WRITE, meaning not all
get_user_pages() are with write=1.

> >  - khugepaged: the same story as with KSM.
> 
> I am assuming you are talking about collapse_huge_page() here, if you look in
> that function there is a comment about GUP_fast. Noneless i believe the comment
> is wrong as i believe there is an existing race window btw pmdp_collapse_flush()
> and __collapse_huge_page_isolate() :
> 
>   get_user_pages_fast()          | collapse_huge_page()
>    gup_pmd_range() -> valid pmd  | ...
>                                  | pmdp_collapse_flush() clear pmd
>                                  | ...
>                                  | __collapse_huge_page_isolate()
>                                  | [Above check page count and see no GUP]
>    gup_pte_range() -> ref page   |
> 
> This is a very unlikely race because get_user_pages_fast() can not be preempted
> while collapse_huge_page() can be preempted btw pmdp_collapse_flush() and
> __collapse_huge_page_isolate(), more over collapse_huge_page() has lot more
> instructions to chew on than get_user_pages_fast() btw gup_pmd_range() and
> gup_pte_range().

Yes, the race window is small, but there.

> So i think this is an unlikely race. I am not sure how to forbid it from
> happening, except maybe in get_user_pages_fast() by checking pmd is still
> valid after gup_pte_range().

Switching to non-fast GUP would help :-P

> > I don't see how we can deliver on the guarantee, especially with lockless
> > GUP_fast.
> > 
> > Or am I missing something important?
> 
> So as said above, i think existing user of get_user_pages() are not sensitive
> to the races you pointed above. I am sure there are some corner case where
> the guarantee that GUP pin a page against a virtual address is violated but
> i do not think they apply to any existing user of GUP.
> 
> Note that i would personaly like that this existing assumption about GUP did
> not exist. I hate it, but fact is that it does exist and nobody can remember
> where the Doc did park the Delorean

The drivers who want the guarantee can provide own ->mmap and have more
control on what is visible in userspace.

Alternatively, we have mmu_notifiers to track changes in userspace
mappings.

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1392299 — Re: GUP guarantees wrt to userspace mappings

On Mon, May 02, 2016 at 06:00:13PM +0300, Kirill A. Shutemov wrote:
> On Mon, May 02, 2016 at 03:39:20PM +0200, Jerome Glisse wrote:
> > On Mon, May 02, 2016 at 03:14:02PM +0300, Kirill A. Shutemov wrote:
> > > On Mon, May 02, 2016 at 01:15:13PM +0200, Jerome Glisse wrote:
> > > > On Mon, May 02, 2016 at 01:41:19PM +0300, Kirill A. Shutemov wrote:
> > > > > Other thing I would like to discuss is if there's a problem on vfio side.
> > > > > To me it looks like vfio expects guarantee from get_user_pages() which it
> > > > > doesn't provide: obtaining pin on the page doesn't guarantee that the page
> > > > > is going to remain mapped into userspace until the pin is gone.
> > > > > 
> > > > > Even with THP COW regressing fixed, vfio would stay fragile: any
> > > > > MADV_DONTNEED/fork()/mremap()/whatever what would make vfio expectation
> > > > > broken.
> > > > > 
> > > > 
> > > > Well i don't think it is fair/accurate assessment of get_user_pages(), page
> > > > must remain mapped to same virtual address until pin is gone. I am ignoring
> > > > mremap() as it is a scient decision from userspace and while virtual address
> > > > change in that case, the pined page behind should move with the mapping.
> > > > Same of MADV_DONTNEED. I agree that get_user_pages() is broken after fork()
> > > > but this have been the case since dawn of time, so it is something expected.
> > > > 
> > > > If not vfio, then direct-io, have been expecting this kind of behavior for
> > > > long time, so i see this as part of get_user_pages() guarantee.
> > > > 
> > > > Concerning vfio, not providing this guarantee will break countless number of
> > > > workload. Thing like qemu/kvm allocate anonymous memory and hand it over to
> > > > the guest kernel which presents it as memory. Now a device driver inside the
> > > > guest kernel need to get bus mapping for a given (guest) page, which from
> > > > host point of view means a mapping from anonymous page to bus mapping but
> > > > for guest to keep accessing the same page the anonymous mapping (ie a
> > > > specific virtual address on the host side) must keep pointing to the same
> > > > page. This have been the case with get_user_pages() until now, so whether
> > > > we like it or not we must keep that guarantee.
> > > > 
> > > > This kind of workload knows that they can't do mremap()/fork()/... and keep
> > > > that guarantee but they at expect existing guarantee and i don't think we
> > > > can break that.
> > > 
> > > Quick look around:
> > > 
> > >  - I don't see any check page_count() around __replace_page() in uprobes,
> > >    so it can easily replace pinned page.
> > 
> > Not an issue for existing user as this is only use to instrument code, existing
> > user do not execute code from virtual address for which they have done a GUP.
> 
> Okay, so we can establish that GUP doesn't provide the guarantee in some
> cases.

Correct but it use to provide that guarantee in respect to THP.


> > >  - KSM has the page_count() check, there's still race wrt GUP_fast: it can
> > >    take the pin between the check and establishing new pte entry.
> > 
> > KSM is not an issue for existing user as they all do get_user_pages() with
> > write = 1 and the KSM first map page read only before considering to replace
> > them and check page refcount. So there can be no race with gup_fast there.
> 
> In vfio case, 'write' is conditional on IOMMU_WRITE, meaning not all
> get_user_pages() are with write=1.

I think this is still fine as it means that device will read only and thus
you can migrate to different page (ie the guest is not expecting to read back
anything writen by the device and device writting to the page would be illegal
and a proper IOMMU would forbid it). So it is like direct-io when you write
from anonymous memory to a file.


> > >  - khugepaged: the same story as with KSM.
> > 
> > I am assuming you are talking about collapse_huge_page() here, if you look in
> > that function there is a comment about GUP_fast. Noneless i believe the comment
> > is wrong as i believe there is an existing race window btw pmdp_collapse_flush()
> > and __collapse_huge_page_isolate() :
> > 
> >   get_user_pages_fast()          | collapse_huge_page()
> >    gup_pmd_range() -> valid pmd  | ...
> >                                  | pmdp_collapse_flush() clear pmd
> >                                  | ...
> >                                  | __collapse_huge_page_isolate()
> >                                  | [Above check page count and see no GUP]
> >    gup_pte_range() -> ref page   |
> > 
> > This is a very unlikely race because get_user_pages_fast() can not be preempted
> > while collapse_huge_page() can be preempted btw pmdp_collapse_flush() and
> > __collapse_huge_page_isolate(), more over collapse_huge_page() has lot more
> > instructions to chew on than get_user_pages_fast() btw gup_pmd_range() and
> > gup_pte_range().
> 
> Yes, the race window is small, but there.

Now that i think again about it, i don't think it exist. pmdp_collapse_flush()
will flush the tlb and thus send an IPI but get_user_pages_fast() can't be
preempted so the flush will have to wait for existing get_user_pages_fast() to
complete. Or am i missunderstanding flush ? So khugepaged is safe from GUP_fast
point of view like the comment, inside it, says.


> > So i think this is an unlikely race. I am not sure how to forbid it from
> > happening, except maybe in get_user_pages_fast() by checking pmd is still
> > valid after gup_pte_range().
> 
> Switching to non-fast GUP would help :-P
> 
> > > I don't see how we can deliver on the guarantee, especially with lockless
> > > GUP_fast.
> > > 
> > > Or am I missing something important?
> > 
> > So as said above, i think existing user of get_user_pages() are not sensitive
> > to the races you pointed above. I am sure there are some corner case where
> > the guarantee that GUP pin a page against a virtual address is violated but
> > i do not think they apply to any existing user of GUP.
> > 
> > Note that i would personaly like that this existing assumption about GUP did
> > not exist. I hate it, but fact is that it does exist and nobody can remember
> > where the Doc did park the Delorean
> 
> The drivers who want the guarantee can provide own ->mmap and have more
> control on what is visible in userspace.
> 
> Alternatively, we have mmu_notifiers to track changes in userspace
> mappings.
> 

Well you can't not rely on special vma here. Qemu alloc anonymous memory and
hand it over to guest, then a guest driver (ie runing in the guest not on the
host) try to map that memory and need valid DMA address for it, this is when
vfio (on the host kernel) starts pining memory of regular anonymous vma (on
the host). That same memory might back some special vma with ->mmap callback
but in the guest. Point is there is no driver on the host and no special vma.
From host point of view this is anonymous memory, but from guest POV it is
just memory.

Requiring special vma would need major change to kvm and probably xen, in
respect on how they support things like PCI passthrough.

In existing workload, host kernel can not make assumption on how anonymous
memory is gonna be use.

Cheers,
Jérôme

[toc] | [prev] | [next] | [standalone]

#1392350 — Re: GUP guarantees wrt to userspace mappings

On Mon, May 02, 2016 at 05:22:49PM +0200, Jerome Glisse wrote:
> On Mon, May 02, 2016 at 06:00:13PM +0300, Kirill A. Shutemov wrote:
> > > > Quick look around:
> > > > 
> > > >  - I don't see any check page_count() around __replace_page() in uprobes,
> > > >    so it can easily replace pinned page.
> > > 
> > > Not an issue for existing user as this is only use to instrument code, existing
> > > user do not execute code from virtual address for which they have done a GUP.
> > 
> > Okay, so we can establish that GUP doesn't provide the guarantee in some
> > cases.
> 
> Correct but it use to provide that guarantee in respect to THP.

Yes, the THP regression need to be fixed. I don't argue with that.

> > > >  - KSM has the page_count() check, there's still race wrt GUP_fast: it can
> > > >    take the pin between the check and establishing new pte entry.
> > > 
> > > KSM is not an issue for existing user as they all do get_user_pages() with
> > > write = 1 and the KSM first map page read only before considering to replace
> > > them and check page refcount. So there can be no race with gup_fast there.
> > 
> > In vfio case, 'write' is conditional on IOMMU_WRITE, meaning not all
> > get_user_pages() are with write=1.
> 
> I think this is still fine as it means that device will read only and thus
> you can migrate to different page (ie the guest is not expecting to read back
> anything writen by the device and device writting to the page would be illegal
> and a proper IOMMU would forbid it). So it is like direct-io when you write
> from anonymous memory to a file.

Hm. Okay.

> > > >  - khugepaged: the same story as with KSM.
> > > 
> > > I am assuming you are talking about collapse_huge_page() here, if you look in
> > > that function there is a comment about GUP_fast. Noneless i believe the comment
> > > is wrong as i believe there is an existing race window btw pmdp_collapse_flush()
> > > and __collapse_huge_page_isolate() :
> > > 
> > >   get_user_pages_fast()          | collapse_huge_page()
> > >    gup_pmd_range() -> valid pmd  | ...
> > >                                  | pmdp_collapse_flush() clear pmd
> > >                                  | ...
> > >                                  | __collapse_huge_page_isolate()
> > >                                  | [Above check page count and see no GUP]
> > >    gup_pte_range() -> ref page   |
> > > 
> > > This is a very unlikely race because get_user_pages_fast() can not be preempted
> > > while collapse_huge_page() can be preempted btw pmdp_collapse_flush() and
> > > __collapse_huge_page_isolate(), more over collapse_huge_page() has lot more
> > > instructions to chew on than get_user_pages_fast() btw gup_pmd_range() and
> > > gup_pte_range().
> > 
> > Yes, the race window is small, but there.
> 
> Now that i think again about it, i don't think it exist. pmdp_collapse_flush()
> will flush the tlb and thus send an IPI but get_user_pages_fast() can't be
> preempted so the flush will have to wait for existing get_user_pages_fast() to
> complete. Or am i missunderstanding flush ? So khugepaged is safe from GUP_fast
> point of view like the comment, inside it, says.

You are right. It's safe too.

> > > So as said above, i think existing user of get_user_pages() are not sensitive
> > > to the races you pointed above. I am sure there are some corner case where
> > > the guarantee that GUP pin a page against a virtual address is violated but
> > > i do not think they apply to any existing user of GUP.
> > > 
> > > Note that i would personaly like that this existing assumption about GUP did
> > > not exist. I hate it, but fact is that it does exist and nobody can remember
> > > where the Doc did park the Delorean
> > 
> > The drivers who want the guarantee can provide own ->mmap and have more
> > control on what is visible in userspace.
> > 
> > Alternatively, we have mmu_notifiers to track changes in userspace
> > mappings.
> > 
> 
> Well you can't not rely on special vma here. Qemu alloc anonymous memory and
> hand it over to guest, then a guest driver (ie runing in the guest not on the
> host) try to map that memory and need valid DMA address for it, this is when
> vfio (on the host kernel) starts pining memory of regular anonymous vma (on
> the host). That same memory might back some special vma with ->mmap callback
> but in the guest. Point is there is no driver on the host and no special vma.
> From host point of view this is anonymous memory, but from guest POV it is
> just memory.
> 
> Requiring special vma would need major change to kvm and probably xen, in
> respect on how they support things like PCI passthrough.
> 
> In existing workload, host kernel can not make assumption on how anonymous
> memory is gonna be use.

Any reason why mmu_notifier is not an option?

-- 
 Kirill A. Shutemov

[toc] | [prev] | [next] | [standalone]

#1392470 — Re: GUP guarantees wrt to userspace mappings

On Mon, May 02, 2016 at 07:12:52PM +0300, Kirill A. Shutemov wrote:
> Any reason why mmu_notifier is not an option?

No way to trigger an hardware re-tried secondary MMU fault as result
of PCI DMA memory access, and expensive to do an MMU notifier
invalidate if it requires waiting for the DMA to complete (but since
MMU notifier is now sleepable the latter is a secondary concern).

[toc] | [prev] | [next] | [standalone]

#1392475 — Re: GUP guarantees wrt to userspace mappings

On Mon, May 02, 2016 at 05:22:49PM +0200, Jerome Glisse wrote:
> I think this is still fine as it means that device will read only and thus
> you can migrate to different page (ie the guest is not expecting to read back
> anything writen by the device and device writting to the page would be illegal
> and a proper IOMMU would forbid it). So it is like direct-io when you write
> from anonymous memory to a file.

Agreed. write=1 is so that if there's an O_DIRECT write() and the app
is only reading, there will be no COW generated on shared anonymous
memory/MAP_PRIVATE-filebacked.

> Now that i think again about it, i don't think it exist. pmdp_collapse_flush()
> will flush the tlb and thus send an IPI but get_user_pages_fast() can't be
> preempted so the flush will have to wait for existing get_user_pages_fast() to
> complete. Or am i missunderstanding flush ? So khugepaged is safe from GUP_fast
> point of view like the comment, inside it, says.

This is exactly correct, there's no race window.

The IPI (or the quiescent point in case of the gup_fast RCU version)
are the things that flush away get_user_pages_fast with pmdp_collapse_flush().

> Well you can't not rely on special vma here. Qemu alloc anonymous memory and
> hand it over to guest, then a guest driver (ie runing in the guest not on the
> host) try to map that memory and need valid DMA address for it, this is when
> vfio (on the host kernel) starts pining memory of regular anonymous vma (on
> the host). That same memory might back some special vma with ->mmap callback
> but in the guest. Point is there is no driver on the host and no special vma.
> From host point of view this is anonymous memory, but from guest POV it is
> just memory.

It's quite important it stays regular tmpfs/anon as device memory is
managed by the device and we'd lose everything (KSM/swapping/NUMA
balancing/compaction/memory-hotunplug/CMA etc..).

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web