* [RFC PATCH v2 1/4] hugetlbfs: revert use of i_mmap_rwsem for pmd sharing and more sync
2020-10-26 23:31 [RFC PATCH v2 0/4] hugetlbfs: introduce hinode_rwsem for pmd sharing synchronization Mike Kravetz
@ 2020-10-26 23:31 ` Mike Kravetz
2020-10-26 23:31 ` [RFC PATCH v2 2/4] hugetlbfs: add hinode_rwsem to hugetlb specific inode Mike Kravetz
` (3 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Mike Kravetz @ 2020-10-26 23:31 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Hugh Dickins, Michal Hocko, Naoya Horiguchi, Aneesh Kumar K . V,
Andrea Arcangeli, Kirill A . Shutemov, Davidlohr Bueso,
Prakash Sangappa, Andrew Morton, Mike Kravetz
i_mmap_rwsem is being used for synchronization of huge pmd sharing.
This required changing the locking order such that i_mmap_rwsem needed
to be taken before a page lock. This can be accommodated in hugetlbfs
specific code, but becomes problematic in the areas of page migration
and memory failure where it is hard to get around the normal locking
order.
In subsequent patches, a new rw semaphore will be added to the hugetlbfs
inode for this type of synchronization. This patch will revert use of
i_mmap_rwsem for pmd sharing and fault/truncate synchronization.
The following commits are reverted:
commit 0bf7b64e6e51 ("hugetlb: add lockdep check for i_mmap_rwsem held in
huge_pmd_share")
commit 34ae204f1851 ("hugetlbfs: remove call to huge_pte_alloc without
i_mmap_rwsem")
commit 1139d336fff4 ("mm/hugetlb.c: fix pages per hugetlb calculation")
commit 87bf91d39bb5 ("hugetlbfs: Use i_mmap_rwsem to address page
fault/truncate race")
commit c0d0381ade79 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing
synchronization")
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
fs/hugetlbfs/inode.c | 30 ++----
include/linux/fs.h | 15 ---
include/linux/hugetlb.h | 16 +---
mm/hugetlb.c | 197 ++++++----------------------------------
mm/memory-failure.c | 29 +-----
mm/migrate.c | 25 +----
mm/rmap.c | 18 +---
mm/userfaultfd.c | 11 +--
8 files changed, 49 insertions(+), 292 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index b5c109703daa..3a1246aeedc4 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -444,9 +444,10 @@ hugetlb_vmdelete_list(struct rb_root_cached *root, pgoff_t start, pgoff_t end)
* In this case, we first scan the range and release found pages.
* After releasing pages, hugetlb_unreserve_pages cleans up region/reserv
* maps and global counts. Page faults can not race with truncation
- * in this routine. hugetlb_no_page() holds i_mmap_rwsem and prevents
- * page faults in the truncated range by checking i_size. i_size is
- * modified while holding i_mmap_rwsem.
+ * in this routine. hugetlb_no_page() prevents page faults in the
+ * truncated range. It checks i_size before allocation, and again after
+ * with the page table lock for the page held. The same lock must be
+ * acquired to unmap a page.
* hole punch is indicated if end is not LLONG_MAX
* In the hole punch case we scan the range and release found pages.
* Only when releasing a page is the associated region/reserv map
@@ -486,15 +487,7 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart,
index = page->index;
hash = hugetlb_fault_mutex_hash(mapping, index);
- if (!truncate_op) {
- /*
- * Only need to hold the fault mutex in the
- * hole punch case. This prevents races with
- * page faults. Races are not possible in the
- * case of truncation.
- */
- mutex_lock(&hugetlb_fault_mutex_table[hash]);
- }
+ mutex_lock(&hugetlb_fault_mutex_table[hash]);
/*
* If page is mapped, it was faulted in after being
@@ -508,9 +501,7 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart,
if (unlikely(page_mapped(page))) {
BUG_ON(truncate_op);
- mutex_unlock(&hugetlb_fault_mutex_table[hash]);
i_mmap_lock_write(mapping);
- mutex_lock(&hugetlb_fault_mutex_table[hash]);
hugetlb_vmdelete_list(&mapping->i_mmap,
index * pages_per_huge_page(h),
(index + 1) * pages_per_huge_page(h));
@@ -537,8 +528,7 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart,
}
unlock_page(page);
- if (!truncate_op)
- mutex_unlock(&hugetlb_fault_mutex_table[hash]);
+ mutex_unlock(&hugetlb_fault_mutex_table[hash]);
}
huge_pagevec_release(&pvec);
cond_resched();
@@ -576,8 +566,8 @@ static int hugetlb_vmtruncate(struct inode *inode, loff_t offset)
BUG_ON(offset & ~huge_page_mask(h));
pgoff = offset >> PAGE_SHIFT;
- i_mmap_lock_write(mapping);
i_size_write(inode, offset);
+ i_mmap_lock_write(mapping);
if (!RB_EMPTY_ROOT(&mapping->i_mmap.rb_root))
hugetlb_vmdelete_list(&mapping->i_mmap, pgoff, 0);
i_mmap_unlock_write(mapping);
@@ -699,11 +689,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
/* addr is the offset within the file (zero based) */
addr = index * hpage_size;
- /*
- * fault mutex taken here, protects against fault path
- * and hole punch. inode_lock previously taken protects
- * against truncation.
- */
+ /* mutex taken here, fault path and hole punch */
hash = hugetlb_fault_mutex_hash(mapping, index);
mutex_lock(&hugetlb_fault_mutex_table[hash]);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 21cc971fd960..8123f281c275 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -493,11 +493,6 @@ static inline void i_mmap_lock_write(struct address_space *mapping)
down_write(&mapping->i_mmap_rwsem);
}
-static inline int i_mmap_trylock_write(struct address_space *mapping)
-{
- return down_write_trylock(&mapping->i_mmap_rwsem);
-}
-
static inline void i_mmap_unlock_write(struct address_space *mapping)
{
up_write(&mapping->i_mmap_rwsem);
@@ -513,16 +508,6 @@ static inline void i_mmap_unlock_read(struct address_space *mapping)
up_read(&mapping->i_mmap_rwsem);
}
-static inline void i_mmap_assert_locked(struct address_space *mapping)
-{
- lockdep_assert_held(&mapping->i_mmap_rwsem);
-}
-
-static inline void i_mmap_assert_write_locked(struct address_space *mapping)
-{
- lockdep_assert_held_write(&mapping->i_mmap_rwsem);
-}
-
/*
* Might pages of this file be mapped into userspace?
*/
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index ebca2ef02212..bf79a5601091 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -154,8 +154,6 @@ u32 hugetlb_fault_mutex_hash(struct address_space *mapping, pgoff_t idx);
pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud);
-struct address_space *hugetlb_page_mapping_lock_write(struct page *hpage);
-
extern int sysctl_hugetlb_shm_group;
extern struct list_head huge_boot_pages;
@@ -165,8 +163,7 @@ pte_t *huge_pte_alloc(struct mm_struct *mm,
unsigned long addr, unsigned long sz);
pte_t *huge_pte_offset(struct mm_struct *mm,
unsigned long addr, unsigned long sz);
-int huge_pmd_unshare(struct mm_struct *mm, struct vm_area_struct *vma,
- unsigned long *addr, pte_t *ptep);
+int huge_pmd_unshare(struct mm_struct *mm, unsigned long *addr, pte_t *ptep);
void adjust_range_if_pmd_sharing_possible(struct vm_area_struct *vma,
unsigned long *start, unsigned long *end);
struct page *follow_huge_addr(struct mm_struct *mm, unsigned long address,
@@ -199,15 +196,8 @@ static inline unsigned long hugetlb_total_pages(void)
return 0;
}
-static inline struct address_space *hugetlb_page_mapping_lock_write(
- struct page *hpage)
-{
- return NULL;
-}
-
-static inline int huge_pmd_unshare(struct mm_struct *mm,
- struct vm_area_struct *vma,
- unsigned long *addr, pte_t *ptep)
+static inline int huge_pmd_unshare(struct mm_struct *mm, unsigned long *addr,
+ pte_t *ptep)
{
return 0;
}
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index fe76f8fd5a73..9a316b6d0b51 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1568,106 +1568,6 @@ int PageHeadHuge(struct page *page_head)
return page_head[1].compound_dtor == HUGETLB_PAGE_DTOR;
}
-/*
- * Find address_space associated with hugetlbfs page.
- * Upon entry page is locked and page 'was' mapped although mapped state
- * could change. If necessary, use anon_vma to find vma and associated
- * address space. The returned mapping may be stale, but it can not be
- * invalid as page lock (which is held) is required to destroy mapping.
- */
-static struct address_space *_get_hugetlb_page_mapping(struct page *hpage)
-{
- struct anon_vma *anon_vma;
- pgoff_t pgoff_start, pgoff_end;
- struct anon_vma_chain *avc;
- struct address_space *mapping = page_mapping(hpage);
-
- /* Simple file based mapping */
- if (mapping)
- return mapping;
-
- /*
- * Even anonymous hugetlbfs mappings are associated with an
- * underlying hugetlbfs file (see hugetlb_file_setup in mmap
- * code). Find a vma associated with the anonymous vma, and
- * use the file pointer to get address_space.
- */
- anon_vma = page_lock_anon_vma_read(hpage);
- if (!anon_vma)
- return mapping; /* NULL */
-
- /* Use first found vma */
- pgoff_start = page_to_pgoff(hpage);
- pgoff_end = pgoff_start + pages_per_huge_page(page_hstate(hpage)) - 1;
- anon_vma_interval_tree_foreach(avc, &anon_vma->rb_root,
- pgoff_start, pgoff_end) {
- struct vm_area_struct *vma = avc->vma;
-
- mapping = vma->vm_file->f_mapping;
- break;
- }
-
- anon_vma_unlock_read(anon_vma);
- return mapping;
-}
-
-/*
- * Find and lock address space (mapping) in write mode.
- *
- * Upon entry, the page is locked which allows us to find the mapping
- * even in the case of an anon page. However, locking order dictates
- * the i_mmap_rwsem be acquired BEFORE the page lock. This is hugetlbfs
- * specific. So, we first try to lock the sema while still holding the
- * page lock. If this works, great! If not, then we need to drop the
- * page lock and then acquire i_mmap_rwsem and reacquire page lock. Of
- * course, need to revalidate state along the way.
- */
-struct address_space *hugetlb_page_mapping_lock_write(struct page *hpage)
-{
- struct address_space *mapping, *mapping2;
-
- mapping = _get_hugetlb_page_mapping(hpage);
-retry:
- if (!mapping)
- return mapping;
-
- /*
- * If no contention, take lock and return
- */
- if (i_mmap_trylock_write(mapping))
- return mapping;
-
- /*
- * Must drop page lock and wait on mapping sema.
- * Note: Once page lock is dropped, mapping could become invalid.
- * As a hack, increase map count until we lock page again.
- */
- atomic_inc(&hpage->_mapcount);
- unlock_page(hpage);
- i_mmap_lock_write(mapping);
- lock_page(hpage);
- atomic_add_negative(-1, &hpage->_mapcount);
-
- /* verify page is still mapped */
- if (!page_mapped(hpage)) {
- i_mmap_unlock_write(mapping);
- return NULL;
- }
-
- /*
- * Get address space again and verify it is the same one
- * we locked. If not, drop lock and retry.
- */
- mapping2 = _get_hugetlb_page_mapping(hpage);
- if (mapping2 != mapping) {
- i_mmap_unlock_write(mapping);
- mapping = mapping2;
- goto retry;
- }
-
- return mapping;
-}
-
pgoff_t __basepage_index(struct page *page)
{
struct page *page_head = compound_head(page);
@@ -3818,7 +3718,6 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
int cow;
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;
@@ -3829,14 +3728,6 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
vma->vm_start,
vma->vm_end);
mmu_notifier_invalidate_range_start(&range);
- } else {
- /*
- * For shared mappings i_mmap_rwsem must be held to call
- * huge_pte_alloc, otherwise the returned ptep could go
- * away if part of a shared pmd and another thread calls
- * huge_pmd_unshare.
- */
- i_mmap_lock_read(mapping);
}
for (addr = vma->vm_start; addr < vma->vm_end; addr += sz) {
@@ -3914,8 +3805,6 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
if (cow)
mmu_notifier_invalidate_range_end(&range);
- else
- i_mmap_unlock_read(mapping);
return ret;
}
@@ -3959,7 +3848,7 @@ void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
continue;
ptl = huge_pte_lock(h, mm, ptep);
- if (huge_pmd_unshare(mm, vma, &address, ptep)) {
+ if (huge_pmd_unshare(mm, &address, ptep)) {
spin_unlock(ptl);
/*
* We just unmapped a page of PMDs by clearing a PUD.
@@ -4335,17 +4224,16 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
}
/*
- * We can not race with truncation due to holding i_mmap_rwsem.
- * i_size is modified when holding i_mmap_rwsem, so check here
- * once for faults beyond end of file.
+ * Use page lock to guard against racing truncation
+ * before we get page_table_lock.
*/
- size = i_size_read(mapping->host) >> huge_page_shift(h);
- if (idx >= size)
- goto out;
-
retry:
page = find_lock_page(mapping, idx);
if (!page) {
+ size = i_size_read(mapping->host) >> huge_page_shift(h);
+ if (idx >= size)
+ goto out;
+
/*
* Check for page in userfault range
*/
@@ -4365,15 +4253,13 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
};
/*
- * hugetlb_fault_mutex and i_mmap_rwsem must be
- * dropped before handling userfault. Reacquire
- * after handling fault to make calling code simpler.
+ * hugetlb_fault_mutex must be dropped before
+ * handling userfault. Reacquire after handling
+ * fault to make calling code simpler.
*/
hash = hugetlb_fault_mutex_hash(mapping, idx);
mutex_unlock(&hugetlb_fault_mutex_table[hash]);
- i_mmap_unlock_read(mapping);
ret = handle_userfault(&vmf, VM_UFFD_MISSING);
- i_mmap_lock_read(mapping);
mutex_lock(&hugetlb_fault_mutex_table[hash]);
goto out;
}
@@ -4451,6 +4337,10 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
}
ptl = huge_pte_lock(h, mm, ptep);
+ size = i_size_read(mapping->host) >> huge_page_shift(h);
+ if (idx >= size)
+ goto backout;
+
ret = 0;
if (!huge_pte_none(huge_ptep_get(ptep)))
goto backout;
@@ -4534,11 +4424,6 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
ptep = huge_pte_offset(mm, haddr, huge_page_size(h));
if (ptep) {
- /*
- * Since we hold no locks, ptep could be stale. That is
- * OK as we are only making decisions based on content and
- * not actually modifying content here.
- */
entry = huge_ptep_get(ptep);
if (unlikely(is_hugetlb_entry_migration(entry))) {
migration_entry_wait_huge(vma, mm, ptep);
@@ -4546,33 +4431,20 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
} else if (unlikely(is_hugetlb_entry_hwpoisoned(entry)))
return VM_FAULT_HWPOISON_LARGE |
VM_FAULT_SET_HINDEX(hstate_index(h));
+ } else {
+ ptep = huge_pte_alloc(mm, haddr, huge_page_size(h));
+ if (!ptep)
+ return VM_FAULT_OOM;
}
- /*
- * Acquire i_mmap_rwsem before calling huge_pte_alloc and hold
- * until finished with ptep. This serves two purposes:
- * 1) It prevents huge_pmd_unshare from being called elsewhere
- * and making the ptep no longer valid.
- * 2) It synchronizes us with i_size modifications during truncation.
- *
- * ptep could have already be assigned via huge_pte_offset. That
- * is OK, as huge_pte_alloc will return the same value unless
- * something has changed.
- */
mapping = vma->vm_file->f_mapping;
- i_mmap_lock_read(mapping);
- ptep = huge_pte_alloc(mm, haddr, huge_page_size(h));
- if (!ptep) {
- i_mmap_unlock_read(mapping);
- return VM_FAULT_OOM;
- }
+ idx = vma_hugecache_offset(h, vma, haddr);
/*
* Serialize hugepage allocation and instantiation, so that we don't
* get spurious allocation failures if two CPUs race to instantiate
* the same page in the page cache.
*/
- idx = vma_hugecache_offset(h, vma, haddr);
hash = hugetlb_fault_mutex_hash(mapping, idx);
mutex_lock(&hugetlb_fault_mutex_table[hash]);
@@ -4660,7 +4532,6 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
}
out_mutex:
mutex_unlock(&hugetlb_fault_mutex_table[hash]);
- i_mmap_unlock_read(mapping);
/*
* Generally it's safe to hold refcount during waiting page lock. But
* here we just wait to defer the next page fault to avoid busy loop and
@@ -5022,7 +4893,7 @@ unsigned long hugetlb_change_protection(struct vm_area_struct *vma,
if (!ptep)
continue;
ptl = huge_pte_lock(h, mm, ptep);
- if (huge_pmd_unshare(mm, vma, &address, ptep)) {
+ if (huge_pmd_unshare(mm, &address, ptep)) {
pages++;
spin_unlock(ptl);
shared_pmd = true;
@@ -5337,18 +5208,10 @@ void adjust_range_if_pmd_sharing_possible(struct vm_area_struct *vma,
* Search for a shareable pmd page for hugetlb. In any case calls pmd_alloc()
* and returns the corresponding pte. While this is not necessary for the
* !shared pmd case because we can allocate the pmd later as well, it makes the
- * code much cleaner.
- *
- * This routine must be called with i_mmap_rwsem held in at least read mode if
- * sharing is possible. For hugetlbfs, this prevents removal of any page
- * table entries associated with the address space. This is important as we
- * are setting up sharing based on existing page table entries (mappings).
- *
- * NOTE: This routine is only called from huge_pte_alloc. Some callers of
- * huge_pte_alloc know that sharing is not possible and do not take
- * i_mmap_rwsem as a performance optimization. This is handled by the
- * if !vma_shareable check at the beginning of the routine. i_mmap_rwsem is
- * only required for subsequent processing.
+ * code much cleaner. pmd allocation is essential for the shared case because
+ * pud has to be populated inside the same i_mmap_rwsem section - otherwise
+ * racing tasks could either miss the sharing (see huge_pte_offset) or select a
+ * bad pmd for sharing.
*/
pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
{
@@ -5365,7 +5228,7 @@ pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
if (!vma_shareable(vma, addr))
return (pte_t *)pmd_alloc(mm, pud, addr);
- i_mmap_assert_locked(mapping);
+ i_mmap_lock_read(mapping);
vma_interval_tree_foreach(svma, &mapping->i_mmap, idx, idx) {
if (svma == vma)
continue;
@@ -5395,6 +5258,7 @@ pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
spin_unlock(ptl);
out:
pte = (pte_t *)pmd_alloc(mm, pud, addr);
+ i_mmap_unlock_read(mapping);
return pte;
}
@@ -5405,19 +5269,17 @@ pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
* indicated by page_count > 1, unmap is achieved by clearing pud and
* decrementing the ref count. If count == 1, the pte page is not shared.
*
- * Called with page table lock held and i_mmap_rwsem held in write mode.
+ * called with page table lock held.
*
* returns: 1 successfully unmapped a shared pte page
* 0 the underlying pte page is not shared, or it is the last user
*/
-int huge_pmd_unshare(struct mm_struct *mm, struct vm_area_struct *vma,
- unsigned long *addr, pte_t *ptep)
+int huge_pmd_unshare(struct mm_struct *mm, unsigned long *addr, pte_t *ptep)
{
pgd_t *pgd = pgd_offset(mm, *addr);
p4d_t *p4d = p4d_offset(pgd, *addr);
pud_t *pud = pud_offset(p4d, *addr);
- i_mmap_assert_write_locked(vma->vm_file->f_mapping);
BUG_ON(page_count(virt_to_page(ptep)) == 0);
if (page_count(virt_to_page(ptep)) == 1)
return 0;
@@ -5435,8 +5297,7 @@ pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
return NULL;
}
-int huge_pmd_unshare(struct mm_struct *mm, struct vm_area_struct *vma,
- unsigned long *addr, pte_t *ptep)
+int huge_pmd_unshare(struct mm_struct *mm, unsigned long *addr, pte_t *ptep)
{
return 0;
}
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index c0bb186bba62..0c4f8cbc772e 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -992,7 +992,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
enum ttu_flags ttu = TTU_IGNORE_MLOCK | TTU_IGNORE_ACCESS;
struct address_space *mapping;
LIST_HEAD(tokill);
- bool unmap_success = true;
+ bool unmap_success;
int kill = 1, forcekill;
struct page *hpage = *hpagep;
bool mlocked = PageMlocked(hpage);
@@ -1054,32 +1054,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
if (kill)
collect_procs(hpage, &tokill, flags & MF_ACTION_REQUIRED);
- if (!PageHuge(hpage)) {
- unmap_success = try_to_unmap(hpage, ttu);
- } else {
- /*
- * For hugetlb pages, try_to_unmap could potentially call
- * huge_pmd_unshare. Because of this, take semaphore in
- * write mode here and set TTU_RMAP_LOCKED to indicate we
- * have taken the lock at this higer level.
- *
- * Note that the call to hugetlb_page_mapping_lock_write
- * is necessary even if mapping is already set. It handles
- * ugliness of potentially having to drop page lock to obtain
- * i_mmap_rwsem.
- */
- mapping = hugetlb_page_mapping_lock_write(hpage);
-
- if (mapping) {
- unmap_success = try_to_unmap(hpage,
- ttu|TTU_RMAP_LOCKED);
- i_mmap_unlock_write(mapping);
- } else {
- pr_info("Memory failure: %#lx: could not find mapping for mapped huge page\n",
- pfn);
- unmap_success = false;
- }
- }
+ unmap_success = try_to_unmap(hpage, ttu);
if (!unmap_success)
pr_err("Memory failure: %#lx: failed to unmap page (mapcount=%d)\n",
pfn, page_mapcount(hpage));
diff --git a/mm/migrate.c b/mm/migrate.c
index 5ca5842df5db..05c3c2e569df 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1280,7 +1280,6 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
int page_was_mapped = 0;
struct page *new_hpage;
struct anon_vma *anon_vma = NULL;
- struct address_space *mapping = NULL;
/*
* Migratability of hugepages depends on architectures and their size.
@@ -1328,36 +1327,18 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
goto put_anon;
if (page_mapped(hpage)) {
- /*
- * try_to_unmap could potentially call huge_pmd_unshare.
- * Because of this, take semaphore in write mode here and
- * set TTU_RMAP_LOCKED to let lower levels know we have
- * taken the lock.
- */
- mapping = hugetlb_page_mapping_lock_write(hpage);
- if (unlikely(!mapping))
- goto unlock_put_anon;
-
try_to_unmap(hpage,
- TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS|
- TTU_RMAP_LOCKED);
+ TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS);
page_was_mapped = 1;
- /*
- * Leave mapping locked until after subsequent call to
- * remove_migration_ptes()
- */
}
if (!page_mapped(hpage))
rc = move_to_new_page(new_hpage, hpage, mode);
- if (page_was_mapped) {
+ if (page_was_mapped)
remove_migration_ptes(hpage,
- rc == MIGRATEPAGE_SUCCESS ? new_hpage : hpage, true);
- i_mmap_unlock_write(mapping);
- }
+ rc == MIGRATEPAGE_SUCCESS ? new_hpage : hpage, false);
-unlock_put_anon:
unlock_page(new_hpage);
put_anon:
diff --git a/mm/rmap.c b/mm/rmap.c
index 1b84945d655c..8ef12940e357 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -22,7 +22,7 @@
*
* inode->i_mutex (while writing or truncating, not reading or faulting)
* mm->mmap_lock
- * page->flags PG_locked (lock_page) * (see huegtlbfs below)
+ * page->flags PG_locked (lock_page)
* hugetlbfs_i_mmap_rwsem_key (in huge_pmd_share)
* mapping->i_mmap_rwsem
* hugetlb_fault_mutex (hugetlbfs specific page fault mutex)
@@ -44,11 +44,6 @@
* anon_vma->rwsem,mapping->i_mutex (memory_failure, collect_procs_anon)
* ->tasklist_lock
* pte map lock
- *
- * * hugetlbfs PageHuge() pages take locks in this order:
- * mapping->i_mmap_rwsem
- * hugetlb_fault_mutex (hugetlbfs specific page fault mutex)
- * page->flags PG_locked (lock_page)
*/
#include <linux/mm.h>
@@ -1413,9 +1408,6 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
/*
* If sharing is possible, start and end will be adjusted
* accordingly.
- *
- * If called for a huge page, caller must hold i_mmap_rwsem
- * in write mode as it is possible to call huge_pmd_unshare.
*/
adjust_range_if_pmd_sharing_possible(vma, &range.start,
&range.end);
@@ -1463,13 +1455,7 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
address = pvmw.address;
if (PageHuge(page)) {
- /*
- * To call huge_pmd_unshare, i_mmap_rwsem must be
- * held in write mode. Caller needs to explicitly
- * do this outside rmap routines.
- */
- VM_BUG_ON(!(flags & TTU_RMAP_LOCKED));
- if (huge_pmd_unshare(mm, vma, &address, pvmw.pte)) {
+ if (huge_pmd_unshare(mm, &address, pvmw.pte)) {
/*
* huge_pmd_unshare unmapped an entire PMD
* page. There is no way of knowing exactly
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index 9a3d451402d7..ab9e0496d601 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -278,14 +278,10 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
BUG_ON(dst_addr >= dst_start + len);
/*
- * Serialize via i_mmap_rwsem and hugetlb_fault_mutex.
- * i_mmap_rwsem ensures the dst_pte remains valid even
- * in the case of shared pmds. fault mutex prevents
- * races with other faulting threads.
+ * Serialize via hugetlb_fault_mutex
*/
- mapping = dst_vma->vm_file->f_mapping;
- i_mmap_lock_read(mapping);
idx = linear_page_index(dst_vma, dst_addr);
+ mapping = dst_vma->vm_file->f_mapping;
hash = hugetlb_fault_mutex_hash(mapping, idx);
mutex_lock(&hugetlb_fault_mutex_table[hash]);
@@ -293,7 +289,6 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
dst_pte = huge_pte_alloc(dst_mm, dst_addr, vma_hpagesize);
if (!dst_pte) {
mutex_unlock(&hugetlb_fault_mutex_table[hash]);
- i_mmap_unlock_read(mapping);
goto out_unlock;
}
@@ -301,7 +296,6 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
dst_pteval = huge_ptep_get(dst_pte);
if (!huge_pte_none(dst_pteval)) {
mutex_unlock(&hugetlb_fault_mutex_table[hash]);
- i_mmap_unlock_read(mapping);
goto out_unlock;
}
@@ -309,7 +303,6 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
dst_addr, src_addr, &page);
mutex_unlock(&hugetlb_fault_mutex_table[hash]);
- i_mmap_unlock_read(mapping);
vm_alloc_shared = vm_shared;
cond_resched();
--
2.25.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* [RFC PATCH v2 2/4] hugetlbfs: add hinode_rwsem to hugetlb specific inode
2020-10-26 23:31 [RFC PATCH v2 0/4] hugetlbfs: introduce hinode_rwsem for pmd sharing synchronization Mike Kravetz
2020-10-26 23:31 ` [RFC PATCH v2 1/4] hugetlbfs: revert use of i_mmap_rwsem for pmd sharing and more sync Mike Kravetz
@ 2020-10-26 23:31 ` Mike Kravetz
2020-10-26 23:31 ` [RFC PATCH v2 3/4] hugetlbfs: use hinode_rwsem for pmd sharing synchronization Mike Kravetz
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Mike Kravetz @ 2020-10-26 23:31 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Hugh Dickins, Michal Hocko, Naoya Horiguchi, Aneesh Kumar K . V,
Andrea Arcangeli, Kirill A . Shutemov, Davidlohr Bueso,
Prakash Sangappa, Andrew Morton, Mike Kravetz
The hugetlb pmd sharing code needs additional synchronization. Ideally,
i_mmap_rwsem would be used for this. However, previous attempts at using
i_mmap_rwsem have failed. This is partly due to conflicts with the existing
uses of i_mmap_rwsem that force a locking order not compatible with it's
use for pmd sharing.
Introduce a rwsem (hinode_rwsem) that resides in the hugetlb specific inode
for the purpose of this synchronization. This patch adds the semaphore to
the inode and also provides routines for using the semaphore. The routines
only acquire the semaphore if pmd sharing is possible in an attempt to
minimize performance impacts. In addition, routines which can be used with
lockdep to help with proper locking are also added.
Use of the new semaphore and supporting routines will be provided in a
subsequent patch.
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
fs/hugetlbfs/inode.c | 12 ++++
include/linux/hugetlb.h | 121 ++++++++++++++++++++++++++++++++++++++++
mm/hugetlb.c | 13 -----
mm/rmap.c | 1 +
4 files changed, 134 insertions(+), 13 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 3a1246aeedc4..0460b43240b9 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -85,6 +85,17 @@ static const struct fs_parameter_spec hugetlb_fs_parameters[] = {
{}
};
+#ifdef CONFIG_ARCH_WANT_HUGE_PMD_SHARE
+static inline void init_hinode_rwsem(struct hugetlbfs_inode_info *info)
+{
+ init_rwsem(&info->hinode_rwsem);
+}
+#else
+static inline void init_hinode_rwsem(struct hugetlbfs_inode_info *info)
+{
+}
+#endif
+
#ifdef CONFIG_NUMA
static inline void hugetlb_set_vma_policy(struct vm_area_struct *vma,
struct inode *inode, pgoff_t index)
@@ -829,6 +840,7 @@ static struct inode *hugetlbfs_get_inode(struct super_block *sb,
inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
inode->i_mapping->private_data = resv_map;
info->seals = F_SEAL_SEAL;
+ init_hinode_rwsem(info);
switch (mode & S_IFMT) {
default:
init_special_inode(inode, mode, dev);
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index bf79a5601091..d3939a866855 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -414,6 +414,9 @@ struct hugetlbfs_inode_info {
struct shared_policy policy;
struct inode vfs_inode;
unsigned int seals;
+#ifdef CONFIG_ARCH_WANT_HUGE_PMD_SHARE
+ struct rw_semaphore hinode_rwsem;
+#endif
};
static inline struct hugetlbfs_inode_info *HUGETLBFS_I(struct inode *inode)
@@ -439,6 +442,101 @@ static inline struct hstate *hstate_inode(struct inode *i)
{
return HUGETLBFS_SB(i->i_sb)->hstate;
}
+
+#ifdef CONFIG_ARCH_WANT_HUGE_PMD_SHARE
+static inline bool vma_shareable(struct vm_area_struct *vma, unsigned long addr)
+{
+ unsigned long base = addr & PUD_MASK;
+ unsigned long end = base + PUD_SIZE;
+
+ /* check on proper vm_flags and page table alignment */
+ if (vma->vm_flags & VM_MAYSHARE && range_in_vma(vma, base, end))
+ return true;
+ return false;
+}
+
+/*
+ * hugetlb specific hinode_rwsem is used for pmd sharing synchronization.
+ * This routine will take the semaphore in read mode if necessary. If vma
+ * and addr are NULL, the routine will always acquire the semaphore. If
+ * values are supplied for vma and addr, they are used to determine if pmd
+ * sharing is actually possible, and only acquire the semaphore if possible.
+ * Returns true if lock was acquired, otherwise false.
+ */
+static inline bool hinode_lock_read(struct inode *inode,
+ struct vm_area_struct *vma,
+ unsigned long addr)
+{
+ if (vma && !addr)
+ addr = round_up(vma->vm_start, PUD_SIZE);
+ if (vma && !vma_shareable(vma, addr))
+ return false;
+
+ down_read(&HUGETLBFS_I(inode)->hinode_rwsem);
+ return true;
+}
+
+static inline void hinode_unlock_read(struct inode *inode)
+{
+ up_read(&HUGETLBFS_I(inode)->hinode_rwsem);
+}
+
+/*
+ * Take hinode_rwsem semaphore in write mode if necessary. See,
+ * hinode_lock_read for details.
+ * Returns true is lock was acquired, otherwise false.
+ */
+static inline bool hinode_lock_write(struct inode *inode,
+ struct vm_area_struct *vma,
+ unsigned long addr)
+{
+ if (vma && !addr)
+ addr = round_up(vma->vm_start, PUD_SIZE);
+ if (vma && !vma_shareable(vma, addr))
+ return false;
+
+ down_write(&HUGETLBFS_I(inode)->hinode_rwsem);
+ return true;
+}
+
+static inline void hinode_unlock_write(struct inode *inode)
+{
+ up_write(&HUGETLBFS_I(inode)->hinode_rwsem);
+}
+
+static inline void hinode_assert_locked(struct address_space *mapping)
+{
+ lockdep_assert_held(&HUGETLBFS_I(mapping->host)->hinode_rwsem);
+}
+
+static inline void hinode_assert_write_locked(struct address_space *mapping)
+{
+ lockdep_assert_held_write(&HUGETLBFS_I(mapping->host)->hinode_rwsem);
+}
+#else
+static inline bool hinode_lock_read(struct inode *inode,
+ struct vm_area_struct *vma,
+ unsigned long addr)
+{
+ return false;
+}
+
+static inline void hinode_unlock_read(struct inode *inode)
+{
+}
+
+static inline bool hinode_lock_write(struct inode *inode,
+ struct vm_area_struct *vma,
+ unsigned long addr)
+{
+ return false;
+}
+
+static inline void hinode_unlock_write(struct inode *inode)
+{
+}
+#endif
+
#else /* !CONFIG_HUGETLBFS */
#define is_file_hugepages(file) false
@@ -913,6 +1011,29 @@ static inline void set_huge_swap_pte_at(struct mm_struct *mm, unsigned long addr
pte_t *ptep, pte_t pte, unsigned long sz)
{
}
+
+static inline bool hinode_lock_read(struct inode *inode,
+ struct vm_area_struct *vma,
+ unsigned long addr)
+{
+ return false;
+}
+
+static inline void hinode_unlock_read(struct inode *inode)
+{
+}
+
+static inline bool hinode_lock_write(struct inode *inode,
+ struct vm_area_struct *vma,
+ unsigned long addr)
+{
+ return false;
+}
+
+static inline void hinode_unlock_write(struct inode *inode)
+{
+}
+
#endif /* CONFIG_HUGETLB_PAGE */
static inline spinlock_t *huge_pte_lock(struct hstate *h,
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 9a316b6d0b51..7b38c91f457b 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5166,19 +5166,6 @@ static unsigned long page_table_shareable(struct vm_area_struct *svma,
return saddr;
}
-static bool vma_shareable(struct vm_area_struct *vma, unsigned long addr)
-{
- unsigned long base = addr & PUD_MASK;
- unsigned long end = base + PUD_SIZE;
-
- /*
- * check on proper vm_flags and page table alignment
- */
- if (vma->vm_flags & VM_MAYSHARE && range_in_vma(vma, base, end))
- return true;
- return false;
-}
-
/*
* Determine if start,end range within vma could be mapped by shared pmd.
* If yes, adjust start and end to cover range associated with possible
diff --git a/mm/rmap.c b/mm/rmap.c
index 8ef12940e357..b30d211bd999 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -22,6 +22,7 @@
*
* inode->i_mutex (while writing or truncating, not reading or faulting)
* mm->mmap_lock
+ * hugetlbfs inode->hinode_rwsem (hugetlbfs specific)
* page->flags PG_locked (lock_page)
* hugetlbfs_i_mmap_rwsem_key (in huge_pmd_share)
* mapping->i_mmap_rwsem
--
2.25.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* [RFC PATCH v2 3/4] hugetlbfs: use hinode_rwsem for pmd sharing synchronization
2020-10-26 23:31 [RFC PATCH v2 0/4] hugetlbfs: introduce hinode_rwsem for pmd sharing synchronization Mike Kravetz
2020-10-26 23:31 ` [RFC PATCH v2 1/4] hugetlbfs: revert use of i_mmap_rwsem for pmd sharing and more sync Mike Kravetz
2020-10-26 23:31 ` [RFC PATCH v2 2/4] hugetlbfs: add hinode_rwsem to hugetlb specific inode Mike Kravetz
@ 2020-10-26 23:31 ` Mike Kravetz
2020-10-26 23:31 ` [RFC PATCH v2 4/4] huegtlbfs: handle page fault/truncate races Mike Kravetz
2020-11-03 0:28 ` [PATCH 0/4] hugetlbfs: use hinode_rwsem for pmd sharing synchronization Mike Kravetz
4 siblings, 0 replies; 11+ messages in thread
From: Mike Kravetz @ 2020-10-26 23:31 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Hugh Dickins, Michal Hocko, Naoya Horiguchi, Aneesh Kumar K . V,
Andrea Arcangeli, Kirill A . Shutemov, Davidlohr Bueso,
Prakash Sangappa, Andrew Morton, Mike Kravetz
Due to pmd sharing, the huge PTE pointer returned by huge_pte_alloc
may not be valid. This can happen if a call to huge_pmd_unshare for
the same pmd is made in another thread.
Use the hinode_rwsem to address this issue:
- hinode_rwsem is taken in read mode before calling huge_pte_alloc, and
held until finished with the returned pte pointer.
- hinode_rwsem is held in write mode whenever huge_pmd_unshare is called.
In the locking hierarchy, hinode_rwsem must be taken before a page lock.
Use the routines hinode_lock_read/write while passing vma and addr if
possible to only take the semaphore if pmd sharing is possible. Also,
add lockdep_assert calls to huge_pmd_share/unshare to help catch callers
not using the proper locking.
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
fs/hugetlbfs/inode.c | 11 +++++-
include/linux/hugetlb.h | 8 ++--
mm/hugetlb.c | 83 ++++++++++++++++++++++++++++++++++-------
mm/memory-failure.c | 15 +++++++-
mm/memory.c | 5 +++
mm/migrate.c | 15 ++++++++
mm/rmap.c | 2 +-
mm/userfaultfd.c | 14 ++++++-
8 files changed, 132 insertions(+), 21 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 0460b43240b9..084a688d9f2e 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -510,13 +510,18 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart,
* Getting here in a truncate operation is a bug.
*/
if (unlikely(page_mapped(page))) {
- BUG_ON(truncate_op);
+ bool hinode_locked;
+ BUG_ON(truncate_op);
+ hinode_locked =
+ hinode_lock_write(inode, NULL, 0UL);
i_mmap_lock_write(mapping);
hugetlb_vmdelete_list(&mapping->i_mmap,
index * pages_per_huge_page(h),
(index + 1) * pages_per_huge_page(h));
i_mmap_unlock_write(mapping);
+ if (hinode_locked)
+ hinode_unlock_write(inode);
}
lock_page(page);
@@ -573,15 +578,19 @@ static int hugetlb_vmtruncate(struct inode *inode, loff_t offset)
pgoff_t pgoff;
struct address_space *mapping = inode->i_mapping;
struct hstate *h = hstate_inode(inode);
+ bool hinode_locked;
BUG_ON(offset & ~huge_page_mask(h));
pgoff = offset >> PAGE_SHIFT;
+ hinode_locked = hinode_lock_write(inode, NULL, 0UL);
i_size_write(inode, offset);
i_mmap_lock_write(mapping);
if (!RB_EMPTY_ROOT(&mapping->i_mmap.rb_root))
hugetlb_vmdelete_list(&mapping->i_mmap, pgoff, 0);
i_mmap_unlock_write(mapping);
+ if (hinode_locked)
+ hinode_unlock_write(inode);
remove_inode_hugepages(inode, offset, LLONG_MAX);
return 0;
}
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index d3939a866855..f1d9730c8bd5 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -163,7 +163,8 @@ pte_t *huge_pte_alloc(struct mm_struct *mm,
unsigned long addr, unsigned long sz);
pte_t *huge_pte_offset(struct mm_struct *mm,
unsigned long addr, unsigned long sz);
-int huge_pmd_unshare(struct mm_struct *mm, unsigned long *addr, pte_t *ptep);
+int huge_pmd_unshare(struct mm_struct *mm, struct vm_area_struct *vma,
+ unsigned long *addr, pte_t *ptep);
void adjust_range_if_pmd_sharing_possible(struct vm_area_struct *vma,
unsigned long *start, unsigned long *end);
struct page *follow_huge_addr(struct mm_struct *mm, unsigned long address,
@@ -196,8 +197,9 @@ static inline unsigned long hugetlb_total_pages(void)
return 0;
}
-static inline int huge_pmd_unshare(struct mm_struct *mm, unsigned long *addr,
- pte_t *ptep)
+static inline int huge_pmd_unshare(struct mm_struct *mm,
+ struct vm_area_struct *vma,
+ unsigned long *addr, pte_t *ptep)
{
return 0;
}
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 7b38c91f457b..4debacb5339c 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3720,6 +3720,7 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
unsigned long sz = huge_page_size(h);
struct mmu_notifier_range range;
int ret = 0;
+ bool hinode_locked;
cow = (vma->vm_flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
@@ -3730,6 +3731,15 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
mmu_notifier_invalidate_range_start(&range);
}
+ /*
+ * For shared mappings hinode_rwsem must be held to call
+ * huge_pte_alloc, otherwise the returned ptep could go
+ * away if part of a shared pmd and another thread calls
+ * huge_pmd_unshare.
+ *
+ */
+ hinode_locked = hinode_lock_read(vma->vm_file->f_inode, vma, 0UL);
+
for (addr = vma->vm_start; addr < vma->vm_end; addr += sz) {
spinlock_t *src_ptl, *dst_ptl;
src_pte = huge_pte_offset(src, addr, sz);
@@ -3805,6 +3815,8 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
if (cow)
mmu_notifier_invalidate_range_end(&range);
+ if (hinode_locked)
+ hinode_unlock_read(vma->vm_file->f_inode);
return ret;
}
@@ -3848,7 +3860,7 @@ void __unmap_hugepage_range(struct mmu_gather *tlb, struct vm_area_struct *vma,
continue;
ptl = huge_pte_lock(h, mm, ptep);
- if (huge_pmd_unshare(mm, &address, ptep)) {
+ if (huge_pmd_unshare(mm, vma, &address, ptep)) {
spin_unlock(ptl);
/*
* We just unmapped a page of PMDs by clearing a PUD.
@@ -4200,7 +4212,8 @@ int huge_add_to_page_cache(struct page *page, struct address_space *mapping,
static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
struct vm_area_struct *vma,
struct address_space *mapping, pgoff_t idx,
- unsigned long address, pte_t *ptep, unsigned int flags)
+ unsigned long address, pte_t *ptep, unsigned int flags,
+ bool hinode_locked)
{
struct hstate *h = hstate_vma(vma);
vm_fault_t ret = VM_FAULT_SIGBUS;
@@ -4253,13 +4266,20 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
};
/*
- * hugetlb_fault_mutex must be dropped before
- * handling userfault. Reacquire after handling
+ * hugetlb_fault_mutex and inode mutex must be dropped
+ * before handling userfault. Reacquire after handling
* fault to make calling code simpler.
*/
hash = hugetlb_fault_mutex_hash(mapping, idx);
mutex_unlock(&hugetlb_fault_mutex_table[hash]);
+ if (hinode_locked)
+ hinode_unlock_read(mapping->host);
+
ret = handle_userfault(&vmf, VM_UFFD_MISSING);
+
+ if (hinode_locked)
+ (void)hinode_lock_read(mapping->host, vma,
+ address);
mutex_lock(&hugetlb_fault_mutex_table[hash]);
goto out;
}
@@ -4421,9 +4441,15 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
struct address_space *mapping;
int need_wait_lock = 0;
unsigned long haddr = address & huge_page_mask(h);
+ bool hinode_locked;
ptep = huge_pte_offset(mm, haddr, huge_page_size(h));
if (ptep) {
+ /*
+ * Since we hold no locks, ptep could be stale. That is
+ * OK as we are only making decisions based on content and
+ * not actually modifying content here.
+ */
entry = huge_ptep_get(ptep);
if (unlikely(is_hugetlb_entry_migration(entry))) {
migration_entry_wait_huge(vma, mm, ptep);
@@ -4431,26 +4457,39 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
} else if (unlikely(is_hugetlb_entry_hwpoisoned(entry)))
return VM_FAULT_HWPOISON_LARGE |
VM_FAULT_SET_HINDEX(hstate_index(h));
- } else {
- ptep = huge_pte_alloc(mm, haddr, huge_page_size(h));
- if (!ptep)
- return VM_FAULT_OOM;
}
+ /*
+ * Acquire hinode_rwsem before calling huge_pte_alloc and hold
+ * until finished with ptep. This prevents huge_pmd_unshare from
+ * being called elsewhere and making the ptep no longer valid.
+ *
+ * ptep could have already be assigned via huge_pte_offset. That
+ * is OK, as huge_pte_alloc will return the same value unless
+ * something has changed.
+ */
mapping = vma->vm_file->f_mapping;
- idx = vma_hugecache_offset(h, vma, haddr);
+ hinode_locked = hinode_lock_read(mapping->host, vma, address);
+ ptep = huge_pte_alloc(mm, haddr, huge_page_size(h));
+ if (!ptep) {
+ ret = VM_FAULT_OOM;
+ goto out_mutex;
+ }
+
/*
* Serialize hugepage allocation and instantiation, so that we don't
* get spurious allocation failures if two CPUs race to instantiate
* the same page in the page cache.
*/
+ idx = vma_hugecache_offset(h, vma, haddr);
hash = hugetlb_fault_mutex_hash(mapping, idx);
mutex_lock(&hugetlb_fault_mutex_table[hash]);
entry = huge_ptep_get(ptep);
if (huge_pte_none(entry)) {
- ret = hugetlb_no_page(mm, vma, mapping, idx, address, ptep, flags);
+ ret = hugetlb_no_page(mm, vma, mapping, idx, address, ptep,
+ flags, hinode_locked);
goto out_mutex;
}
@@ -4532,6 +4571,8 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
}
out_mutex:
mutex_unlock(&hugetlb_fault_mutex_table[hash]);
+ if (hinode_locked)
+ hinode_unlock_read(mapping->host);
/*
* Generally it's safe to hold refcount during waiting page lock. But
* here we just wait to defer the next page fault to avoid busy loop and
@@ -4872,6 +4913,7 @@ unsigned long hugetlb_change_protection(struct vm_area_struct *vma,
unsigned long pages = 0;
bool shared_pmd = false;
struct mmu_notifier_range range;
+ bool hinode_locked;
/*
* In the case of shared PMDs, the area to flush could be beyond
@@ -4886,6 +4928,8 @@ unsigned long hugetlb_change_protection(struct vm_area_struct *vma,
flush_cache_range(vma, range.start, range.end);
mmu_notifier_invalidate_range_start(&range);
+ hinode_locked = hinode_lock_write(vma->vm_file->f_inode, vma,
+ range.start);
i_mmap_lock_write(vma->vm_file->f_mapping);
for (; address < end; address += huge_page_size(h)) {
spinlock_t *ptl;
@@ -4893,7 +4937,7 @@ unsigned long hugetlb_change_protection(struct vm_area_struct *vma,
if (!ptep)
continue;
ptl = huge_pte_lock(h, mm, ptep);
- if (huge_pmd_unshare(mm, &address, ptep)) {
+ if (huge_pmd_unshare(mm, vma, &address, ptep)) {
pages++;
spin_unlock(ptl);
shared_pmd = true;
@@ -4948,6 +4992,8 @@ unsigned long hugetlb_change_protection(struct vm_area_struct *vma,
* See Documentation/vm/mmu_notifier.rst
*/
i_mmap_unlock_write(vma->vm_file->f_mapping);
+ if (hinode_locked)
+ hinode_unlock_write(vma->vm_file->f_inode);
mmu_notifier_invalidate_range_end(&range);
return pages << h->order;
@@ -5199,6 +5245,10 @@ void adjust_range_if_pmd_sharing_possible(struct vm_area_struct *vma,
* pud has to be populated inside the same i_mmap_rwsem section - otherwise
* racing tasks could either miss the sharing (see huge_pte_offset) or select a
* bad pmd for sharing.
+ *
+ * This should be called with hinode_rwsem held in read mode. Otherwise, it
+ * could race with huge_pmd_unshare and the pte_t pointer could become invalid
+ * before being returned to the caller.
*/
pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
{
@@ -5214,6 +5264,7 @@ pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
if (!vma_shareable(vma, addr))
return (pte_t *)pmd_alloc(mm, pud, addr);
+ hinode_assert_locked(mapping);
i_mmap_lock_read(mapping);
vma_interval_tree_foreach(svma, &mapping->i_mmap, idx, idx) {
@@ -5256,12 +5307,13 @@ pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
* indicated by page_count > 1, unmap is achieved by clearing pud and
* decrementing the ref count. If count == 1, the pte page is not shared.
*
- * called with page table lock held.
+ * Called with page table lock held and hinode_rwsem held in write mode.
*
* returns: 1 successfully unmapped a shared pte page
* 0 the underlying pte page is not shared, or it is the last user
*/
-int huge_pmd_unshare(struct mm_struct *mm, unsigned long *addr, pte_t *ptep)
+int huge_pmd_unshare(struct mm_struct *mm, struct vm_area_struct *vma,
+ unsigned long *addr, pte_t *ptep)
{
pgd_t *pgd = pgd_offset(mm, *addr);
p4d_t *p4d = p4d_offset(pgd, *addr);
@@ -5271,6 +5323,8 @@ int huge_pmd_unshare(struct mm_struct *mm, unsigned long *addr, pte_t *ptep)
if (page_count(virt_to_page(ptep)) == 1)
return 0;
+ hinode_assert_write_locked(vma->vm_file->f_mapping);
+
pud_clear(pud);
put_page(virt_to_page(ptep));
mm_dec_nr_pmds(mm);
@@ -5284,7 +5338,8 @@ pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
return NULL;
}
-int huge_pmd_unshare(struct mm_struct *mm, unsigned long *addr, pte_t *ptep)
+int huge_pmd_unshare(struct mm_struct *mm, struct vm_area_struct *vma,
+ unsigned long *addr, pte_t *ptep)
{
return 0;
}
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 0c4f8cbc772e..593c109a3c80 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1054,7 +1054,20 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
if (kill)
collect_procs(hpage, &tokill, flags & MF_ACTION_REQUIRED);
- unmap_success = try_to_unmap(hpage, ttu);
+ if (PageHuge(hpage) && !PageAnon(hpage) && mapping) {
+ bool hinode_locked;
+ /*
+ * For hugetlb pages, try_to_unmap could potentially call
+ * huge_pmd_unshare. Because of this, take hinode_rwsem
+ * in write mode before calling.
+ */
+ hinode_locked = hinode_lock_write(mapping->host, NULL, 0UL);
+ unmap_success = try_to_unmap(hpage, ttu);
+ if (hinode_locked)
+ hinode_unlock_write(mapping->host);
+ } else {
+ unmap_success = try_to_unmap(hpage, ttu);
+ }
if (!unmap_success)
pr_err("Memory failure: %#lx: failed to unmap page (mapcount=%d)\n",
pfn, page_mapcount(hpage));
diff --git a/mm/memory.c b/mm/memory.c
index c48f8df6e502..315d92bb68ff 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1465,9 +1465,14 @@ static void unmap_single_vma(struct mmu_gather *tlb,
* safe to do nothing in this case.
*/
if (vma->vm_file) {
+ bool hinode_locked;
+
+ hinode_locked = hinode_lock_write(vma->vm_file->f_inode, vma, 0UL);
i_mmap_lock_write(vma->vm_file->f_mapping);
__unmap_hugepage_range_final(tlb, vma, start, end, NULL);
i_mmap_unlock_write(vma->vm_file->f_mapping);
+ if (hinode_locked)
+ hinode_unlock_write(vma->vm_file->f_inode);
}
} else
unmap_page_range(tlb, vma, start, end, details);
diff --git a/mm/migrate.c b/mm/migrate.c
index 05c3c2e569df..a5685565cf1a 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1327,9 +1327,24 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
goto put_anon;
if (page_mapped(hpage)) {
+ struct address_space *mapping = NULL;
+ bool hinode_locked = false;
+
+ /*
+ * try_to_unmap could potentially call huge_pmd_unshare.
+ * Take hinode_rwsem if sharing is possible.
+ */
+ if (!PageAnon(hpage)) {
+ mapping = page_mapping(hpage);
+ if (mapping)
+ hinode_locked = hinode_lock_write(mapping->host,
+ NULL, 0UL);
+ }
try_to_unmap(hpage,
TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS);
page_was_mapped = 1;
+ if (hinode_locked)
+ hinode_unlock_write(mapping->host);
}
if (!page_mapped(hpage))
diff --git a/mm/rmap.c b/mm/rmap.c
index b30d211bd999..9e65c9c95449 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1456,7 +1456,7 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
address = pvmw.address;
if (PageHuge(page)) {
- if (huge_pmd_unshare(mm, &address, pvmw.pte)) {
+ if (huge_pmd_unshare(mm, vma, &address, pvmw.pte)) {
/*
* huge_pmd_unshare unmapped an entire PMD
* page. There is no way of knowing exactly
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index ab9e0496d601..665efb24ba93 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -220,6 +220,7 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
pgoff_t idx;
u32 hash;
struct address_space *mapping;
+ bool hinode_locked;
/*
* There is no default zero huge page for all huge page sizes as
@@ -278,10 +279,15 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
BUG_ON(dst_addr >= dst_start + len);
/*
- * Serialize via hugetlb_fault_mutex
+ * Serialize via hinode_rwsem and hugetlb_fault_mutex.
+ * hinode_rwsem ensures the dst_pte remains valid even
+ * in the case of shared pmds. fault mutex prevents
+ * races with other faulting threads.
*/
idx = linear_page_index(dst_vma, dst_addr);
mapping = dst_vma->vm_file->f_mapping;
+ hinode_locked = hinode_lock_read(mapping->host, dst_vma,
+ dst_addr);
hash = hugetlb_fault_mutex_hash(mapping, idx);
mutex_lock(&hugetlb_fault_mutex_table[hash]);
@@ -289,6 +295,8 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
dst_pte = huge_pte_alloc(dst_mm, dst_addr, vma_hpagesize);
if (!dst_pte) {
mutex_unlock(&hugetlb_fault_mutex_table[hash]);
+ if (hinode_locked)
+ hinode_unlock_read(mapping->host);
goto out_unlock;
}
@@ -296,6 +304,8 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
dst_pteval = huge_ptep_get(dst_pte);
if (!huge_pte_none(dst_pteval)) {
mutex_unlock(&hugetlb_fault_mutex_table[hash]);
+ if (hinode_locked)
+ hinode_unlock_read(mapping->host);
goto out_unlock;
}
@@ -303,6 +313,8 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
dst_addr, src_addr, &page);
mutex_unlock(&hugetlb_fault_mutex_table[hash]);
+ if (hinode_locked)
+ hinode_unlock_read(mapping->host);
vm_alloc_shared = vm_shared;
cond_resched();
--
2.25.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* [RFC PATCH v2 4/4] huegtlbfs: handle page fault/truncate races
2020-10-26 23:31 [RFC PATCH v2 0/4] hugetlbfs: introduce hinode_rwsem for pmd sharing synchronization Mike Kravetz
` (2 preceding siblings ...)
2020-10-26 23:31 ` [RFC PATCH v2 3/4] hugetlbfs: use hinode_rwsem for pmd sharing synchronization Mike Kravetz
@ 2020-10-26 23:31 ` Mike Kravetz
2020-11-03 0:28 ` [PATCH 0/4] hugetlbfs: use hinode_rwsem for pmd sharing synchronization Mike Kravetz
4 siblings, 0 replies; 11+ messages in thread
From: Mike Kravetz @ 2020-10-26 23:31 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Hugh Dickins, Michal Hocko, Naoya Horiguchi, Aneesh Kumar K . V,
Andrea Arcangeli, Kirill A . Shutemov, Davidlohr Bueso,
Prakash Sangappa, Andrew Morton, Mike Kravetz
A huegtlb page fault can race with page truncation. Make the code
identifying and handling these races more robust.
Page fault handling needs to back out pages added to page cache beyond
file size (i_size). When backing out the page, take care to restore
reserve map entries and counts as necessary.
File truncation (remove_inode_hugepages) needs to handle page mapping
changes that could have happened before locking the page. This could
happen if page was added to page cache and later backed out in fault
processing.
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
fs/hugetlbfs/inode.c | 34 ++++++++++++++++++++--------------
mm/hugetlb.c | 40 ++++++++++++++++++++++++++++++++++++++--
2 files changed, 58 insertions(+), 16 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 084a688d9f2e..b0b5be644bd9 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -526,23 +526,29 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart,
lock_page(page);
/*
- * We must free the huge page and remove from page
- * cache (remove_huge_page) BEFORE removing the
- * region/reserve map (hugetlb_unreserve_pages). In
- * rare out of memory conditions, removal of the
- * region/reserve map could fail. Correspondingly,
- * the subpool and global reserve usage count can need
- * to be adjusted.
+ * After locking page, make sure mapping is the same.
+ * We could have raced with page fault populate and
+ * backout code.
*/
- VM_BUG_ON(PagePrivate(page));
- remove_huge_page(page);
- freed++;
- if (!truncate_op) {
- if (unlikely(hugetlb_unreserve_pages(inode,
+ if (page_mapping(page) == mapping) {
+ /*
+ * We must free the huge page and remove from
+ * page cache (remove_huge_page) BEFORE
+ * removing the region/reserve map. In rare
+ * out of memory conditions, removal of the
+ * region/reserve map could fail and the
+ * subpool and global reserve usage count
+ * will need to be adjusted.
+ */
+ VM_BUG_ON(PagePrivate(page));
+ remove_huge_page(page);
+ freed++;
+ if (!truncate_op) {
+ if (unlikely(hugetlb_unreserve_pages(inode,
index, index + 1, 1)))
- hugetlb_fix_reserve_counts(inode);
+ hugetlb_fix_reserve_counts(inode);
+ }
}
-
unlock_page(page);
mutex_unlock(&hugetlb_fault_mutex_table[hash]);
}
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 4debacb5339c..325c16150a4d 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -4224,6 +4224,9 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
spinlock_t *ptl;
unsigned long haddr = address & huge_page_mask(h);
bool new_page = false;
+ bool page_cache = false;
+ bool reserve_alloc = false;
+ bool beyond_i_size = false;
/*
* Currently, we are forced to kill the process in the event the
@@ -4311,6 +4314,8 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
clear_huge_page(page, address, pages_per_huge_page(h));
__SetPageUptodate(page);
new_page = true;
+ if (PagePrivate(page))
+ reserve_alloc = true;
if (vma->vm_flags & VM_MAYSHARE) {
int err = huge_add_to_page_cache(page, mapping, idx);
@@ -4320,6 +4325,7 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
goto retry;
goto out;
}
+ page_cache = true;
} else {
lock_page(page);
if (unlikely(anon_vma_prepare(vma))) {
@@ -4358,8 +4364,10 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
ptl = huge_pte_lock(h, mm, ptep);
size = i_size_read(mapping->host) >> huge_page_shift(h);
- if (idx >= size)
+ if (idx >= size) {
+ beyond_i_size = true;
goto backout;
+ }
ret = 0;
if (!huge_pte_none(huge_ptep_get(ptep)))
@@ -4397,8 +4405,36 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
backout:
spin_unlock(ptl);
backout_unlocked:
+ if (new_page) {
+ if (page_cache && beyond_i_size) {
+ /*
+ * Back out pages added to page cache beyond i_size.
+ * Otherwise, they will 'sit' there until the file
+ * is removed.
+ */
+ ClearPageDirty(page);
+ ClearPageUptodate(page);
+ delete_from_page_cache(page);
+ }
+
+ if (reserve_alloc) {
+ /*
+ * If reserve was consumed, set PagePrivate so that
+ * it will be restored in free_huge_page().
+ */
+ SetPagePrivate(page);
+ }
+
+ if (!beyond_i_size) {
+ /*
+ * Do not restore reserve map entries beyond i_size.
+ * there will be leaks when the file is removed.
+ */
+ restore_reserve_on_error(h, vma, haddr, page);
+ }
+
+ }
unlock_page(page);
- restore_reserve_on_error(h, vma, haddr, page);
put_page(page);
goto out;
}
--
2.25.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 0/4] hugetlbfs: use hinode_rwsem for pmd sharing synchronization
2020-10-26 23:31 [RFC PATCH v2 0/4] hugetlbfs: introduce hinode_rwsem for pmd sharing synchronization Mike Kravetz
` (3 preceding siblings ...)
2020-10-26 23:31 ` [RFC PATCH v2 4/4] huegtlbfs: handle page fault/truncate races Mike Kravetz
@ 2020-11-03 0:28 ` Mike Kravetz
2020-11-03 0:28 ` [PATCH 1/4] Revert hugetlbfs: Use i_mmap_rwsem to address page fault/truncate race Mike Kravetz
` (4 more replies)
4 siblings, 5 replies; 11+ messages in thread
From: Mike Kravetz @ 2020-11-03 0:28 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Hugh Dickins, Naoya Horiguchi, Michal Hocko, Aneesh Kumar K . V,
Andrea Arcangeli, Kirill A . Shutemov, Davidlohr Bueso,
Prakash Sangappa, Andrew Morton, Mike Kravetz
The RFC series reverted all patches where i_mmap_rwsem was used for
pmd sharing synchronization, and then added code to use hinode_rwsem.
This series ends up with the same code in the end, but is structured
as follows:
- Revert the page fault/truncation race patch which depends on i_mmap_rwsem
always being taken in page fault path.
- Add hinode_rwsem to hugetlbfs specific inode and supporting routines.
- Convert code from using i_mmap_rwsem for pmd sharing synchronization
to using hinode_rwsem.
- Add code to more robustly deal with page fault/truncation races.
My hope is that this will be easier to review.
Mike Kravetz (4):
Revert hugetlbfs: Use i_mmap_rwsem to address page fault/truncate race
hugetlbfs: add hinode_rwsem to hugetlb specific inode
hugetlbfs: use hinode_rwsem for pmd sharing synchronization
huegtlbfs: handle page fault/truncate races
fs/hugetlbfs/inode.c | 105 +++++++++-------
include/linux/fs.h | 15 ---
include/linux/hugetlb.h | 129 ++++++++++++++++++--
mm/hugetlb.c | 262 ++++++++++++++++------------------------
mm/memory-failure.c | 34 ++----
mm/memory.c | 5 +
mm/migrate.c | 34 +++---
mm/rmap.c | 21 ++--
mm/userfaultfd.c | 17 ++-
9 files changed, 334 insertions(+), 288 deletions(-)
--
2.28.0
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/4] Revert hugetlbfs: Use i_mmap_rwsem to address page fault/truncate race
2020-11-03 0:28 ` [PATCH 0/4] hugetlbfs: use hinode_rwsem for pmd sharing synchronization Mike Kravetz
@ 2020-11-03 0:28 ` Mike Kravetz
2020-11-03 0:28 ` [PATCH 2/4] hugetlbfs: add hinode_rwsem to hugetlb specific inode Mike Kravetz
` (3 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Mike Kravetz @ 2020-11-03 0:28 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Hugh Dickins, Naoya Horiguchi, Michal Hocko, Aneesh Kumar K . V,
Andrea Arcangeli, Kirill A . Shutemov, Davidlohr Bueso,
Prakash Sangappa, Andrew Morton, Mike Kravetz, stable
Commit 87bf91d39bb5 ("hugetlbfs: Use i_mmap_rwsem to address page
fault/truncate race") was made possible because a prior
commit c0d0381ade79 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing
synchronization") took i_mmap_rwsem in read mode during huge page
faults. Using i_mmap_rwsem for pmd sharing synchronization has proven
problematic and will be removed in later patches. As a result, the
assumptions upon which this patch was based will no longer be true.
This reverts commit 87bf91d39bb52b688fb411d668fbe7df278b29ae
Fixes 7bf91d39bb5 ("hugetlbfs: Use i_mmap_rwsem to address page
fault/truncate race")
Cc: <stable@vger.kernel.org>
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
fs/hugetlbfs/inode.c | 28 ++++++++--------------------
mm/hugetlb.c | 23 ++++++++++++-----------
2 files changed, 20 insertions(+), 31 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index b5c109703daa..c1057378dbf4 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -444,9 +444,10 @@ hugetlb_vmdelete_list(struct rb_root_cached *root, pgoff_t start, pgoff_t end)
* In this case, we first scan the range and release found pages.
* After releasing pages, hugetlb_unreserve_pages cleans up region/reserv
* maps and global counts. Page faults can not race with truncation
- * in this routine. hugetlb_no_page() holds i_mmap_rwsem and prevents
- * page faults in the truncated range by checking i_size. i_size is
- * modified while holding i_mmap_rwsem.
+ * in this routine. hugetlb_no_page() prevents page faults in the
+ * truncated range. It checks i_size before allocation, and again after
+ * with the page table lock for the page held. The same lock must be
+ * acquired to unmap a page.
* hole punch is indicated if end is not LLONG_MAX
* In the hole punch case we scan the range and release found pages.
* Only when releasing a page is the associated region/reserv map
@@ -486,15 +487,7 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart,
index = page->index;
hash = hugetlb_fault_mutex_hash(mapping, index);
- if (!truncate_op) {
- /*
- * Only need to hold the fault mutex in the
- * hole punch case. This prevents races with
- * page faults. Races are not possible in the
- * case of truncation.
- */
- mutex_lock(&hugetlb_fault_mutex_table[hash]);
- }
+ mutex_lock(&hugetlb_fault_mutex_table[hash]);
/*
* If page is mapped, it was faulted in after being
@@ -537,8 +530,7 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart,
}
unlock_page(page);
- if (!truncate_op)
- mutex_unlock(&hugetlb_fault_mutex_table[hash]);
+ mutex_unlock(&hugetlb_fault_mutex_table[hash]);
}
huge_pagevec_release(&pvec);
cond_resched();
@@ -576,8 +568,8 @@ static int hugetlb_vmtruncate(struct inode *inode, loff_t offset)
BUG_ON(offset & ~huge_page_mask(h));
pgoff = offset >> PAGE_SHIFT;
- i_mmap_lock_write(mapping);
i_size_write(inode, offset);
+ i_mmap_lock_write(mapping);
if (!RB_EMPTY_ROOT(&mapping->i_mmap.rb_root))
hugetlb_vmdelete_list(&mapping->i_mmap, pgoff, 0);
i_mmap_unlock_write(mapping);
@@ -699,11 +691,7 @@ static long hugetlbfs_fallocate(struct file *file, int mode, loff_t offset,
/* addr is the offset within the file (zero based) */
addr = index * hpage_size;
- /*
- * fault mutex taken here, protects against fault path
- * and hole punch. inode_lock previously taken protects
- * against truncation.
- */
+ /* mutex taken here, fault path and hole punch */
hash = hugetlb_fault_mutex_hash(mapping, index);
mutex_lock(&hugetlb_fault_mutex_table[hash]);
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index fe76f8fd5a73..8a82b90ca3ee 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -4335,17 +4335,16 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
}
/*
- * We can not race with truncation due to holding i_mmap_rwsem.
- * i_size is modified when holding i_mmap_rwsem, so check here
- * once for faults beyond end of file.
+ * Use page lock to guard against racing truncation
+ * before we get page_table_lock.
*/
- size = i_size_read(mapping->host) >> huge_page_shift(h);
- if (idx >= size)
- goto out;
-
retry:
page = find_lock_page(mapping, idx);
if (!page) {
+ size = i_size_read(mapping->host) >> huge_page_shift(h);
+ if (idx >= size)
+ goto out;
+
/*
* Check for page in userfault range
*/
@@ -4451,6 +4450,10 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
}
ptl = huge_pte_lock(h, mm, ptep);
+ size = i_size_read(mapping->host) >> huge_page_shift(h);
+ if (idx >= size)
+ goto backout;
+
ret = 0;
if (!huge_pte_none(huge_ptep_get(ptep)))
goto backout;
@@ -4550,10 +4553,8 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
/*
* Acquire i_mmap_rwsem before calling huge_pte_alloc and hold
- * until finished with ptep. This serves two purposes:
- * 1) It prevents huge_pmd_unshare from being called elsewhere
- * and making the ptep no longer valid.
- * 2) It synchronizes us with i_size modifications during truncation.
+ * until finished with ptep. This prevents huge_pmd_unshare from
+ * being called elsewhere and making the ptep no longer valid.
*
* ptep could have already be assigned via huge_pte_offset. That
* is OK, as huge_pte_alloc will return the same value unless
--
2.28.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 2/4] hugetlbfs: add hinode_rwsem to hugetlb specific inode
2020-11-03 0:28 ` [PATCH 0/4] hugetlbfs: use hinode_rwsem for pmd sharing synchronization Mike Kravetz
2020-11-03 0:28 ` [PATCH 1/4] Revert hugetlbfs: Use i_mmap_rwsem to address page fault/truncate race Mike Kravetz
@ 2020-11-03 0:28 ` Mike Kravetz
2020-11-03 0:28 ` [PATCH 3/4] hugetlbfs: use hinode_rwsem for pmd sharing synchronization Mike Kravetz
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Mike Kravetz @ 2020-11-03 0:28 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Hugh Dickins, Naoya Horiguchi, Michal Hocko, Aneesh Kumar K . V,
Andrea Arcangeli, Kirill A . Shutemov, Davidlohr Bueso,
Prakash Sangappa, Andrew Morton, Mike Kravetz, stable
The hugetlb pmd sharing code needs additional synchronization. This is
because sharing established via a call huge_pte_alloc, could be undone
before control is returned to the caller. As a result, the returned
value may be invalid. Ideally, i_mmap_rwsem would be used for this type
of synchronization. However, previous attempts at using i_mmap_rwsem
have failed. This is partly due to conflicts with the existing uses
of i_mmap_rwsem that force a locking order not compatible with it's use
for pmd sharing.
Introduce a rwsem (hinode_rwsem) that resides in the hugetlb specific inode
for the purpose of pmd sharing synchronization. This patch adds the
semaphore to the inode and also provides routines for using the semaphore.
To minimize performance impacts, the routines only acquire the semaphore
if pmd sharing is possible. In addition, routines which can be used with
lockdep to help ensure proper locking are also added.
Use of the new semaphore and supporting routines will be provided in a
later patch.
Fixes: c0d0381ade79 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing
synchronization")
Cc: <stable@vger.kernel.org>
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
fs/hugetlbfs/inode.c | 12 ++++
include/linux/hugetlb.h | 121 ++++++++++++++++++++++++++++++++++++++++
mm/hugetlb.c | 13 -----
3 files changed, 133 insertions(+), 13 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index c1057378dbf4..4f1404b9f354 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -85,6 +85,17 @@ static const struct fs_parameter_spec hugetlb_fs_parameters[] = {
{}
};
+#ifdef CONFIG_ARCH_WANT_HUGE_PMD_SHARE
+static inline void init_hinode_rwsem(struct hugetlbfs_inode_info *info)
+{
+ init_rwsem(&info->hinode_rwsem);
+}
+#else
+static inline void init_hinode_rwsem(struct hugetlbfs_inode_info *info)
+{
+}
+#endif
+
#ifdef CONFIG_NUMA
static inline void hugetlb_set_vma_policy(struct vm_area_struct *vma,
struct inode *inode, pgoff_t index)
@@ -831,6 +842,7 @@ static struct inode *hugetlbfs_get_inode(struct super_block *sb,
inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
inode->i_mapping->private_data = resv_map;
info->seals = F_SEAL_SEAL;
+ init_hinode_rwsem(info);
switch (mode & S_IFMT) {
default:
init_special_inode(inode, mode, dev);
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index ebca2ef02212..c6a59c2dbc30 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -424,6 +424,9 @@ struct hugetlbfs_inode_info {
struct shared_policy policy;
struct inode vfs_inode;
unsigned int seals;
+#ifdef CONFIG_ARCH_WANT_HUGE_PMD_SHARE
+ struct rw_semaphore hinode_rwsem;
+#endif
};
static inline struct hugetlbfs_inode_info *HUGETLBFS_I(struct inode *inode)
@@ -449,6 +452,101 @@ static inline struct hstate *hstate_inode(struct inode *i)
{
return HUGETLBFS_SB(i->i_sb)->hstate;
}
+
+#ifdef CONFIG_ARCH_WANT_HUGE_PMD_SHARE
+static inline bool vma_shareable(struct vm_area_struct *vma, unsigned long addr)
+{
+ unsigned long base = addr & PUD_MASK;
+ unsigned long end = base + PUD_SIZE;
+
+ /* check on proper vm_flags and page table alignment */
+ if (vma->vm_flags & VM_MAYSHARE && range_in_vma(vma, base, end))
+ return true;
+ return false;
+}
+
+/*
+ * hugetlb specific hinode_rwsem is used for pmd sharing synchronization.
+ * This routine will take the semaphore in read mode if necessary. If vma
+ * and addr are NULL, the routine will always acquire the semaphore. If
+ * values are supplied for vma and addr, they are used to determine if pmd
+ * sharing is actually possible, and only acquire the semaphore if possible.
+ * Returns true if lock was acquired, otherwise false.
+ */
+static inline bool hinode_lock_read(struct inode *inode,
+ struct vm_area_struct *vma,
+ unsigned long addr)
+{
+ if (vma && !addr)
+ addr = round_up(vma->vm_start, PUD_SIZE);
+ if (vma && !vma_shareable(vma, addr))
+ return false;
+
+ down_read(&HUGETLBFS_I(inode)->hinode_rwsem);
+ return true;
+}
+
+static inline void hinode_unlock_read(struct inode *inode)
+{
+ up_read(&HUGETLBFS_I(inode)->hinode_rwsem);
+}
+
+/*
+ * Take hinode_rwsem semaphore in write mode if necessary. See,
+ * hinode_lock_read for details.
+ * Returns true is lock was acquired, otherwise false.
+ */
+static inline bool hinode_lock_write(struct inode *inode,
+ struct vm_area_struct *vma,
+ unsigned long addr)
+{
+ if (vma && !addr)
+ addr = round_up(vma->vm_start, PUD_SIZE);
+ if (vma && !vma_shareable(vma, addr))
+ return false;
+
+ down_write(&HUGETLBFS_I(inode)->hinode_rwsem);
+ return true;
+}
+
+static inline void hinode_unlock_write(struct inode *inode)
+{
+ up_write(&HUGETLBFS_I(inode)->hinode_rwsem);
+}
+
+static inline void hinode_assert_locked(struct address_space *mapping)
+{
+ lockdep_assert_held(&HUGETLBFS_I(mapping->host)->hinode_rwsem);
+}
+
+static inline void hinode_assert_write_locked(struct address_space *mapping)
+{
+ lockdep_assert_held_write(&HUGETLBFS_I(mapping->host)->hinode_rwsem);
+}
+#else
+static inline bool hinode_lock_read(struct inode *inode,
+ struct vm_area_struct *vma,
+ unsigned long addr)
+{
+ return false;
+}
+
+static inline void hinode_unlock_read(struct inode *inode)
+{
+}
+
+static inline bool hinode_lock_write(struct inode *inode,
+ struct vm_area_struct *vma,
+ unsigned long addr)
+{
+ return false;
+}
+
+static inline void hinode_unlock_write(struct inode *inode)
+{
+}
+#endif
+
#else /* !CONFIG_HUGETLBFS */
#define is_file_hugepages(file) false
@@ -923,6 +1021,29 @@ static inline void set_huge_swap_pte_at(struct mm_struct *mm, unsigned long addr
pte_t *ptep, pte_t pte, unsigned long sz)
{
}
+
+static inline bool hinode_lock_read(struct inode *inode,
+ struct vm_area_struct *vma,
+ unsigned long addr)
+{
+ return false;
+}
+
+static inline void hinode_unlock_read(struct inode *inode)
+{
+}
+
+static inline bool hinode_lock_write(struct inode *inode,
+ struct vm_area_struct *vma,
+ unsigned long addr)
+{
+ return false;
+}
+
+static inline void hinode_unlock_write(struct inode *inode)
+{
+}
+
#endif /* CONFIG_HUGETLB_PAGE */
static inline spinlock_t *huge_pte_lock(struct hstate *h,
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 8a82b90ca3ee..da57018926e4 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5296,19 +5296,6 @@ static unsigned long page_table_shareable(struct vm_area_struct *svma,
return saddr;
}
-static bool vma_shareable(struct vm_area_struct *vma, unsigned long addr)
-{
- unsigned long base = addr & PUD_MASK;
- unsigned long end = base + PUD_SIZE;
-
- /*
- * check on proper vm_flags and page table alignment
- */
- if (vma->vm_flags & VM_MAYSHARE && range_in_vma(vma, base, end))
- return true;
- return false;
-}
-
/*
* Determine if start,end range within vma could be mapped by shared pmd.
* If yes, adjust start and end to cover range associated with possible
--
2.28.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 3/4] hugetlbfs: use hinode_rwsem for pmd sharing synchronization
2020-11-03 0:28 ` [PATCH 0/4] hugetlbfs: use hinode_rwsem for pmd sharing synchronization Mike Kravetz
2020-11-03 0:28 ` [PATCH 1/4] Revert hugetlbfs: Use i_mmap_rwsem to address page fault/truncate race Mike Kravetz
2020-11-03 0:28 ` [PATCH 2/4] hugetlbfs: add hinode_rwsem to hugetlb specific inode Mike Kravetz
@ 2020-11-03 0:28 ` Mike Kravetz
2020-11-03 0:28 ` [PATCH 4/4] huegtlbfs: handle page fault/truncate races Mike Kravetz
2020-11-05 21:59 ` [PATCH 0/4] hugetlbfs: use hinode_rwsem for pmd sharing synchronization Mike Kravetz
4 siblings, 0 replies; 11+ messages in thread
From: Mike Kravetz @ 2020-11-03 0:28 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Hugh Dickins, Naoya Horiguchi, Michal Hocko, Aneesh Kumar K . V,
Andrea Arcangeli, Kirill A . Shutemov, Davidlohr Bueso,
Prakash Sangappa, Andrew Morton, Mike Kravetz, stable
Commit c0d0381ade79 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing
synchronization") required changes to mm locking order that are hugetlb
specific. Specifically, i_mmap_rwsem had to be taken before the page
lock. This is not a big issue in hugetlb specific code, but becomes
more problematic in the areas of page migration and memory failure where
generic mm code had to deal with this change to lock ordering. An ugly
routine 'hugetlb_page_mapping_lock_write' was added to help with these
issues.
Recently, Hugh Dickins diagnosed a migration BUG as caused by code
introduced with hugetlb i_mmap_rwsem synchronization [1]. Subsequent
discussion in that thread pointed out additional problems in the code.
In the previous patch, a rw_semaphore (hinode_rwsem) was added to the
hugetlbfs inode. Using hinode_rwsem instead of i_mmap_rwsem is actually
a 'cleaner' approach to this problem as it can be inserted in the lock
hierarchy where needed. And, there is no issue with other parts of the
mm using this rw_semaphore.
Change code to use hinode_rwsem instead of i_mmap_rwsem.
[1] https://lore.kernel.org/linux-mm/alpine.LSU.2.11.2010071833100.2214@eggly.anvils/
Fixes: c0d0381ade79 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing synchronization")
Cc: <stable@vger.kernel.org>
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
fs/hugetlbfs/inode.c | 31 +++++--
include/linux/fs.h | 15 ----
include/linux/hugetlb.h | 8 --
mm/hugetlb.c | 188 +++++++++++-----------------------------
mm/memory-failure.c | 34 +++-----
mm/memory.c | 5 ++
mm/migrate.c | 34 ++++----
mm/rmap.c | 21 ++---
mm/userfaultfd.c | 17 ++--
9 files changed, 124 insertions(+), 229 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 4f1404b9f354..bc9979382a1e 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -501,24 +501,35 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart,
mutex_lock(&hugetlb_fault_mutex_table[hash]);
/*
- * If page is mapped, it was faulted in after being
- * unmapped in caller. Unmap (again) now after taking
- * the fault mutex. The mutex will prevent faults
- * until we finish removing the page.
- *
- * This race can only happen in the hole punch case.
- * Getting here in a truncate operation is a bug.
+ * After taking fault mutex, check if page is mapped.
+ * If so, it was faulted in after being unmapped in
+ * caller.
*/
if (unlikely(page_mapped(page))) {
+ bool hinode_locked;
+
+ /*
+ * Unmap (again) now after taking the fault
+ * mutex. The mutex will prevent faults until
+ * we finish removing the page. Be sure to
+ * take locks in the correct order.
+ *
+ * This race can only happen in the hole punch
+ * case. Getting here in a truncate operation
+ * is a bug.
+ */
BUG_ON(truncate_op);
-
mutex_unlock(&hugetlb_fault_mutex_table[hash]);
+ hinode_locked =
+ hinode_lock_write(inode, NULL, 0UL);
i_mmap_lock_write(mapping);
mutex_lock(&hugetlb_fault_mutex_table[hash]);
hugetlb_vmdelete_list(&mapping->i_mmap,
index * pages_per_huge_page(h),
(index + 1) * pages_per_huge_page(h));
i_mmap_unlock_write(mapping);
+ if (hinode_locked)
+ hinode_unlock_write(inode);
}
lock_page(page);
@@ -575,15 +586,19 @@ static int hugetlb_vmtruncate(struct inode *inode, loff_t offset)
pgoff_t pgoff;
struct address_space *mapping = inode->i_mapping;
struct hstate *h = hstate_inode(inode);
+ bool hinode_locked;
BUG_ON(offset & ~huge_page_mask(h));
pgoff = offset >> PAGE_SHIFT;
+ hinode_locked = hinode_lock_write(inode, NULL, 0UL);
i_size_write(inode, offset);
i_mmap_lock_write(mapping);
if (!RB_EMPTY_ROOT(&mapping->i_mmap.rb_root))
hugetlb_vmdelete_list(&mapping->i_mmap, pgoff, 0);
i_mmap_unlock_write(mapping);
+ if (hinode_locked)
+ hinode_unlock_write(inode);
remove_inode_hugepages(inode, offset, LLONG_MAX);
return 0;
}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 21cc971fd960..8123f281c275 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -493,11 +493,6 @@ static inline void i_mmap_lock_write(struct address_space *mapping)
down_write(&mapping->i_mmap_rwsem);
}
-static inline int i_mmap_trylock_write(struct address_space *mapping)
-{
- return down_write_trylock(&mapping->i_mmap_rwsem);
-}
-
static inline void i_mmap_unlock_write(struct address_space *mapping)
{
up_write(&mapping->i_mmap_rwsem);
@@ -513,16 +508,6 @@ static inline void i_mmap_unlock_read(struct address_space *mapping)
up_read(&mapping->i_mmap_rwsem);
}
-static inline void i_mmap_assert_locked(struct address_space *mapping)
-{
- lockdep_assert_held(&mapping->i_mmap_rwsem);
-}
-
-static inline void i_mmap_assert_write_locked(struct address_space *mapping)
-{
- lockdep_assert_held_write(&mapping->i_mmap_rwsem);
-}
-
/*
* Might pages of this file be mapped into userspace?
*/
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index c6a59c2dbc30..a03475cccb77 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -154,8 +154,6 @@ u32 hugetlb_fault_mutex_hash(struct address_space *mapping, pgoff_t idx);
pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud);
-struct address_space *hugetlb_page_mapping_lock_write(struct page *hpage);
-
extern int sysctl_hugetlb_shm_group;
extern struct list_head huge_boot_pages;
@@ -199,12 +197,6 @@ static inline unsigned long hugetlb_total_pages(void)
return 0;
}
-static inline struct address_space *hugetlb_page_mapping_lock_write(
- struct page *hpage)
-{
- return NULL;
-}
-
static inline int huge_pmd_unshare(struct mm_struct *mm,
struct vm_area_struct *vma,
unsigned long *addr, pte_t *ptep)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index da57018926e4..957abc2d02ff 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1568,106 +1568,6 @@ int PageHeadHuge(struct page *page_head)
return page_head[1].compound_dtor == HUGETLB_PAGE_DTOR;
}
-/*
- * Find address_space associated with hugetlbfs page.
- * Upon entry page is locked and page 'was' mapped although mapped state
- * could change. If necessary, use anon_vma to find vma and associated
- * address space. The returned mapping may be stale, but it can not be
- * invalid as page lock (which is held) is required to destroy mapping.
- */
-static struct address_space *_get_hugetlb_page_mapping(struct page *hpage)
-{
- struct anon_vma *anon_vma;
- pgoff_t pgoff_start, pgoff_end;
- struct anon_vma_chain *avc;
- struct address_space *mapping = page_mapping(hpage);
-
- /* Simple file based mapping */
- if (mapping)
- return mapping;
-
- /*
- * Even anonymous hugetlbfs mappings are associated with an
- * underlying hugetlbfs file (see hugetlb_file_setup in mmap
- * code). Find a vma associated with the anonymous vma, and
- * use the file pointer to get address_space.
- */
- anon_vma = page_lock_anon_vma_read(hpage);
- if (!anon_vma)
- return mapping; /* NULL */
-
- /* Use first found vma */
- pgoff_start = page_to_pgoff(hpage);
- pgoff_end = pgoff_start + pages_per_huge_page(page_hstate(hpage)) - 1;
- anon_vma_interval_tree_foreach(avc, &anon_vma->rb_root,
- pgoff_start, pgoff_end) {
- struct vm_area_struct *vma = avc->vma;
-
- mapping = vma->vm_file->f_mapping;
- break;
- }
-
- anon_vma_unlock_read(anon_vma);
- return mapping;
-}
-
-/*
- * Find and lock address space (mapping) in write mode.
- *
- * Upon entry, the page is locked which allows us to find the mapping
- * even in the case of an anon page. However, locking order dictates
- * the i_mmap_rwsem be acquired BEFORE the page lock. This is hugetlbfs
- * specific. So, we first try to lock the sema while still holding the
- * page lock. If this works, great! If not, then we need to drop the
- * page lock and then acquire i_mmap_rwsem and reacquire page lock. Of
- * course, need to revalidate state along the way.
- */
-struct address_space *hugetlb_page_mapping_lock_write(struct page *hpage)
-{
- struct address_space *mapping, *mapping2;
-
- mapping = _get_hugetlb_page_mapping(hpage);
-retry:
- if (!mapping)
- return mapping;
-
- /*
- * If no contention, take lock and return
- */
- if (i_mmap_trylock_write(mapping))
- return mapping;
-
- /*
- * Must drop page lock and wait on mapping sema.
- * Note: Once page lock is dropped, mapping could become invalid.
- * As a hack, increase map count until we lock page again.
- */
- atomic_inc(&hpage->_mapcount);
- unlock_page(hpage);
- i_mmap_lock_write(mapping);
- lock_page(hpage);
- atomic_add_negative(-1, &hpage->_mapcount);
-
- /* verify page is still mapped */
- if (!page_mapped(hpage)) {
- i_mmap_unlock_write(mapping);
- return NULL;
- }
-
- /*
- * Get address space again and verify it is the same one
- * we locked. If not, drop lock and retry.
- */
- mapping2 = _get_hugetlb_page_mapping(hpage);
- if (mapping2 != mapping) {
- i_mmap_unlock_write(mapping);
- mapping = mapping2;
- goto retry;
- }
-
- return mapping;
-}
-
pgoff_t __basepage_index(struct page *page)
{
struct page *page_head = compound_head(page);
@@ -3818,9 +3718,9 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
int cow;
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;
+ bool hinode_locked;
cow = (vma->vm_flags & (VM_SHARED | VM_MAYWRITE)) == VM_MAYWRITE;
@@ -3829,16 +3729,17 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
vma->vm_start,
vma->vm_end);
mmu_notifier_invalidate_range_start(&range);
- } else {
- /*
- * For shared mappings i_mmap_rwsem must be held to call
- * huge_pte_alloc, otherwise the returned ptep could go
- * away if part of a shared pmd and another thread calls
- * huge_pmd_unshare.
- */
- i_mmap_lock_read(mapping);
}
+ /*
+ * For shared mappings hinode_rwsem must be held to call
+ * huge_pte_alloc, otherwise the returned ptep could go
+ * away if part of a shared pmd and another thread calls
+ * huge_pmd_unshare.
+ *
+ */
+ hinode_locked = hinode_lock_read(vma->vm_file->f_inode, vma, 0UL);
+
for (addr = vma->vm_start; addr < vma->vm_end; addr += sz) {
spinlock_t *src_ptl, *dst_ptl;
src_pte = huge_pte_offset(src, addr, sz);
@@ -3914,8 +3815,8 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
if (cow)
mmu_notifier_invalidate_range_end(&range);
- else
- i_mmap_unlock_read(mapping);
+ if (hinode_locked)
+ hinode_unlock_read(vma->vm_file->f_inode);
return ret;
}
@@ -4311,7 +4212,8 @@ int huge_add_to_page_cache(struct page *page, struct address_space *mapping,
static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
struct vm_area_struct *vma,
struct address_space *mapping, pgoff_t idx,
- unsigned long address, pte_t *ptep, unsigned int flags)
+ unsigned long address, pte_t *ptep, unsigned int flags,
+ bool hinode_locked)
{
struct hstate *h = hstate_vma(vma);
vm_fault_t ret = VM_FAULT_SIGBUS;
@@ -4364,15 +4266,20 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
};
/*
- * hugetlb_fault_mutex and i_mmap_rwsem must be
- * dropped before handling userfault. Reacquire
- * after handling fault to make calling code simpler.
+ * hugetlb_fault_mutex and inode mutex must be dropped
+ * before handling userfault. Reacquire after handling
+ * fault to make calling code simpler.
*/
hash = hugetlb_fault_mutex_hash(mapping, idx);
mutex_unlock(&hugetlb_fault_mutex_table[hash]);
- i_mmap_unlock_read(mapping);
+ if (hinode_locked)
+ hinode_unlock_read(mapping->host);
+
ret = handle_userfault(&vmf, VM_UFFD_MISSING);
- i_mmap_lock_read(mapping);
+
+ if (hinode_locked)
+ (void)hinode_lock_read(mapping->host, vma,
+ address);
mutex_lock(&hugetlb_fault_mutex_table[hash]);
goto out;
}
@@ -4534,6 +4441,7 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
struct address_space *mapping;
int need_wait_lock = 0;
unsigned long haddr = address & huge_page_mask(h);
+ bool hinode_locked;
ptep = huge_pte_offset(mm, haddr, huge_page_size(h));
if (ptep) {
@@ -4552,7 +4460,7 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
}
/*
- * Acquire i_mmap_rwsem before calling huge_pte_alloc and hold
+ * Acquire hinode_rwsem before calling huge_pte_alloc and hold
* until finished with ptep. This prevents huge_pmd_unshare from
* being called elsewhere and making the ptep no longer valid.
*
@@ -4561,11 +4469,11 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
* something has changed.
*/
mapping = vma->vm_file->f_mapping;
- i_mmap_lock_read(mapping);
+ hinode_locked = hinode_lock_read(mapping->host, vma, address);
ptep = huge_pte_alloc(mm, haddr, huge_page_size(h));
if (!ptep) {
- i_mmap_unlock_read(mapping);
- return VM_FAULT_OOM;
+ ret = VM_FAULT_OOM;
+ goto out_mutex;
}
/*
@@ -4579,7 +4487,8 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
entry = huge_ptep_get(ptep);
if (huge_pte_none(entry)) {
- ret = hugetlb_no_page(mm, vma, mapping, idx, address, ptep, flags);
+ ret = hugetlb_no_page(mm, vma, mapping, idx, address, ptep,
+ flags, hinode_locked);
goto out_mutex;
}
@@ -4661,7 +4570,8 @@ vm_fault_t hugetlb_fault(struct mm_struct *mm, struct vm_area_struct *vma,
}
out_mutex:
mutex_unlock(&hugetlb_fault_mutex_table[hash]);
- i_mmap_unlock_read(mapping);
+ if (hinode_locked)
+ hinode_unlock_read(mapping->host);
/*
* Generally it's safe to hold refcount during waiting page lock. But
* here we just wait to defer the next page fault to avoid busy loop and
@@ -5002,6 +4912,7 @@ unsigned long hugetlb_change_protection(struct vm_area_struct *vma,
unsigned long pages = 0;
bool shared_pmd = false;
struct mmu_notifier_range range;
+ bool hinode_locked;
/*
* In the case of shared PMDs, the area to flush could be beyond
@@ -5016,6 +4927,8 @@ unsigned long hugetlb_change_protection(struct vm_area_struct *vma,
flush_cache_range(vma, range.start, range.end);
mmu_notifier_invalidate_range_start(&range);
+ hinode_locked = hinode_lock_write(vma->vm_file->f_inode, vma,
+ range.start);
i_mmap_lock_write(vma->vm_file->f_mapping);
for (; address < end; address += huge_page_size(h)) {
spinlock_t *ptl;
@@ -5078,6 +4991,8 @@ unsigned long hugetlb_change_protection(struct vm_area_struct *vma,
* See Documentation/vm/mmu_notifier.rst
*/
i_mmap_unlock_write(vma->vm_file->f_mapping);
+ if (hinode_locked)
+ hinode_unlock_write(vma->vm_file->f_inode);
mmu_notifier_invalidate_range_end(&range);
return pages << h->order;
@@ -5327,16 +5242,11 @@ void adjust_range_if_pmd_sharing_possible(struct vm_area_struct *vma,
* !shared pmd case because we can allocate the pmd later as well, it makes the
* code much cleaner.
*
- * This routine must be called with i_mmap_rwsem held in at least read mode if
- * sharing is possible. For hugetlbfs, this prevents removal of any page
- * table entries associated with the address space. This is important as we
- * are setting up sharing based on existing page table entries (mappings).
- *
- * NOTE: This routine is only called from huge_pte_alloc. Some callers of
- * huge_pte_alloc know that sharing is not possible and do not take
- * i_mmap_rwsem as a performance optimization. This is handled by the
- * if !vma_shareable check at the beginning of the routine. i_mmap_rwsem is
- * only required for subsequent processing.
+ * This must be called with hinode_rwsem held in read mode if sharing is
+ * possible. Otherwise, it could race with huge_pmd_unshare and the pte_t
+ * pointer could become invalid before being returned to the caller. Callers
+ * should use the helper routine hinode_lock_read() which will determine if
+ * sharing is possible and acquire the rwsem if necessary.
*/
pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
{
@@ -5352,8 +5262,9 @@ pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
if (!vma_shareable(vma, addr))
return (pte_t *)pmd_alloc(mm, pud, addr);
+ hinode_assert_locked(mapping);
- i_mmap_assert_locked(mapping);
+ i_mmap_lock_read(mapping);
vma_interval_tree_foreach(svma, &mapping->i_mmap, idx, idx) {
if (svma == vma)
continue;
@@ -5383,6 +5294,7 @@ pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
spin_unlock(ptl);
out:
pte = (pte_t *)pmd_alloc(mm, pud, addr);
+ i_mmap_unlock_read(mapping);
return pte;
}
@@ -5393,7 +5305,10 @@ pte_t *huge_pmd_share(struct mm_struct *mm, unsigned long addr, pud_t *pud)
* indicated by page_count > 1, unmap is achieved by clearing pud and
* decrementing the ref count. If count == 1, the pte page is not shared.
*
- * Called with page table lock held and i_mmap_rwsem held in write mode.
+ * Called with page table lock held and hinode_rwsem held in write mode if
+ * sharing is possible. Callers should use the helper routine
+ * hinode_lock_write() which will determine if sharing is possible and acquire
+ * the rwsem if necessary.
*
* returns: 1 successfully unmapped a shared pte page
* 0 the underlying pte page is not shared, or it is the last user
@@ -5405,11 +5320,12 @@ int huge_pmd_unshare(struct mm_struct *mm, struct vm_area_struct *vma,
p4d_t *p4d = p4d_offset(pgd, *addr);
pud_t *pud = pud_offset(p4d, *addr);
- i_mmap_assert_write_locked(vma->vm_file->f_mapping);
BUG_ON(page_count(virt_to_page(ptep)) == 0);
if (page_count(virt_to_page(ptep)) == 1)
return 0;
+ hinode_assert_write_locked(vma->vm_file->f_mapping);
+
pud_clear(pud);
put_page(virt_to_page(ptep));
mm_dec_nr_pmds(mm);
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index c0bb186bba62..593c109a3c80 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -992,7 +992,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
enum ttu_flags ttu = TTU_IGNORE_MLOCK | TTU_IGNORE_ACCESS;
struct address_space *mapping;
LIST_HEAD(tokill);
- bool unmap_success = true;
+ bool unmap_success;
int kill = 1, forcekill;
struct page *hpage = *hpagep;
bool mlocked = PageMlocked(hpage);
@@ -1054,31 +1054,19 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
if (kill)
collect_procs(hpage, &tokill, flags & MF_ACTION_REQUIRED);
- if (!PageHuge(hpage)) {
- unmap_success = try_to_unmap(hpage, ttu);
- } else {
+ if (PageHuge(hpage) && !PageAnon(hpage) && mapping) {
+ bool hinode_locked;
/*
* For hugetlb pages, try_to_unmap could potentially call
- * huge_pmd_unshare. Because of this, take semaphore in
- * write mode here and set TTU_RMAP_LOCKED to indicate we
- * have taken the lock at this higer level.
- *
- * Note that the call to hugetlb_page_mapping_lock_write
- * is necessary even if mapping is already set. It handles
- * ugliness of potentially having to drop page lock to obtain
- * i_mmap_rwsem.
+ * huge_pmd_unshare. Because of this, take hinode_rwsem
+ * in write mode before calling.
*/
- mapping = hugetlb_page_mapping_lock_write(hpage);
-
- if (mapping) {
- unmap_success = try_to_unmap(hpage,
- ttu|TTU_RMAP_LOCKED);
- i_mmap_unlock_write(mapping);
- } else {
- pr_info("Memory failure: %#lx: could not find mapping for mapped huge page\n",
- pfn);
- unmap_success = false;
- }
+ hinode_locked = hinode_lock_write(mapping->host, NULL, 0UL);
+ unmap_success = try_to_unmap(hpage, ttu);
+ if (hinode_locked)
+ hinode_unlock_write(mapping->host);
+ } else {
+ unmap_success = try_to_unmap(hpage, ttu);
}
if (!unmap_success)
pr_err("Memory failure: %#lx: failed to unmap page (mapcount=%d)\n",
diff --git a/mm/memory.c b/mm/memory.c
index c48f8df6e502..315d92bb68ff 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1465,9 +1465,14 @@ static void unmap_single_vma(struct mmu_gather *tlb,
* safe to do nothing in this case.
*/
if (vma->vm_file) {
+ bool hinode_locked;
+
+ hinode_locked = hinode_lock_write(vma->vm_file->f_inode, vma, 0UL);
i_mmap_lock_write(vma->vm_file->f_mapping);
__unmap_hugepage_range_final(tlb, vma, start, end, NULL);
i_mmap_unlock_write(vma->vm_file->f_mapping);
+ if (hinode_locked)
+ hinode_unlock_write(vma->vm_file->f_inode);
}
} else
unmap_page_range(tlb, vma, start, end, details);
diff --git a/mm/migrate.c b/mm/migrate.c
index 5ca5842df5db..a5685565cf1a 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1280,7 +1280,6 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
int page_was_mapped = 0;
struct page *new_hpage;
struct anon_vma *anon_vma = NULL;
- struct address_space *mapping = NULL;
/*
* Migratability of hugepages depends on architectures and their size.
@@ -1328,36 +1327,33 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
goto put_anon;
if (page_mapped(hpage)) {
+ struct address_space *mapping = NULL;
+ bool hinode_locked = false;
+
/*
* try_to_unmap could potentially call huge_pmd_unshare.
- * Because of this, take semaphore in write mode here and
- * set TTU_RMAP_LOCKED to let lower levels know we have
- * taken the lock.
+ * Take hinode_rwsem if sharing is possible.
*/
- mapping = hugetlb_page_mapping_lock_write(hpage);
- if (unlikely(!mapping))
- goto unlock_put_anon;
-
+ if (!PageAnon(hpage)) {
+ mapping = page_mapping(hpage);
+ if (mapping)
+ hinode_locked = hinode_lock_write(mapping->host,
+ NULL, 0UL);
+ }
try_to_unmap(hpage,
- TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS|
- TTU_RMAP_LOCKED);
+ TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS);
page_was_mapped = 1;
- /*
- * Leave mapping locked until after subsequent call to
- * remove_migration_ptes()
- */
+ if (hinode_locked)
+ hinode_unlock_write(mapping->host);
}
if (!page_mapped(hpage))
rc = move_to_new_page(new_hpage, hpage, mode);
- if (page_was_mapped) {
+ if (page_was_mapped)
remove_migration_ptes(hpage,
- rc == MIGRATEPAGE_SUCCESS ? new_hpage : hpage, true);
- i_mmap_unlock_write(mapping);
- }
+ rc == MIGRATEPAGE_SUCCESS ? new_hpage : hpage, false);
-unlock_put_anon:
unlock_page(new_hpage);
put_anon:
diff --git a/mm/rmap.c b/mm/rmap.c
index 1b84945d655c..bb05ec810734 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -22,7 +22,8 @@
*
* inode->i_mutex (while writing or truncating, not reading or faulting)
* mm->mmap_lock
- * page->flags PG_locked (lock_page) * (see huegtlbfs below)
+ * hugetlbfs inode->hinode_rwsem (hugetlbfs specific, see below)
+ * page->flags PG_locked (lock_page)
* hugetlbfs_i_mmap_rwsem_key (in huge_pmd_share)
* mapping->i_mmap_rwsem
* hugetlb_fault_mutex (hugetlbfs specific page fault mutex)
@@ -45,10 +46,11 @@
* ->tasklist_lock
* pte map lock
*
- * * hugetlbfs PageHuge() pages take locks in this order:
- * mapping->i_mmap_rwsem
- * hugetlb_fault_mutex (hugetlbfs specific page fault mutex)
- * page->flags PG_locked (lock_page)
+ * hugetlbfs PageHuge() pages take locks in this order:
+ * hugetlbfs inode->hinode_rwsem
+ * mapping->i_mmap_rwsem
+ * hugetlb_fault_mutex ((hugetlbfs specific page fault mutex)
+ * page->flags PG_locked (NOT acquired with mapping->i_mmap_rwsem)
*/
#include <linux/mm.h>
@@ -1413,9 +1415,6 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
/*
* If sharing is possible, start and end will be adjusted
* accordingly.
- *
- * If called for a huge page, caller must hold i_mmap_rwsem
- * in write mode as it is possible to call huge_pmd_unshare.
*/
adjust_range_if_pmd_sharing_possible(vma, &range.start,
&range.end);
@@ -1463,12 +1462,6 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
address = pvmw.address;
if (PageHuge(page)) {
- /*
- * To call huge_pmd_unshare, i_mmap_rwsem must be
- * held in write mode. Caller needs to explicitly
- * do this outside rmap routines.
- */
- VM_BUG_ON(!(flags & TTU_RMAP_LOCKED));
if (huge_pmd_unshare(mm, vma, &address, pvmw.pte)) {
/*
* huge_pmd_unshare unmapped an entire PMD
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index 9a3d451402d7..b94101591027 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -220,6 +220,7 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
pgoff_t idx;
u32 hash;
struct address_space *mapping;
+ bool hinode_locked;
/*
* There is no default zero huge page for all huge page sizes as
@@ -278,13 +279,14 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
BUG_ON(dst_addr >= dst_start + len);
/*
- * Serialize via i_mmap_rwsem and hugetlb_fault_mutex.
- * i_mmap_rwsem ensures the dst_pte remains valid even
+ * Serialize via hinode_rwsem and hugetlb_fault_mutex.
+ * hinode_rwsem ensures the dst_pte remains valid even
* in the case of shared pmds. fault mutex prevents
* races with other faulting threads.
*/
mapping = dst_vma->vm_file->f_mapping;
- i_mmap_lock_read(mapping);
+ hinode_locked = hinode_lock_read(mapping->host, dst_vma,
+ dst_addr);
idx = linear_page_index(dst_vma, dst_addr);
hash = hugetlb_fault_mutex_hash(mapping, idx);
mutex_lock(&hugetlb_fault_mutex_table[hash]);
@@ -293,7 +295,8 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
dst_pte = huge_pte_alloc(dst_mm, dst_addr, vma_hpagesize);
if (!dst_pte) {
mutex_unlock(&hugetlb_fault_mutex_table[hash]);
- i_mmap_unlock_read(mapping);
+ if (hinode_locked)
+ hinode_unlock_read(mapping->host);
goto out_unlock;
}
@@ -301,7 +304,8 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
dst_pteval = huge_ptep_get(dst_pte);
if (!huge_pte_none(dst_pteval)) {
mutex_unlock(&hugetlb_fault_mutex_table[hash]);
- i_mmap_unlock_read(mapping);
+ if (hinode_locked)
+ hinode_unlock_read(mapping->host);
goto out_unlock;
}
@@ -309,7 +313,8 @@ static __always_inline ssize_t __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
dst_addr, src_addr, &page);
mutex_unlock(&hugetlb_fault_mutex_table[hash]);
- i_mmap_unlock_read(mapping);
+ if (hinode_locked)
+ hinode_unlock_read(mapping->host);
vm_alloc_shared = vm_shared;
cond_resched();
--
2.28.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 4/4] huegtlbfs: handle page fault/truncate races
2020-11-03 0:28 ` [PATCH 0/4] hugetlbfs: use hinode_rwsem for pmd sharing synchronization Mike Kravetz
` (2 preceding siblings ...)
2020-11-03 0:28 ` [PATCH 3/4] hugetlbfs: use hinode_rwsem for pmd sharing synchronization Mike Kravetz
@ 2020-11-03 0:28 ` Mike Kravetz
2020-11-05 21:59 ` [PATCH 0/4] hugetlbfs: use hinode_rwsem for pmd sharing synchronization Mike Kravetz
4 siblings, 0 replies; 11+ messages in thread
From: Mike Kravetz @ 2020-11-03 0:28 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Hugh Dickins, Naoya Horiguchi, Michal Hocko, Aneesh Kumar K . V,
Andrea Arcangeli, Kirill A . Shutemov, Davidlohr Bueso,
Prakash Sangappa, Andrew Morton, Mike Kravetz, stable
A huegtlb page fault can race with page truncation. Make the code
identifying and handling these races more robust.
Page fault handling needs to back out pages added to page cache beyond
file size (i_size). When backing out the page, take care to restore
reserve map entries and counts as necessary.
File truncation (remove_inode_hugepages) needs to handle page mapping
changes before locking the page. This could happen if page was added
to page cache and later backed out in fault processing.
Fixes 7bf91d39bb5 ("hugetlbfs: Use i_mmap_rwsem to address page
fault/truncate race")
Cc: <stable@vger.kernel.org>
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
fs/hugetlbfs/inode.c | 34 ++++++++++++++++++++--------------
mm/hugetlb.c | 40 ++++++++++++++++++++++++++++++++++++++--
2 files changed, 58 insertions(+), 16 deletions(-)
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index bc9979382a1e..6b975377558e 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -534,23 +534,29 @@ static void remove_inode_hugepages(struct inode *inode, loff_t lstart,
lock_page(page);
/*
- * We must free the huge page and remove from page
- * cache (remove_huge_page) BEFORE removing the
- * region/reserve map (hugetlb_unreserve_pages). In
- * rare out of memory conditions, removal of the
- * region/reserve map could fail. Correspondingly,
- * the subpool and global reserve usage count can need
- * to be adjusted.
+ * After locking page, make sure mapping is the same.
+ * We could have raced with page fault populate and
+ * backout code.
*/
- VM_BUG_ON(PagePrivate(page));
- remove_huge_page(page);
- freed++;
- if (!truncate_op) {
- if (unlikely(hugetlb_unreserve_pages(inode,
+ if (page_mapping(page) == mapping) {
+ /*
+ * We must free the huge page and remove from
+ * page cache (remove_huge_page) BEFORE
+ * removing the region/reserve map. In rare
+ * out of memory conditions, removal of the
+ * region/reserve map could fail and the
+ * subpool and global reserve usage count
+ * will need to be adjusted.
+ */
+ VM_BUG_ON(PagePrivate(page));
+ remove_huge_page(page);
+ freed++;
+ if (!truncate_op) {
+ if (unlikely(hugetlb_unreserve_pages(inode,
index, index + 1, 1)))
- hugetlb_fix_reserve_counts(inode);
+ hugetlb_fix_reserve_counts(inode);
+ }
}
-
unlock_page(page);
mutex_unlock(&hugetlb_fault_mutex_table[hash]);
}
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 957abc2d02ff..6b348d344f23 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -4224,6 +4224,9 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
spinlock_t *ptl;
unsigned long haddr = address & huge_page_mask(h);
bool new_page = false;
+ bool page_cache = false;
+ bool reserve_alloc = false;
+ bool beyond_i_size = false;
/*
* Currently, we are forced to kill the process in the event the
@@ -4311,6 +4314,8 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
clear_huge_page(page, address, pages_per_huge_page(h));
__SetPageUptodate(page);
new_page = true;
+ if (PagePrivate(page))
+ reserve_alloc = true;
if (vma->vm_flags & VM_MAYSHARE) {
int err = huge_add_to_page_cache(page, mapping, idx);
@@ -4320,6 +4325,7 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
goto retry;
goto out;
}
+ page_cache = true;
} else {
lock_page(page);
if (unlikely(anon_vma_prepare(vma))) {
@@ -4358,8 +4364,10 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
ptl = huge_pte_lock(h, mm, ptep);
size = i_size_read(mapping->host) >> huge_page_shift(h);
- if (idx >= size)
+ if (idx >= size) {
+ beyond_i_size = true;
goto backout;
+ }
ret = 0;
if (!huge_pte_none(huge_ptep_get(ptep)))
@@ -4397,8 +4405,36 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
backout:
spin_unlock(ptl);
backout_unlocked:
+ if (new_page) {
+ if (page_cache && beyond_i_size) {
+ /*
+ * Back out pages added to page cache beyond i_size.
+ * Otherwise, they will 'sit' there until the file
+ * is removed.
+ */
+ ClearPageDirty(page);
+ ClearPageUptodate(page);
+ delete_from_page_cache(page);
+ }
+
+ if (reserve_alloc) {
+ /*
+ * If reserve was consumed, set PagePrivate so that
+ * it will be restored in free_huge_page().
+ */
+ SetPagePrivate(page);
+ }
+
+ if (!beyond_i_size) {
+ /*
+ * Do not restore reserve map entries beyond i_size.
+ * there will be leaks when the file is removed.
+ */
+ restore_reserve_on_error(h, vma, haddr, page);
+ }
+
+ }
unlock_page(page);
- restore_reserve_on_error(h, vma, haddr, page);
put_page(page);
goto out;
}
--
2.28.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 0/4] hugetlbfs: use hinode_rwsem for pmd sharing synchronization
2020-11-03 0:28 ` [PATCH 0/4] hugetlbfs: use hinode_rwsem for pmd sharing synchronization Mike Kravetz
` (3 preceding siblings ...)
2020-11-03 0:28 ` [PATCH 4/4] huegtlbfs: handle page fault/truncate races Mike Kravetz
@ 2020-11-05 21:59 ` Mike Kravetz
4 siblings, 0 replies; 11+ messages in thread
From: Mike Kravetz @ 2020-11-05 21:59 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Hugh Dickins, Naoya Horiguchi, Michal Hocko, Aneesh Kumar K . V,
Andrea Arcangeli, Kirill A . Shutemov, Davidlohr Bueso,
Prakash Sangappa, Andrew Morton
On 11/2/20 4:28 PM, Mike Kravetz wrote:
> The RFC series reverted all patches where i_mmap_rwsem was used for
> pmd sharing synchronization, and then added code to use hinode_rwsem.
> This series ends up with the same code in the end, but is structured
> as follows:
>
> - Revert the page fault/truncation race patch which depends on i_mmap_rwsem
> always being taken in page fault path.
> - Add hinode_rwsem to hugetlbfs specific inode and supporting routines.
> - Convert code from using i_mmap_rwsem for pmd sharing synchronization
> to using hinode_rwsem.
> - Add code to more robustly deal with page fault/truncation races.
>
> My hope is that this will be easier to review.
My apologies. Please do not spend any time on this patch series and it
certainly is not something to be sent to stable.
I have created a patch to address the BUG and propose that for stable [1].
After further thought, the approach in this series also has lock ordering
issues. Here is a simple summary of the problem.
With pmd sharing, the pte pointer returned by huge_pte_alloc is not
guaranteed to be valid. This is because another thread could have made
a call to huge_pmd_unshare and 'unshared' the pmd page containing the
pte pointer.
The current i_mmap_rwsem locking and the inode locking proposed in this
series acquire the semaphore in read mode before calling huge_pte_alloc.
Code continues to hold the semaphore until finished with the pte pointer.
Callers of huge_pmd_unshare hold the semaphore in write mode. Thus, the
semaphore prevents the race.
The problem with this type of approach is lock ordering. The semaphore is
acquired in read mode during page fault processing. The first thing this
code does is 'allocate' a pte with huge_pte_alloc. It will then, find or
allocate a page and lock it. Finally, it will lock the page table to update
the pte. Two instances where we may need to take the semaphore in write
mode are page migration and memory failure. In these cases, the first thing
we need to do is lock the page. Only after locking the page can we locate
the semaphore which needs to be acquired in write mode. Hence we end up with
a classic cause for ABBA deadlocks.
I'm starting to think that adding more synchronization is not the best way
to approach this issue. Rather, we should always validate pte pointers after
acquiring the page table lock. At the lowest level, pmd sharing is
synchronized by the page table lock. We already do some validation of the
pte after acquiring the page table lock. For example, checking if
huge_pte_none() is still true. Before even checking for none, we would need
to lookup the pte again (huge_pte_offset) and compare to the pte we previously
acquired. If they are not the same, then we would need to backout and retry.
Unless someone has another suggestion, I'll start exploring this approach.
[1] https://lore.kernel.org/linux-mm/20201105195058.78401-1-mike.kravetz@oracle.com/
--
Mike Kravetz
^ permalink raw reply [flat|nested] 11+ messages in thread