Re: [RFC][PATCH -mm 1/2] mm: make shrink_all_memory overflow-resistant

["Jesper Juhl" <jesper.juhl@gmail.com>]

On 2/27/06, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> Make shrink_all_memory() overflow-resistant.
>
>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  include/linux/swap.h |    2 +-
>  mm/vmscan.c          |    9 +++++----
>  2 files changed, 6 insertions(+), 5 deletions(-)
>
> Index: linux-2.6.16-rc4-mm2/mm/vmscan.c
> ===================================================================
> --- linux-2.6.16-rc4-mm2.orig/mm/vmscan.c
> +++ linux-2.6.16-rc4-mm2/mm/vmscan.c
> @@ -1785,18 +1785,19 @@ void wakeup_kswapd(struct zone *zone, in
>   * Try to free `nr_pages' of memory, system-wide.  Returns the number of freed
>   * pages.
>   */
> -int shrink_all_memory(unsigned long nr_pages)
> +unsigned long shrink_all_memory(unsigned int nr_pages)

What about the callers of shrink_all_memory() who currently expect it
to return an int, have you checked how they will react to you changing
the return type (and signedness) ?

>  {
>         pg_data_t *pgdat;
> -       unsigned long nr_to_free = nr_pages;
> -       int ret = 0;
> +       long long nr_to_free = nr_pages;

'nr_pages' passed to the function is an unsigned int, why this change?
also, nr_to_free is later passed to balance_pgdat() as the second
argument and balance_pgdat() expects to be passed an int.


> +       unsigned long ret = 0;
>         struct reclaim_state reclaim_state = {
>                 .reclaimed_slab = 0,
>         };
>
>         current->reclaim_state = &reclaim_state;
>         for_each_pgdat(pgdat) {
> -               int freed;
> +               unsigned long freed;

balance_pgdat() returns a plain int, so what's the point of making
'freed' an unsigned long?


> +
>                 freed = balance_pgdat(pgdat, nr_to_free, 0);
>                 ret += freed;
>                 nr_to_free -= freed;
> Index: linux-2.6.16-rc4-mm2/include/linux/swap.h
> ===================================================================
> --- linux-2.6.16-rc4-mm2.orig/include/linux/swap.h
> +++ linux-2.6.16-rc4-mm2/include/linux/swap.h
> @@ -173,7 +173,7 @@ extern void swap_setup(void);
>
>  /* linux/mm/vmscan.c */
>  extern unsigned long try_to_free_pages(struct zone **, gfp_t);
> -extern int shrink_all_memory(unsigned long nr_pages);
> +extern unsigned long shrink_all_memory(unsigned int nr_pages);
>  extern int vm_swappiness;
>
>  #ifdef CONFIG_NUMA
>


This patch may be correct or it may be wrong, I've not spend enough
time staring at it and follow the code it calls and gets called by to
say which, but I for one would like an explanation of why these
changes are made and why they are correct.
There's a distinct lack of a changelog/explanation with this patch.
Or maybe I'm just dense and can't see the obvious correctness, but if
that's the case I'd still like to be enlightened :)


--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/