> 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. 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. 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> > + 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) -- 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