linux-kernel - Re: [patch 2/4] mm: vmscan: rework compaction-ready signaling in direct reclaim

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] [day] [month] [year] [list]

Message-ID: <20140625095526.GX10819@suse.de>
Date:	Wed, 25 Jun 2014 10:55:26 +0100
From:	Mel Gorman <mgorman@...e.de>
To:	Johannes Weiner <hannes@...xchg.org>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Rik van Riel <riel@...hat.com>,
	Michal Hocko <mhocko@...e.cz>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: [patch 2/4] mm: vmscan: rework compaction-ready signaling in
 direct reclaim

On Mon, Jun 23, 2014 at 01:20:56PM -0400, Johannes Weiner wrote:
> Hi Mel,
> 
> On Mon, Jun 23, 2014 at 02:07:05PM +0100, Mel Gorman wrote:
> > On Fri, Jun 20, 2014 at 12:33:48PM -0400, Johannes Weiner wrote:
> > > Page reclaim for a higher-order page runs until compaction is ready,
> > > then aborts and signals this situation through the return value of
> > > shrink_zones().  This is an oddly specific signal to encode in the
> > > return value of shrink_zones(), though, and can be quite confusing.
> > > 
> > > Introduce sc->compaction_ready and signal the compactability of the
> > > zones out-of-band to free up the return value of shrink_zones() for
> > > actual zone reclaimability.
> > > 
> > > Signed-off-by: Johannes Weiner <hannes@...xchg.org>
> > > ---
> > >  mm/vmscan.c | 67 ++++++++++++++++++++++++++++---------------------------------
> > >  1 file changed, 31 insertions(+), 36 deletions(-)
> > > 
> > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > index 19b5b8016209..ed1efb84c542 100644
> > > --- a/mm/vmscan.c
> > > +++ b/mm/vmscan.c
> > > @@ -65,6 +65,9 @@ struct scan_control {
> > >  	/* Number of pages freed so far during a call to shrink_zones() */
> > >  	unsigned long nr_reclaimed;
> > >  
> > > +	/* One of the zones is ready for compaction */
> > > +	int compaction_ready;
> > > +
> > >  	/* How many pages shrink_list() should reclaim */
> > >  	unsigned long nr_to_reclaim;
> > >  
> > 
> > You are not the criminal here but scan_control is larger than it needs
> > to be and the stack usage of reclaim has reared its head again.
> > 
> > Add a preparation patch that convert sc->may* and sc->hibernation_mode
> > to bool and moves them towards the end of the struct. Then add
> > compaction_ready as a bool.
> 
> Good idea, I'll do that.
> 

Thanks.

> > > @@ -2292,15 +2295,11 @@ static void shrink_zone(struct zone *zone, struct scan_control *sc)
> > >  }
> > >  
> > >  /* Returns true if compaction should go ahead for a high-order request */
> > > -static inline bool compaction_ready(struct zone *zone, struct scan_control *sc)
> > > +static inline bool compaction_ready(struct zone *zone, int order)
> > > 
> > >  {
> > 
> > Why did you remove the use of sc->order? In this patch there is only one
> > called of compaction_ready and it looks like
> > 
> >                      if (IS_ENABLED(CONFIG_COMPACTION) &&
> >                          sc->order > PAGE_ALLOC_COSTLY_ORDER &&
> >                          zonelist_zone_idx(z) <= requested_highidx &&
> >                          compaction_ready(zone, sc->order)) {
> > 
> > So it's unclear why you changed the signature.
> 
> Everything else in compaction_ready() is about internal compaction
> requirements, like checking for free pages and deferred compaction,
> whereas this order check is more of a reclaim policy rule according to
> the comment in the caller:
> 
> 			 ...
> 			 * Even though compaction is invoked for any
> 			 * non-zero order, only frequent costly order
> 			 * reclamation is disruptive enough to become a
> 			 * noticeable problem, like transparent huge
> 			 * page allocations.
> 			 */
> 
> But it's an unrelated in-the-area-anyway change, I can split it out -
> or drop it entirely - if you prefer.
> 

It's ok as-is. It just seemed unrelated and seemed to do nothing. I was
wondering if this was a rebasing artifact and some other change that
required it got lost along the way by accident.

-- 
Mel Gorman
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/