lkml.org 
[lkml]   [2017]   [Jul]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] mm, vmscan: do not loop on too_many_isolated for ever
On Fri 21-07-17 16:01:04, Andrew Morton wrote:
> On Thu, 20 Jul 2017 08:56:26 +0200 Michal Hocko <mhocko@kernel.org> wrote:
> >
> > > > --- a/mm/vmscan.c
> > > > +++ b/mm/vmscan.c
> > > > @@ -1713,9 +1713,15 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
> > > > int file = is_file_lru(lru);
> > > > struct pglist_data *pgdat = lruvec_pgdat(lruvec);
> > > > struct zone_reclaim_stat *reclaim_stat = &lruvec->reclaim_stat;
> > > > + bool stalled = false;
> > > >
> > > > while (unlikely(too_many_isolated(pgdat, file, sc))) {
> > > > - congestion_wait(BLK_RW_ASYNC, HZ/10);
> > > > + if (stalled)
> > > > + return 0;
> > > > +
> > > > + /* wait a bit for the reclaimer. */
> > > > + schedule_timeout_interruptible(HZ/10);
> > >
> > > a) if this task has signal_pending(), this falls straight through
> > > and I suspect the code breaks?
> >
> > It will not break. It will return to the allocation path more quickly
> > but no over-reclaim will happen and it will/should get throttled there.
> > So nothing critical.
> >
> > > b) replacing congestion_wait() with schedule_timeout_interruptible()
> > > means this task no longer contributes to load average here and it's
> > > a (slightly) user-visible change.
> >
> > you are right. I am not sure it matters but it might be visible.
> >
> > > c) msleep_interruptible() is nicer
> > >
> > > d) IOW, methinks we should be using msleep() here?
> >
> > OK, I do not have objections. Are you going to squash this in or want a
> > separate patch explaining all the above?
>
> I'd prefer to have a comment explaining why interruptible sleep is
> being used, because that "what if signal_pending()" case is rather a
> red flag.

I didn't really consider interruptible vs. uninterruptible sleep so it
wasn't really a deliberate decision. Now, that you have brought up the
above points I am OK changing that the uninterruptible.

Here is a fix up. I am fine with this either folded in or as a separate
patch.
---
From 4b295fc1625d499a2e1283b036aea005158b33cc Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Mon, 24 Jul 2017 08:43:23 +0200
Subject: [PATCH] mm, vmscan: throttle too_many_isolated by uninterruptible
sleep

"mm, vmscan: do not loop on too_many_isolated for ever" has added an
interruptible sleep into shrink_inactive_list when we have isolated too
many pages. Andrew has noticed that we used to sleep in uninterruptible
sleep previously (in congestion_wait) and that changing that is wrong
for at least two reasons
- waiting task would not participate in the load average anymore
- task with a pending signal will not sleep and bail out
immediately
While none of those issues are critical in any way but they are
unnecessary. The interruptible sleep was more an oversight than a
deliberate decision. Fix this by using msleep instead.

Spotted-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
mm/vmscan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index 7ba9467b6d58..ed0c29a3db16 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1749,7 +1749,7 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
return 0;

/* wait a bit for the reclaimer. */
- schedule_timeout_interruptible(HZ/10);
+ msleep(100);
stalled = true;

/* We are about to die and free our memory. Return now. */
--
2.13.2

--
Michal Hocko
SUSE Labs

\
 
 \ /
  Last update: 2017-07-24 08:51    [W:0.178 / U:1.620 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site