Em 30-10-2012 19:04, Andrew Morton escreveu:
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.
The frontswap_init() method is being passed the swap_info_struct's ->type, which it uses to get back the swap_info_struct itself, which it then uses to check if the frontswap_map allocation succeeded. As noted by the commit message for commit 38b5faf (mm: frontswap: core swap subsystem hooks and headers), that allocation can fail, which will do nothing more than not enable frontswap for that swap area.
The same parameter is then passed down to the ->init() method, which proceeds to sumarily ignore it on the three in-tree implementations I looked at.
Yeah, it looks like a violation of YAGNI to me, and doing things in a more roundabount way than is justified. Why pass the ->type and then get the pointer from it instead of just passing the pointer in the first place? Or better yet, why not pass the frontswap_map pointer? Even better, why not a boolean saying whether it worked? Even better, *why not just put the conditional inside enable_swap_info* and pass no parameter at all?
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.
I believe the swap_lock is protecting the poolid. It is possible that other things in the frontswap code are being called before the first swapon with a successful frontswap_map allocation (which is when the poolid would get allocated).
A quick look suggests the poolid only gets set on an initcall or in the ->init() method; perhaps a local mutex (to prevent double allocation) and an atomic update of the poolid would be enough to move it outside the lock (and also outside the swapon_mutex).
But that would work only if no out-of-tree frontswap module needs it within the swap_lock.
And if we can do that, perhaps we can fix the regrettable GFP_ATOMIC in zcache_new_pool().
-- Cesar Eduardo Barros cesarb@xxxxxxxxxx cesar.barros@xxxxxxxxx -- 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/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>