On Fri, 2011-10-28 at 06:50 +0800, Minchan Kim wrote:
> 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.
I'll ask.
> > 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@xxxxxxxxx>
> Reviewed-by: Minchan Kim <minchan.kim@xxxxxxxxx>
>
> 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, I haven't test case exposing this issue, but just thought the
logic looks buggy.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. 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@xxxxxxxxx";> email@xxxxxxxxx </a>