Optimize page_remove_rmap for anon pages
Subsystems (2)
PATCH 1/1 Optimize page_remove_rmap for anon pages
Greetings,
with a recent performance analysis we discovered something interesting in regard to the physical dirty bits found on s390. The page_remove_rmap function stands out when areas of anonymous memory gets unmapped. The reason is the transfer of the dirty bit from the page storage key to the struct page when the last mapper of a page is removed. For anonymous pages that cease to exist this is superfluous. Without the storage key operations process termination is noticable faster, e.g. for a gcc test case we got a speedup of 2%.
To get this done page_remove_rmap needs to know if the page dirty bit can be ignored. The page_test_dirty / page_clear_dirty call can only be avoided if page_remove_rmap is called from zap_pte_range or do_wp_page. If it is called from any other place - in particular try_to_unmap_one - the page dirty bit may not be ignored.
The patch below introduces a new function to do that, in lack of a better name I called it page_zap_rmap. Comments ?
Signature · 176 lines
blue skies, Martin. "Reality continues to ruin my life." - Calvin. --- Subject: [PATCH] Optimize page_remove_rmap for anon pages From: Martin Schwidefsky <[email protected]> For anonymous pages the check for the physical dirty bit in page_remove_rmap is unnecessary if page_remove_rmap is called from zap_pte_range or do_wp_page. The instruction that are used to check and reset the dirty bit are expensive. Removing the check noticably speeds up process exit. The micro benchmark which repeatedly executes an empty shell script gets about 4% faster. Signed-off-by: Martin Schwidefsky <[email protected]> --- include/linux/rmap.h | 5 +++ mm/memory.c | 9 +++--- mm/rmap.c | 73 ++++++++++++++++++++++++++++++++++++++++----------- 3 files changed, 68 insertions(+), 19 deletions(-) diff -urpN linux-2.6/include/linux/rmap.h linux-2.6-patched/include/linux/rmap.h --- linux-2.6/include/linux/rmap.h 2008-04-17 04:49:44.000000000 +0200 +++ linux-2.6-patched/include/linux/rmap.h 2008-05-29 14:17:06.000000000 +0200 @@ -74,6 +74,11 @@ void page_add_anon_rmap(struct page *, s void page_add_new_anon_rmap(struct page *, struct vm_area_struct *, unsigned long); void page_add_file_rmap(struct page *); void page_remove_rmap(struct page *, struct vm_area_struct *); +#ifdef __HAVE_ARCH_PAGE_TEST_DIRTY +void page_zap_rmap(struct page *, struct vm_area_struct *); +#else +#define page_zap_rmap page_remove_rmap +#endif #ifdef CONFIG_DEBUG_VM void page_dup_rmap(struct page *page, struct vm_area_struct *vma, unsigned long address); diff -urpN linux-2.6/mm/memory.c linux-2.6-patched/mm/memory.c --- linux-2.6/mm/memory.c 2008-05-29 14:16:48.000000000 +0200 +++ linux-2.6-patched/mm/memory.c 2008-05-29 14:17:06.000000000 +0200 @@ -731,16 +731,17 @@ static unsigned long zap_pte_range(struc addr) != page->index) set_pte_at(mm, addr, pte, pgoff_to_pte(page->index)); - if (PageAnon(page)) + if (PageAnon(page)) { anon_rss--; - else { + page_zap_rmap(page, vma); + } else { if (pte_dirty(ptent)) set_page_dirty(page); if (pte_young(ptent)) SetPageReferenced(page); file_rss--; + page_remove_rmap(page, vma); } - page_remove_rmap(page, vma); tlb_remove_page(tlb, page); continue; } @@ -1757,7 +1758,7 @@ gotten: page_table = pte_offset_map_lock(mm, pmd, address, &ptl); if (likely(pte_same(*page_table, orig_pte))) { if (old_page) { - page_remove_rmap(old_page, vma); + page_zap_rmap(old_page, vma); if (!PageAnon(old_page)) { dec_mm_counter(mm, file_rss); inc_mm_counter(mm, anon_rss); diff -urpN linux-2.6/mm/rmap.c linux-2.6-patched/mm/rmap.c --- linux-2.6/mm/rmap.c 2008-05-29 14:16:48.000000000 +0200 +++ linux-2.6-patched/mm/rmap.c 2008-05-29 14:17:06.000000000 +0200 @@ -645,6 +645,33 @@ void page_dup_rmap(struct page *page, st #endif /** + * page_check_mapcount - check for negative number of page mappers + * @page: page to check mapping count + * @vma: the vm area in which the mapping is removed + */ +static void page_check_mapcount(struct page *page, struct vm_area_struct *vma) +{ + if (likely(page_mapcount(page) >= 0)) + return; + printk (KERN_EMERG "Eeek! page_mapcount(page) went negative! (%d)\n", + page_mapcount(page)); + printk (KERN_EMERG " page pfn = %lx\n", page_to_pfn(page)); + printk (KERN_EMERG " page->flags = %lx\n", page->flags); + printk (KERN_EMERG " page->count = %x\n", page_count(page)); + printk (KERN_EMERG " page->mapping = %p\n", page->mapping); + print_symbol (KERN_EMERG " vma->vm_ops = %s\n", + (unsigned long)vma->vm_ops); + if (vma->vm_ops) { + print_symbol (KERN_EMERG " vma->vm_ops->fault = %s\n", + (unsigned long)vma->vm_ops->fault); + } + if (vma->vm_file && vma->vm_file->f_op) + print_symbol (KERN_EMERG " vma->vm_file->f_op->mmap = %s\n", + (unsigned long)vma->vm_file->f_op->mmap); + BUG(); +} + +/** * page_remove_rmap - take down pte mapping from a page * @page: page to remove mapping from * @vma: the vm area in which the mapping is removed @@ -654,21 +681,7 @@ void page_dup_rmap(struct page *page, st void page_remove_rmap(struct page *page, struct vm_area_struct *vma) { if (atomic_add_negative(-1, &page->_mapcount)) { - if (unlikely(page_mapcount(page) < 0)) { - printk (KERN_EMERG "Eeek! page_mapcount(page) went negative! (%d)\n", page_mapcount(page)); - printk (KERN_EMERG " page pfn = %lx\n", page_to_pfn(page)); - printk (KERN_EMERG " page->flags = %lx\n", page->flags); - printk (KERN_EMERG " page->count = %x\n", page_count(page)); - printk (KERN_EMERG " page->mapping = %p\n", page->mapping); - print_symbol (KERN_EMERG " vma->vm_ops = %s\n", (unsigned long)vma->vm_ops); - if (vma->vm_ops) { - print_symbol (KERN_EMERG " vma->vm_ops->fault = %s\n", (unsigned long)vma->vm_ops->fault); - } - if (vma->vm_file && vma->vm_file->f_op) - print_symbol (KERN_EMERG " vma->vm_file->f_op->mmap = %s\n", (unsigned long)vma->vm_file->f_op->mmap); - BUG(); - } - + page_check_mapcount(page, vma); /* * It would be tidy to reset the PageAnon mapping here, * but that might overwrite a racing page_add_anon_rmap @@ -689,6 +702,36 @@ void page_remove_rmap(struct page *page, } } +#ifdef __HAVE_ARCH_PAGE_TEST_DIRTY +/** + * page_zap_rmap - take down pte mapping from a page without dirty bit check + * @page: page to remove mapping from + * @vma: the vm area in which the mapping is removed + * + * The caller needs to hold the pte lock. For s390 page_zap_rmap differs + * from page_remove_rmap, it will not check the physical page dirty bit. + */ +void page_zap_rmap(struct page *page, struct vm_area_struct *vma) +{ + if (atomic_add_negative(-1, &page->_mapcount)) { + page_check_mapcount(page, vma); + /* + * It would be tidy to reset the PageAnon mapping here, + * but that might overwrite a racing page_add_anon_rmap + * which increments mapcount after us but sets mapping + * before us: so leave the reset to free_hot_cold_page, + * and remember that it's only reliable while mapped. + * Leaving it set also helps swapoff to reinstate ptes + * faster for those pages still in swapcache. + */ + mem_cgroup_uncharge_page(page); + + __dec_zone_page_state(page, + PageAnon(page) ? NR_ANON_PAGES : NR_FILE_MAPPED); + } +} +#endif + /* * Subfunctions of try_to_unmap: try_to_unmap_one called * repeatedly from either try_to_unmap_anon or try_to_unmap_file. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 8 lines · Martin Schwidefsky
Greetings,
with a recent performance analysis we discovered something interesting in regard to the physical dirty bits found on s390. Thepage_remove_rmapfunction stands out when areas of anonymous memory gets unmapped. The reason is the transfer of the dirty bit from the page storage key to the struct page when the last mapper of a page is removed. For anonymous pages that cease to exist this is superfluous. Without the storage key operations process termination is noticable faster, e.g. for a gcc test
Quoted 8 lines
case we got a speedup of 2%.
To get this donepage_remove_rmapneeds to know if the page dirty bit can be ignored. Thepage_test_dirty/page_clear_dirtycall can only be avoided ifpage_remove_rmapis called fromzap_pte_rangeordo_wp_page. If it is called from any other place - in particulartry_to_unmap_one- the page dirty bit may not be ignored.
The patch below introduces a new function to do that, in lack of a better name I called itpage_zap_rmap. Comments ?
I don't know if it is that simple, is it?
I don't know how you are guaranteeing the given page ceases to exist. Even checking for the last mapper of the page (which you don't appear to do anyway) isn't enough because there could be a swapcount, in which case you should still have to mark the page as dirty.
For example (I think, unless s390 somehow propogates the dirty page bit some other way that I've missed), wouldn't the following break:
process p1 allocates anonymous page A
p1 dirties A
p1 forks p2, A now has a mapcount of 2
p2 VM_LOCKs A (something to prevent it being swapped) page reclaim unmaps p1's pte, fails on p2
p2 exits, page_dirty does not get checked because of this patch page has mapcount 0, PG_dirty is clear
Page reclaim can drop it without writing it to swap
Or am I misunderstanding something?
As far as the general idea goes, it might be possible to avoid the check somehow, but you'd want to be pretty sure of yourself before diverging the s390 path further from the common code base, no?
The "easy" way to do it might be just unconditionally mark the page as dirty in this path (if the pte was writeable), so you can avoid the page_test_dirty check and be sure of not missing the dirty bit.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
On Tue, 2008-06-03 at 09:57 +1000, Nick Piggin wrote:
First of all: thanks for looking into this. Games with the dirty bit are scary and any change needs careful consideration.
Quoted 1 line
I don't know if it is that simple, is it?
It should be analog to the fact that for the two place the page_zap_rmap function is supposed to be used the pte dirty bit isn't checked as well.
Quoted 8 lines
I don't know how you are guaranteeing the given page ceases to exist. Even checking for the last mapper of the page (which you don't appear to do anyway) isn't enough because there could be a swapcount, in which case you should still have to mark the page as dirty.
For example (I think, unless s390 somehow propogates the dirty page bit some other way that I've missed), wouldn't the following break:
Quoted 8 lines
process p1 allocates anonymous page A
p1 dirties A
p1 forks p2, A now has a mapcount of 2
p2VM_LOCKsA (something to prevent it being swapped) page reclaim unmaps p1's pte, fails on p2
p2 exits,page_dirtydoes not get checked because of this patch page has mapcount 0,PG_dirtyis clear
Page reclaim can drop it without writing it to swap
Indeed, this would break. Even without the VM_LOCK there is a race of try_to_unmap vs. process exit.
Quoted 3 lines
As far as the general idea goes, it might be possible to avoid the check somehow, but you'd want to be pretty sure of yourself before diverging the s390 path further from the common code base, no?
I don't want to diverge more than necessary. But the performance gains of the SSKE/ISKE avoidance makes it worthwhile for s390, no?
Quoted 3 lines
The "easy" way to do it might be just unconditionally mark the page as dirty in this path (if the pte was writeable), so you can avoid the
page_test_dirtycheck and be sure of not missing the dirty bit.
Hmm, but then an mprotect() can change the pte to read-ony and we'd miss the dirty bit again. Back to the drawing board.
By the way there is another SSKE I want to get rid of: __SetPageUptodate does a page_clear_dirty(). For all uses of __SetPageUptodate the page will be dirty after the application did its first write. To clear the page dirty bit only to have it set again shortly after doesn't make much sense to me. Has there been any particular reason for the page_clear_dirty in __SetPageUptodate ?
Signature · 11 lines
blue skies, Martin. "Reality continues to ruin my life." - Calvin. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 9 lines · Martin Schwidefsky
On Tue, 2008-06-03 at 09:57 +1000, Nick Piggin wrote:
First of all: thanks for looking into this. Games with the dirty bit are scary and any change needs careful consideration.
Quoted 1 line
I don't know if it is that simple, is it?
It should be analog to the fact that for the two place the
page_zap_rmapfunction is supposed to be used the pte dirty bit isn't checked as well.
pte dirty bit is checked in zap_pte_range. In do_wp_page, the pte dirty bit is not checked because it cannot have been dirtied via that mapping. However, this may not necessarily be true in the s390 case where it might be dirtied by _another_ mapping which has subsequently exited but not propogated the physical dirty bit (I don't know, but I'm just wary about it).
Quoted 20 lines
Quoted 8 lines
I don't know how you are guaranteeing the given page ceases to exist. Even checking for the last mapper of the page (which you don't appear to do anyway) isn't enough because there could be a swapcount, in which case you should still have to mark the page as dirty.
For example (I think, unless s390 somehow propogates the dirty page bit some other way that I've missed), wouldn't the following break:
Quoted 8 lines
process p1 allocates anonymous page A
p1 dirties A
p1 forks p2, A now has a mapcount of 2
p2VM_LOCKsA (something to prevent it being swapped) page reclaim unmaps p1's pte, fails on p2
p2 exits,page_dirtydoes not get checked because of this patch page has mapcount 0,PG_dirtyis clear
Page reclaim can drop it without writing it to swapIndeed, this would break. Even without the
VM_LOCKthere is a race oftry_to_unmapvs. process exit.
Quoted 6 lines
Quoted 3 lines
As far as the general idea goes, it might be possible to avoid the check somehow, but you'd want to be pretty sure of yourself before diverging the s390 path further from the common code base, no?
I don't want to diverge more than necessary. But the performance gains of the SSKE/ISKE avoidance makes it worthwhile for s390, no?
I guess it's worth exploring.
Quoted 6 lines
Quoted 3 lines
The "easy" way to do it might be just unconditionally mark the page as dirty in this path (if the pte was writeable), so you can avoid the
page_test_dirtycheck and be sure of not missing the dirty bit.Hmm, but then an
mprotect()can change the pte to read-ony and we'd miss the dirty bit again. Back to the drawing board.
Hmm, I guess you _could_ set_page_dirty in mprotect.
Quoted 6 lines
By the way there is another SSKE I want to get rid of:
__SetPageUptodatedoes apage_clear_dirty(). For all uses of__SetPageUptodatethe page will be dirty after the application did its first write. To clear the page dirty bit only to have it set again shortly after doesn't make much sense to me. Has there been any particular reason for thepage_clear_dirtyin__SetPageUptodate?
I guess it is just to match SetPageUptodate. Not all __SetPageUptodate paths may necessarily dirty the page, FWIW.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Quoted 6 lines · Nick Piggin
pte dirty bit is checked in
zap_pte_range. Indo_wp_page, the pte dirty bit is not checked because it cannot have been dirtied via that mapping. However, this may not necessarily be true in the s390 case where it might be dirtied by _another_ mapping which has subsequently exited but not propogated the physical dirty bit (I don't know, but I'm just wary about it).
Yes, what is needed is a check that tells us if the page content is still needed after the last mapper has gone. For anonymous pages the check "!PageAnon(page) || PageSwapCache(page)" should do. The idea is that on each path that replaces a valid pte with a swap pte the PageSwapCache bit is set first, then page_remove_rmap is called. As far as I can see this is true for the current code.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
The "easy" way to do it might be just unconditionally mark the page as dirty in this path (if the pte was writeable), so you can avoid the
page_test_dirtycheck and be sure of not missing the dirty bit.Hmm, but then an
mprotect()can change the pte to read-ony and we'd miss the dirty bit again. Back to the drawing board.Hmm, I guess you _could_
set_page_dirtyin mprotect.
Yeah, but we don't really want to do that.
Quoted 9 lines
Quoted 6 lines
By the way there is another SSKE I want to get rid of:
__SetPageUptodatedoes apage_clear_dirty(). For all uses of__SetPageUptodatethe page will be dirty after the application did its first write. To clear the page dirty bit only to have it set again shortly after doesn't make much sense to me. Has there been any particular reason for thepage_clear_dirtyin__SetPageUptodate?I guess it is just to match SetPageUptodate. Not all
__SetPageUptodatepaths may necessarily dirty the page, FWIW.
The current users of __SetPageUptodate are:
* do_wp_page
* do_anonymous_page
* do_fault for (flags & FAULT_FLAG_WRITE)
For these three cases the pte is created with pte_mkdirty. Clearing the physical page dirty bit is pointless.
* hugetlb_cow
* hugetlb_no_page
The dirty bits are of no interest to hugetlbfs, no?
I think it is safe to remove the page_clear_dirty from __SetPageUptodate New patch below.
Signature · 57 lines
blue skies, Martin. "Reality continues to ruin my life." - Calvin. --- Subject: [PATCH] Optimize storage key operations for anon pages From: Martin Schwidefsky <[email protected]> For anonymous pages without a swap cache backing the check for the physical dirty bit in page_remove_rmap is unnecessary. The instruction that are used to check and reset the dirty bit are expensive. Removing the check noticably speeds up process exit. In addition the clearing of the dirty bit in __SetPageUptodate is pointless as well. With these two changes there is no storage key operation for an anonymous page anymore if it does not hit the swap space. The micro benchmark which repeatedly executes an empty shell script gets about 5% faster. Signed-off-by: Martin Schwidefsky <[email protected]> --- diff -urpN linux-2.6/include/linux/page-flags.h linux-2.6-patched/include/linux/page-flags.h --- linux-2.6/include/linux/page-flags.h 2008-06-03 09:46:50.000000000 +0200 +++ linux-2.6-patched/include/linux/page-flags.h 2008-06-03 11:37:27.000000000 +0200 @@ -217,9 +217,6 @@ static inline void __SetPageUptodate(str { smp_wmb(); __set_bit(PG_uptodate, &(page)->flags); -#ifdef CONFIG_S390 - page_clear_dirty(page); -#endif } static inline void SetPageUptodate(struct page *page) diff -urpN linux-2.6/mm/rmap.c linux-2.6-patched/mm/rmap.c --- linux-2.6/mm/rmap.c 2008-06-03 11:34:55.000000000 +0200 +++ linux-2.6-patched/mm/rmap.c 2008-06-03 11:36:27.000000000 +0200 @@ -678,7 +678,8 @@ void page_remove_rmap(struct page *page, * Leaving it set also helps swapoff to reinstate ptes * faster for those pages still in swapcache. */ - if (page_test_dirty(page)) { + if ((!PageAnon(page) || PageSwapCache(page)) && + page_test_dirty(page)) { page_clear_dirty(page); set_page_dirty(page); } -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>