On Tue, 1 Jun 2010 13:40:59 +0530
Balbir Singh <balbir@xxxxxxxxxxxxxxxxxx> wrote:
> * KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx> [2010-06-01 12:29:41]:
>
> > CC to memcg folks.
> >
> > > I agree with the direction of this patch, but I am seeing a hang when
> > > testing with mmotm-2010-05-21-16-05. The following test hangs, unless I
> > > remove this patch from mmotm:
> > > mount -t cgroup none /cgroups -o memory
> > > mkdir /cgroups/cg1
> > > echo $$ > /cgroups/cg1/tasks
> > > dd bs=1024 count=1024 if=/dev/null of=/data/foo
> > > echo $$ > /cgroups/tasks
> > > echo 1 > /cgroups/cg1/memory.force_empty
> > >
> > > I think the hang is caused by the following portion of
> > > mem_cgroup_force_empty():
> > > while (nr_retries && mem->res.usage > 0) {
> > > int progress;
> > >
> > > if (signal_pending(current)) {
> > > ret = -EINTR;
> > > goto out;
> > > }
> > > progress = try_to_free_mem_cgroup_pages(mem, GFP_KERNEL,
> > > false, get_swappiness(mem));
> > > if (!progress) {
> > > nr_retries--;
> > > /* maybe some writeback is necessary */
> > > congestion_wait(BLK_RW_ASYNC, HZ/10);
> > > }
> > >
> > > }
> > >
> > > With this patch applied, it is possible that when do_try_to_free_pages()
> > > calls shrink_zones() for priority 0 that shrink_zones() may return 1
> > > indicating progress, even though no pages may have been reclaimed.
> > > Because this is a cgroup operation, scanning_global_lru() is false and
> > > the following portion of do_try_to_free_pages() fails to set ret=0.
> > > > if (ret && scanning_global_lru(sc))
> > > > ret = sc->nr_reclaimed;
> > > This leaves ret=1 indicating that do_try_to_free_pages() reclaimed 1
> > > page even though it did not reclaim any pages. Therefore
> > > mem_cgroup_force_empty() erroneously believes that
> > > try_to_free_mem_cgroup_pages() is making progress (one page at a time),
> > > so there is an endless loop.
> >
> > Good catch!
> >
> > Yeah, your analysis is fine. thank you for both your testing and
> > making analysis.
> >
> > Unfortunatelly, this logic need more fix. because It have already been
> > corrupted by another regression. my point is, if priority==0 reclaim
> > failure occur, "ret = sc->nr_reclaimed" makes no sense at all.
> >
> > The fixing patch is here. What do you think?
> >
> >
> >
> > From 49a395b21fe1b2f864112e71d027ffcafbdc9fc0 Mon Sep 17 00:00:00 2001
> > From: KOSAKI Motohiro <kosaki.motohiro@xxxxxxxxxxxxxx>
> > Date: Tue, 1 Jun 2010 11:29:50 +0900
> > Subject: [PATCH] vmscan: Fix do_try_to_free_pages() return value when priority==0 reclaim failure
> >
> > Greg Thelen reported recent Johannes's stack diet patch makes kernel
> > hang. His test is following.
> >
> > mount -t cgroup none /cgroups -o memory
> > mkdir /cgroups/cg1
> > echo $$ > /cgroups/cg1/tasks
> > dd bs=1024 count=1024 if=/dev/null of=/data/foo
> > echo $$ > /cgroups/tasks
> > echo 1 > /cgroups/cg1/memory.force_empty
> >
> > Actually, This OOM hard to try logic have been corrupted
> > since following two years old patch.
> >
> > commit a41f24ea9fd6169b147c53c2392e2887cc1d9247
> > Author: Nishanth Aravamudan <nacc@xxxxxxxxxx>
> > Date: Tue Apr 29 00:58:25 2008 -0700
> >
> > page allocator: smarter retry of costly-order allocations
> >
> > Original intention was "return success if the system have shrinkable
> > zones though priority==0 reclaim was failure". But the above patch
> > changed to "return nr_reclaimed if .....". Oh, That forgot nr_reclaimed
> > may be 0 if priority==0 reclaim failure.
> >
> > And Johannes's patch made more corrupt. Originally, priority==0 recliam
> > failure on memcg return 0, but this patch changed to return 1. It
> > totally confused memcg.
> >
> > This patch fixes it completely.
> >
>
> The patch seems reasonable to me, although I've not tested it
>
> Acked-by: Balbir Singh <balbir@xxxxxxxxxxxxxxxxxx>
>
Don't worry, I tested.
Thanks,
-Kame
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>