Date: Tue, 30 Oct 2012 14:04:17 -0700
From: Andrew Morton <akpm@linux-foundation.org>
To: Cesar Eduardo Barros <cesarb@cesarb.net>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        Dan Magenheimer <dan.magenheimer@oracle.com>,
        Mel Gorman <mgorman@suse.de>, Rik van Riel <riel@redhat.com>,
        KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>,
        Johannes Weiner <hannes@cmpxchg.org>
Subject: Re: [PATCH 1/2] mm: refactor reinsert of swap_info in sys_swapoff
Message-Id: <20121030140417.988c2437.akpm@linux-foundation.org>
In-Reply-To: <1351372847-13625-2-git-send-email-cesarb@cesarb.net>
References: <1351372847-13625-1-git-send-email-cesarb@cesarb.net>
	<1351372847-13625-2-git-send-email-cesarb@cesarb.net>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1787
Lines: 38

On Sat, 27 Oct 2012 19:20:46 -0200
Cesar Eduardo Barros <cesarb@cesarb.net> wrote:

> The block within sys_swapoff which re-inserts the swap_info into the
> swap_list in case of failure of try_to_unuse() reads a few values outside
> the swap_lock. While this is safe at that point, it is subtle code.
> 
> Simplify the code by moving the reading of these values to a separate
> function, refactoring it a bit so they are read from within the
> swap_lock. This is easier to understand, and matches better the way it
> worked before I unified the insertion of the swap_info from both
> sys_swapon and sys_swapoff.
> 
> This change should make no functional difference. The only real change
> is moving the read of two or three structure fields to within the lock
> (frontswap_map_get() is nothing more than a read of p->frontswap_map).

Your patch doesn't change this, but...  it is very unusual for any
subsystem's ->init method to be called under a spinlock.  Because it is
highly likely that such a method will wish to do things such as memory
allocation.

It is rare and unlikely for an ->init() method to *need* such external
locking, because all the objects it is dealing with cannot be looked up
by other threads because nothing has been registered anywhere yet.

So either frontswap is doing something wrong here or there's some
subtlety which escapes me.  If the former then we should try to get
that ->init call to happen outside swap_lock.

And if we can do that, perhaps we can fix the regrettable GFP_ATOMIC
in zcache_new_pool().

--
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/