linux-kernel - Re: mm: pages are not freed from lru_add

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Wed, 11 May 2016 09:53:13 +0200
From:	Michal Hocko <mhocko@...nel.org>
To:	Dave Hansen <dave.hansen@...el.com>
Cc:	"Odzioba, Lukasz" <lukasz.odzioba@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	"Shutemov, Kirill" <kirill.shutemov@...el.com>,
	"Anaczkowski, Lukasz" <lukasz.anaczkowski@...el.com>
Subject: Re: mm: pages are not freed from lru_add_pvecs after process
 termination

On Fri 06-05-16 09:04:34, Dave Hansen wrote:
> On 05/06/2016 08:10 AM, Odzioba, Lukasz wrote:
> > On Thu 05-05-16 09:21:00, Michal Hocko wrote: 
> >> Or maybe the async nature of flushing turns
> >> out to be just impractical and unreliable and we will end up skipping
> >> THP (or all compound pages) for pcp LRU add cache. Let's see...
> > 
> > What if we simply skip lru_add pvecs for compound pages?
> > That way we still have compound pages on LRU's, but the problem goes
> > away.  It is not quite what this naïve patch does, but it works nice for me.
> > 
> > diff --git a/mm/swap.c b/mm/swap.c
> > index 03aacbc..c75d5e1 100644
> > --- a/mm/swap.c
> > +++ b/mm/swap.c
> > @@ -392,7 +392,9 @@ static void __lru_cache_add(struct page *page)
> >         get_page(page);
> >         if (!pagevec_space(pvec))
> >                 __pagevec_lru_add(pvec);
> >         pagevec_add(pvec, page);
> > +       if (PageCompound(page))
> > +               __pagevec_lru_add(pvec);
> >         put_cpu_var(lru_add_pvec);
> >  }
> 
> That's not _quite_ what I had in mind since that drains the entire pvec
> every time a large page is encountered.  But I'm conflicted about what
> the right behavior _is_.
> 
> We'd taking the LRU lock for 'page' anyway, so we might as well drain
> the pvec.

Yes I think this makes sense. The only case where it would be suboptimal
is when the pagevec was already full and then we just created a single
page pvec to drain it. This can be handled better though by:

diff --git a/mm/swap.c b/mm/swap.c
index 95916142fc46..3fe4f180e8bf 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -391,9 +391,8 @@ static void __lru_cache_add(struct page *page)
 	struct pagevec *pvec = &get_cpu_var(lru_add_pvec);
 
 	get_page(page);
-	if (!pagevec_space(pvec))
+	if (!pagevec_add(pvec, page) || PageCompound(page))
 		__pagevec_lru_add(pvec);
-	pagevec_add(pvec, page);
 	put_cpu_var(lru_add_pvec);
 }
 

> Or, does the additional work to put the page on to a pvec and then
> immediately drain it overwhelm that advantage?

pagevec_add is quite trivial so I would be really surprised if it
mattered.
-- 
Michal Hocko
SUSE Labs