On Tue, Oct 30, 2012 at 02:04:17PM -0700, Andrew Morton wrote:

On Sat, 27 Oct 2012 19:20:46 -0200
Cesar Eduardo Barros [off-list ref] wrote:

quoted

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.

I don't believe it actually needs that locking. Dan, do you recall
the details of this?

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.

Agreed.

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

Ouch. Yes.


FYI, thanks for pulling those two patches - they looked good to me
but I hadn't had a chance to test them so did not want to comment on them
until that happen. Dan beat me to it and he did test them.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>