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 <han...@cmpxchg.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.
> > @@ -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.
> > @@ -2500,12 +2492,15 @@ static unsigned long do_try_to_free_pages(struct
> > zonelist *zonelist,
> > vmpressure_prio(sc->gfp_mask, sc->target_mem_cgroup,
> > sc->priority);
> > sc->nr_scanned = 0;
> > - aborted_reclaim = shrink_zones(zonelist, sc);
> > + shrink_zones(zonelist, sc);
> >
> > total_scanned += sc->nr_scanned;
> > if (sc->nr_reclaimed >= sc->nr_to_reclaim)
> > goto out;
> >
> > + if (sc->compaction_ready)
> > + goto out;
> > +
>
> break?
>
> Convert the other one to break as well. out label seems unnecessary in
> this context.
Makes sense, I'll include this in v2.
Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
