Re: [PATCH] kswapd shall not sleep during page shortage

Marcelo Tosatti
2004-11-09T18:26:22Z

On Tue, Nov 09, 2004 at 01:33:43PM -0800, Andrew Morton wrote:
> Marcelo Tosatti <marcelo.tosatti@cyclades.com> wrote:
> >
> > On Tue, Nov 09, 2004 at 12:19:45PM -0800, Andrew Morton wrote:
> > > Marcelo Tosatti <marcelo.tosatti@cyclades.com> wrote:
> > > >
> > > >
> > > > Andrew,
> > > >
> > > > I was wrong last time I read balance_pgdat() when I thought kswapd
> > > > couldnt sleep under page shortage.
> > > >
> > > > It can, because all_zones_ok is set to "1" inside the
> > > > "priority=DEF_PRIORITY; priority >= 0; priority--" loop.
> > > >
> > > > So this patch sets "all_zones_ok" to zero even if all_unreclaimable
> > > > is set, avoiding it from sleeping when zones are under page short.
> > > >
> > >
> > > Does this solve any observed problem? What testing was done, and what were
> > > the results??
> >
> >
> > The observed problem are the page allocation failures!
>
> But the patch doesn't have any effect on that, which I can see.

Andrew, it avoids kswapd from sleeping when the machine is OOM.

> > No testing has been done, but it is an obvious problem if you read the
> > code.
>
> Not really. The move of the all_unreclaimable test doesn't seem to do
> anything, because we'll just skip that zone anyway in the next loop.
>
> Maybe you moved the all_unreclaimable test just so that there's an
> opportunity to clear all_zones_ok? I dunno.

Yes, exactly. I moved all_unreclaimable test because then there is
an opportunity to clear all_zones_ok. Otherwise all_zones_ok keeps set
even if all_zones are not OK at all!

> AFAICT, the early clearing of all_zones_ok will have no effect on kswapd
> throttling because the total_scanned logic is disabled.

It makes this at the end of balance_pgdat

if (!all_zones_ok) {
cond_resched();
goto loop_again;
}

happen.

> What I think your patch will do is to cause kswapd to do the `goto
> loop_again' thing if all zones are unreclaimable. Which appears to risk
> putting kswapd into a busy loop when we're out of memory.

Yes, this is exactly what the patch does.

And kswapd has to be into a busy loop when we're out of memory! It has
to be looking for free pages - it should not sleep for god sakes!

Note that it wont cause excessive CPU usage because kswapd will be "polling"
slowly (with priority = DEF_PRIORITY) on the active/inactive lists (shrink_zone).

The cond_resched at the end of balance_pgdat() makes sure no harmful exclusivity
of CPU will happen.

So this way it still does not cause the excessive CPU usage which is avoided by
all_unreclaimable (ie we wont be scanning huge amounts of pages at low priorities)
but at the same time avoids kswapd from possibly sleeping, which is IMO
very bad.

> So I'm all confused and concerned. It would help if you were to explain
> your thinking more completely...

I think now you can understand what I'm thinking.

Does it makes sense to you?