Messages in this thread | | | Date | Mon, 27 Feb 2006 19:53:47 +0100 | From | "Jesper Juhl" <> | Subject | Re: [RFC][PATCH -mm 1/2] mm: make shrink_all_memory overflow-resistant |
| |
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/
| |