lkml.org 
[lkml]   [2011]   [Aug]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] shrinker: fix a bug when callback returns -1
On Mon, Aug 29, 2011 at 03:36:48PM -0400, Mikulas Patocka wrote:
> Hi
>
> This patch fixes a lockup when shrinker callback returns -1.

What lockup is that? I haven't seen any bug reports, and this code
has been like this for several years, so I'm kind of wondering why
this is suddenly an issue....

> BTW. shouldn't the value returned by callback be long instead of int? On
> 64-bit architectures, there may be more than 2^32 entries allocated.

The API hasn't changed since the early 2.5 series, so that wasn't a
consideration when it was originally written. As it is, I make this
exact change in the shrinker API update patchset I proposed
recently for exactly the reasons you suggest:

http://thread.gmane.org/gmane.linux.kernel.mm/67326


> Mikulas
>
> ---
>
> shrinker: fix a bug when callback returns -1
>
> Shrinker callback can return -1 if it is at a risk of deadlock.
> However, this is not tested at some places.
>
> If do_shrinker_shrink returns -1 here
> "max_pass = do_shrinker_shrink(shrinker, shrink, 0)",
> it is converted to an unsigned long integer. This may result in excessive
> total_scan value and a lockup due to code looping too much in
> "while (total_scan >= batch_size)" cycle.

>
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
>
> ---
> mm/vmscan.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> Index: linux-3.1-rc3-fast/mm/vmscan.c
> ===================================================================
> --- linux-3.1-rc3-fast.orig/mm/vmscan.c 2011-08-29 20:34:27.000000000 +0200
> +++ linux-3.1-rc3-fast/mm/vmscan.c 2011-08-29 20:37:38.000000000 +0200
> @@ -250,6 +250,7 @@ unsigned long shrink_slab(struct shrink_
> unsigned long long delta;
> unsigned long total_scan;
> unsigned long max_pass;
> + int sr;
> int shrink_ret = 0;
> long nr;
> long new_nr;
> @@ -266,7 +267,10 @@ unsigned long shrink_slab(struct shrink_
> } while (cmpxchg(&shrinker->nr, nr, 0) != nr);
>
> total_scan = nr;
> - max_pass = do_shrinker_shrink(shrinker, shrink, 0);
> + sr = do_shrinker_shrink(shrinker, shrink, 0);
> + if (sr == -1)
> + continue;

IIRC from my recent shrinker audit, none of the existing shrinkers
return return -1 when nr_to_scan == 0, so this check has never been
necessary.

> + max_pass = sr;
> delta = (4 * nr_pages_scanned) / shrinker->seeks;
> delta *= max_pass;
> do_div(delta, lru_pages + 1);
> @@ -309,6 +313,8 @@ unsigned long shrink_slab(struct shrink_
> int nr_before;
>
> nr_before = do_shrinker_shrink(shrinker, shrink, 0);
> + if (nr_before == -1)
> + break;

Same here.

> shrink_ret = do_shrinker_shrink(shrinker, shrink,
> batch_size);
> if (shrink_ret == -1)

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com

\
 
 \ /
  Last update: 2011-08-30 04:15    [W:0.103 / U:0.188 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site