On Fri, Aug 5, 2011 at 12:47 AM, Dan Magenheimer <dan.magenheimer@xxxxxxxxxx> wrote: >> From: Michal Hocko [mailto:mhocko@xxxxxxx] >> Sent: Thursday, August 04, 2011 3:00 AM >> Subject: Re: [PATCH 2/4] frontswap: using vzalloc instead of vmalloc >> >> On Thu 04-08-11 10:14:07, Johannes Weiner wrote: >> > On Thu, Aug 04, 2011 at 09:57:30AM +0200, Michal Hocko wrote: >> > > On Thu 04-08-11 11:09:48, Bob Liu wrote: >> > > > This patch also add checking whether alloc frontswap_map memory >> > > > failed. >> > > > >> > > > Signed-off-by: Bob Liu <lliubbo@xxxxxxxxx> >> > > > --- >> > > > mm/swapfile.c | 6 +++--- >> > > > 1 files changed, 3 insertions(+), 3 deletions(-) >> > > > >> > > > diff --git a/mm/swapfile.c b/mm/swapfile.c >> > > > index ffdd06a..8fe9e88 100644 >> > > > --- a/mm/swapfile.c >> > > > +++ b/mm/swapfile.c >> > > > @@ -2124,9 +2124,9 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags) >> > > > } >> > > > /* frontswap enabled? set up bit-per-page map for frontswap */ >> > > > if (frontswap_enabled) { >> > > > - frontswap_map = vmalloc(maxpages / sizeof(long)); >> > > > - if (frontswap_map) >> > > > - memset(frontswap_map, 0, maxpages / sizeof(long)); >> > > > + frontswap_map = vzalloc(maxpages / sizeof(long)); >> > > > + if (!frontswap_map) >> > > > + goto bad_swap; >> > > >> > > vzalloc part looks good but shouldn't we disable frontswap rather than >> > > fail? >> > >> > Silently dropping explicitely enabled features is not a good idea, >> > IMO. >> >> Sure, I didn't mean silently. It should be a big fat warning that there >> is not enough memory to enable the feature. >> >> > But from a quick look, this seems to be actually happening as >> > frontswap's bitmap tests check for whether there is even a bitmap >> > allocated and it should essentially never do anything for real if >> > there isn't. >> >> Yes, that was my impression as well. I wasn't 100% sure about that >> though, because there are many places which check frontswap_enabled and >> do not check the map. I though that disabling the feature should be >> safer. >> >> > How about printing a warning as to why the swapon fails and give the >> > admin a choice to disable it on her own? >> >> I am not that familiar with the code but drivers/staging/zcache/zcache.c >> says: >> /* >> * zcache initialization >> * NOTE FOR NOW zcache MUST BE PROVIDED AS A KERNEL BOOT PARAMETER OR >> * NOTHING HAPPENS! >> */ >> >> Is there something admin can do about it? >> >> > >> > It's outside this patch's scope, though, just as changing the >> > behaviour to fail swapon is. >> >> Agreed. The patch should just use vzalloc and the allocation failure >> should be handled separately - if needed at all. > > Agreed here too. The frontswap_enabled flag is global (enabling frontswap > across all frontswap devices) whereas failure to allocate the frontswap_map > will disable frontswap for only one swap device. And since frontswap is > strictly a performance enhancement, there's no reason to fail the swapon > for the entire swap device. > Agreed. > I am fairly sure that the failed allocation is handled gracefully > through the remainder of the frontswap code, but will re-audit to > confirm. A warning might be nice though. > There is a place i think maybe have problem. function __frontswap_flush_area() in file frontswap.c called memset(sis->frontswap_map, .., ..); But if frontswap_map allocation fail there is a null pointer access ? > In any case: > >> - frontswap_map = vmalloc(maxpages / sizeof(long)); >> - if (frontswap_map) >> - memset(frontswap_map, 0, maxpages / sizeof(long)); >> + frontswap_map = vzalloc(maxpages / sizeof(long)); > > Acked-by: Dan Magenheimer <dan.magenheimer@xxxxxxxxxx> > Thanks, >> + if (!frontswap_map) >> + goto bad_swap; > > NAK > > Dan > > Thanks... for the memory! > I really could use more / my throughput's on the floor > The balloon is flat / my swap disk's fat / I've OOM's in store > Overcommitted so much > (with apologies to Bob Hope) > :) -- Regards, --Bob -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: <a href