lkml.org 
[lkml]   [2012]   [Oct]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/2] mm: refactor reinsert of swap_info in sys_swapoff
    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@cesarb.net
    cesar.barros@gmail.com


    \
     
     \ /
      Last update: 2012-10-31 00:21    [W:2.872 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site