Re: [PATCH 2/6] mm: vmscan: Scale number of pages reclaimed byreclaim/compaction based on failures

[Minchan Kim]

Hi Mel,

On Fri, Aug 10, 2012 at 09:34:38AM +0100, Mel Gorman wrote:
> On Fri, Aug 10, 2012 at 08:27:33AM +0900, Minchan Kim wrote:
> > > <SNIP>
> > >
> > > The intention is that an allocation can fail but each subsequent attempt will
> > > try harder until there is success. Each allocation request does a portion
> > > of the necessary work to spread the cost between multiple requests. Take
> > > THP for example where there is a constant request for THP allocations
> > > for whatever reason (heavy fork workload, large buffer allocation being
> > > populated etc.). Some of those allocations fail but if they do, future
> > > THP requests will reclaim more pages. When compaction resumes again, it
> > > will be more likely to succeed and compact_defer_shift gets reset. In the
> > > specific case of THP there will be allocations that fail but khugepaged
> > > will promote them later if the process is long-lived.
> > 
> > You assume high-order allocation are *constant* and I guess your test enviroment
> > is optimal for it.
> 
> Ok, my example stated they were constant because it was the easiest to
> illustrate but it does not necessarily have to be the case. The high-order
> allocation requests can be separated by any length of time with a read or
> write stream running in the background applying a small amount of memory
> pressure and the same scenario applies.
> 
> > I agree your patch if we can make sure such high-order
> > allocation are always constant. But, is it true? Otherwise, your patch could reclaim
> > too many pages unnecessary and it could reduce system performance by eviction
> 
> The "too many pages unnecessarily" is unlikely. For compact_defer_shift to be
> elevated there has to have been recent failures by try_to_compact_pages(). If
> compact_defer_shift is elevated and a large process exited then
> try_to_compact_pages() may succeed and reset compact_defer_shift without
> calling direct reclaim and entering this path at all.
> 
> > of page cache and swap out of workingset part. That's a concern to me.
> > In summary, I think your patch is rather agressive so how about this?
> > 
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 66e4310..0cb2593 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -1708,6 +1708,7 @@ static inline bool should_continue_reclaim(struct lruvec *lruvec,
> >  {
> >         unsigned long pages_for_compaction;
> >         unsigned long inactive_lru_pages;
> > +       struct zone *zone;
> > 
> >         /* If not in reclaim/compaction mode, stop */
> >         if (!in_reclaim_compaction(sc))
> > @@ -1741,6 +1742,15 @@ static inline bool should_continue_reclaim(struct lruvec *lruvec,
> >          * inactive lists are large enough, continue reclaiming
> >          */
> >         pages_for_compaction = (2UL << sc->order);
> > +
> > +       /*
> > +        * If compaction is deferred for this order then scale the number of
> > +        * pages reclaimed based on the number of consecutive allocation
> > +        * failures
> > +        */
> > +       zone = lruvec_zone(lruvec);
> > +       if (zone->compact_order_failed <= sc->order) {
> > +               if (zone->compact_defer_shift)
> > +                       /*
> > +                        * We can't make sure deferred requests will come again
> > +                        * The probability is 50:50.
> > +                        */
> > +                       pages_for_compaction <<= (zone->compact_defer_shift - 1);
> 
> This patch is not doing anything radically different to my own patch.
> compact_defer_shift == 0 if allocations succeeded recently using
> reclaim/compaction at its normal level. Functionally the only difference
> is that you delay when more pages get reclaim by one failure.
> 
> Was that what you intended? If so, it's not clear why you think this patch
> is better or how you concluded that the probability of another failure was
> "50:50".

Please ignore my comment about this patch.
I got confused between compat_considered and compact_defer_shift.
compact_defer_shift is indication of constant high order page
allocationfailing so I have no objection any more.
Sorry for the noise. :(

-- 
Kind regards,
Minchan Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/