linux-kernel - RE: [PATCHv2 1/9] staging: zsmalloc: add gfp flags to zs_create

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b3ab2d2f-a0a1-44ff-ac8f-bb0ed73d8978@default>
Date:	Fri, 25 Jan 2013 07:56:29 -0800 (PST)
From:	Dan Magenheimer <dan.magenheimer@...cle.com>
To:	Seth Jennings <sjenning@...ux.vnet.ibm.com>,
	Minchan Kim <minchan@...nel.org>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Nitin Gupta <ngupta@...are.org>,
	Konrad Wilk <konrad.wilk@...cle.com>,
	Robert Jennings <rcj@...ux.vnet.ibm.com>,
	Jenifer Hopper <jhopper@...ibm.com>,
	Mel Gorman <mgorman@...e.de>,
	Johannes Weiner <jweiner@...hat.com>,
	Rik van Riel <riel@...hat.com>,
	Larry Woodman <lwoodman@...hat.com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, devel@...verdev.osuosl.org
Subject: RE: [PATCHv2 1/9] staging: zsmalloc: add gfp flags to zs_create_pool

> From: Seth Jennings [mailto:sjenning@...ux.vnet.ibm.com]
> 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@...ux.vnet.ibm.com> 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
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.

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.

Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/