v5:
- patch 4: change "int cow" into "bool cow"
- collect r-bs for Jason
v4:
- add r-b for Mike on the last patch, add some more commit message explains
that why we don't need wr-protect trick
- fix one warning of unused var in copy_present_page() [Gal]
v3:
- rebase to linux-next/akpm, switch to the new HPAGE helpers [MikeK]
- correct error check for alloc_huge_page(); test it this time to make sure
fork() fails gracefully when overcommit [MikeK]
- move page copy out of pgtable lock: this changed quite a bit of the logic in
the last patch, prealloc is dropped since I found it easier to understand
without looping at all [MikeK]
v2:
- pass in 1 to alloc_huge_page() last param [Mike]
- reduce comment, unify the comment in one place [Linus]
- add r-bs for Mike and Miaohe
---- original cover letter ----
As reported by Gal [1], we still miss the code clip to handle early cow for
hugetlb case, which is true. Again, it still feels odd to fork() after using a
few huge pages, especially if they're privately mapped to me.. However I do
agree with Gal and Jason in that we should still have that since that'll
complete the early cow on fork effort at least, and it'll still fix issues
where buffers are not well under control and not easy to apply MADV_DONTFORK.
The first two patches (1-2) are some cleanups I noticed when reading into the
hugetlb reserve map code. I think it's good to have but they're not necessary
for fixing the fork issue.
The last two patches (3-4) is the real fix.
I tested this with a fork() after some vfio-pci assignment, so I'm pretty sure
the page copy path could trigger well (page will be accounted right after the
fork()), but I didn't do data check since the card I assigned is some random
nic. Gal, please feel free to try this if you have better way to verify the
series.
https://github.com/xzpeter/linux/tree/fork-cow-pin-huge
Please review, thanks!
[1] https://lore.kernel.org/lkml/[email protected]/
Peter Xu (5):
hugetlb: Dedup the code to add a new file_region
hugetlg: Break earlier in add_reservation_in_range() when we can
mm: Introduce page_needs_cow_for_dma() for deciding whether cow
mm: Use is_cow_mapping() across tree where proper
hugetlb: Do early cow when page pinned on src mm
drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c | 4 +-
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 2 +-
fs/proc/task_mmu.c | 2 -
include/linux/mm.h | 21 ++++
mm/huge_memory.c | 8 +-
mm/hugetlb.c | 123 +++++++++++++++------
mm/internal.h | 5 -
mm/memory.c | 8 +-
8 files changed, 117 insertions(+), 56 deletions(-)
--
2.26.2
After is_cow_mapping() is exported in mm.h, replace some manual checks
elsewhere throughout the tree but start to use the new helper.
Cc: VMware Graphics <[email protected]>
Cc: Roland Scheidegger <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: Mike Kravetz <[email protected]>
Cc: Alexey Dobriyan <[email protected]>
Cc: Andrew Morton <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Peter Xu <[email protected]>
---
drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c | 4 +---
drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 2 +-
fs/proc/task_mmu.c | 2 --
mm/hugetlb.c | 4 +---
4 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c b/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
index 0a900afc66ff..45c9c6a7f1d6 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c
@@ -500,8 +500,6 @@ vm_fault_t vmw_bo_vm_huge_fault(struct vm_fault *vmf,
vm_fault_t ret;
pgoff_t fault_page_size;
bool write = vmf->flags & FAULT_FLAG_WRITE;
- bool is_cow_mapping =
- (vma->vm_flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
switch (pe_size) {
case PE_SIZE_PMD:
@@ -518,7 +516,7 @@ vm_fault_t vmw_bo_vm_huge_fault(struct vm_fault *vmf,
}
/* Always do write dirty-tracking and COW on PTE level. */
- if (write && (READ_ONCE(vbo->dirty) || is_cow_mapping))
+ if (write && (READ_ONCE(vbo->dirty) || is_cow_mapping(vma->vm_flags)))
return VM_FAULT_FALLBACK;
ret = ttm_bo_vm_reserve(bo, vmf);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
index 3c03b1746661..cb9975889e2f 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c
@@ -49,7 +49,7 @@ int vmw_mmap(struct file *filp, struct vm_area_struct *vma)
vma->vm_ops = &vmw_vm_ops;
/* Use VM_PFNMAP rather than VM_MIXEDMAP if not a COW mapping */
- if ((vma->vm_flags & (VM_SHARED | VM_MAYWRITE)) != VM_MAYWRITE)
+ if (!is_cow_mapping(vma->vm_flags))
vma->vm_flags = (vma->vm_flags & ~VM_MIXEDMAP) | VM_PFNMAP;
return 0;
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 3cec6fbef725..e862cab69583 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1036,8 +1036,6 @@ struct clear_refs_private {
#ifdef CONFIG_MEM_SOFT_DIRTY
-#define is_cow_mapping(flags) (((flags) & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE)
-
static inline bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr, pte_t pte)
{
struct page *page;
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 2ba4ea4ab46e..8379224e1d43 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3731,15 +3731,13 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
pte_t *src_pte, *dst_pte, entry, dst_entry;
struct page *ptepage;
unsigned long addr;
- int cow;
+ bool cow = is_cow_mapping(vma->vm_flags);
struct hstate *h = hstate_vma(vma);
unsigned long sz = huge_page_size(h);
struct address_space *mapping = vma->vm_file->f_mapping;
struct mmu_notifier_range range;
int ret = 0;
- cow = (vma->vm_flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
-
if (cow) {
mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, src,
vma->vm_start,
--
2.26.2
We've got quite a few places (pte, pmd, pud) that explicitly checked against
whether we should break the cow right now during fork(). It's easier to
provide a helper, especially before we work the same thing on hugetlbfs.
Since we'll reference is_cow_mapping() in mm.h, move it there too. Actually it
suites mm.h more since internal.h is mm/ only, but mm.h is exported to the
whole kernel. With that we should expect another patch to use is_cow_mapping()
whenever we can across the kernel since we do use it quite a lot but it's
always done with raw code against VM_* flags.
Reviewed-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Peter Xu <[email protected]>
---
include/linux/mm.h | 21 +++++++++++++++++++++
mm/huge_memory.c | 8 ++------
mm/internal.h | 5 -----
mm/memory.c | 8 +-------
4 files changed, 24 insertions(+), 18 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 77e64e3eac80..64a71bf20536 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1300,6 +1300,27 @@ static inline bool page_maybe_dma_pinned(struct page *page)
GUP_PIN_COUNTING_BIAS;
}
+static inline bool is_cow_mapping(vm_flags_t flags)
+{
+ return (flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
+}
+
+/*
+ * This should most likely only be called during fork() to see whether we
+ * should break the cow immediately for a page on the src mm.
+ */
+static inline bool page_needs_cow_for_dma(struct vm_area_struct *vma,
+ struct page *page)
+{
+ if (!is_cow_mapping(vma->vm_flags))
+ return false;
+
+ if (!atomic_read(&vma->vm_mm->has_pinned))
+ return false;
+
+ return page_maybe_dma_pinned(page);
+}
+
#if defined(CONFIG_SPARSEMEM) && !defined(CONFIG_SPARSEMEM_VMEMMAP)
#define SECTION_IN_PAGE_FLAGS
#endif
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 395c75111d33..da1d63a41aec 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1100,9 +1100,7 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
* best effort that the pinned pages won't be replaced by another
* random page during the coming copy-on-write.
*/
- if (unlikely(is_cow_mapping(vma->vm_flags) &&
- atomic_read(&src_mm->has_pinned) &&
- page_maybe_dma_pinned(src_page))) {
+ if (unlikely(page_needs_cow_for_dma(vma, src_page))) {
pte_free(dst_mm, pgtable);
spin_unlock(src_ptl);
spin_unlock(dst_ptl);
@@ -1214,9 +1212,7 @@ int copy_huge_pud(struct mm_struct *dst_mm, struct mm_struct *src_mm,
}
/* Please refer to comments in copy_huge_pmd() */
- if (unlikely(is_cow_mapping(vma->vm_flags) &&
- atomic_read(&src_mm->has_pinned) &&
- page_maybe_dma_pinned(pud_page(pud)))) {
+ if (unlikely(page_needs_cow_for_dma(vma, pud_page(pud)))) {
spin_unlock(src_ptl);
spin_unlock(dst_ptl);
__split_huge_pud(vma, src_pud, addr);
diff --git a/mm/internal.h b/mm/internal.h
index 9902648f2206..1432feec62df 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -296,11 +296,6 @@ static inline unsigned int buddy_order(struct page *page)
*/
#define buddy_order_unsafe(page) READ_ONCE(page_private(page))
-static inline bool is_cow_mapping(vm_flags_t flags)
-{
- return (flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
-}
-
/*
* These three helpers classifies VMAs for virtual memory accounting.
*/
diff --git a/mm/memory.c b/mm/memory.c
index eac765e1c6b9..e50e488b8ea3 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -809,12 +809,8 @@ copy_present_page(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma
pte_t *dst_pte, pte_t *src_pte, unsigned long addr, int *rss,
struct page **prealloc, pte_t pte, struct page *page)
{
- struct mm_struct *src_mm = src_vma->vm_mm;
struct page *new_page;
- if (!is_cow_mapping(src_vma->vm_flags))
- return 1;
-
/*
* What we want to do is to check whether this page may
* have been pinned by the parent process. If so,
@@ -828,9 +824,7 @@ copy_present_page(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma
* the page count. That might give false positives for
* for pinning, but it will work correctly.
*/
- if (likely(!atomic_read(&src_mm->has_pinned)))
- return 1;
- if (likely(!page_maybe_dma_pinned(page)))
+ if (likely(!page_needs_cow_for_dma(src_vma, page)))
return 1;
new_page = *prealloc;
--
2.26.2
This is the last missing piece of the COW-during-fork effort when there're
pinned pages found. One can reference 70e806e4e645 ("mm: Do early cow for
pinned pages during fork() for ptes", 2020-09-27) for more information, since
we do similar things here rather than pte this time, but just for hugetlb.
Note that after Jason's recent work on 57efa1fe5957 ("mm/gup: prevent gup_fast
from racing with COW during fork", 2020-12-15) which is safer and easier to
understand, we're safe now within the whole copy_page_range() against gup-fast,
we don't need the wr-protect trick that proposed in 70e806e4e645 anymore.
Reviewed-by: Mike Kravetz <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Peter Xu <[email protected]>
---
mm/hugetlb.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 62 insertions(+), 4 deletions(-)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 8379224e1d43..0b45ff7df708 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3725,6 +3725,18 @@ static bool is_hugetlb_entry_hwpoisoned(pte_t pte)
return false;
}
+static void
+hugetlb_install_page(struct vm_area_struct *vma, pte_t *ptep, unsigned long addr,
+ struct page *new_page)
+{
+ __SetPageUptodate(new_page);
+ set_huge_pte_at(vma->vm_mm, addr, ptep, make_huge_pte(vma, new_page, 1));
+ hugepage_add_new_anon_rmap(new_page, vma, addr);
+ hugetlb_count_add(pages_per_huge_page(hstate_vma(vma)), vma->vm_mm);
+ ClearHPageRestoreReserve(new_page);
+ SetHPageMigratable(new_page);
+}
+
int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
struct vm_area_struct *vma)
{
@@ -3734,6 +3746,7 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
bool cow = is_cow_mapping(vma->vm_flags);
struct hstate *h = hstate_vma(vma);
unsigned long sz = huge_page_size(h);
+ unsigned long npages = pages_per_huge_page(h);
struct address_space *mapping = vma->vm_file->f_mapping;
struct mmu_notifier_range range;
int ret = 0;
@@ -3782,6 +3795,7 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
entry = huge_ptep_get(src_pte);
dst_entry = huge_ptep_get(dst_pte);
+again:
if (huge_pte_none(entry) || !huge_pte_none(dst_entry)) {
/*
* Skip if src entry none. Also, skip in the
@@ -3805,6 +3819,52 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
}
set_huge_swap_pte_at(dst, addr, dst_pte, entry, sz);
} else {
+ entry = huge_ptep_get(src_pte);
+ ptepage = pte_page(entry);
+ get_page(ptepage);
+
+ /*
+ * This is a rare case where we see pinned hugetlb
+ * pages while they're prone to COW. We need to do the
+ * COW earlier during fork.
+ *
+ * When pre-allocating the page or copying data, we
+ * need to be without the pgtable locks since we could
+ * sleep during the process.
+ */
+ if (unlikely(page_needs_cow_for_dma(vma, ptepage))) {
+ pte_t src_pte_old = entry;
+ struct page *new;
+
+ spin_unlock(src_ptl);
+ spin_unlock(dst_ptl);
+ /* Do not use reserve as it's private owned */
+ new = alloc_huge_page(vma, addr, 1);
+ if (IS_ERR(new)) {
+ put_page(ptepage);
+ ret = PTR_ERR(new);
+ break;
+ }
+ copy_user_huge_page(new, ptepage, addr, vma,
+ npages);
+ put_page(ptepage);
+
+ /* Install the new huge page if src pte stable */
+ dst_ptl = huge_pte_lock(h, dst, dst_pte);
+ src_ptl = huge_pte_lockptr(h, src, src_pte);
+ spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
+ entry = huge_ptep_get(src_pte);
+ if (!pte_same(src_pte_old, entry)) {
+ put_page(new);
+ /* dst_entry won't change as in child */
+ goto again;
+ }
+ hugetlb_install_page(vma, dst_pte, addr, new);
+ spin_unlock(src_ptl);
+ spin_unlock(dst_ptl);
+ continue;
+ }
+
if (cow) {
/*
* No need to notify as we are downgrading page
@@ -3815,12 +3875,10 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
*/
huge_ptep_set_wrprotect(src, addr, src_pte);
}
- entry = huge_ptep_get(src_pte);
- ptepage = pte_page(entry);
- get_page(ptepage);
+
page_dup_rmap(ptepage, true);
set_huge_pte_at(dst, addr, dst_pte, entry);
- hugetlb_count_add(pages_per_huge_page(h), dst);
+ hugetlb_count_add(npages, dst);
}
spin_unlock(src_ptl);
spin_unlock(dst_ptl);
--
2.26.2
On Wed, Feb 17, 2021 at 06:35:42PM -0500, Peter Xu wrote:
> v5:
> - patch 4: change "int cow" into "bool cow"
> - collect r-bs for Jason
Andrew,
I just noticed 5.12-rc1 has released; is this series still possible to make it
for 5.12, or needs to wait for 5.13?
Thanks,
--
Peter Xu
On Mon, 1 Mar 2021 09:11:51 -0500 Peter Xu <[email protected]> wrote:
> On Wed, Feb 17, 2021 at 06:35:42PM -0500, Peter Xu wrote:
> > v5:
> > - patch 4: change "int cow" into "bool cow"
> > - collect r-bs for Jason
>
> Andrew,
>
> I just noticed 5.12-rc1 has released; is this series still possible to make it
> for 5.12, or needs to wait for 5.13?
>
It has taken a while to settle down. What is the case for
fast-tracking it into 5.12?
On Mon, Mar 01, 2021 at 04:28:46PM -0800, Andrew Morton wrote:
> On Mon, 1 Mar 2021 09:11:51 -0500 Peter Xu <[email protected]> wrote:
>
> > On Wed, Feb 17, 2021 at 06:35:42PM -0500, Peter Xu wrote:
> > > v5:
> > > - patch 4: change "int cow" into "bool cow"
> > > - collect r-bs for Jason
> >
> > Andrew,
> >
> > I just noticed 5.12-rc1 has released; is this series still possible to make it
> > for 5.12, or needs to wait for 5.13?
> >
>
> It has taken a while to settle down. What is the case for
> fast-tracking it into 5.12?
IIRC hugetlb users and fork and DMA will get the unexpected VA
corruption that triggered all this work.
Jason
Yes, such user includes libfabric (https://ofiwg.github.io/libfabric/) . which uses hugetlb pages.
On 3/1/21, 4:30 PM, "Jason Gunthorpe" <[email protected]> wrote:
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
On Mon, Mar 01, 2021 at 04:28:46PM -0800, Andrew Morton wrote:
> On Mon, 1 Mar 2021 09:11:51 -0500 Peter Xu <[email protected]> wrote:
>
> > On Wed, Feb 17, 2021 at 06:35:42PM -0500, Peter Xu wrote:
> > > v5:
> > > - patch 4: change "int cow" into "bool cow"
> > > - collect r-bs for Jason
> >
> > Andrew,
> >
> > I just noticed 5.12-rc1 has released; is this series still possible to make it
> > for 5.12, or needs to wait for 5.13?
> >
>
> It has taken a while to settle down. What is the case for
> fast-tracking it into 5.12?
IIRC hugetlb users and fork and DMA will get the unexpected VA
corruption that triggered all this work.
Jason
On Mon, Mar 01, 2021 at 04:28:46PM -0800, Andrew Morton wrote:
> On Mon, 1 Mar 2021 09:11:51 -0500 Peter Xu <[email protected]> wrote:
>
> > On Wed, Feb 17, 2021 at 06:35:42PM -0500, Peter Xu wrote:
> > > v5:
> > > - patch 4: change "int cow" into "bool cow"
> > > - collect r-bs for Jason
> >
> > Andrew,
> >
> > I just noticed 5.12-rc1 has released; is this series still possible to make it
> > for 5.12, or needs to wait for 5.13?
> >
>
> It has taken a while to settle down. What is the case for
> fast-tracking it into 5.12?
Andrew,
As Jason and Wei pointed out, I think some userspace still got corrupted data
without this series when using hugetlb backend. I don't think it'll suite for
a late RC release but it'll still be great if it can be considered as an early
rc candidate, ideally rc1 of course. If you prefer the other way, I can also
repost it before 5.13 merge window opens.
Thanks,
--
Peter Xu
On Tue, Mar 02, 2021 at 06:45:49PM -0800, Linus Torvalds wrote:
> On Tue, Mar 2, 2021 at 5:47 PM Peter Xu <[email protected]> wrote:
> >
> > As Jason and Wei pointed out, I think some userspace still got corrupted data
> > without this series when using hugetlb backend. I don't think it'll suite for
> > a late RC release but it'll still be great if it can be considered as an early
> > rc candidate, ideally rc1 of course. If you prefer the other way, I can also
> > repost it before 5.13 merge window opens.
>
> I think with my merge window delay issue, you guys have the perfect
> excuse for pushing it a bit late and it still hitting 5.12
Andrew did you need something further from Peter? I don't see it in linux-next?
Jason
On Tue, Mar 2, 2021 at 5:47 PM Peter Xu <[email protected]> wrote:
>
> As Jason and Wei pointed out, I think some userspace still got corrupted data
> without this series when using hugetlb backend. I don't think it'll suite for
> a late RC release but it'll still be great if it can be considered as an early
> rc candidate, ideally rc1 of course. If you prefer the other way, I can also
> repost it before 5.13 merge window opens.
I think with my merge window delay issue, you guys have the perfect
excuse for pushing it a bit late and it still hitting 5.12
Linus