lkml.org 
[lkml]   [2021]   [Apr]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 1/5] mm/swapfile: add percpu_ref support for swap
    Date
    "Huang, Ying" <ying.huang@intel.com> writes:

    > Miaohe Lin <linmiaohe@huawei.com> writes:
    >
    >> We will use percpu-refcount to serialize against concurrent swapoff. This
    >> patch adds the percpu_ref support for later fixup.
    >>
    >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
    >> ---
    >> include/linux/swap.h | 2 ++
    >> mm/swapfile.c | 25 ++++++++++++++++++++++---
    >> 2 files changed, 24 insertions(+), 3 deletions(-)
    >>
    >> diff --git a/include/linux/swap.h b/include/linux/swap.h
    >> index 144727041e78..849ba5265c11 100644
    >> --- a/include/linux/swap.h
    >> +++ b/include/linux/swap.h
    >> @@ -240,6 +240,7 @@ struct swap_cluster_list {
    >> * The in-memory structure used to track swap areas.
    >> */
    >> struct swap_info_struct {
    >> + struct percpu_ref users; /* serialization against concurrent swapoff */
    >> unsigned long flags; /* SWP_USED etc: see above */
    >> signed short prio; /* swap priority of this type */
    >> struct plist_node list; /* entry in swap_active_head */
    >> @@ -260,6 +261,7 @@ struct swap_info_struct {
    >> struct block_device *bdev; /* swap device or bdev of swap file */
    >> struct file *swap_file; /* seldom referenced */
    >> unsigned int old_block_size; /* seldom referenced */
    >> + struct completion comp; /* seldom referenced */
    >> #ifdef CONFIG_FRONTSWAP
    >> unsigned long *frontswap_map; /* frontswap in-use, one bit per page */
    >> atomic_t frontswap_pages; /* frontswap pages in-use counter */
    >> diff --git a/mm/swapfile.c b/mm/swapfile.c
    >> index 149e77454e3c..724173cd7d0c 100644
    >> --- a/mm/swapfile.c
    >> +++ b/mm/swapfile.c
    >> @@ -39,6 +39,7 @@
    >> #include <linux/export.h>
    >> #include <linux/swap_slots.h>
    >> #include <linux/sort.h>
    >> +#include <linux/completion.h>
    >>
    >> #include <asm/tlbflush.h>
    >> #include <linux/swapops.h>
    >> @@ -511,6 +512,15 @@ static void swap_discard_work(struct work_struct *work)
    >> spin_unlock(&si->lock);
    >> }
    >>
    >> +static void swap_users_ref_free(struct percpu_ref *ref)
    >> +{
    >> + struct swap_info_struct *si;
    >> +
    >> + si = container_of(ref, struct swap_info_struct, users);
    >> + complete(&si->comp);
    >> + percpu_ref_exit(&si->users);
    >
    > Because percpu_ref_exit() is used, we cannot use percpu_ref_tryget() in
    > get_swap_device(), better to add comments there.

    I just noticed that the comments of percpu_ref_tryget_live() says,

    * This function is safe to call as long as @ref is between init and exit.

    While we need to call get_swap_device() almost at any time, so it's
    better to avoid to call percpu_ref_exit() at all. This will waste some
    memory, but we need to follow the API definition to avoid potential
    issues in the long term.

    And we need to call percpu_ref_init() before insert the swap_info_struct
    into the swap_info[].

    >> +}
    >> +
    >> static void alloc_cluster(struct swap_info_struct *si, unsigned long idx)
    >> {
    >> struct swap_cluster_info *ci = si->cluster_info;
    >> @@ -2500,7 +2510,7 @@ static void enable_swap_info(struct swap_info_struct *p, int prio,
    >> * Guarantee swap_map, cluster_info, etc. fields are valid
    >> * between get/put_swap_device() if SWP_VALID bit is set
    >> */
    >> - synchronize_rcu();
    >> + percpu_ref_reinit(&p->users);
    >
    > Although the effect is same, I think it's better to use
    > percpu_ref_resurrect() here to improve code readability.

    Check the original commit description for commit eb085574a752 "mm, swap:
    fix race between swapoff and some swap operations" and discussion email
    thread as follows again,

    https://lore.kernel.org/linux-mm/20171219053650.GB7829@linux.vnet.ibm.com/

    I found that the synchronize_rcu() here is to avoid to call smp_rmb() or
    smp_load_acquire() in get_swap_device(). Now we will use
    percpu_ref_tryget_live() in get_swap_device(), so we will need to add
    the necessary memory barrier, or make sure percpu_ref_tryget_live() has
    ACQUIRE semantics. Per my understanding, we need to change
    percpu_ref_tryget_live() for that.

    >> spin_lock(&swap_lock);
    >> spin_lock(&p->lock);
    >> _enable_swap_info(p);
    >> @@ -2621,11 +2631,13 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
    >> p->flags &= ~SWP_VALID; /* mark swap device as invalid */
    >> spin_unlock(&p->lock);
    >> spin_unlock(&swap_lock);
    >> +
    >> + percpu_ref_kill(&p->users);
    >> /*
    >> * wait for swap operations protected by get/put_swap_device()
    >> * to complete
    >> */
    >> - synchronize_rcu();
    >> + wait_for_completion(&p->comp);
    >
    > Better to move percpu_ref_kill() after the comments. And maybe revise
    > the comments.

    After reading the original commit description as above, I found that we
    need synchronize_rcu() here to protect the accessing to the swap cache
    data structure. Because there's call_rcu() during percpu_ref_kill(), it
    appears OK to keep the synchronize_rcu() here. And we need to revise
    the comments to make it clear what is protected by which operation.

    Best Regards,
    Huang, Ying

    [snip]

    \
     
     \ /
      Last update: 2021-04-12 09:29    [W:2.456 / U:0.140 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site