| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 15 Nov 2012 10:29:36 +0200
From: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
To: David Rientjes <rientjes@...gle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
Andrea Arcangeli <aarcange@...hat.com>, linux-mm@...ck.org,
Andi Kleen <ak@...ux.intel.com>,
"H. Peter Anvin" <hpa@...ux.intel.com>,
linux-kernel@...r.kernel.org,
"Kirill A. Shutemov" <kirill@...temov.name>
Subject: Re: [PATCH v5 04/11] thp: do_huge_pmd_wp_page(): handle huge zero
page
On Wed, Nov 14, 2012 at 03:08:45PM -0800, David Rientjes wrote:
> On Wed, 7 Nov 2012, Kirill A. Shutemov wrote:
>
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index fa06804..fe329da 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -516,6 +516,14 @@ static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct *vma)
> > }
> > #endif
> >
> > +#ifndef my_zero_pfn
> > +static inline unsigned long my_zero_pfn(unsigned long addr)
> > +{
> > + extern unsigned long zero_pfn;
>
> I don't think you should be declaring this inside an inlined function, you
> probably should be protecting the declarations of the variable and the
> function instead. Perhaps by CONFIG_MMU?
mips and s390 use declaration inside inline function to implement
is_zero_pfn(). I wanted to be consistent with that.
I have patch to cleanup zero page helpers a bit. It's on top of
this patchset.
http://article.gmane.org/gmane.linux.kernel.mm/87387
> > + return zero_pfn;
> > +}
> > +#endif
> > +
> > /*
> > * Multiple processes may "see" the same page. E.g. for untouched
> > * mappings of /dev/null, all processes see the same page full of
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 0d903bf..d767a7c 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -824,6 +824,88 @@ out:
> > return ret;
> > }
> >
> > +/* no "address" argument so destroys page coloring of some arch */
> > +pgtable_t get_pmd_huge_pte(struct mm_struct *mm)
> > +{
>
> Umm, this is a copy and paste of pgtable_trans_huge_withdraw() from the
> generic page table handling. Why can't you reuse that and support (and/or
> modify) the s390 and sparc code?
My bad. It's mistake on conflict solving. I'll fix that.
> > + pgtable_t pgtable;
> > +
> > + assert_spin_locked(&mm->page_table_lock);
> > +
> > + /* FIFO */
> > + pgtable = mm->pmd_huge_pte;
> > + if (list_empty(&pgtable->lru))
> > + mm->pmd_huge_pte = NULL;
> > + else {
> > + mm->pmd_huge_pte = list_entry(pgtable->lru.next,
> > + struct page, lru);
> > + list_del(&pgtable->lru);
> > + }
> > + return pgtable;
> > +}
> > +
> > +static int do_huge_pmd_wp_zero_page_fallback(struct mm_struct *mm,
> > + struct vm_area_struct *vma, unsigned long address,
> > + pmd_t *pmd, unsigned long haddr)
>
> This whole function is extremely similar to the implementation of
> do_huge_pmd_wp_page_fallback(), there really is no way to fold the two?
It's similar by structure (I used do_huge_pmd_wp_page_fallback() as a
template) but details are different in many places and I fail to see how
to combine them without making result ugly.
> Typically in cases like this it's helpful to split out different logical
> segments of a function into smaller functions that would handle both
> page and !page accordingly.
>
> > +{
> > + pgtable_t pgtable;
> > + pmd_t _pmd;
> > + struct page *page;
> > + int i, ret = 0;
> > + unsigned long mmun_start; /* For mmu_notifiers */
> > + unsigned long mmun_end; /* For mmu_notifiers */
> > +
> > + page = alloc_page_vma(GFP_HIGHUSER_MOVABLE, vma, address);
> > + if (!page) {
> > + ret |= VM_FAULT_OOM;
> > + goto out;
> > + }
> > +
> > + if (mem_cgroup_newpage_charge(page, mm, GFP_KERNEL)) {
> > + put_page(page);
> > + ret |= VM_FAULT_OOM;
> > + goto out;
> > + }
> > +
> > + clear_user_highpage(page, address);
> > + __SetPageUptodate(page);
> > +
> > + mmun_start = haddr;
> > + mmun_end = haddr + HPAGE_PMD_SIZE;
> > + mmu_notifier_invalidate_range_start(mm, mmun_start, mmun_end);
> > +
> > + spin_lock(&mm->page_table_lock);
> > + pmdp_clear_flush(vma, haddr, pmd);
> > + /* leave pmd empty until pte is filled */
> > +
> > + pgtable = get_pmd_huge_pte(mm);
> > + pmd_populate(mm, &_pmd, pgtable);
> > +
> > + for (i = 0; i < HPAGE_PMD_NR; i++, haddr += PAGE_SIZE) {
> > + pte_t *pte, entry;
> > + if (haddr == (address & PAGE_MASK)) {
> > + entry = mk_pte(page, vma->vm_page_prot);
> > + entry = maybe_mkwrite(pte_mkdirty(entry), vma);
> > + page_add_new_anon_rmap(page, vma, haddr);
> > + } else {
> > + entry = pfn_pte(my_zero_pfn(haddr), vma->vm_page_prot);
> > + entry = pte_mkspecial(entry);
> > + }
> > + pte = pte_offset_map(&_pmd, haddr);
> > + VM_BUG_ON(!pte_none(*pte));
> > + set_pte_at(mm, haddr, pte, entry);
> > + pte_unmap(pte);
> > + }
> > + smp_wmb(); /* make pte visible before pmd */
> > + pmd_populate(mm, pmd, pgtable);
> > + spin_unlock(&mm->page_table_lock);
> > +
> > + mmu_notifier_invalidate_range_end(mm, mmun_start, mmun_end);
> > +
> > + ret |= VM_FAULT_WRITE;
> > +out:
> > + return ret;
> > +}
> > +
> > static int do_huge_pmd_wp_page_fallback(struct mm_struct *mm,
> > struct vm_area_struct *vma,
> > unsigned long address,
> > @@ -930,19 +1012,21 @@ int do_huge_pmd_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,
> > unsigned long address, pmd_t *pmd, pmd_t orig_pmd)
> > {
> > int ret = 0;
> > - struct page *page, *new_page;
> > + struct page *page = NULL, *new_page;
> > unsigned long haddr;
> > unsigned long mmun_start; /* For mmu_notifiers */
> > unsigned long mmun_end; /* For mmu_notifiers */
> >
> > VM_BUG_ON(!vma->anon_vma);
> > + haddr = address & HPAGE_PMD_MASK;
> > + if (is_huge_zero_pmd(orig_pmd))
> > + goto alloc;
> > spin_lock(&mm->page_table_lock);
> > if (unlikely(!pmd_same(*pmd, orig_pmd)))
> > goto out_unlock;
> >
> > page = pmd_page(orig_pmd);
> > VM_BUG_ON(!PageCompound(page) || !PageHead(page));
> > - haddr = address & HPAGE_PMD_MASK;
> > if (page_mapcount(page) == 1) {
> > pmd_t entry;
> > entry = pmd_mkyoung(orig_pmd);
> > @@ -954,7 +1038,7 @@ int do_huge_pmd_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,
> > }
> > get_page(page);
> > spin_unlock(&mm->page_table_lock);
> > -
> > +alloc:
>
> This could all use a minor restructuring to make it much more cleaner,
> perhaps by extracting the page_mapcount(page) == 1 case to be a separate
> function that deals with non-copying writes?
Makes sense. I'll do it as a separate patch on top of the series.
>
> > if (transparent_hugepage_enabled(vma) &&
> > !transparent_hugepage_debug_cow())
> > new_page = alloc_hugepage_vma(transparent_hugepage_defrag(vma),
> > @@ -964,24 +1048,34 @@ int do_huge_pmd_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,
> >
> > if (unlikely(!new_page)) {
> > count_vm_event(THP_FAULT_FALLBACK);
> > - ret = do_huge_pmd_wp_page_fallback(mm, vma, address,
> > - pmd, orig_pmd, page, haddr);
> > - if (ret & VM_FAULT_OOM)
> > - split_huge_page(page);
> > - put_page(page);
> > + if (is_huge_zero_pmd(orig_pmd)) {
> > + ret = do_huge_pmd_wp_zero_page_fallback(mm, vma,
> > + address, pmd, haddr);
> > + } else {
> > + ret = do_huge_pmd_wp_page_fallback(mm, vma, address,
> > + pmd, orig_pmd, page, haddr);
> > + if (ret & VM_FAULT_OOM)
> > + split_huge_page(page);
> > + put_page(page);
> > + }
> > goto out;
> > }
> > count_vm_event(THP_FAULT_ALLOC);
> >
> > if (unlikely(mem_cgroup_newpage_charge(new_page, mm, GFP_KERNEL))) {
> > put_page(new_page);
> > - split_huge_page(page);
> > - put_page(page);
> > + if (page) {
> > + split_huge_page(page);
> > + put_page(page);
> > + }
> > ret |= VM_FAULT_OOM;
> > goto out;
> > }
> >
> > - copy_user_huge_page(new_page, page, haddr, vma, HPAGE_PMD_NR);
> > + if (is_huge_zero_pmd(orig_pmd))
> > + clear_huge_page(new_page, haddr, HPAGE_PMD_NR);
> > + else
> > + copy_user_huge_page(new_page, page, haddr, vma, HPAGE_PMD_NR);
> > __SetPageUptodate(new_page);
> >
> > mmun_start = haddr;
> > @@ -989,7 +1083,8 @@ int do_huge_pmd_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,
> > mmu_notifier_invalidate_range_start(mm, mmun_start, mmun_end);
> >
> > spin_lock(&mm->page_table_lock);
> > - put_page(page);
> > + if (page)
> > + put_page(page);
> > if (unlikely(!pmd_same(*pmd, orig_pmd))) {
> > spin_unlock(&mm->page_table_lock);
> > mem_cgroup_uncharge_page(new_page);
> > @@ -997,7 +1092,6 @@ int do_huge_pmd_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,
> > goto out_mn;
> > } else {
> > pmd_t entry;
> > - VM_BUG_ON(!PageHead(page));
> > entry = mk_pmd(new_page, vma->vm_page_prot);
> > entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
> > entry = pmd_mkhuge(entry);
> > @@ -1005,8 +1099,13 @@ int do_huge_pmd_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,
> > page_add_new_anon_rmap(new_page, vma, haddr);
> > set_pmd_at(mm, haddr, pmd, entry);
> > update_mmu_cache_pmd(vma, address, pmd);
> > - page_remove_rmap(page);
> > - put_page(page);
> > + if (is_huge_zero_pmd(orig_pmd))
> > + add_mm_counter(mm, MM_ANONPAGES, HPAGE_PMD_NR);
> > + if (page) {
>
> Couldn't this be an "else" instead?
Yes. I'll update.
> > + VM_BUG_ON(!PageHead(page));
> > + page_remove_rmap(page);
> > + put_page(page);
> > + }
> > ret |= VM_FAULT_WRITE;
> > }
> > spin_unlock(&mm->page_table_lock);
--
Kirill A. Shutemov
Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)
Powered by blists - more mailing lists