linux-kernel - Re: [PATCHv9-rebased2 01/37] mm, thp: make swapin readahead under down_read of mmap

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160616100854.GB18137@node.shutemov.name>
Date:	Thu, 16 Jun 2016 13:08:54 +0300
From:	"Kirill A. Shutemov" <kirill@...temov.name>
To:	Hillf Danton <hillf.zj@...baba-inc.com>
Cc:	'Ebru Akagunduz' <ebru.akagunduz@...il.com>,
	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
	linux-kernel <linux-kernel@...r.kernel.org>, linux-mm@...ck.org
Subject: Re: [PATCHv9-rebased2 01/37] mm, thp: make swapin readahead under
 down_read of mmap_sem

On Thu, Jun 16, 2016 at 02:52:52PM +0800, Hillf Danton wrote:
> > 
> > From: Ebru Akagunduz <ebru.akagunduz@...il.com>
> > 
> > Currently khugepaged makes swapin readahead under down_write.  This patch
> > supplies to make swapin readahead under down_read instead of down_write.
> > 
> > The patch was tested with a test program that allocates 800MB of memory,
> > writes to it, and then sleeps.  The system was forced to swap out all.
> > Afterwards, the test program touches the area by writing, it skips a page
> > in each 20 pages of the area.
> > 
> > Link: http://lkml.kernel.org/r/1464335964-6510-4-git-send-email-ebru.akagunduz@gmail.com
> > Signed-off-by: Ebru Akagunduz <ebru.akagunduz@...il.com>
> > Cc: Hugh Dickins <hughd@...gle.com>
> > Cc: Rik van Riel <riel@...hat.com>
> > Cc: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
> > Cc: Naoya Horiguchi <n-horiguchi@...jp.nec.com>
> > Cc: Andrea Arcangeli <aarcange@...hat.com>
> > Cc: Joonsoo Kim <iamjoonsoo.kim@....com>
> > Cc: Cyrill Gorcunov <gorcunov@...nvz.org>
> > Cc: Mel Gorman <mgorman@...e.de>
> > Cc: David Rientjes <rientjes@...gle.com>
> > Cc: Vlastimil Babka <vbabka@...e.cz>
> > Cc: Aneesh Kumar K.V <aneesh.kumar@...ux.vnet.ibm.com>
> > Cc: Johannes Weiner <hannes@...xchg.org>
> > Cc: Michal Hocko <mhocko@...e.cz>
> > Cc: Minchan Kim <minchan.kim@...il.com>
> > Signed-off-by: Andrew Morton <akpm@...ux-foundation.org>
> > ---
> >  mm/huge_memory.c | 92 ++++++++++++++++++++++++++++++++++++++------------------
> >  1 file changed, 63 insertions(+), 29 deletions(-)
> > 
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index f2bc57c45d2f..96dfe3f09bf6 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -2378,6 +2378,35 @@ static bool hugepage_vma_check(struct vm_area_struct *vma)
> >  }
> > 
> >  /*
> > + * If mmap_sem temporarily dropped, revalidate vma
> > + * before taking mmap_sem.
> 
> See below

> > @@ -2401,11 +2430,18 @@ static void __collapse_huge_page_swapin(struct mm_struct *mm,
> >  			continue;
> >  		swapped_in++;
> >  		ret = do_swap_page(mm, vma, _address, pte, pmd,
> > -				   FAULT_FLAG_ALLOW_RETRY|FAULT_FLAG_RETRY_NOWAIT,
> > +				   FAULT_FLAG_ALLOW_RETRY,
> 
> Add a description in change log for it please.

Ebru, would you address it?

> >  				   pteval);
> > +		/* do_swap_page returns VM_FAULT_RETRY with released mmap_sem */
> > +		if (ret & VM_FAULT_RETRY) {
> > +			down_read(&mm->mmap_sem);
> > +			/* vma is no longer available, don't continue to swapin */
> > +			if (hugepage_vma_revalidate(mm, vma, address))
> > +				return false;
> 
> Revalidate vma _after_ acquiring mmap_sem, but the above comment says _before_.

Ditto.

> > +	if (!__collapse_huge_page_swapin(mm, vma, address, pmd)) {
> > +		up_read(&mm->mmap_sem);
> > +		goto out;
> 
> Jump out with mmap_sem released, 
> 
> > +	result = hugepage_vma_revalidate(mm, vma, address);
> > +	if (result)
> > +		goto out;
> 
> but jump out again with mmap_sem held.
> 
> They are cleaned up in subsequent darns?

I didn't fold fixups for these
> 

-- 
 Kirill A. Shutemov