Re: [patch v3]vmscan: correctly detect GFP_ATOMIC allocation failure

[Minchan Kim <minchan.kim@gmail.com>]

On Tue, Oct 18, 2011 at 10:13:52AM +0800, Shaohua Li wrote:
> On Wed, 2011-10-12 at 15:59 +0800, Minchan Kim wrote:
> > On Wed, Oct 12, 2011 at 10:48:59AM +0800, Shaohua Li wrote:
> >  > > there are two cases one zone is below min_watermark.
> > > > > 1. the zone is below min_watermark for allocation in the zone. in this
> > > > > case we need hurry up.
> > > > > 2. the zone is below min_watermark for allocation from high zone. we
> > > > > don't really need hurry up if other zone is above min_watermark.
> > > > > Since low zone need to reserve pages for high zone, the second case
> > > > > could be common.
> > > > 
> > > > You mean "lowmem_reserve"?
> > > > It means opposite. It is a mechanism to defend using of lowmem pages from high zone allocation
> > > > because it could be fatal to allow process pages to be allocated from low zone.
> > > > Also, We could set each ratio for reserved pages of zones.
> > > > How could we make sure lower zones have enough free pages for higher zone?
> > > lowmem_reserve causes the problem, but it's not a fault of
> > > lowmem_reserve. I'm thinking changing it.
> > > 
> > > > > Yes, keeping kswapd running in this case can reduce the chance
> > > > > GFP_ATOMIC failure. But my patch will not cause immediate failure
> > > > > because there is still some zones which are above min_watermark and can
> > > > > meet the GFP_ATOMIC allocation. And keeping kswapd running has some
> > > > 
> > > > True. It was why I said "I don't mean you are wrong but we are very careful about this."
> > > > Normally, it could handle but might fail on sudden peak of atomic allocation stream.
> > > > Recently, we have suffered from many reporting of GFP_AOTMIC allocation than olded.
> > > > So I would like to be very careful and that's why I suggest we need at least some experiment.
> > > > Through it, Andrew could make final call.
> > > sure.
> > > 
> > > > > drawbacks:
> > > > > 1. cpu overhead
> > > > > 2. extend isolate window size, so trash working set.
> > > > > Considering DMA zone, we almost always have DMA zone min_watermark not
> > > > > ok for any allocation from high zone. So we will always have such
> > > > > drawbacks.
> > > > 
> > > > I agree with you in that it's a problem.
> > > > I think the real solution is to remove the zone from allocation fallback list in such case
> > > > because the lower zone could never meet any allocation for the higher zone.
> > > > But it makes code rather complicated as we have to consider admin who can change
> > > > reserved pages anytime.
> > > Not worthy the complication.
> > > 
> > > > So how about this?
> > > > 
> > > > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > > > index 8913374..f71ed2f 100644
> > > > --- a/mm/vmscan.c
> > > > +++ b/mm/vmscan.c
> > > > @@ -2693,8 +2693,16 @@ loop_again:
> > > >                                  * failure risk. Hurry up!
> > > >                                  */
> > > >                                 if (!zone_watermark_ok_safe(zone, order,
> > > > -                                           min_wmark_pages(zone), end_zone, 0))
> > > > -                                       has_under_min_watermark_zone = 1;
> > > > +                                           min_wmark_pages(zone), end_zone, 0)) {
> > > > +                                       /*
> > > > +                                        * In case of big reserved page for higher zone,
> > > > +                                        * it is pointless to try reclaimaing pages quickly
> > > > +                                        * because it could never meet the requirement.
> > > > +                                        */
> > > > +                                       if (zone->present_pages >
> > > > +                                               min_wmark_pages(zone) + zone->lowmem_reserve[end_zone])
> > > > +                                               has_under_min_watermark_zone = 1;
> > > > +                               }
> > > >                         } else {
> > > >                                 /*
> > > >                                  * If a zone reaches its high watermark,
> > > This looks like a workaround just for DMA zone. present_pages could be
> > > bigger than min_mwark+lowmem_reserve. And We still suffered from the
> > > issue, for example, a DMA32 zone with some pages allocated for DMA, or a
> > > zone which has some lru pages, but still much smaller than high zone.
> > 
> > Right. I thought about it but couldn't have a good idea for it. :(
> > 
> > > 
> > > > Even, we could apply this at starting of the loop so that we can avoid unnecessary scanning st the beginning.
> > > > In that case, we have to apply zone->lowmem_reserve[end_zone] only because we have to consider NO_WATERMARK alloc case.
> > > yes, we can do this to avoid unnecessary scan. but DMA zone hasn't lru
> > > pages, so not sure how big the benefit is here.
> > 
> > At least, we can prevent has_under_min_watermark_zone from set.
> > But it still have a problem you pointed out earlier.
> > 
> > > 
> > > > > Or is something below better? we can avoid the big reserved pages
> > > > > accounting to the min_wmark_pages for low zone. if high zone is under
> > > > > min_wmark, kswapd will not sleep.
> > > > >                                if (!zone_watermark_ok_safe(zone, order,
> > > > > -                                            min_wmark_pages(zone), end_zone, 0))
> > > > > +                                            min_wmark_pages(zone), 0, 0))
> > > > >                                         has_under_min_watermark_zone = 1;
> > > > 
> > > > I think it's not a good idea since page allocator always considers classzone_idx.
> > > > So although we fix kswapd issue through your changes, page allocator still can't allocate memory
> > > > and wakes up kswapd, again.
> > > why kswapd will be waked up again? The high zone itself still has
> > > min_wark+low_reserve ok for the allocation(classzone_idx 0 means
> > > checking the low_reserve for allocation from the zone itself), so the
> > > allocation can be met.
> > 
> > You're absolutely right.
> > I got confused. Sorry about that.
> > 
> > I like this than your old version.
> > That's because it could rush if one of zonelist is consumed as below min_watermak.
> > It could mitigate GFP_ALLOC fail than yours old version but still would be higher than now.
> > So, we need the number.
> > 
> > Could you repost this as formal patch with good comment and number?
> > Personally, I like description based on scenario with kind step-by-step.
> > Feel free to use my description in my patch if you want.
> > 
> > Thanks for patient discussion in spite of my irregular reply, Shaohua.
> Thanks for your time. Here is patch. I'm trying to get some number, but
> didn't find a proper workload to demonstrate the atomic allocation
> failure. Any suggestion for the workload?

Sorry for really really late response.

Hmm.. I didn't have a such test.
But it seems recently we had a such report, again.
https://lkml.org/lkml/2011/10/18/131
If you want it, you could ask him.

> 
> Thanks,
> Shaohua
> 
> 
> Subject: vmscan: correctly detect GFP_ATOMIC allocation failure -v3
> 
> has_under_min_watermark_zone is used to detect if there is GFP_ATOMIC allocation
> failure risk. Current logic is if any zone has min watermark not ok, we have
> risk.
> 
> Low zone needs reserve memory to avoid fallback from high zone. The reserved
> memory is zone->lowmem_reserve[]. If high zone is big, low zone's
> min_wmark + lowmem_reserve[] usually is big. Sometimes min_wmark +
> lowmem_reserve[] could even be higher than zone->present_pages. An example is
> DMA zone. Other low zones could have the similar high reserved memory though
> might still have margins between reserved pages and present pages. So in kswapd
> loop, if end_zone is a high zone, has_under_min_watermark_zone could be easily
> set or always set for DMA.
> 
> Let's consider end_zone is a high zone and it has high_mwark not ok, but
> min_mwark ok. A DMA zone always has present_pages less than reserved pages, so
> has_under_min_watermark_zone is always set. When kswapd is running, there are
> some drawbacks:
> 1. kswapd can keep unnecessary running without congestion_wait. high zone
> already can meet GFP_ATOMIC. The running will waste some CPU.
> 2. kswapd can scan much more pages to trash working set. congestion_wait can
> slow down scan if kswapd has trouble. Now congestion_wait is skipped, kswapd
> will keep scanning unnecessary pages.
> 
> So since DMA zone always set has_under_min_watermark_zone, current logic actually
> equals to that kswapd keeps running without congestion_wait till high zone has
> high wmark ok when it has trouble. This is not intended.
> 
> In this path, we test the min_mwark against the zone itself. This doesn't change
> the behavior of high zone. For low zone, we now exclude lowmem_reserve for high
> zone to avoid unnecessary running.
> 
> Note: With this patch, we could have potential higher risk of GFP_ATOMIC failure.
> 
> v3: Uses a less intrusive method to determine if has_under_min_watermark_zone
> should be set after discussion with Minchan.
> v2: use bool and clear has_under_min_watermark_zone for zone with watermark ok
> as suggested by David Rientjes.
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
Reviewed-by: Minchan Kim <minchan.kim@gmail.com>

As you know, I give my reviewed-by as a token of "code looks good to me".
But still, we see atomic allocation failure messasge recently.
So you need to get a acked-by as a toekn of "I support this idea" of
others(ie, Mel/Rik are right person).

I hope it would be better to write down it in description that
it's real problem of your system and the patch solves it.

Thanks!

> ---
>  mm/vmscan.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Index: linux/mm/vmscan.c
> ===================================================================
> --- linux.orig/mm/vmscan.c	2011-10-17 16:02:30.000000000 +0800
> +++ linux/mm/vmscan.c	2011-10-18 09:32:23.000000000 +0800
> @@ -2463,7 +2463,7 @@ loop_again:
>  
>  	for (priority = DEF_PRIORITY; priority >= 0; priority--) {
>  		unsigned long lru_pages = 0;
> -		int has_under_min_watermark_zone = 0;
> +		bool has_under_min_watermark_zone = false;
>  
>  		/* The swap token gets in the way of swapout... */
>  		if (!priority)
> @@ -2593,8 +2593,8 @@ loop_again:
>  				 * failure risk. Hurry up!
>  				 */
>  				if (!zone_watermark_ok_safe(zone, order,
> -					    min_wmark_pages(zone), end_zone, 0))
> -					has_under_min_watermark_zone = 1;
> +					    min_wmark_pages(zone), 0, 0))
> +					has_under_min_watermark_zone = true;
>  			} else {
>  				/*
>  				 * If a zone reaches its high watermark,
> 
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>