On Wed, Jan 30, 2013 at 11:11:47AM -0500, Konrad Rzeszutek Wilk wrote: > On Mon, Jan 28, 2013 at 11:59:17AM +0900, Minchan Kim wrote: > > On Fri, Jan 25, 2013 at 07:56:29AM -0800, Dan Magenheimer wrote: > > > > From: Seth Jennings [mailto:sjenning@xxxxxxxxxxxxxxxxxx] > > > > Subject: Re: [PATCHv2 1/9] staging: zsmalloc: add gfp flags to zs_create_pool > > > > > > > > On 01/24/2013 07:33 PM, Minchan Kim wrote: > > > > > Hi Seth, frontswap guys > > > > > > > > > > On Tue, Jan 8, 2013 at 5:24 AM, Seth Jennings > > > > > <sjenning@xxxxxxxxxxxxxxxxxx> wrote: > > > > >> zs_create_pool() currently takes a gfp flags argument > > > > >> that is used when growing the memory pool. However > > > > >> it is not used in allocating the metadata for the pool > > > > >> itself. That is currently hardcoded to GFP_KERNEL. > > > > >> > > > > >> zswap calls zs_create_pool() at swapon time which is done > > > > >> in atomic context, resulting in a "might sleep" warning. > > > > > > > > > > I didn't review this all series, really sorry but totday I saw Nitin > > > > > added Acked-by so I'm afraid Greg might get it under my radar. I'm not > > > > > strong against but I would like know why we should call frontswap_init > > > > > under swap_lock? Is there special reason? > > > > > > > > The call stack is: > > > > > > > > SYSCALL_DEFINE2(swapon.. <-- swapon_mutex taken here > > > > enable_swap_info() <-- swap_lock taken here > > > > frontswap_init() > > > > __frontswap_init() > > > > zswap_frontswap_init() > > > > zs_create_pool() > > > > > > > > It isn't entirely clear to me why frontswap_init() is called under > > > > lock. Then again, I'm not entirely sure what the swap_lock protects. > > > > There are no comments near the swap_lock definition to tell me. > > > > > > > > I would guess that the intent is to block any writes to the swap > > > > device until frontswap_init() has completed. > > > > > > > > Dan care to weigh in? > > > > > > I think frontswap's first appearance needs to be atomic, i.e. > > > the transition from (a) frontswap is not present and will fail > > > all calls, to (b) frontswap is fully functional... that transition > > > must be atomic. And, once Konrad's module patches are in, the > > To be fair it can be "delayed". Say the swap disk is in heavy usage and > the backend is registered. The time between the backend going online and > the frontswap_store functions calling in the backend can be delayed (so > we can use a racy unsigned long to check when the backend is on). > > Obviously the opposite is not acceptable (so unsigned long says > backend is enabled, but in reality the backend has not yet been > initialized). > > > > opposite transition must be atomic also. But there are most > > > likely other ways to do those transitions atomically that > > > don't need to hold swap_lock. > > Right. The opposite transition would be when a backend is unloaded. > Which is something we don't do yet. For that to work we would need > to make the "gatekeeper" (this unsigned long I've been referring to) > be atomic. Or at least in some fashion - either via spinlocks or perhaps > using static_key to patch the branching of the code. Naturally to > unload a module extra things such as flushing all the pages the backend > has to the disk is required. > > > > It could be raced once swap_info is registered. > > But what's the problem if we call frontswap_init before calling > > _enable_swap_info out of lock? > > So, we have two locks - the mutex and the spin_lock. I think we are > fine without the spinlock (swap_lock). > > > Swap subsystem never do I/O before it register new swap_info_struct. > > > > And IMHO, if frontswap is to be atomic, it would be better to have > > own scheme without dependency of swap_lock if it's possible. > > I think that can be independent of that lock. We are still under > the mutex (swapon_mutex) which protects us against two threads doing > swapon/swapoff and messing things up. > > > > > > Honestly, I never really focused on the initialization code > > > so I am very open to improvements as long as they work for > > > all the various frontswap backends. > > > > How about this? > > > > From 157a3edf49feb93be0595574beb153b322ddf7d2 Mon Sep 17 00:00:00 2001 > > From: Minchan Kim <minchan@xxxxxxxxxx> > > Date: Mon, 28 Jan 2013 11:34:00 +0900 > > Subject: [PATCH] frontswap: Get rid of swap_lock dependency > > > > Frontswap initialization routine depends on swap_lock, which want > > to be atomic about frontswap's first appearance. > > IOW, frontswap is not present and will fail all calls OR frontswap is > > fully functional but if new swap_info_struct isn't registered > > by enable_swap_info, swap subsystem doesn't start I/O so there is no race > > between init procedure and page I/O working on frontswap. > > > > So let's remove unncessary swap_lock dependency. > > This looks good. I hadn't yet had a chance to test it out though. I hope you pick up if it pass your test. Thanks, Konrad! -- Kind regards, Minchan Kim _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
