EMM Notifier V2
Subsystems (14)
cover EMM Notifier V2
[Note that I will be giving talks next week at the OpenFabrics Forum and at the Linux Collab Summit in Austin on memory pinning etc. It would be great if I could get some feedback on the approach then]
V1->V2:
- Additional optimizations in the VM
- Convert vm spinlocks to rw sems.
- Add XPMEM driver (requires sleeping in callbacks)
- Add XPMEM example
This patch implements a simple callback for device drivers that establish their own references to pages (KVM, GRU, XPmem, RDMA/Infiniband, DMA engines etc). These references are unknown to the VM (therefore external).
With these callbacks it is possible for the device driver to release external references when the VM requests it. This enables swapping, page migration and allows support of remapping, permission changes etc etc for the externally mapped memory.
With this functionality it becomes also possible to avoid pinning or mlocking pages (commonly done to stop the VM from unmapping device mapped pages).
A device driver must subscribe to a process using
emm_register_notifier(struct emm_notifier *, struct mm_struct *)
The VM will then perform callbacks for operations that unmap or change permissions of pages in that address space. When the process terminates the callback function is called with emm_release.
Callbacks are performed before and after the unmapping action of the VM.
emm_invalidate_start before
emm_invalidate_end after
The device driver must hold off establishing new references to pages in the range specified between a callback with emm_invalidate_start and the subsequent call with emm_invalidate_end set. This allows the VM to ensure that no concurrent driver actions are performed on an address range while performing remapping or unmapping operations.
This patchset contains additional modifications needed to ensure that the callbacks can sleep. For that purpose two key locks in the vm need to be converted to rw_sems. These patches are brand new, invasive and need extensive discussion and evaluation.
The first patch alone may be applied if callbacks in atomic context are sufficient for a device driver (likely the case for KVM and GRU and simple DMA drivers).
Following the VM modifications is the XPMEM device driver that allows sharing of memory between processes running on different instances of Linux. This is also a prototype. It is known to run trivial sample programs included as the last patch.
PATCH 1/9 EMM Notifier: The notifier calls
This patch implements a simple callback for device drivers that establish their own references to pages (KVM, GRU, XPmem, RDMA/Infiniband, DMA engines etc). These references are unknown to the VM (therefore external).
With these callbacks it is possible for the device driver to release external references when the VM requests it. This enables swapping, page migration and allows support of remapping, permission changes etc etc for externally mapped memory.
With this functionality it becomes also possible to avoid pinning or mlocking pages (commonly done to stop the VM from unmapping device mapped pages).
A device driver must subscribe to a process using
emm_register_notifier(struct emm_notifier *, struct mm_struct *)
The VM will then perform callbacks for operations that unmap or change permissions of pages in that address space. When the process terminates the callback function is called with emm_release.
Callbacks are performed before and after the unmapping action of the VM.
emm_invalidate_start before
emm_invalidate_end after
The device driver must hold off establishing new references to pages in the range specified between a callback with emm_invalidate_start and the subsequent call with emm_invalidate_end set. This allows the VM to ensure that no concurrent driver actions are performed on an address range while performing remapping or unmapping operations.
Callbacks are mostly performed in a non atomic context. However, in various places spinlocks are held to traverse rmaps. So this patch here is only useful for those devices that can remove mappings in an atomic context (f.e. KVM/GRU).
If the rmap spinlocks are converted to semaphores then all callbacks will be performed in a nonatomic context. No additional changes will be necessary to this patchset.
V1->V2:
- page_referenced_one: Do not increment reference count if it is already != 0.
- Use rcu_assign_pointer and rcu_derefence_pointer instead of putting in our own barriers.
--- include/linux/mm_types.h | 3 + include/linux/rmap.h | 50 +++++++++++++++++++++++++++++ kernel/fork.c | 3 + mm/Kconfig | 5 ++ mm/filemap_xip.c | 4 ++ mm/fremap.c | 2 + mm/hugetlb.c | 3 + mm/memory.c | 42 +++++++++++++++++++----- mm/mmap.c | 3 + mm/mprotect.c | 3 + mm/mremap.c | 4 ++ mm/rmap.c | 80 +++++++++++++++++++++++++++++++++++++++++++++-- 12 files changed, 192 insertions(+), 10 deletions(-)
include/linux/mm_types.h +3 -0
@@ -225,6 +225,9 @@ struct mm_struct {#ifdef CONFIG_CGROUP_MEM_RES_CTLR struct mem_cgroup *mem_cgroup;#endif +#ifdef CONFIG_EMM_NOTIFIER +struct emm_notifier *emm_notifier; +#endif };#endif /* _LINUX_MM_TYPES_H */
mm/Kconfig +5 -0
@@ -193,3 +193,8 @@ config NR_QUICK config VIRT_TO_BUS def_bool y depends on !ARCH_NO_VIRT_TO_BUS + +config EMM_NOTIFIER + def_bool n + bool "External Mapped Memory Notifier for drivers directly mapping memory" +
include/linux/rmap.h +50 -0
56 lines not on this page · raw message
mm/rmap.c +78 -2
138 lines not on this page · raw message
kernel/fork.c +3 -0
@@ -362,6 +362,9 @@ static struct mm_struct * mm_init(structif (likely (!mm_alloc_pgd (mm))) { mm->def_flags =0 ; +#ifdef CONFIG_EMM_NOTIFIER + mm->emm_notifier =NULL ; +#endif return mm; }
mm/memory.c +34 -8
137 lines not on this page · raw message
mm/mmap.c +3 -0
@@ -1744,6 +1744,7 @@ static void unmap_region(struct mm_strucstruct mmu_gather *tlb;unsigned long nr_accounted =0 ; +emm_notify (mm, emm_invalidate_start, start, end);lru_add_drain (); tlb =tlb_gather_mmu (mm,0 );update_hiwater_rss (mm);
@@ -1752,6 +1753,7 @@ static void unmap_region(struct mm_strucfree_pgtables (&tlb, vma, prev? prev->vm_end:FIRST_USER_ADDRESS , next? next->vm_start:0 );tlb_finish_mmu (tlb, start, end); +emm_notify (mm, emm_invalidate_end, start, end); }/*
@@ -2038,6 +2040,7 @@ void exit_mmap(struct mm_struct *mm)/* mm's last user has gone, and its about to be pulled down */ arch_exit_mmap (mm); +emm_notify (mm, emm_release,0 ,TASK_SIZE );lru_add_drain ();flush_cache_mm (mm);
mm/mprotect.c +3 -0
@@ -21,6 +21,7 @@#include <linux/syscalls.h> #include <linux/swap.h> #include <linux/swapops.h> +#include <linux/rmap.h> #include <asm/uaccess.h> #include <asm/pgtable.h> #include <asm/cacheflush.h>
@@ -198,10 +199,12 @@ success: dirty_accountable =1 ; } +emm_notify (mm, emm_invalidate_start, start, end);if (is_vm_hugetlb_page (vma))hugetlb_change_protection (vma, start, end, vma->vm_page_prot);else change_protection (vma, start, end, vma->vm_page_prot, dirty_accountable); +emm_notify (mm, emm_invalidate_end, start, end);vm_stat_account (mm, oldflags, vma->vm_file, -nrpages);vm_stat_account (mm, newflags, vma->vm_file, nrpages);return 0 ;
mm/mremap.c +4 -0
@@ -18,6 +18,7 @@#include <linux/highmem.h> #include <linux/security.h> #include <linux/syscalls.h> +#include <linux/rmap.h> #include <asm/uaccess.h> #include <asm/cacheflush.h>
@@ -74,7 +75,9 @@ static void move_ptes(struct vm_area_strstruct mm_struct *mm = vma->vm_mm;pte_t *old_pte, *new_pte, pte;spinlock_t *old_ptl, *new_ptl; +unsigned long old_start = old_addr; +emm_notify (mm, emm_invalidate_start, old_start, old_end);if (vma->vm_file) {/* * Subtle point from Rajesh Venkatasubramanian: before
@@ -116,6 +119,7 @@ static void move_ptes(struct vm_area_strpte_unmap_unlock (old_pte -1 , old_ptl);if (mapping)spin_unlock (&mapping->i_mmap_lock); +emm_notify (mm, emm_invalidate_end, old_start, old_end); }#define LATENCY_LIMIT (64 * PAGE_SIZE)
mm/filemap_xip.c +4 -0
@@ -190,6 +190,8 @@ __xip_unmap (struct address_space * mapp address = vma->vm_start + ((pgoff - vma->vm_pgoff) <<PAGE_SHIFT );BUG_ON (address < vma->vm_start || address >= vma->vm_end); +emm_notify (mm, emm_invalidate_start, + address, address +PAGE_SIZE ); pte =page_check_address (page, mm, address, &ptl);if (pte) {/* Nuke the page table entry. */
@@ -201,6 +203,8 @@ __xip_unmap (struct address_space * mapppte_unmap_unlock (pte, ptl);page_cache_release (page); } +emm_notify (mm, emm_invalidate_end, + address, address +PAGE_SIZE ); }spin_unlock (&mapping->i_mmap_lock); }
mm/fremap.c +2 -0
@@ -214,7 +214,9 @@ asmlinkage long sys_remap_file_pages(unsspin_unlock (&mapping->i_mmap_lock); } +emm_notify (mm, emm_invalidate_start, start, end); err =populate_range (mm, vma, start, size, pgoff); +emm_notify (mm, emm_invalidate_end, start, end);if (!err && !(flags &MAP_NONBLOCK )) {if (unlikely (has_write_lock)) {downgrade_write (&mm->mmap_sem);
mm/hugetlb.c +3 -0
@@ -14,6 +14,7 @@#include <linux/mempolicy.h> #include <linux/cpuset.h> #include <linux/mutex.h> +#include <linux/rmap.h> #include <asm/page.h> #include <asm/pgtable.h>
@@ -799,6 +800,7 @@ void __unmap_hugepage_range(struct vm_arBUG_ON (start & ~HPAGE_MASK );BUG_ON (end & ~HPAGE_MASK ); +emm_notify (mm, emm_invalidate_start, start, end);spin_lock (&mm->page_table_lock);for (address = start; address < end; address +=HPAGE_SIZE ) { ptep =huge_pte_offset (mm, address);
@@ -819,6 +821,7 @@ void __unmap_hugepage_range(struct vm_ar }spin_unlock (&mm->page_table_lock);flush_tlb_range (vma, start, end); +emm_notify (mm, emm_invalidate_end, start, end);list_for_each_entry_safe (page, tmp, &page_list, lru) {list_del (&page->lru);put_page (page);
Signature · 5 lines
------------------------------------------------------------------------- Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
Re: [patch 1/9] EMM Notifier: The notifier calls
(Christoph, why are your CCs so often messed up?)
Quoted 29 lines · Christoph Lameter
plain text document attachment (
emm_notifier)mm/rmap.c +27
-0+/* Register a notifier */ +void emm_notifier_register (struct emm_notifier *e,struct mm_struct *mm) +{ + e->next = mm->emm_notifier; +/* +* The update to emm_notifier (e->next) must be visible +* before the pointer becomes visible. +* rcu_assign_pointer() does exactly what we need. +*/ +rcu_assign_pointer (mm->emm_notifier, e); +} +EXPORT_SYMBOL_GPL (emm_notifier_register); + +/* Perform a callback */ +int __emm_notify (struct mm_struct *mm,enum emm_operation op, +unsigned long start,unsigned long end) +{ +struct emm_notifier *e =rcu_dereference (mm)->emm_notifier; +int x; + +while (e) { + +if (e->callback) { + x = e->callback (e, mm, op, start, end); +if (x) +return x; + }
mm/rmap.c pte_t *page_check_address(struct page *p · line 325
Quoted 10 lines
mm/rmap.c +10
-0+/* +* emm_notifier contents (e) must be fetched after +* the retrival of the pointer to the notifier. +*/ + e = rcu_dereference(e)->next; + } +return 0 ; +} +EXPORT_SYMBOL_GPL (__emm_notify); +#endif
Those rcu_dereference()s are wrong. They should read:
e = rcu_dereference(mm->emm_notifier);
and
e = rcu_dereference(e->next);
Re: [patch 1/9] EMM Notifier: The notifier calls
Quoted 45 lines · Peter Zijlstra
(Christoph, why are your CCs so often messed up?)
Quoted 29 lines · Christoph Lameter
plain text document attachment (
emm_notifier)mm/rmap.c +27
-0+/* Register a notifier */ +void emm_notifier_register (struct emm_notifier *e,struct mm_struct *mm) +{ + e->next = mm->emm_notifier; +/* +* The update to emm_notifier (e->next) must be visible +* before the pointer becomes visible. +* rcu_assign_pointer() does exactly what we need. +*/ +rcu_assign_pointer (mm->emm_notifier, e); +} +EXPORT_SYMBOL_GPL (emm_notifier_register); + +/* Perform a callback */ +int __emm_notify (struct mm_struct *mm,enum emm_operation op, +unsigned long start,unsigned long end) +{ +struct emm_notifier *e =rcu_dereference (mm)->emm_notifier; +int x; + +while (e) { + +if (e->callback) { + x = e->callback (e, mm, op, start, end); +if (x) +return x; + }mm/rmap.c pte_t *page_check_address(struct page *p · line 325
Quoted 10 lines
mm/rmap.c +10
-0+/* +* emm_notifier contents (e) must be fetched after +* the retrival of the pointer to the notifier. +*/ + e = rcu_dereference(e)->next; + } +return 0 ; +} +EXPORT_SYMBOL_GPL (__emm_notify); +#endif Those rcu_dereference()s are wrong. They should read:
Quoted 5 lines
e = rcu_dereference(mm->emm_notifier);and
e = rcu_dereference(e->next);
Peter has it right. You need to rcu_dereference() the same thing that you rcu_assign_pointer() to.
Thanx, Paul
Re: [patch 1/9] EMM Notifier: The notifier calls
Quoted 2 lines · Paul E. McKenney
Peter has it right. You need to
rcu_dereference()the same thing that yourcu_assign_pointer()to.
Ah. Ok. Thanks.
EMM: Fix rcu handling and spelling
In reply to Paul E. McKenneySubject: EMM: Fix rcu handling and spelling
Fix the way rcu_dereference is done.
--- include/linux/rmap.h | 2 +- mm/rmap.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
include/linux/rmap.h +1 -1
@@ -91,7 +91,7 @@ static inline void page_dup_rmap(struct* when the VM removes references to pages. */ enum emm_operation { - emm_release,/* Process existing, */ + emm_release,/* Process exiting, */ emm_invalidate_start,/* Before the VM unmaps pages */ emm_invalidate_end,/* After the VM unmapped pages */ emm_referenced/* Check if a range was referenced */
mm/rmap.c +2 -2
@@ -303,7 +303,7 @@ EXPORT_SYMBOL_GPL(emm_notifier_register)int __emm_notify (struct mm_struct *mm,enum emm_operation op,unsigned long start,unsigned long end) { -struct emm_notifier *e =rcu_dereference (mm)->emm_notifier; +struct emm_notifier *e =rcu_dereference (mm->emm_notifier);int x;while (e) {
@@ -317,7 +317,7 @@ int __emm_notify(struct mm_struct *mm, e* emm_notifier contents (e) must be fetched after * the retrival of the pointer to the notifier. */ - e =rcu_dereference (e)->next; + e =rcu_dereference (e->next); }return 0 ; }
Paul E. McKenney
Acked-byRe: EMM: Fix rcu handling and spelling
Quoted 3 lines · Christoph Lameter
Subject: EMM: Fix rcu handling and spelling
Fix the way
rcu_dereferenceis done.
Quoted 43 lines
Signed-off-by Christoph Lameter <[email protected]>--- include/linux/rmap.h | 2 +- mm/rmap.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)Index:
linux-2.6/include/linux/rmap.h
===================================================================include/linux/rmap.h +1
-1@@ -91,7 +91,7 @@ static inline void page_dup_rmap(struct* when the VM removes references to pages. */ enum emm_operation { - emm_release,/* Process existing, */ + emm_release,/* Process exiting, */ emm_invalidate_start,/* Before the VM unmaps pages */ emm_invalidate_end,/* After the VM unmapped pages */ emm_referenced/* Check if a range was referenced */ Index:
linux-2.6/mm/rmap.c
===================================================================mm/rmap.c +2
-2@@ -303,7 +303,7 @@ EXPORT_SYMBOL_GPL(emm_notifier_register)int __emm_notify (struct mm_struct *mm,enum emm_operation op,unsigned long start,unsigned long end) { -struct emm_notifier *e =rcu_dereference (mm)->emm_notifier; +struct emm_notifier *e =rcu_dereference (mm->emm_notifier);int x;while (e) {@@ -317,7 +317,7 @@ int __emm_notify(struct mm_struct *mm, e* emm_notifier contents (e) must be fetched after * the retrival of the pointer to the notifier. */ - e =rcu_dereference (e)->next; + e =rcu_dereference (e->next); }return 0 ; }
Re: [patch 1/9] EMM Notifier: The notifier calls
In reply to Christoph Lametermm/rmap.c pte_t *page_check_address(struct page *p · line 314
Quoted 13 lines · Christoph Lameter
mm/rmap.c +13
-0+/* Perform a callback */ +int __emm_notify (struct mm_struct *mm,enum emm_operation op, +unsigned long start,unsigned long end) +{ +struct emm_notifier *e =rcu_dereference (mm)->emm_notifier; +int x; + +while (e) { + +if (e->callback) { + x = e->callback (e, mm, op, start, end); +if (x) +return x;
There are much bigger issues besides the rcu safety in this patch, proper aging of the secondary mmu through access bits set by hardware is unfixable with this model (you would need to do age |= e->callback), which is the proof of why this isn't flexibile enough by forcing the same parameter and retvals for all methods. No idea why you go for such inferior solution that will never get the aging right and will likely fall apart if we add more methods in the future.
For example the "switch" you have to add in xpmem_emm_notifier_callback doesn't look good, at least gcc may be able to optimize it with an array indexing simulating proper pointer to function like in #v9.
Most other patches will apply cleanly on top of my coming mmu notifiers #v10 that I hope will go in -mm.
For #v10 the only two left open issues to discuss are:
1) the moment you remove rcu_read_lock from the methods (my #v9 had rcu_read_lock so synchronize_rcu() in Jack's patch was working with my #v9) GRU has no way to ensure the methods will fire immediately after registering. To fix this race after removing the rcu_read_lock (to prepare for the later patches that allows the VM to schedule when the mmu notifiers methods are invoked) I can replace rcu_read_lock with seqlock locking in the same way as I did in a previous patch posted here (seqlock_write around the registration method, and seqlock_read replying all callbacks if the race happened). then synchronize_rcu become unnecessary and the methods will be correctly replied allowing GRU not to corrupt memory after the registration method. EMM would also need a fix like this for GRU to be safe on top of EMM.
Another less obviously safe approach is to allow the register method to succeed only when mm_users=1 and the task is single threaded. This way if all the places where the mmu notifers aren't invoked on the mm not by the current task, are only doing invalidates after/before zapping ptes, if the istantiation of new ptes is single threaded too, we shouldn't worry if we miss an invalidate for a pte that is zero and doesn't point to any physical page. In the places where current->mm != mm I'm using invalidate_page 99% of the time, and that only follows the ptep_clear_flush. The problem are the range_begin that will happen before zapping the pte in places where current->mm != mm. Unfortunately in my incremental patch where I move all invalidate_page outside of the PT lock to prepare for allowing sleeping inside the mmu notifiers, I used range_begin/end in places like try_to_unmap_cluster where current->mm != mm. In general this solution looks more fragile than the seqlock.
2) I'm uncertain how the driver can handle a range_end called before range_begin. Also multiple range_begin can happen in parallel later followed by range_end, so if there's a global seqlock that serializes the secondary mmu page fault, that will screwup (you can't seqlock_write in range_begin and sequnlock_write in range_end). The write side of the seqlock must be serialized and calling seqlock_write twice in a row before any sequnlock operation will break.
A recursive rwsem taken in range_begin and released in range_end seems to be the only way to stop the secondary mmu page faults.
If I would remove all range_begin/end in places where current->mm != mm, then I could as well bail out in mmu_notifier_register if use mm_users != 1 to solve problem 2 too.
My solution to this is that I believe the driver is safe if the range_end is being missed if range_end is followed by an invalidate event like in invalidate_range_end, so the driver is ok to just have a static value that accounts if range_begin has ever happened and it will just return from range_end without doing anything if no range_begin ever happened.
Notably I'll be trying to use range_begin in KVM too so I got to deal with 2) too. For Nick: the reason for using range_begin is supposedly an optimization: to guarantee that the last free of the page will happen outside the mmu_lock, so KVM internally to the mmu_lock is free to do:
spin_lock(kvm->mmu_lock)
put_page()
spte = nonpresent
flush secondary tlb()
spin_unlock(kvm->mmu_lock)
The above ordering is unsafe if the page could ever reach the freelist before the tlb flush happened. The range_begin will take the mmu_lock and will hold off kvm new page faults to allow kvm to free as many page it wants, invalidate all ptes and only at the end do a single tlb flush, while still being allowed to madvise(don't need) or munmap parts of the memory mapped by sptes. It's uncertain if the ordering should be changed to be robust against put_page putting the page in the freelist immediately, instead of using range_begin to serialize against the page going out of ptes immediately after put_page is called. If we go for a range_end-only usage of the mmu notifiers kvm will need some reordering and zapping a large number of ptes will require multiple tlb flushes as the pages have to be pointed by an array and the array is of limited size (the size of the array decides the frequency of the tlb flushes). The suggested usage of range_begin allows to do a single tlb flush for an unlimited number of sptes being zapped.
Re: [patch 1/9] EMM Notifier: The notifier calls
Quoted 4 lines · Andrea Arcangeli
Most other patches will apply cleanly on top of my coming mmu notifiers #v10 that I hope will go in -mm.
For #v10 the only two left open issues to discuss are:
Does your v10 allow sleeping inside the callbacks?
Thanks,
Robin
Re: [patch 1/9] EMM Notifier: The notifier calls
Quoted 7 lines · Robin Holt
Quoted 4 lines · Andrea Arcangeli
Most other patches will apply cleanly on top of my coming mmu notifiers #v10 that I hope will go in -mm.
For #v10 the only two left open issues to discuss are:
Does your v10 allow sleeping inside the callbacks?
Yes if you apply all the patches. But not if you apply the first patch only, most patches in EMM serie will apply cleanly or with minor rejects to #v10 too, Christoph's further work to make EEM sleep capable looks very good and it's going to be 100% shared, it's also going to be a lot more controversial for merging than the two #v10 or EMM first patch. EMM also doesn't allow sleeping inside the callbacks if you only apply the first patch in the serie.
My priority is to get #v9 or the coming #v10 merged in -mm (only difference will be the replacement of rcu_read_lock with the seqlock to avoid breaking the synchronize_rcu in GRU code). I will mix seqlock with rcu ordered writes. EMM indeed breaks GRU by making synchronize_rcu a noop and by not providing any alternative (I will obsolete synchronize_rcu making it a noop instead). This assumes Jack used synchronize_rcu for whatever good reason. But this isn't the real strong point against EMM, adding seqlock to EMM is as easy as adding it to #v10 (admittedly with #v10 is a bit easier because I didn't expand the hlist operations for zero gain like in EMM).
-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
Re: [patch 1/9] EMM Notifier: The notifier calls
I must have missed v10. Could you repost so I can build xpmem against it to see how it operates? To help reduce confusion, you should probably comandeer the patches from Christoph's set which you think are needed to make it sleep.
Thanks,
Robin
Quoted 32 lines · Andrea Arcangeli
Quoted 7 lines · Robin Holt
Quoted 4 lines · Andrea Arcangeli
Most other patches will apply cleanly on top of my coming mmu notifiers #v10 that I hope will go in -mm.
For #v10 the only two left open issues to discuss are:
Does your v10 allow sleeping inside the callbacks?
Yes if you apply all the patches. But not if you apply the first patch only, most patches in EMM serie will apply cleanly or with minor rejects to #v10 too, Christoph's further work to make EEM sleep capable looks very good and it's going to be 100% shared, it's also going to be a lot more controversial for merging than the two #v10 or EMM first patch. EMM also doesn't allow sleeping inside the callbacks if you only apply the first patch in the serie.
My priority is to get #v9 or the coming #v10 merged in -mm (only difference will be the replacement of
rcu_read_lockwith the seqlock to avoid breaking thesynchronize_rcuin GRU code). I will mix seqlock with rcu ordered writes. EMM indeed breaks GRU by makingsynchronize_rcua noop and by not providing any alternative (I will obsoletesynchronize_rcumaking it a noop instead). This assumes Jack usedsynchronize_rcufor whatever good reason. But this isn't the real strong point against EMM, adding seqlock to EMM is as easy as adding it to #v10 (admittedly with #v10 is a bit easier because I didn't expand the hlist operations for zero gain like in EMM).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 1/9] EMM Notifier: The notifier calls
In reply to Andrea ArcangeliQuoted 7 lines · Andrea Arcangeli
There are much bigger issues besides the rcu safety in this patch, proper aging of the secondary mmu through access bits set by hardware is unfixable with this model (you would need to do age |=
e->callback), which is the proof of why this isn't flexibile enough by forcing the same parameter and retvals for all methods. No idea why you go for such inferior solution that will never get the aging right and will likely fall apart if we add more methods in the future.
There is always the possibility to add special functions in the same way as done in the mmu notifier series if it really becomes necessary. EMM does in no way preclude that.
Here f.e. We can add a special emm_age() function that iterates differently and does the | for you.
Quoted 4 lines
For example the "switch" you have to add in
xpmem_emm_notifier_callbackdoesn't look good, at least gcc may be able to optimize it with an array indexing simulating proper pointer to function like in #v9.
Actually the switch looks really good because it allows code to run for all callbacks like f.e. xpmem_tg_ref(). Otherwise the refcounting code would have to be added to each callback.
Quoted 5 lines
Most other patches will apply cleanly on top of my coming mmu notifiers #v10 that I hope will go in -mm.
For #v10 the only two left open issues to discuss are:
Did I see #v10? Could you start a new subject when you post please? Do not respond to some old message otherwise the threading will be wrong.
Quoted 3 lines
methods will be correctly replied allowing GRU not to corrupt memory after the registration method. EMM would also need a fix like this for GRU to be safe on top of EMM.
How exactly does the GRU corrupt memory?
Quoted 8 lines
Another less obviously safe approach is to allow the register method to succeed only when mm_users=1 and the task is single threaded. This way if all the places where the mmu notifers aren't invoked on the mm not by the current task, are only doing invalidates after/before zapping ptes, if the istantiation of new ptes is single threaded too, we shouldn't worry if we miss an invalidate for a pte that is zero and doesn't point to any physical page. In the places where
current->mm!= mm I'm using
Quoted 8 lines
invalidate_page99% of the time, and that only follows theptep_clear_flush. The problem are therange_beginthat will happen before zapping the pte in places wherecurrent->mm!= mm. Unfortunately in my incremental patch where I move allinvalidate_pageoutside of the PT lock to prepare for allowing sleeping inside the mmu notifiers, I used range_begin/end in places liketry_to_unmap_clusterwherecurrent->mm!= mm. In general this solution looks more fragile than the seqlock.
Hmmm... Okay that is one solution that would just require a BUG_ON in the registration methods.
Quoted 8 lines
2) I'm uncertain how the driver can handle a
range_endcalled beforerange_begin. Also multiplerange_begincan happen in parallel later followed byrange_end, so if there's a global seqlock that serializes the secondary mmu page fault, that will screwup (you can'tseqlock_writeinrange_beginandsequnlock_writeinrange_end). The write side of the seqlock must be serialized and callingseqlock_writetwice in a row before any sequnlock operation will break.
Well doesnt the requirement of just one execution thread also deal with that issue?
EMM: Fixup return value handling of emm_notify()
Quoted 2 lines · Christoph Lameter
Here f.e. We can add a special
emm_age()function that iterates differently and does the | for you.
Well maybe not really necessary. How about this fix? Its likely a problem to stop callbacks if one callback returned an error.
Subject: EMM: Fixup return value handling of emm_notify()
Right now we stop calling additional subsystems if one callback returned an error. That has the potential for causing additional trouble with the subsystems that do not receive the callbacks they expect if one has failed.
So change the handling of error code to continue callbacks to other subsystems but return the first error code encountered.
If a callback returns a positive return value then add up all the value from all the calls. That can be used to establish how many references exist (xpmem may want this feature at some point) or ensure that the aging works the way Andrea wants it to (KVM, XPmem so far do not care too much).
--- mm/rmap.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-)
mm/rmap.c +23 -5
@@ -299,27 +299,45 @@ void emm_notifier_register(struct emm_no }EXPORT_SYMBOL_GPL (emm_notifier_register); -/* Perform a callback */ +/* +* Perform a callback +* +* The return of this function is either a negative error of the first +* callback that failed or a consolidated count of all the positive +* values that were returned by the callbacks. +*/ int __emm_notify (struct mm_struct *mm,enum emm_operation op,unsigned long start,unsigned long end) {struct emm_notifier *e =rcu_dereference (mm->emm_notifier);int x; +int result =0 ;while (e) { -if (e->callback) { x = e->callback (e, mm, op, start, end); -if (x) -return x; + +/* +* Callback may return a positive value to indicate a count +* or a negative error code. We keep the first error code +* but continue to perform callbacks to other subscribed +* subsystems. +*/ +if (x && result >=0 ) { +if (x >=0 ) + result += x; +else + result = x; + } } +/* * emm_notifier contents (e) must be fetched after * the retrival of the pointer to the notifier. */ e =rcu_dereference (e->next); } -return 0 ; +return result; }EXPORT_SYMBOL_GPL (__emm_notify);#endif
Re: EMM: Fixup return value handling of emm_notify()
Quoted 4 lines · Christoph Lameter
mm/rmap.c +4
-0+/* +* Callback may return a positive value to indicate a count +* or a negative error code. We keep the first error code +* but continue to perform callbacks to other subscribed
mm/rmap.c void emm_notifier_register(struct emm_no · line 333
Quoted 10 lines
mm/rmap.c +9
-0+* subsystems. +*/ +if (x && result >=0 ) { +if (x >=0 ) + result += x; +else + result = x; + } } +
Now think of when one of the kernel janitors will micro-optimize PG_dirty to be returned by invalidate_page so a single set_page_dirty will be invoked... Keep in mind this is a kernel internal APIs, ask Greg if we can change it in order to optimize later in the future. I think my #v9 is optimal enough while being simple at the same time, but anyway it's silly to be hardwired to such an interface that worst of all requires switch statements instead of proper pointer to functions and a fixed set of parameters and retval semantics for all methods.
Re: EMM: Fixup return value handling of emm_notify()
Quoted 4 lines · Andrea Arcangeli
but anyway it's silly to be hardwired to such an interface that worst of all requires switch statements instead of proper pointer to functions and a fixed set of parameters and retval semantics for all methods.
The EMM API with a single callback is the simplest approach at this point. A common callback for all operations allows the driver to implement common entry and exit code as seen in XPMem.
I guess we can complicate this more by switching to a different API or adding additional emm_xxx() callback if need be but I really want to have a strong case for why this would be needed. There is the danger of adding frills with special callbacks in this and that situation that could make the notifier complicated and specific to a certain usage scenario.
Having this generic simple interface will hopefully avoid such things.
Re: EMM: Fixup return value handling of emm_notify()
Quoted 10 lines · Christoph Lameter
Quoted 4 lines · Andrea Arcangeli
but anyway it's silly to be hardwired to such an interface that worst of all requires switch statements instead of proper pointer to functions and a fixed set of parameters and retval semantics for all methods.
The EMM API with a single callback is the simplest approach at this point. A common callback for all operations allows the driver to implement common entry and exit code as seen in XPMem.
It seems to me that common code can be shared using functions? No need to stuff everything into a single function. We have method vectors all over the kernel, we could do a_ops as a single callback too, but we dont.
FWIW I prefer separate methods.
Quoted 9 lines
I guess we can complicate this more by switching to a different API or adding additional
emm_xxx()callback if need be but I really want to have a strong case for why this would be needed. There is the danger of adding frills with special callbacks in this and that situation that could make the notifier complicated and specific to a certain usage scenario.Having this generic simple interface will hopefully avoid such things.
Re: EMM: Fixup return value handling of emm_notify()
Quoted 2 lines · Peter Zijlstra
It seems to me that common code can be shared using functions? No need FWIW I prefer separate methods.
kvm patch using mmu notifiers shares 99% of the code too between the two different methods implemented indeed. Code sharing is the same and if something pointer to functions will be faster if gcc isn't smart or can't create a compile time hash to jump into the right address without having to check every case: .
Re: EMM: Fixup return value handling of emm_notify()
In reply to Peter ZijlstraQuoted 6 lines · Peter Zijlstra
It seems to me that common code can be shared using functions? No need to stuff everything into a single function. We have method vectors all over the kernel, we could do
a_opsas a single callback too, but we dont.FWIW I prefer separate methods.
Ok. It seems that I already added some new methods which do not use all parameters. So lets switch back to the old scheme for the next release.
EMM: Require single threadedness for registration.
In reply to Christoph LameterHere is a patch to require single threaded execution during emm_register. This also allows an easy implementation of an unregister function and gets rid of the races that Andrea worried about.
The approach here is similar to what was used in selinux for security context changes (see selinux_setprocattr).
Is it okay for the users of emm to require single threadedness for registration?
Subject: EMM: Require single threaded execution for register and unregister
We can avoid the concurrency issues arising at registration if we only allow registration of notifiers when the process has only a single thread. That even allows to avoid the use of rcu.
--- mm/rmap.c | 46 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 9 deletions(-)
mm/rmap.c +37 -9
@@ -286,20 +286,48 @@ void emm_notifier_release(struct mm_stru } } -/* Register a notifier */ +/* +* Register a notifier +* +* mmap_sem is held writably. +* +* Process must be single threaded. +*/ void emm_notifier_register (struct emm_notifier *e,struct mm_struct *mm) { +BUG_ON (atomic_read (&mm->mm_users) !=1 ); + e->next = mm->emm_notifier; -/* -* The update to emm_notifier (e->next) must be visible -* before the pointer becomes visible. -* rcu_assign_pointer() does exactly what we need. -*/ -rcu_assign_pointer (mm->emm_notifier, e); + mm->emm_notifier = e; }EXPORT_SYMBOL_GPL (emm_notifier_register);/* +* Unregister a notifier +* +* mmap_sem is held writably +* +* Process must be single threaded +*/ +void emm_notifier_unregister (struct emm_notifier *e,struct mm_struct *mm) +{ +struct emm_notifier *p = mm->emm_notifier; + +BUG_ON (atomic_read (&mm->mm_users) !=1 ); + +if (e == p) + mm->emm_notifier = e->next; +else { +while (p->next != e) + p = p->next; + + p->next = e->next; + } + e->callback (e, mm, emm_release,0 ,TASK_SIZE ); +} +EXPORT_SYMBOL_GPL (emm_notifier_unregister); + +/* * Perform a callback * * The return of this function is either a negative error of the first
@@ -309,7 +337,7 @@ EXPORT_SYMBOL_GPL(emm_notifier_register)int __emm_notify (struct mm_struct *mm,enum emm_operation op,unsigned long start,unsigned long end) { -struct emm_notifier *e =rcu_dereference (mm->emm_notifier); +struct emm_notifier *e = mm->emm_notifier;int x;int result =0 ;
@@ -335,7 +363,7 @@ int __emm_notify(struct mm_struct *mm, e* emm_notifier contents (e) must be fetched after * the retrival of the pointer to the notifier. */ - e =rcu_dereference (e->next); + e = e->next; }return result; }
Re: EMM: Require single threadedness for registration.
Quoted 3 lines · Christoph Lameter
Here is a patch to require single threaded execution during
emm_register. This also allows an easy implementation of an unregister function and gets rid of the races that Andrea worried about.
That would work for #v10 if I remove the invalidate_range_start from try_to_unmap_cluster, it can't work for EMM because you've emm_invalidate_start firing anywhere outside the context of the current task (even regular rmap code, not just nonlinear corner case will trigger the race). In short the single threaded approach would be workable only thanks to the fact #v10 has the notion of invalidate_page for flushing the tlb _after_ and to avoid blocking the secondary page fault during swapping. In the kvm case I don't want to block the page fault for anything but madvise which is strictly only used after guest inflated the balloon, and the existence of invalidate_page allows that optimization, and allows not to serialize against the kvm page fault during all regular page faults when the invalidate_page is called while the page is pinned by the VM.
The requirement for invalidate_page is that the pte and linux tlb are flushed _before_ and the page is freed _after_ the invalidate_page method. that's not the case for _begin/_end. The page is freed well before _end runs, hence the need of _begin and to block the secondary mmu page fault during the vma-mangling operations.
#v10 takes care of all this, and despite I could perhaps fix the remaining two issues using the single-threaded enforcement I suggested, I preferred to go safe and spend an unsigned per-mm in case anybody needs to attach at runtime, the single threaded restriction didn't look very clean.
Re: EMM: Require single threadedness for registration.
Quoted 5 lines · Andrea Arcangeli
That would work for #v10 if I remove the
invalidate_range_startfromtry_to_unmap_cluster, it can't work for EMM because you'veemm_invalidate_startfiring anywhere outside the context of the current task (even regular rmap code, not just nonlinear corner case will trigger the race). In short the single threaded approach would be
But in that case it will be firing for a callback to another mm_struct. The notifiers are bound to mm_structs and keep separate contexts.
Quoted 5 lines
The requirement for
invalidate_pageis that the pte and linux tlb are flushed _before_ and the page is freed _after_ theinvalidate_pagemethod. that's not the case for _begin/_end. The page is freed well before_endruns, hence the need of_beginand to block the secondary mmu page fault during the vma-mangling operations.
You could flush in _begin and free on _end? I thought you are taking a refcount on the page? You can drop the refcount only on _end to ensure that the page does not go away before.
Re: EMM: Require single threadedness for registration.
Quoted 10 lines · Christoph Lameter
Quoted 5 lines · Andrea Arcangeli
That would work for #v10 if I remove the
invalidate_range_startfromtry_to_unmap_cluster, it can't work for EMM because you'veemm_invalidate_startfiring anywhere outside the context of the current task (even regular rmap code, not just nonlinear corner case will trigger the race). In short the single threaded approach would beBut in that case it will be firing for a callback to another
mm_struct. The notifiers are bound tomm_structsand keep separate contexts.
Why can't it fire on the mm_struct where GRU just registered? That mm_struct existed way before GRU registered, and VM is free to unmap it w/o mmap_sem if there was any memory pressure.
Quoted 3 lines
You could flush in
_beginand free on_end? I thought you are taking a refcount on the page? You can drop the refcount only on_endto ensure that the page does not go away before.
we're going to lock + flush on begin and unlock on _end w/o refcounting to microoptimize. Free is done by unmap_vmas/madvise/munmap at will. That's a very slow path, inflating the balloon is not problematic. But invalidate_page allows to avoid blocking page faults during swapping so minor faults can happen and refresh the pte young bits etc... When the VM unmaps the page while holding the page pin, there's no race and that's where invalidate_page is being used to generate lower invalidation overhead.
Re: EMM: Require single threadedness for registration.
Quoted 3 lines · Andrea Arcangeli
Why can't it fire on the
mm_structwhere GRU just registered? Thatmm_structexisted way before GRU registered, and VM is free to unmap it w/ommap_semif there was any memory pressure.
Right. Hmmm... Bad situation. We would have invalidate_start take a lock to prevent registration until _end has run.
We could use stop_machine_run to register the notifier.... ;-).
EMM: disable other notifiers before register and unregister
In reply to Andrea ArcangeliOk lets forget about the single theaded thing to solve the registration races. As Andrea pointed out this still has ssues with other subscribed subsystems (and also try_to_unmap). We could do something like what stop_machine_run does: First disable all running subsystems before registering a new one.
Maybe this is a possible solution.
Subject: EMM: disable other notifiers before register and unregister
As Andrea has pointed out: There are races during registration if other subsystem notifiers are active while we register a callback.
Solve that issue by adding two new notifiers:
emm_stop
Stops the notifier operations. Notifier must block on invalidate_start and emm_referenced from this point on. If an invalidate_start has not been completed by a call to invalidate_end then the driver must wait until the operation is complete before returning.
emm_start
Restart notifier operations.
Before registration all other subscribed subsystems are stopped. Then the new subsystem is subscribed and things can get running without consistency issues.
Subsystems are restarted after the lists have been updated.
This also works for unregistering. If we can get all subsystems to stop then we can also reliably unregister a subsystem. So provide that callback.
--- include/linux/rmap.h | 10 +++++++--- mm/rmap.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-)
include/linux/rmap.h +7 -3
@@ -94,7 +94,9 @@ enum emm_operation { emm_release,/* Process exiting, */ emm_invalidate_start,/* Before the VM unmaps pages */ emm_invalidate_end,/* After the VM unmapped pages */ - emm_referenced/* Check if a range was referenced */ + emm_referenced,/* Check if a range was referenced */ + emm_stop,/* Halt all faults/invalidate_starts */ + emm_start, /* Restart operations */ }; struct emm_notifier {
@@ -126,13 +128,15 @@ static inline int emm_notify(struct mm_s/* * Register a notifier with an mm struct. Release occurs when the process -* terminates by calling the notifier function with emm_release. +* terminates by calling the notifier function with emm_release or when +* emm_notifier_unregister is called. * * Must hold the mmap_sem for write. */ extern void emm_notifier_register (struct emm_notifier *e,struct mm_struct *mm); - +extern void emm_notifier_unregister (struct emm_notifier *e, +struct mm_struct *mm);/* * Called from mm/vmscan.c to handle paging out
mm/rmap.c +30 -0
@@ -289,16 +289,46 @@ void emm_notifier_release(struct mm_stru/* Register a notifier */ void emm_notifier_register (struct emm_notifier *e,struct mm_struct *mm) { +/* Bring all other notifiers into a quiescent state */ +emm_notify (mm, emm_stop,0 ,TASK_SIZE ); + e->next = mm->emm_notifier; +/* * The update to emm_notifier (e->next) must be visible * before the pointer becomes visible. * rcu_assign_pointer() does exactly what we need. */ rcu_assign_pointer (mm->emm_notifier, e); + +/* Continue notifiers */ +emm_notify (mm, emm_start,0 ,TASK_SIZE ); }EXPORT_SYMBOL_GPL (emm_notifier_register); +/* Unregister a notifier */ +void emm_notifier_unregister (struct emm_notifier *e,struct mm_struct *mm) +{ +struct emm_notifier *p; + +emm_notify (mm, emm_stop,0 ,TASK_SIZE ); + + p = mm->emm_notifier; +if (e == p) + mm->emm_notifier = e->next; +else { +while (p->next != e) + p = p->next; + + p->next = e->next; + } + e->next = mm->emm_notifier; + +emm_notify (mm, emm_start,0 ,TASK_SIZE ); + e->callback (e, mm, emm_release,0 ,TASK_SIZE ); +} +EXPORT_SYMBOL_GPL (emm_notifier_unregister); +/* * Perform a callback *
-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
Re: EMM: disable other notifiers before register and unregister
Quoted 23 lines · Christoph Lameter
Ok lets forget about the single theaded thing to solve the registration races. As Andrea pointed out this still has ssues with other subscribed subsystems (and also
try_to_unmap). We could do something like whatstop_machine_rundoes: First disable all running subsystems before registering a new one.Maybe this is a possible solution.
Subject: EMM: disable other notifiers before register and unregister
As Andrea has pointed out: There are races during registration if other subsystem notifiers are active while we register a callback.
Solve that issue by adding two new notifiers:
emm_stop
Stops the notifier operations. Notifier must block oninvalidate_startandemm_referencedfrom this point on. If aninvalidate_starthas not been completed by a call toinvalidate_endthen the driver must wait until the operation is complete before returning.
Quoted 2 lines
emm_start
Restart notifier operations.
Please use pause and resume or something like that. stop-start is an unnatural order; we usually start before we stop, whereas we pause first and resume later.
Quoted 102 lines
Before registration all other subscribed subsystems are stopped. Then the new subsystem is subscribed and things can get running without consistency issues.
Subsystems are restarted after the lists have been updated.
This also works for unregistering. If we can get all subsystems to stop then we can also reliably unregister a subsystem. So provide that callback.
Signed-off-by Christoph Lameter <[email protected]>--- include/linux/rmap.h | 10 +++++++--- mm/rmap.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-)Index:
linux-2.6/include/linux/rmap.h
===================================================================include/linux/rmap.h +7
-3@@ -94,7 +94,9 @@ enum emm_operation { emm_release,/* Process exiting, */ emm_invalidate_start,/* Before the VM unmaps pages */ emm_invalidate_end,/* After the VM unmapped pages */ - emm_referenced/* Check if a range was referenced */ + emm_referenced,/* Check if a range was referenced */ + emm_stop,/* Halt all faults/invalidate_starts */ + emm_start, /* Restart operations */ }; struct emm_notifier {@@ -126,13 +128,15 @@ static inline int emm_notify(struct mm_s/* * Register a notifier with an mm struct. Release occurs when the process -* terminates by calling the notifier function with emm_release. +* terminates by calling the notifier function with emm_release or when +* emm_notifier_unregister is called. * * Must hold the mmap_sem for write. */ extern void emm_notifier_register (struct emm_notifier *e,struct mm_struct *mm); - +extern void emm_notifier_unregister (struct emm_notifier *e, +struct mm_struct *mm);/* * Called from mm/vmscan.c to handle paging out Index:
linux-2.6/mm/rmap.c
===================================================================mm/rmap.c +30
-0@@ -289,16 +289,46 @@ void emm_notifier_release(struct mm_stru/* Register a notifier */ void emm_notifier_register (struct emm_notifier *e,struct mm_struct *mm) { +/* Bring all other notifiers into a quiescent state */ +emm_notify (mm, emm_stop,0 ,TASK_SIZE ); + e->next = mm->emm_notifier; +/* * The update to emm_notifier (e->next) must be visible * before the pointer becomes visible. * rcu_assign_pointer() does exactly what we need. */ rcu_assign_pointer (mm->emm_notifier, e); + +/* Continue notifiers */ +emm_notify (mm, emm_start,0 ,TASK_SIZE ); }EXPORT_SYMBOL_GPL (emm_notifier_register); +/* Unregister a notifier */ +void emm_notifier_unregister (struct emm_notifier *e,struct mm_struct *mm) +{ +struct emm_notifier *p; + +emm_notify (mm, emm_stop,0 ,TASK_SIZE ); + + p = mm->emm_notifier; +if (e == p) + mm->emm_notifier = e->next; +else { +while (p->next != e) + p = p->next; + + p->next = e->next; + } + e->next = mm->emm_notifier; + +emm_notify (mm, emm_start,0 ,TASK_SIZE ); + e->callback (e, mm, emm_release,0 ,TASK_SIZE ); +} +EXPORT_SYMBOL_GPL (emm_notifier_unregister); +/* * Perform a callback *
Re: EMM: disable other notifiers before register and unregister
In reply to Christoph LameterQuoted 7 lines · Christoph Lameter
Ok lets forget about the single theaded thing to solve the registration races. As Andrea pointed out this still has ssues with other subscribed subsystems (and also
try_to_unmap). We could do something like whatstop_machine_rundoes: First disable all running subsystems before registering a new one.Maybe this is a possible solution.
It still doesn't solve this kernel crash.
CPU0 CPU1
range_start (mmu notifier chain is empty)
range_start returns
mmu_notifier_register
kvm_emm_stop (how kvm can ever know
the other cpu is in the middle of the critical section?) kvm page fault (kvm thinks mmu_notifier_register serialized)
zap ptes
free_page mapped by spte/GRU and not pinned -> crash
There's no way the lowlevel can stop mmu_notifier_register and if mmu_notifier_register returns, then sptes will be instantiated and it'll corrupt memory the same way.
The seqlock was fine, what is wrong is the assumption that we can let the lowlevel driver handle a range_end happening without range_begin before it. The problem is that by design the lowlevel can't handle a range_end happening without a range_begin before it. This is the core kernel crashing problem we have (it's a kernel crashing issue only for drivers that don't pin the pages, so XPMEM wouldn't crash but still it would leak memory, which is a more graceful failure than random mm corruption).
The basic trouble is that sometime range_begin/end critical sections run outside the mmap_sem (see try_to_unmap_cluster in #v10 or even try_to_unmap_one only in EMM-V2).
My attempt to fix this once and for all is to walk all vmas of the "mm" inside mmu_notifier_register and take all anon_vma locks and i_mmap_locks in virtual address order in a row. It's ok to take those inside the mmap_sem. Supposedly if anybody will ever take a double lock it'll do in order too. Then I can dump all the other locking and remove the seqlock, and the driver is guaranteed there will be a single call of range_begin followed by a single call of range_end the whole time and no race could ever happen, and there won't be replied calls of range_begin that would screwup a recursive semaphore locking. The patch won't be pretty, I guess I'll vmalloc an array of pointers to locks to reorder them. It doesn't need to be fast. Also the locks can't go away from under us while we hold the down_write(mmap_sem) because the vmas can be altered only with down_write(mmap_sem) (modulo vm_start/vm_end that can be modified with only down_read(mmap_sem) + page_table_lock like in growsdown page faults). So it should be ok to take all those locks inside the mmap_sem and implement a lock_vm(mm) unlock_vm(mm). I'll think more about this hammer approach while I try to implement it...
Re: EMM: disable other notifiers before register and unregister
Quoted 5 lines · Andrea Arcangeli
My attempt to fix this once and for all is to walk all vmas of the "mm" inside
mmu_notifier_registerand take allanon_vmalocks andi_mmap_locksin virtual address order in a row. It's ok to take those inside themmap_sem. Supposedly if anybody will ever take a double lock it'll do in order too. Then I can dump all the other locking and
What about concurrent mmu_notifier registrations from two mm_structs that have shared mappings? Isnt there a potential deadlock situation?
Quoted 3 lines
faults). So it should be ok to take all those locks inside the
mmap_semand implement alock_vm(mm)unlock_vm(mm). I'll think more about this hammer approach while I try to implement it...
Well good luck. Hopefully we will get to something that works.
Re: EMM: disable other notifiers before register and unregister
Quoted 5 lines · Christoph Lameter
Quoted 3 lines
faults). So it should be ok to take all those locks inside the
mmap_semand implement alock_vm(mm)unlock_vm(mm). I'll think more about this hammer approach while I try to implement it...Well good luck. Hopefully we will get to something that works.
Another hammer to use may be the freezer from software suspend. With that you can get all tasks of a process into a definite state. Then take the mmap_sem writably. But then there is still try_to_unmap and friends that can race.
Re: EMM: disable other notifiers before register and unregister
In reply to Christoph LameterQuoted 10 lines · Christoph Lameter
Quoted 5 lines · Andrea Arcangeli
My attempt to fix this once and for all is to walk all vmas of the "mm" inside
mmu_notifier_registerand take allanon_vmalocks andi_mmap_locksin virtual address order in a row. It's ok to take those inside themmap_sem. Supposedly if anybody will ever take a double lock it'll do in order too. Then I can dump all the other locking andWhat about concurrent
mmu_notifierregistrations from twomm_structsthat have shared mappings? Isnt there a potential deadlock situation?
No, the ordering of the lock avoids that. Here a snippnet.
/*
* This operation locks against the VM for all pte/vma/mm related
* operations that could ever happen on a certain mm. This includes
* vmtruncate, try_to_unmap, and all page faults. The holder
* must not hold any mm related lock. A single task can't take more
* than one mm lock in a row or it would deadlock.
*/
So you can't do:
mm_lock(mm1); mm_lock(mm2);
But if two different tasks run the mm_lock everything is ok. Each task in the system can lock at most 1 mm at time.
Quoted 1 line
Well good luck. Hopefully we will get to something that works.
Looks good so far but I didn't finish it yet.
-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
PATCH 1/1 mmu notifier #v11
In reply to Christoph LameterThis should guarantee that nobody can register when any of the mmu notifiers is running avoiding all the races including guaranteeing range_start not to be missed. I'll adapt the other patches to provide the sleeping-feature on top of this (only needed by XPMEM) soon. KVM seems to run fine on top of this one.
Andrew can you apply this to -mm?
include/linux/mm.h +3 -0
@@ -1050,6 +1050,9 @@ unsigned long addr, unsigned long len, unsigned long flags, struct page **pages); +extern void mm_lock (struct mm_struct *mm); +extern void mm_unlock (struct mm_struct *mm); +extern unsigned long get_unmapped_area (struct file *,unsigned long ,unsigned long ,unsigned long ,unsigned long ); extern unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
include/linux/mm_types.h +3 -0
@@ -225,6 +225,9 @@#ifdef CONFIG_CGROUP_MEM_RES_CTLR struct mem_cgroup *mem_cgroup;#endif +#ifdef CONFIG_MMU_NOTIFIER +struct hlist_head mmu_notifier_list; +#endif };#endif /* _LINUX_MM_TYPES_H */
include/linux/mmu_notifier.h new file +175 -0
175 lines not on this page · raw message
kernel/fork.c +2 -0
@@ -53,6 +53,7 @@#include <linux/tty.h> #include <linux/proc_fs.h> #include <linux/blkdev.h> +#include <linux/mmu_notifier.h> #include <asm/pgtable.h> #include <asm/pgalloc.h>
@@ -362,6 +363,7 @@if (likely (!mm_alloc_pgd (mm))) { mm->def_flags =0 ; +mmu_notifier_mm_init (mm);return mm; }
mm/Kconfig +4 -0
@@ -193,3 +193,7 @@ config VIRT_TO_BUS def_bool y depends on !ARCH_NO_VIRT_TO_BUS + +config MMU_NOTIFIER + def_bool y + bool "MMU notifier, for paging KVM/RDMA"
mm/Makefile +1 -0
@@ -33,4 +33,5 @@ obj-$(CONFIG_SMP) += allocpercpu.o obj-$(CONFIG_QUICKLIST) += quicklist.o obj-$(CONFIG_CGROUP_MEM_RES_CTLR) += memcontrol.o +obj-$(CONFIG_MMU_NOTIFIER) += mmu_notifier.o
mm/filemap_xip.c +1 -1
@@ -194,7 +194,7 @@if (pte) {/* Nuke the page table entry. */ flush_cache_page (vma, address,pte_pfn (*pte)); - pteval =ptep_clear_flush (vma, address, pte); + pteval =ptep_clear_flush_notify (vma, address, pte);page_remove_rmap (page, vma);dec_mm_counter (mm, file_rss);BUG_ON (pte_dirty (pteval));
mm/fremap.c +3 -0
@@ -15,6 +15,7 @@#include <linux/rmap.h> #include <linux/module.h> #include <linux/syscalls.h> +#include <linux/mmu_notifier.h> #include <asm/mmu_context.h> #include <asm/cacheflush.h>
@@ -214,7 +215,9 @@spin_unlock (&mapping->i_mmap_lock); } +mmu_notifier_invalidate_range_start (mm, start, start + size); err =populate_range (mm, vma, start, size, pgoff); +mmu_notifier_invalidate_range_end (mm, start, start + size);if (!err && !(flags &MAP_NONBLOCK )) {if (unlikely (has_write_lock)) {downgrade_write (&mm->mmap_sem);
mm/hugetlb.c +3 -0
@@ -14,6 +14,7 @@#include <linux/mempolicy.h> #include <linux/cpuset.h> #include <linux/mutex.h> +#include <linux/mmu_notifier.h> #include <asm/page.h> #include <asm/pgtable.h>
@@ -799,6 +800,7 @@BUG_ON (start & ~HPAGE_MASK );BUG_ON (end & ~HPAGE_MASK ); +mmu_notifier_invalidate_range_start (mm, start, end);spin_lock (&mm->page_table_lock);for (address = start; address < end; address +=HPAGE_SIZE ) { ptep =huge_pte_offset (mm, address);
@@ -819,6 +821,7 @@ }spin_unlock (&mm->page_table_lock);flush_tlb_range (vma, start, end); +mmu_notifier_invalidate_range_end (mm, start, end);list_for_each_entry_safe (page, tmp, &page_list, lru) {list_del (&page->lru);put_page (page);
mm/memory.c +15 -2
@@ -51,6 +51,7 @@#include <linux/init.h> #include <linux/writeback.h> #include <linux/memcontrol.h> +#include <linux/mmu_notifier.h> #include <asm/pgalloc.h> #include <asm/uaccess.h>
@@ -611,6 +612,9 @@if (is_vm_hugetlb_page (vma))return copy_hugetlb_page_range (dst_mm, src_mm, vma); +if (is_cow_mapping (vma->vm_flags)) +mmu_notifier_invalidate_range_start (src_mm, addr, end); + dst_pgd =pgd_offset (dst_mm, addr); src_pgd =pgd_offset (src_mm, addr); do {
@@ -621,6 +625,11 @@ vma, addr, next))return -ENOMEM ; }while (dst_pgd++, src_pgd++, addr = next, addr != end); + +if (is_cow_mapping (vma->vm_flags)) +mmu_notifier_invalidate_range_end (src_mm, + vma->vm_start, end); +return 0 ; }
@@ -897,7 +906,9 @@lru_add_drain (); tlb =tlb_gather_mmu (mm,0 );update_hiwater_rss (mm); +mmu_notifier_invalidate_range_start (mm, address, end); end =unmap_vmas (&tlb, vma, address, end, &nr_accounted, details); +mmu_notifier_invalidate_range_end (mm, address, end);if (tlb)tlb_finish_mmu (tlb, address, end);return end;
@@ -1463,10 +1474,11 @@ {pgd_t *pgd;unsigned long next; -unsigned long end = addr + size; +unsigned long start = addr, end = addr + size;int err;BUG_ON (addr >= end); +mmu_notifier_invalidate_range_start (mm, start, end); pgd =pgd_offset (mm, addr); do { next = pgd_addr_end(addr, end);
@@ -1474,6 +1486,7 @@if (err)break ; }while (pgd++, addr = next, addr != end); +mmu_notifier_invalidate_range_end (mm, start, end);return err; }EXPORT_SYMBOL_GPL (apply_to_page_range);
@@ -1675,7 +1688,7 @@* seen in the presence of one thread doing SMC and another * thread doing COW. */ -ptep_clear_flush (vma, address, page_table); +ptep_clear_flush_notify (vma, address, page_table);set_pte_at (mm, address, page_table, entry);update_mmu_cache (vma, address, entry);lru_cache_add_active (new_page);
mm/mmap.c +70 -0
96 lines not on this page · raw message
mm/mmu_notifier.c new file +100 -0
100 lines not on this page · raw message
mm/mprotect.c +3 -0
@@ -21,6 +21,7 @@#include <linux/syscalls.h> #include <linux/swap.h> #include <linux/swapops.h> +#include <linux/mmu_notifier.h> #include <asm/uaccess.h> #include <asm/pgtable.h> #include <asm/cacheflush.h>
@@ -198,10 +199,12 @@ dirty_accountable =1 ; } +mmu_notifier_invalidate_range_start (mm, start, end);if (is_vm_hugetlb_page (vma))hugetlb_change_protection (vma, start, end, vma->vm_page_prot);else change_protection (vma, start, end, vma->vm_page_prot, dirty_accountable); +mmu_notifier_invalidate_range_end (mm, start, end);vm_stat_account (mm, oldflags, vma->vm_file, -nrpages);vm_stat_account (mm, newflags, vma->vm_file, nrpages);return 0 ;
mm/mremap.c +6 -0
@@ -18,6 +18,7 @@#include <linux/highmem.h> #include <linux/security.h> #include <linux/syscalls.h> +#include <linux/mmu_notifier.h> #include <asm/uaccess.h> #include <asm/cacheflush.h>
@@ -74,7 +75,11 @@struct mm_struct *mm = vma->vm_mm;pte_t *old_pte, *new_pte, pte;spinlock_t *old_ptl, *new_ptl; +unsigned long old_start; + old_start = old_addr; +mmu_notifier_invalidate_range_start (vma->vm_mm, + old_start, old_end);if (vma->vm_file) {/* * Subtle point from Rajesh Venkatasubramanian: before
@@ -116,6 +121,7 @@pte_unmap_unlock (old_pte -1 , old_ptl);if (mapping)spin_unlock (&mapping->i_mmap_lock); +mmu_notifier_invalidate_range_end (vma->vm_mm, old_start, old_end); }#define LATENCY_LIMIT (64 * PAGE_SIZE)
mm/rmap.c +7 -6
@@ -49,6 +49,7 @@#include <linux/module.h> #include <linux/kallsyms.h> #include <linux/memcontrol.h> +#include <linux/mmu_notifier.h> #include <asm/tlbflush.h>
@@ -287,7 +288,7 @@if (vma->vm_flags &VM_LOCKED ) { referenced++; *mapcount =1 ;/* break early from loop */ - }else if (ptep_clear_flush_young (vma, address, pte)) + }else if (ptep_clear_flush_young_notify (vma, address, pte)) referenced++;/* Pretend the page is referenced if the task has the
@@ -456,7 +457,7 @@pte_t entry;flush_cache_page (vma, address,pte_pfn (*pte)); - entry =ptep_clear_flush (vma, address, pte); + entry =ptep_clear_flush_notify (vma, address, pte); entry =pte_wrprotect (entry); entry =pte_mkclean (entry);set_pte_at (mm, address, pte, entry);
@@ -717,14 +718,14 @@* skipped over this mm) then we should reactivate it. */ if (!migration && ((vma->vm_flags &VM_LOCKED ) || - (ptep_clear_flush_young (vma, address, pte)))) { + (ptep_clear_flush_young_notify (vma, address, pte)))) { ret =SWAP_FAIL ; goto out_unmap; }/* Nuke the page table entry. */ flush_cache_page (vma, address,page_to_pfn (page)); - pteval =ptep_clear_flush (vma, address, pte); + pteval =ptep_clear_flush_notify (vma, address, pte);/* Move the dirty bit to the physical page now the pte is gone. */ if (pte_dirty (pteval))
@@ -849,12 +850,12 @@ page =vm_normal_page (vma, address, *pte);BUG_ON (!page ||PageAnon (page)); -if (ptep_clear_flush_young (vma, address, pte)) +if (ptep_clear_flush_young_notify (vma, address, pte))continue ;/* Nuke the page table entry. */ flush_cache_page (vma, address,pte_pfn (*pte)); - pteval =ptep_clear_flush (vma, address, pte); + pteval =ptep_clear_flush_notify (vma, address, pte);/* If nonlinear, store the file page offset in the pte. */ if (page->index !=linear_page_index (vma, address))
Re: [PATCH] mmu notifier #v11
I am always the guy doing the cleanup after Andrea it seems. Sigh.
Here is the mm_lock/mm_unlock logic separated out for easier review. Adds some comments. Still objectionable is the multiple ways of invalidating pages in #v11. Callout now has similar locking to emm.
From: Christoph Lameter <[email protected]>
Subject: mm_lock: Lock a process against reclaim
Provide a way to lock an mm_struct against reclaim (try_to_unmap etc). This is necessary for the invalidate notifier approaches so that they can reliably add and remove a notifier.
--- include/linux/mm.h | 10 ++++++++ mm/mmap.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+)
include/linux/mm.h +10 -0
@@ -1050,6 +1050,16 @@ extern int install_special_mapping(struc unsigned long addr, unsigned long len, unsigned long flags, struct page **pages); +/* +* Locking and unlocking an mm against reclaim. +* +* mm_lock will take mmap_sem writably (to prevent additional vmas from being +* added) and then take all mapping locks of the existing vmas. With that +* reclaim is effectively stopped. +*/ +extern void mm_lock (struct mm_struct *mm); +extern void mm_unlock (struct mm_struct *mm); +extern unsigned long get_unmapped_area (struct file *,unsigned long ,unsigned long ,unsigned long ,unsigned long ); extern unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
mm/mmap.c +66 -0
69 lines not on this page · raw message
Re: [PATCH] mmu notifier #v11
Quoted 2 lines · Christoph Lameter
Adds some comments. Still objectionable is the multiple ways of invalidating pages in #v11. Callout now has similar locking to emm.
range_begin exists because range_end is called after the page has already been freed. invalidate_page is called _before_ the page is freed but _after_ the pte has been zapped.
In short when working with single pages it's a waste to block the secondary-mmu page fault, because it's zero cost to invalidate_page before put_page. Not even GRU need to do that.
Instead for the multiple-pte-zapping we have to call range_end _after_ the page is already freed. This is so that there is a single range_end call for an huge amount of address space. So we need a range_begin for the subsystems not using page pinning for example. When working with single pages (try_to_unmap_one, do_wp_page) invalidate_page avoids to block the secondary mmu page fault, and it's in turn faster.
Besides avoiding need of serializing the secondary mmu page fault, invalidate_page also reduces the overhead when the mmu notifiers are disarmed (i.e. kvm not running).
Re: [PATCH] mmu notifier #v11
Quoted 3 lines · Andrea Arcangeli
In short when working with single pages it's a waste to block the secondary-mmu page fault, because it's zero cost to
invalidate_pagebeforeput_page. Not even GRU need to do that.
That depends on what the notifier is being used for. Some serialization with the external mappings has to be done anyways. And its cleaner to have one API that does a lock/unlock scheme. Atomic operations can easily lead to races.
-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Register now and save $200. Hurry, offer ends at 11:59 p.m., Monday, April 7! Use priority code J8TLD2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
Re: [PATCH] mmu notifier #v11
Quoted 2 lines · Christoph Lameter
That depends on what the notifier is being used for. Some serialization with the external mappings has to be done anyways. And its cleaner to have
As far as I can tell no, you don't need to serialize against the secondary mmu page fault in invalidate_page, like you instead have to do in range_begin if you don't unpin the pages in range_end.
Quoted 2 lines
one API that does a lock/unlock scheme. Atomic operations can easily lead to races.
What races? Note that if you don't want to optimize XPMEM and GRU can feel free to implement their own invalidate_page as this:
invalidate_page(mm, addr) {
range_begin(mm, addr, addr+PAGE_SIZE)
range_end(mm, addr, addr+PAGE_SIZE)
}
There's zero risk of adding races if they do this, but I doubt they want to run as slow as with EMM so I guess they'll exploit the optimization by going lock-free vs the spte page fault in invalidate_page.
Re: [patch 1/9] EMM Notifier: The notifier calls
In reply to Christoph LameterQuoted 2 lines · Christoph Lameter
Did I see #v10? Could you start a new subject when you post please? Do not respond to some old message otherwise the threading will be wrong.
I wasn't clear enough, #v10 was in the works... I was thinking about the last two issues before posting it.
Quoted 1 line
How exactly does the GRU corrupt memory?
Jack added synchronize_rcu, I assume for a reason.
Quoted 18 lines
Quoted 17 lines
Another less obviously safe approach is to allow the register method to succeed only when mm_users=1 and the task is single threaded. This way if all the places where the mmu notifers aren't invoked on the mm not by the current task, are only doing invalidates after/before zapping ptes, if the istantiation of new ptes is single threaded too, we shouldn't worry if we miss an invalidate for a pte that is zero and doesn't point to any physical page. In the places where
current->mm!= mm I'm usinginvalidate_page99% of the time, and that only follows theptep_clear_flush. The problem are therange_beginthat will happen before zapping the pte in places wherecurrent->mm!= mm. Unfortunately in my incremental patch where I move allinvalidate_pageoutside of the PT lock to prepare for allowing sleeping inside the mmu notifiers, I used range_begin/end in places liketry_to_unmap_clusterwherecurrent->mm!= mm. In general this solution looks more fragile than the seqlock.
Quoted 2 lines
Hmmm... Okay that is one solution that would just require a
BUG_ONin the registration methods.
Perhaps you didn't notice that this solution can't work if you call range_begin/end not in the "current" context and try_to_unmap_cluster does exactly that for both my patchset and yours. Missing an _end is ok, missing a _begin is never ok.
Quoted 2 lines
Well doesnt the requirement of just one execution thread also deal with that issue?
Yes, except again it can't work for try_to_unmap_cluster.
This solution is only applicable to #v10 if I fix try_to_unmap_cluster to only call invalidate_page (relaying on the fact the VM holds a pin and a lock on any page that is being mmu-notifier-invalidated).
You can't use the single threaded approach to solve either 1 or 2, because your _begin call is called anywhere and that's where you call the secondary-tlb flush and it's fatal to miss it.
invalidate_page is called always after, so it enforced the tlb flush to be called _after_ and so it's inherently safe.
Re: [patch 1/9] EMM Notifier: The notifier calls
Quoted 7 lines · Andrea Arcangeli
Quoted 2 lines
Hmmm... Okay that is one solution that would just require a
BUG_ONin the registration methods.Perhaps you didn't notice that this solution can't work if you call range_begin/end not in the "current" context and
try_to_unmap_clusterdoes exactly that for both my patchset and yours. Missing an_endis ok, missing a_beginis never ok.
If you look at the patch you will see a requirement of holding a writelock on mmap_sem which will keep out get_user_pages().
Re: [patch 1/9] EMM Notifier: The notifier calls
Quoted 5 lines · Christoph Lameter
Quoted 3 lines · Andrea Arcangeli
Quoted 2 lines
Hmmm... Okay that is one solution that would just require a
BUG_ONin the registration methods.
Quoted 7 lines
Quoted 4 lines
Perhaps you didn't notice that this solution can't work if you call range_begin/end not in the "current" context and
try_to_unmap_clusterdoes exactly that for both my patchset and yours. Missing an_endis ok, missing a_beginis never ok.If you look at the patch you will see a requirement of holding a writelock on
mmap_semwhich will keep outget_user_pages().
I said try_to_unmap_cluster, not get_user_pages.
CPU0 CPU1
try_to_unmap_cluster:
emm_invalidate_start in EMM (or mmu_notifier_invalidate_range_start in #v10) walking the list by hand in EMM (or with hlist cleaner in #v10) xpmem method invoked
schedule for a long while inside invalidate_range_start while skbs are sent
gru registers synchronize_rcu (sorry useless now) single threaded, so taking a page fault secondary tlb instantiated
xpm method returns
end of the list (didn't notice that it has to restart to flush the gru) zap pte
free the page
gru corrupts memory
CPU 1 was single threaded, CPU0 doesn't hold any mmap_sem or any other lock that could ever serialize against the GRU as far as I can tell.
In general my #v10 solution mixing seqlock + rcu looks more robust and allows multithreaded attachment of mmu notifers as well. I could have fixed it with the single threaded thanks to the fact the only place outside the mm->mmap_sem is try_to_unmap_cluster for me but it wasn't simple to convert, nor worth it, given nonlinear isn't worth optimizing for (not even the core VM cares about try_to_unmap_cluster which is infact the only place in the VM with a O(N) complexity for each try_to_unmap call, where N is the size of the mapping divided by page_size).
Re: [patch 1/9] EMM Notifier: The notifier calls
Quoted 10 lines · Andrea Arcangeli
I said
try_to_unmap_cluster, notget_user_pages.CPU0 CPU1
try_to_unmap_cluster:
emm_invalidate_startin EMM (ormmu_notifier_invalidate_range_startin #v10) walking the list by hand in EMM (or with hlist cleaner in #v10) xpmem method invoked
schedule for a long while insideinvalidate_range_startwhile skbs are sent
gru registerssynchronize_rcu(sorry useless now)
All of this would be much easier if you could stop the drivel. The sync rcu was for an earlier release of the mmu notifier. Why the sniping?
Quoted 2 lines
single threaded, so taking a page fault secondary tlb instantiated
The driver must not allow faults to occur between start and end. The trouble here is that GRU and xpmem are mixed. If CPU0 would have been running GRU instead of XPMEM then the fault would not have occurred because the gru would have noticed that a range op is active. If both systems would have run xpmem then the same would have worked.
I guess this means that an address space cannot reliably registered to multiple subsystems if some of those do not take a refcount. If all drivers would be required to take a refcount then this would also not occur.
Quoted 2 lines
In general my #v10 solution mixing seqlock + rcu looks more robust and allows multithreaded attachment of mmu notifers as well. I could have
Well its easy to say that if no one else has looked at it yet. I expressed some concerns in reply to your post of #v10.
PATCH 2/9 Move tlb flushing into free_pgtables
In reply to Christoph LameterMove the tlb flushing into free_pgtables. The conversion of the locks taken for reverse map scanning would require taking sleeping locks in free_pgtables(). Moving the tlb flushing into free_pgtables allows sleeping in parts of free_pgtables().
This means that we do a tlb_finish_mmu() before freeing the page tables. Strictly speaking there may not be the need to do another tlb flush after freeing the tables. But its the only way to free a series of page table pages from the tlb list. And we do not want to call into the page allocator for performance reasons. Aim9 numbers look okay after this patch.
--- include/linux/mm.h | 4 ++-- mm/memory.c | 14 ++++++++++---- mm/mmap.c | 6 +++--- 3 files changed, 15 insertions(+), 9 deletions(-)
include/linux/mm.h +2 -2
@@ -751,8 +751,8 @@ int walk_page_range(const struct mm_stru void *private);void free_pgd_range (struct mmu_gather **tlb,unsigned long addr,unsigned long end,unsigned long floor,unsigned long ceiling); -void free_pgtables (struct mmu_gather **tlb,struct vm_area_struct *start_vma, -unsigned long floor,unsigned long ceiling); +void free_pgtables (struct vm_area_struct *start_vma,unsigned long floor, +unsigned long ceiling);int copy_page_range (struct mm_struct *dst,struct mm_struct *src,struct vm_area_struct *vma); void unmap_mapping_range(struct address_space *mapping,
mm/memory.c +10 -4
@@ -271,9 +271,11 @@ void free_pgd_range(struct mmu_gather ** }while (pgd++, addr = next, addr != end); } -void free_pgtables (struct mmu_gather **tlb,struct vm_area_struct *vma, -unsigned long floor,unsigned long ceiling) +void free_pgtables (struct vm_area_struct *vma,unsigned long floor, +unsigned long ceiling) { +struct mmu_gather *tlb; +while (vma) {struct vm_area_struct *next = vma->vm_next;unsigned long addr = vma->vm_start;
@@ -285,8 +287,10 @@ void free_pgtables(struct mmu_gather **tunlink_file_vma (vma);if (is_vm_hugetlb_page (vma)) { -hugetlb_free_pgd_range (tlb, addr, vma->vm_end, + tlb =tlb_gather_mmu (vma->vm_mm,0 ); +hugetlb_free_pgd_range (&tlb, addr, vma->vm_end, floor, next? next->vm_start: ceiling); +tlb_finish_mmu (tlb, addr, vma->vm_end); }else {/* * Optimization: gather nearby vmas into one call down
@@ -298,8 +302,10 @@ void free_pgtables(struct mmu_gather **tanon_vma_unlink (vma);unlink_file_vma (vma); } -free_pgd_range (tlb, addr, vma->vm_end, + tlb =tlb_gather_mmu (vma->vm_mm,0 ); +free_pgd_range (&tlb, addr, vma->vm_end, floor, next? next->vm_start: ceiling); +tlb_finish_mmu (tlb, addr, vma->vm_end); } vma = next; }
mm/mmap.c +3 -3
@@ -1750,9 +1750,9 @@ static void unmap_region(struct mm_strucupdate_hiwater_rss (mm);unmap_vmas (&tlb, vma, start, end, &nr_accounted,NULL );vm_unacct_memory (nr_accounted); -free_pgtables (&tlb, vma, prev? prev->vm_end:FIRST_USER_ADDRESS , - next? next->vm_start:0 );tlb_finish_mmu (tlb, start, end); +free_pgtables (vma, prev? prev->vm_end:FIRST_USER_ADDRESS , + next? next->vm_start:0 );emm_notify (mm, emm_invalidate_end, start, end); }
@@ -2049,8 +2049,8 @@ void exit_mmap(struct mm_struct *mm)/* Use -1 here to ensure all VMAs in the mm are unmapped */ end =unmap_vmas (&tlb, vma,0 ,-1 , &nr_accounted,NULL );vm_unacct_memory (nr_accounted); -free_pgtables (&tlb, vma,FIRST_USER_ADDRESS ,0 );tlb_finish_mmu (tlb,0 , end); +free_pgtables (vma,FIRST_USER_ADDRESS ,0 );/* * Walk the list again, actually closing and freeing it,
PATCH 3/9 Convert i_mmap_lock to i_mmap_sem
In reply to Christoph LameterThe conversion to a rwsem allows callbacks during rmap traversal for files in a non atomic context. A rw style lock also allows concurrent walking of the reverse map. This is fairly straightforward if one removes pieces of the resched checking.
[Restarting unmapping is an issue to be discussed].
This slightly increases Aim9 performance results on an 8p.
--- arch/x86/mm/hugetlbpage.c | 4 ++-- fs/hugetlbfs/inode.c | 4 ++-- fs/inode.c | 2 +- include/linux/fs.h | 2 +- include/linux/mm.h | 2 +- kernel/fork.c | 4 ++-- mm/filemap.c | 8 ++++---- mm/filemap_xip.c | 4 ++-- mm/fremap.c | 4 ++-- mm/hugetlb.c | 10 +++++----- mm/memory.c | 29 +++++++++-------------------- mm/migrate.c | 4 ++-- mm/mmap.c | 16 ++++++++-------- mm/mremap.c | 4 ++-- mm/rmap.c | 20 +++++++++----------- 15 files changed, 52 insertions(+), 65 deletions(-)
arch/x86/mm/hugetlbpage.c +2 -2
@@ -69,7 +69,7 @@ static void huge_pmd_share(struct mm_strif (!vma_shareable (vma, addr))return ; -spin_lock (&mapping->i_mmap_lock); +down_read (&mapping->i_mmap_sem); vma_prio_tree_foreach(svma, &iter, &mapping->i_mmap, idx, idx) {if (svma == vma)continue ;
@@ -94,7 +94,7 @@ static void huge_pmd_share(struct mm_strput_page (virt_to_page (spte));spin_unlock (&mm->page_table_lock); out: -spin_unlock (&mapping->i_mmap_lock); +up_read (&mapping->i_mmap_sem); }/*
fs/hugetlbfs/inode.c +2 -2
@@ -454,10 +454,10 @@ static int hugetlb_vmtruncate(struct ino pgoff = offset >>PAGE_SHIFT ;i_size_write (inode, offset); -spin_lock (&mapping->i_mmap_lock); +down_read (&mapping->i_mmap_sem);if (!prio_tree_empty (&mapping->i_mmap))hugetlb_vmtruncate_list (&mapping->i_mmap, pgoff); -spin_unlock (&mapping->i_mmap_lock); +up_read (&mapping->i_mmap_sem);truncate_hugepages (inode, offset);return 0 ; }
fs/inode.c +1 -1
@@ -210,7 +210,7 @@ void inode_init_once(struct inode *inodeINIT_LIST_HEAD (&inode->i_devices);INIT_RADIX_TREE (&inode->i_data.page_tree,GFP_ATOMIC );rwlock_init (&inode->i_data.tree_lock); -spin_lock_init (&inode->i_data.i_mmap_lock); +init_rwsem (&inode->i_data.i_mmap_sem);INIT_LIST_HEAD (&inode->i_data.private_list);spin_lock_init (&inode->i_data.private_lock);INIT_RAW_PRIO_TREE_ROOT (&inode->i_data.i_mmap);
include/linux/fs.h +1 -1
@@ -503,7 +503,7 @@ struct address_space {unsigned int i_mmap_writable;/* count VM_SHARED mappings */ struct prio_tree_root i_mmap;/* tree of private and shared mappings */ struct list_head i_mmap_nonlinear;/*list VM_NONLINEAR mappings */ -spinlock_t i_mmap_lock;/* protect tree, count, list */ +struct rw_semaphore i_mmap_sem;/* protect tree, count, list */ unsigned int truncate_count;/* Cover race condition with truncate */ unsigned long nrpages;/* number of total pages */ pgoff_t writeback_index;/* writeback starts here */
include/linux/mm.h +1 -1
@@ -716,7 +716,7 @@ struct zap_details {struct address_space *check_mapping;/* Check page->mapping if set */ pgoff_t first_index;/* Lowest page->index to unmap */ pgoff_t last_index;/* Highest page->index to unmap */ -spinlock_t *i_mmap_lock;/* For unmap_mapping_range: */ +struct rw_semaphore *i_mmap_sem;/* For unmap_mapping_range: */ unsigned long truncate_count;/* Compare vm_truncate_count */ };
kernel/fork.c +2 -2
@@ -273,12 +273,12 @@ static int dup_mmap(struct mm_struct *mmatomic_dec (&inode->i_writecount);/* insert tmp into the share list, just after mpnt */ -spin_lock (&file->f_mapping->i_mmap_lock); +down_write (&file->f_mapping->i_mmap_sem); tmp->vm_truncate_count = mpnt->vm_truncate_count;flush_dcache_mmap_lock (file->f_mapping);vma_prio_tree_add (tmp, mpnt);flush_dcache_mmap_unlock (file->f_mapping); -spin_unlock (&file->f_mapping->i_mmap_lock); +up_write (&file->f_mapping->i_mmap_sem); }/*
mm/filemap.c +4 -4
@@ -61,16 +61,16 @@ generic_file_direct_IO(int rw, struct ki/* * Lock ordering: * -* ->i_mmap_lock (vmtruncate) +* ->i_mmap_sem (vmtruncate) * ->private_lock (__free_pte->__set_page_dirty_buffers) * ->swap_lock (exclusive_swap_page, others) * ->mapping->tree_lock * * ->i_mutex -* ->i_mmap_lock (truncate->unmap_mapping_range) +* ->i_mmap_sem (truncate->unmap_mapping_range) * * ->mmap_sem -* ->i_mmap_lock +* ->i_mmap_sem * ->page_table_lock or pte_lock (various, mainly in memory.c) * ->mapping->tree_lock (arch-dependent flush_dcache_mmap_lock) *
@@ -87,7 +87,7 @@ generic_file_direct_IO(int rw, struct ki* ->sb_lock (fs/fs-writeback.c) * ->mapping->tree_lock (__sync_single_inode) * -* ->i_mmap_lock +* ->i_mmap_sem * ->anon_vma.lock (vma_adjust) * * ->anon_vma.lock
mm/filemap_xip.c +2 -2
@@ -184,7 +184,7 @@ __xip_unmap (struct address_space * mappif (!page)return ; -spin_lock (&mapping->i_mmap_lock); +down_read (&mapping->i_mmap_sem); vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, pgoff, pgoff) { mm = vma->vm_mm; address = vma->vm_start +
@@ -206,7 +206,7 @@ __xip_unmap (struct address_space * mappemm_notify (mm, emm_invalidate_end, address, address +PAGE_SIZE ); } -spin_unlock (&mapping->i_mmap_lock); +up_read (&mapping->i_mmap_sem); }/*
mm/fremap.c +2 -2
@@ -205,13 +205,13 @@ asmlinkage long sys_remap_file_pages(uns } goto out; } -spin_lock (&mapping->i_mmap_lock); +down_write (&mapping->i_mmap_sem);flush_dcache_mmap_lock (mapping); vma->vm_flags |=VM_NONLINEAR ;vma_prio_tree_remove (vma, &mapping->i_mmap);vma_nonlinear_insert (vma, &mapping->i_mmap_nonlinear);flush_dcache_mmap_unlock (mapping); -spin_unlock (&mapping->i_mmap_lock); +up_write (&mapping->i_mmap_sem); }emm_notify (mm, emm_invalidate_start, start, end);
mm/hugetlb.c +5 -5
@@ -790,7 +790,7 @@ void __unmap_hugepage_range(struct vm_arstruct page *page;struct page *tmp;/* -* A page gathering list, protected by per file i_mmap_lock. The +* A page gathering list, protected by per file i_mmap_sem. The * lock is used to avoid list corruption from multiple unmapping * of the same page since we are using page->lru. */
@@ -840,9 +840,9 @@ void unmap_hugepage_range(struct vm_area* do nothing in this case. */ if (vma->vm_file) { -spin_lock (&vma->vm_file->f_mapping->i_mmap_lock); +down_write (&vma->vm_file->f_mapping->i_mmap_sem);__unmap_hugepage_range (vma, start, end); -spin_unlock (&vma->vm_file->f_mapping->i_mmap_lock); +up_write (&vma->vm_file->f_mapping->i_mmap_sem); } }
@@ -1085,7 +1085,7 @@ void hugetlb_change_protection(struct vmBUG_ON (address >= end);flush_cache_range (vma, address, end); -spin_lock (&vma->vm_file->f_mapping->i_mmap_lock); +down_write (&vma->vm_file->f_mapping->i_mmap_sem);spin_lock (&mm->page_table_lock);for (; address < end; address +=HPAGE_SIZE ) { ptep =huge_pte_offset (mm, address);
@@ -1100,7 +1100,7 @@ void hugetlb_change_protection(struct vm } }spin_unlock (&mm->page_table_lock); -spin_unlock (&vma->vm_file->f_mapping->i_mmap_lock); +up_write (&vma->vm_file->f_mapping->i_mmap_sem);flush_tlb_range (vma, start, end); }
mm/memory.c +9 -20
@@ -839,7 +839,6 @@ unsigned long unmap_vmas(struct mmu_gathunsigned long tlb_start =0 ;/* For tlb_finish_mmu */ int tlb_start_valid =0 ;unsigned long start = start_addr; -spinlock_t *i_mmap_lock = details? details->i_mmap_lock:NULL ;int fullmm = (*tlbp)->fullmm;for ( ; vma && vma->vm_start < end_addr; vma = vma->vm_next) {
@@ -876,22 +875,12 @@ unsigned long unmap_vmas(struct mmu_gath }tlb_finish_mmu (*tlbp, tlb_start, start); - -if (need_resched () || - (i_mmap_lock &&spin_needbreak (i_mmap_lock))) { -if (i_mmap_lock) { - *tlbp =NULL ; - goto out; - } -cond_resched (); - } - +cond_resched (); *tlbp =tlb_gather_mmu (vma->vm_mm, fullmm); tlb_start_valid =0 ; zap_work =ZAP_BLOCK_SIZE ; } } -out:return start;/* which is now the end (or restart) address */ }
@@ -1757,7 +1746,7 @@ unwritable_page:/* * Helper functions for unmap_mapping_range(). * -* __ Notes on dropping i_mmap_lock to reduce latency while unmapping __ +* __ Notes on dropping i_mmap_sem to reduce latency while unmapping __ * * We have to restart searching the prio_tree whenever we drop the lock, * since the iterator is only valid while the lock is held, and anyway
@@ -1776,7 +1765,7 @@ unwritable_page:* can't efficiently keep all vmas in step with mapping->truncate_count: * so instead reset them all whenever it wraps back to 0 (then go to 1). * mapping->truncate_count and vma->vm_truncate_count are protected by -* i_mmap_lock. +* i_mmap_sem. * * In order to make forward progress despite repeatedly restarting some * large vma, note the restart_addr from unmap_vmas when it breaks out:
@@ -1826,7 +1815,7 @@ again: restart_addr =zap_page_range (vma, start_addr, end_addr - start_addr, details); - need_break =need_resched () ||spin_needbreak (details->i_mmap_lock); + need_break =need_resched ();if (restart_addr >= end_addr) {/* We have now completed this vma: mark it so */
@@ -1840,9 +1829,9 @@ again: goto again; } -spin_unlock (details->i_mmap_lock); +up_write (details->i_mmap_sem);cond_resched (); -spin_lock (details->i_mmap_lock); +down_write (details->i_mmap_sem);return -EINTR ; }
@@ -1936,9 +1925,9 @@ void unmap_mapping_range(struct address_ details.last_index = hba + hlen -1 ;if (details.last_index < details.first_index) details.last_index =ULONG_MAX ; - details.i_mmap_lock = &mapping->i_mmap_lock; + details.i_mmap_sem = &mapping->i_mmap_sem; -spin_lock (&mapping->i_mmap_lock); +down_write (&mapping->i_mmap_sem);/* Protect against endless unmapping loops */ mapping->truncate_count++;
@@ -1953,7 +1942,7 @@ void unmap_mapping_range(struct address_unmap_mapping_range_tree (&mapping->i_mmap, &details);if (unlikely (!list_empty (&mapping->i_mmap_nonlinear)))unmap_mapping_range_list (&mapping->i_mmap_nonlinear, &details); -spin_unlock (&mapping->i_mmap_lock); +up_write (&mapping->i_mmap_sem); }EXPORT_SYMBOL (unmap_mapping_range);
mm/migrate.c +2 -2
@@ -211,12 +211,12 @@ static void remove_file_migration_ptes(sif (!mapping)return ; -spin_lock (&mapping->i_mmap_lock); +down_read (&mapping->i_mmap_sem); vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, pgoff, pgoff)remove_migration_pte (vma, old, new); -spin_unlock (&mapping->i_mmap_lock); +up_read (&mapping->i_mmap_sem); }/*
mm/mmap.c +8 -8
@@ -186,7 +186,7 @@ error: }/* -* Requires inode->i_mapping->i_mmap_lock +* Requires inode->i_mapping->i_mmap_sem */ static void __remove_shared_vm_struct (struct vm_area_struct *vma,struct file *file,struct address_space *mapping)
@@ -214,9 +214,9 @@ void unlink_file_vma(struct vm_area_struif (file) {struct address_space *mapping = file->f_mapping; -spin_lock (&mapping->i_mmap_lock); +down_write (&mapping->i_mmap_sem);__remove_shared_vm_struct (vma, file, mapping); -spin_unlock (&mapping->i_mmap_lock); +up_write (&mapping->i_mmap_sem); } }
@@ -439,7 +439,7 @@ static void vma_link(struct mm_struct *m mapping = vma->vm_file->f_mapping;if (mapping) { -spin_lock (&mapping->i_mmap_lock); +down_write (&mapping->i_mmap_sem); vma->vm_truncate_count = mapping->truncate_count; }anon_vma_lock (vma);
@@ -449,7 +449,7 @@ static void vma_link(struct mm_struct *manon_vma_unlock (vma);if (mapping) -spin_unlock (&mapping->i_mmap_lock); +up_write (&mapping->i_mmap_sem); mm->map_count++;validate_mm (mm);
@@ -536,7 +536,7 @@ again: remove_next = 1 + (end > next-> mapping = file->f_mapping;if (!(vma->vm_flags &VM_NONLINEAR )) root = &mapping->i_mmap; -spin_lock (&mapping->i_mmap_lock); +down_write (&mapping->i_mmap_sem);if (importer && vma->vm_truncate_count != next->vm_truncate_count) {/*
@@ -620,7 +620,7 @@ again: remove_next = 1 + (end > next->if (anon_vma)spin_unlock (&anon_vma->lock);if (mapping) -spin_unlock (&mapping->i_mmap_lock); +up_write (&mapping->i_mmap_sem);if (remove_next) {if (file)
@@ -2064,7 +2064,7 @@ void exit_mmap(struct mm_struct *mm)/* Insert vm structure into process list sorted by address * and into the inode's i_mmap tree. If vm_file is non-NULL -* then i_mmap_lock is taken here. +* then i_mmap_sem is taken here. */ int insert_vm_struct (struct mm_struct * mm,struct vm_area_struct * vma) {
mm/mremap.c +2 -2
@@ -86,7 +86,7 @@ static void move_ptes(struct vm_area_str* and we propagate stale pages into the dst afterward. */ mapping = vma->vm_file->f_mapping; -spin_lock (&mapping->i_mmap_lock); +down_write (&mapping->i_mmap_sem);if (new_vma->vm_truncate_count && new_vma->vm_truncate_count != vma->vm_truncate_count) new_vma->vm_truncate_count =0 ;
@@ -118,7 +118,7 @@ static void move_ptes(struct vm_area_strpte_unmap_nested (new_pte -1 );pte_unmap_unlock (old_pte -1 , old_ptl);if (mapping) -spin_unlock (&mapping->i_mmap_lock); +up_write (&mapping->i_mmap_sem);emm_notify (mm, emm_invalidate_end, old_start, old_end); }
mm/rmap.c +9 -11
@@ -24,7 +24,7 @@* inode->i_alloc_sem (vmtruncate_range) * mm->mmap_sem * page->flags PG_locked (lock_page) -* mapping->i_mmap_lock +* mapping->i_mmap_sem * anon_vma->lock * mm->page_table_lock or pte_lock * zone->lru_lock (in mark_page_accessed, isolate_lru_page)
@@ -430,14 +430,14 @@ static int page_referenced_file(struct p* The page lock not only makes sure that page->mapping cannot * suddenly be NULLified by truncation, it makes sure that the * structure at mapping cannot be freed and reused yet, -* so we can safely take mapping->i_mmap_lock. +* so we can safely take mapping->i_mmap_sem. */ BUG_ON (!PageLocked (page)); -spin_lock (&mapping->i_mmap_lock); +down_read (&mapping->i_mmap_sem);/* -* i_mmap_lock does not stabilize mapcount at all, but mapcount +* i_mmap_sem does not stabilize mapcount at all, but mapcount * is more likely to be accurate if we note it after spinning. */ mapcount =page_mapcount (page);
@@ -460,7 +460,7 @@ static int page_referenced_file(struct pbreak ; } -spin_unlock (&mapping->i_mmap_lock); +up_read (&mapping->i_mmap_sem);return referenced; }
@@ -546,12 +546,12 @@ static int page_mkclean_file(struct addrBUG_ON (PageAnon (page)); -spin_lock (&mapping->i_mmap_lock); +down_read (&mapping->i_mmap_sem); vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, pgoff, pgoff) {if (vma->vm_flags &VM_SHARED ) ret +=page_mkclean_one (page, vma); } -spin_unlock (&mapping->i_mmap_lock); +up_read (&mapping->i_mmap_sem);return ret; }
@@ -990,7 +990,7 @@ static int try_to_unmap_file(struct pageunsigned long max_nl_size =0 ;unsigned int mapcount; -spin_lock (&mapping->i_mmap_lock); +down_read (&mapping->i_mmap_sem); vma_prio_tree_foreach(vma, &iter, &mapping->i_mmap, pgoff, pgoff) { ret =try_to_unmap_one (page, vma, migration);if (ret ==SWAP_FAIL || !page_mapped (page))
@@ -1027,7 +1027,6 @@ static int try_to_unmap_file(struct page mapcount =page_mapcount (page);if (!mapcount) goto out; -cond_resched_lock (&mapping->i_mmap_lock); max_nl_size = (max_nl_size +CLUSTER_SIZE -1 ) &CLUSTER_MASK ;if (max_nl_cursor ==0 )
@@ -1049,7 +1048,6 @@ static int try_to_unmap_file(struct page } vma->vm_private_data = (void *) max_nl_cursor; } -cond_resched_lock (&mapping->i_mmap_lock); max_nl_cursor +=CLUSTER_SIZE ; }while (max_nl_cursor <= max_nl_size);
@@ -1061,7 +1059,7 @@ static int try_to_unmap_file(struct pagelist_for_each_entry (vma, &mapping->i_mmap_nonlinear, shared.vm_set.list) vma->vm_private_data =NULL ; out: -spin_unlock (&mapping->i_mmap_lock); +up_write (&mapping->i_mmap_sem);return ret; }
PATCH 4/9 Remove tlb pointer from the parameters of unmap vmas
In reply to Christoph LameterWe no longer abort unmapping in unmap vmas because we can reschedule while unmapping since we are holding a semaphore. This would allow moving more of the tlb flusing into unmap_vmas reducing code in various places.
--- include/linux/mm.h | 3 +-- mm/memory.c | 43 +++++++++++++++++-------------------------- mm/mmap.c | 18 +++--------------- 3 files changed, 21 insertions(+), 43 deletions(-)
include/linux/mm.h +1 -2
@@ -723,8 +723,7 @@ struct zap_details {struct page *vm_normal_page (struct vm_area_struct *,unsigned long ,pte_t );unsigned long zap_page_range (struct vm_area_struct *vma,unsigned long address,unsigned long size,struct zap_details *); -unsigned long unmap_vmas (struct mmu_gather **tlb, -struct vm_area_struct *start_vma,unsigned long start_addr, +unsigned long unmap_vmas (struct vm_area_struct *start_vma,unsigned long start_addr,unsigned long end_addr,unsigned long *nr_accounted,struct zap_details *);
mm/memory.c +17 -26
92 lines not on this page · raw message
mm/mmap.c +3 -15
@@ -1741,19 +1741,12 @@ static void unmap_region(struct mm_struc unsigned long start, unsigned long end) {struct vm_area_struct *next = prev? prev->vm_next: mm->mmap; -struct mmu_gather *tlb;unsigned long nr_accounted =0 ; -emm_notify (mm, emm_invalidate_start, start, end); -lru_add_drain (); - tlb =tlb_gather_mmu (mm,0 ); -update_hiwater_rss (mm); -unmap_vmas (&tlb, vma, start, end, &nr_accounted,NULL ); +unmap_vmas (vma, start, end, &nr_accounted,NULL );vm_unacct_memory (nr_accounted); -tlb_finish_mmu (tlb, start, end);free_pgtables (vma, prev? prev->vm_end:FIRST_USER_ADDRESS , next? next->vm_start:0 ); -emm_notify (mm, emm_invalidate_end, start, end); }/*
@@ -2033,7 +2026,6 @@ EXPORT_SYMBOL(do_brk);/* Release all mmaps. */ void exit_mmap (struct mm_struct *mm) { -struct mmu_gather *tlb;struct vm_area_struct *vma = mm->mmap;unsigned long nr_accounted =0 ;unsigned long end;
@@ -2041,15 +2033,11 @@ void exit_mmap(struct mm_struct *mm)/* mm's last user has gone, and its about to be pulled down */ arch_exit_mmap (mm);emm_notify (mm, emm_release,0 ,TASK_SIZE ); -lru_add_drain ();flush_cache_mm (mm); - tlb =tlb_gather_mmu (mm,1 ); -/* Don't update_hiwater_rss(mm) here, do_exit already did */ -/* Use -1 here to ensure all VMAs in the mm are unmapped */ - end =unmap_vmas (&tlb, vma,0 ,-1 , &nr_accounted,NULL ); + + end =unmap_vmas (vma,0 ,-1 , &nr_accounted,NULL );vm_unacct_memory (nr_accounted); -tlb_finish_mmu (tlb,0 , end);free_pgtables (vma,FIRST_USER_ADDRESS ,0 );/*
Signature · 5 lines
------------------------------------------------------------------------- Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
PATCH 5/9 Convert anon_vma lock to rw_sem and refcount
In reply to Christoph LameterConvert the anon_vma spinlock to a rw semaphore. This allows concurrent traversal of reverse maps for try_to_unmap and page_mkclean. It also allows the calling of sleeping functions from reverse map traversal.
An additional complication is that rcu is used in some context to guarantee the presence of the anon_vma while we acquire the lock. We cannot take a semaphore within an rcu critical section. Add a refcount to the anon_vma structure which allow us to give an existence guarantee for the anon_vma structure independent of the spinlock or the list contents.
The refcount can then be taken within the RCU section. If it has been taken successfully then the refcount guarantees the existence of the anon_vma. The refcount in anon_vma also allows us to fix a nasty issue in page migration where we fudged by using rcu for a long code path to guarantee the existence of the anon_vma.
The refcount in general allows a shortening of RCU critical sections since we can do a rcu_unlock after taking the refcount. This is particularly relevant if the anon_vma chains contain hundreds of entries.
Issues:
- Atomic overhead increases in situations where a new reference to the anon_vma has to be established or removed. Overhead also increases when a speculative reference is used (try_to_unmap, page_mkclean, page migration). There is also the more frequent processor change due to up_xxx letting waiting tasks run first.
This results in f.e. the Aim9 brk performance test to got down by 10-15%.
--- include/linux/rmap.h | 20 ++++++++++++++++--- mm/migrate.c | 26 ++++++++++--------------- mm/mmap.c | 4 +-- mm/rmap.c | 53 +++++++++++++++++++++++++++++---------------------- 4 files changed, 61 insertions(+), 42 deletions(-)
include/linux/rmap.h +17 -3
@@ -25,7 +25,8 @@* pointing to this anon_vma once its vma list is empty. */ struct anon_vma { -spinlock_t lock;/* Serialize access to vma list */ +atomic_t refcount;/* vmas on the list */ +struct rw_semaphore sem;/* Serialize access to vma list */ struct list_head head;/* List of private "related" vmas */ };
@@ -43,18 +44,31 @@ static inline void anon_vma_free(structkmem_cache_free (anon_vma_cachep, anon_vma); } +struct anon_vma *grab_anon_vma (struct page *page); + +static inline void get_anon_vma (struct anon_vma *anon_vma) +{ +atomic_inc (&anon_vma->refcount); +} + +static inline void put_anon_vma (struct anon_vma *anon_vma) +{ +if (atomic_dec_and_test (&anon_vma->refcount)) +anon_vma_free (anon_vma); +} +static inline void anon_vma_lock (struct vm_area_struct *vma) {struct anon_vma *anon_vma = vma->anon_vma;if (anon_vma) -spin_lock (&anon_vma->lock); +down_write (&anon_vma->sem); }static inline void anon_vma_unlock (struct vm_area_struct *vma) {struct anon_vma *anon_vma = vma->anon_vma;if (anon_vma) -spin_unlock (&anon_vma->lock); +up_write (&anon_vma->sem); }/*
mm/migrate.c +11 -15
@@ -235,15 +235,16 @@ static void remove_anon_migration_ptes(sreturn ;/* -* We hold the mmap_sem lock. So no need to call page_lock_anon_vma. +* We hold either the mmap_sem lock or a reference on the +* anon_vma. So no need to call page_lock_anon_vma. */ anon_vma = (struct anon_vma *) (mapping -PAGE_MAPPING_ANON ); -spin_lock (&anon_vma->lock); +down_read (&anon_vma->sem);list_for_each_entry (vma, &anon_vma->head, anon_vma_node)remove_migration_pte (vma, old, new); -spin_unlock (&anon_vma->lock); +up_read (&anon_vma->sem); }/*
@@ -623,7 +624,7 @@ static int unmap_and_move(new_page_t getint rc =0 ;int *result =NULL ;struct page *newpage =get_new_page (page, private, &result); -int rcu_locked =0 ; +struct anon_vma *anon_vma =NULL ;int charge =0 ;if (!newpage)
@@ -647,16 +648,14 @@ static int unmap_and_move(new_page_t get }/* * By try_to_unmap(), page->mapcount goes down to 0 here. In this case, -* we cannot notice that anon_vma is freed while we migrates a page. +* we cannot notice that anon_vma is freed while we migrate a page. * This rcu_read_lock() delays freeing anon_vma pointer until the end * of migration. File cache pages are no problem because of page_lock() * File Caches may use write_page() or lock_page() in migration, then, * just care Anon page here. */ -if (PageAnon (page)) { -rcu_read_lock (); - rcu_locked =1 ; - } +if (PageAnon (page)) + anon_vma =grab_anon_vma (page);/* * Corner case handling:
@@ -674,10 +673,7 @@ static int unmap_and_move(new_page_t getif (!PageAnon (page) &&PagePrivate (page)) {/* * Go direct to try_to_free_buffers() here because -* a) that's what try_to_release_page() would do anyway -* b) we may be under rcu_read_lock() here, so we can't -* use GFP_KERNEL which is what try_to_release_page() -* needs to be effective. +* that's what try_to_release_page() would do anyway */ try_to_free_buffers (page); }
@@ -698,8 +694,8 @@ static int unmap_and_move(new_page_t get }else if (charge)mem_cgroup_end_migration (newpage); rcu_unlock: -if (rcu_locked) -rcu_read_unlock (); +if (anon_vma) +put_anon_vma (anon_vma); unlock:
mm/rmap.c +31 -22
127 lines not on this page · raw message
mm/mmap.c +2 -2
@@ -564,7 +564,7 @@ again: remove_next = 1 + (end > next->if (vma->anon_vma) anon_vma = vma->anon_vma;if (anon_vma) { -spin_lock (&anon_vma->lock); +down_write (&anon_vma->sem);/* * Easily overlooked: when mprotect shifts the boundary, * make sure the expanding vma has anon_vma set if the
@@ -618,7 +618,7 @@ again: remove_next = 1 + (end > next-> }if (anon_vma) -spin_unlock (&anon_vma->lock); +up_write (&anon_vma->sem);if (mapping)up_write (&mapping->i_mmap_sem);
Signature · 5 lines
------------------------------------------------------------------------- Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
Re: [patch 5/9] Convert anon_vma lock to rw_sem and refcount
Quoted 1 line · Christoph Lameter
This results in f.e. the Aim9 brk performance test to got down by 10-15%.
I guess it's more likely because of overscheduling for small crtitical sections, did you counted the total number of context switches? I guess there will be a lot more with your patch applied. That regression is a showstopper and it is the reason why I've suggested before to add a CONFIG_XPMEM or CONFIG_MMU_NOTIFIER_SLEEP config option to make the VM locks sleep capable only when XPMEM=y (PREEMPT_RT will enable it too). Thanks for doing the benchmark work!
-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
Re: [patch 5/9] Convert anon_vma lock to rw_sem and refcount
Quoted 10 lines · Andrea Arcangeli
Quoted 1 line · Christoph Lameter
This results in f.e. the Aim9 brk performance test to got down by 10-15%.
I guess it's more likely because of overscheduling for small crtitical sections, did you counted the total number of context switches? I guess there will be a lot more with your patch applied. That regression is a showstopper and it is the reason why I've suggested before to add a
CONFIG_XPMEMorCONFIG_MMU_NOTIFIER_SLEEPconfig option to make the VM locks sleep capable only when XPMEM=y (PREEMPT_RTwill enable it too). Thanks for doing the benchmark work!
There are more context switches if locks are contended.
But that has actually also some good aspects because we avoid busy loops and can potentially continue work in another process.
-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
Re: [patch 5/9] Convert anon_vma lock to rw_sem and refcount
Quoted 13 lines · Christoph Lameter
Quoted 10 lines · Andrea Arcangeli
Quoted 1 line · Christoph Lameter
This results in f.e. the Aim9 brk performance test to got down by 10-15%.
I guess it's more likely because of overscheduling for small crtitical sections, did you counted the total number of context switches? I guess there will be a lot more with your patch applied. That regression is a showstopper and it is the reason why I've suggested before to add a
CONFIG_XPMEMorCONFIG_MMU_NOTIFIER_SLEEPconfig option to make the VM locks sleep capable only when XPMEM=y (PREEMPT_RTwill enable it too). Thanks for doing the benchmark work!
Quoted 4 lines
There are more context switches if locks are contended.
But that has actually also some good aspects because we avoid busy loops and can potentially continue work in another process.
That would be the case if the "wait time" would be longer than the scheduling time, the whole point is that with anonvma the write side is so fast it's likely never worth scheduling (probably not even with preempt-rt for the write side, the read side is an entirely different matter but the read side can run concurrently if the system is heavy paging), hence the slowdown. What you benchmarked is the write side, which is also the fast path when the system is heavily CPU bound. I've to say aim is a great benchmark to test this regression.
But I think a config option will solve all of this.
Re: [patch 5/9] Convert anon_vma lock to rw_sem and refcount
Quoted 3 lines · Andrea Arcangeli
paging), hence the slowdown. What you benchmarked is the write side, which is also the fast path when the system is heavily CPU bound. I've to say aim is a great benchmark to test this regression.
I am a bit surprised that brk performance is that important. There may be other measurement that have to be made to assess how this would impact a real load.
Re: [patch 5/9] Convert anon_vma lock to rw_sem and refcount
Quoted 1 line · Christoph Lameter
I am a bit surprised that brk performance is that important. There may be
I think it's not brk but fork that is being slowed down, did you oprofile? AIM forks a lot... The write side fast path generating the overscheduling I guess is when the new vmas are created for the child and queued in the parent anon-vma in O(1), so immediate, even preempt-rt would be ok with it spinning and not scheduling, it's just a list_add (much faster than schedule() indeed). Every time there's a collision when multiple child forks simultaneously and they all try to queue in the same anon-vma, things will slowdown.
PATCH 6/9 This patch exports zap_page_range as it is needed by XPMEM.
In reply to Christoph LameterXPMEM would have used sys_madvise() except that madvise_dontneed() returns an -EINVAL if VM_PFNMAP is set, which is always true for the pages XPMEM imports from other partitions and is also true for uncached pages allocated locally via the mspec allocator. XPMEM needs zap_page_range() functionality for these types of pages as well as 'normal' pages.
--- mm/memory.c | 1 + 1 file changed, 1 insertion(+)
mm/memory.c +1 -0
@@ -901,6 +901,7 @@ unsigned long zap_page_range(struct vm_areturn unmap_vmas (vma, address, end, &nr_accounted, details); } +EXPORT_SYMBOL_GPL (zap_page_range);/* * Do a quick page-table lookup for a single page.
Signature · 5 lines
------------------------------------------------------------------------- Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
PATCH 7/9 Locking rules for taking multiple mmap_sem locks.
In reply to Christoph LameterThis patch adds a lock ordering rule to avoid a potential deadlock when multiple mmap_sems need to be locked.
--- mm/filemap.c | 3 +++ 1 file changed, 3 insertions(+)
mm/filemap.c +3 -0
@@ -80,6 +80,9 @@ generic_file_direct_IO(int rw, struct ki* ->i_mutex (generic_file_buffered_write) * ->mmap_sem (fault_in_pages_readable->do_page_fault) * +* When taking multiple mmap_sems, one should lock the lowest-addressed +* one first proceeding on up to the highest-addressed one. +* * ->i_mutex * ->i_alloc_sem (various) *
Signature · 5 lines
------------------------------------------------------------------------- Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
PATCH 8/9 XPMEM: The device driver
In reply to Christoph LameterXPmem device driver that allows sharing of address spaces across different instances of Linux. [Experimental, lots of issues still to be fixed].
drivers/misc/xp/Makefile new file +16 -0
@@ -0,0 +1,16 @@ +# drivers/misc/xp/Makefile +# +# This file is subject to the terms and conditions of the GNU General Public +# License. See the file "COPYING" in the main directory of this archive +# for more details. +# +# Copyright (C) 1999,2001-2008 Silicon Graphics, Inc. All Rights Reserved. +# + +# This is just temporary. Please do not comment. I am waiting for Dean +# Nelson's XPC patches to go in and will modify files introduced by his patches +# to enable. +obj-m += xpmem.o +xpmem-y := xpmem_main.o xpmem_make.o xpmem_get.o \ + xpmem_attach.o xpmem_pfn.o \ + xpmem_misc.o
drivers/misc/xp/xpmem_attach.c new file +824 -0
824 lines not on this page · raw message
drivers/misc/xp/xpmem_get.c new file +343 -0
343 lines not on this page · raw message
drivers/misc/xp/xpmem_main.c new file +440 -0
440 lines not on this page · raw message
drivers/misc/xp/xpmem_make.c new file +249 -0
249 lines not on this page · raw message
drivers/misc/xp/xpmem_misc.c new file +367 -0
367 lines not on this page · raw message
drivers/misc/xp/xpmem_pfn.c new file +1242 -0
761 lines not on this page · raw message
drivers/misc/xp/xpmem.h new file +130 -0
130 lines not on this page · raw message
drivers/misc/xp/xpmem_private.h new file +783 -0
783 lines not on this page · raw message
The rest of this diff is not shown. View the raw message.
Signature · 5 lines
------------------------------------------------------------------------- Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
Part of the diff is not shown here. View the raw message.
PATCH 9/9 XPMEM: Simple example
In reply to Christoph LameterA simple test program (well actually a pair). They are fairly easy to use.
NOTE: the xpmem.h is copied from the kernel/drivers/misc/xp/xpmem.h file.
Type make. Then from one session, type ./A1. Grab the first line of output which should begin with ./A2 and paste the whole line into a second session. Paste as many times as you like. Each pass will increment the value one additional time. When you are tired, hit enter in the first window. You should see the same value printed from A1 as you most recently received from A2.
--- xpmem_test/A1.c | 64 +++++++++++++++++++++++++ xpmem_test/A2.c | 70 ++++++++++++++++++++++++++++ xpmem_test/Makefile | 14 +++++ xpmem_test/xpmem.h | 130 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 278 insertions(+)
xpmem_test/A1.c new file +64 -0
@@ -0,0 +1,64 @@ +/* +* Simple test program. Makes a segment then waits for an input line +* and finally prints the value of the first integer of that segment. +*/ + +#include <errno.h> +#include <fcntl.h> +#include <stdio.h> +#include <stdlib.h> +#include <stropts.h> +#include <sys/mman.h> +#include <sys/stat.h> +#include <sys/types.h> +#include <unistd.h> + +#include "xpmem.h" + +int xpmem_fd; + +int +main (int argc,char **argv) +{ +char input[32 ]; +struct xpmem_cmd_make make_info; +int *data_block; +int ret; +__s64 segid; + + xpmem_fd =open ("/dev/xpmem" ,O_RDWR ); +if (xpmem_fd ==-1 ) { +perror ("Opening /dev/xpmem" ); +return -1 ; + } + + data_block =mmap (NULL ,getpagesize (),PROT_READ |PROT_WRITE , +MAP_SHARED |MAP_ANONYMOUS ,0 ,0 ); +if (data_block ==MAP_FAILED ) { +perror ("Creating mapping." ); +return -1 ; + } + data_block[0 ] =1 ; + + make_info.vaddr = (__u64 ) data_block; + make_info.size =getpagesize (); + make_info.permit_type =XPMEM_PERMIT_MODE ; + make_info.permit_value = (__u64 )0600 ; + ret =ioctl (xpmem_fd,XPMEM_CMD_MAKE , &make_info); +if (ret !=0 ) { +perror ("xpmem_make" ); +return -1 ; + } + + segid = make_info.segid; +printf ("./A2 %d %d %d %d\ndata_block[0] = %d\n" , + (int )(segid >>48 &0xffff ), (int )(segid >>32 &0xffff ), + (int )(segid >>16 &0xffff ), (int )(segid &0xffff ), + data_block[0 ]); +printf ("Waiting for input before exiting.\n" ); +fscanf (stdin,"%s" , input); + +printf ("data_block[0] = %d\n" , data_block[0 ]); + +return 0 ; +}
xpmem_test/A2.c new file +70 -0
70 lines not on this page · raw message
xpmem_test/Makefile new file +14 -0
@@ -0,0 +1,14 @@ + +default: A1 A2 + +A1: A1.c xpmem.h + gcc -Wall -o A1 A1.c + +A2: A2.c xpmem.h + gcc -Wall -o A2 A2.c + +indent: + indent -npro -kr -i8 -ts8 -sob -l80 -ss -ncs -cp1 -psl -npcs A1.c A2.c + +clean: + rm -f A1 A2 *~
xpmem_test/xpmem.h new file +130 -0
130 lines not on this page · raw message
Signature · 5 lines
------------------------------------------------------------------------- Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace