From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8F65CC433ED for ; Mon, 19 Apr 2021 02:54:37 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 606E4610CA for ; Mon, 19 Apr 2021 02:54:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233084AbhDSCzF (ORCPT ); Sun, 18 Apr 2021 22:55:05 -0400 Received: from mga11.intel.com ([192.55.52.93]:55258 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229845AbhDSCzE (ORCPT ); Sun, 18 Apr 2021 22:55:04 -0400 IronPort-SDR: CME6AwAlVCvo4gr90hMIMlB9cDSR5dh4J3ZmjzxKOB5VC6ljXWmdPONjDDuEWCyvF+wwc1glz+ 6OBkiIQx5AbA== X-IronPort-AV: E=McAfee;i="6200,9189,9958"; a="192071066" X-IronPort-AV: E=Sophos;i="5.82,233,1613462400"; d="scan'208";a="192071066" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Apr 2021 19:54:34 -0700 IronPort-SDR: eAzKJsHcHzyNdV5oBsYqLNL0tkbWI3w8Zv77hHvCohRtf/KK6AxZxuyTxFPWfsxV29YBauFXw3 cK2WURUxjbfw== X-IronPort-AV: E=Sophos;i="5.82,233,1613462400"; d="scan'208";a="419831701" Received: from yhuang6-desk1.sh.intel.com (HELO yhuang6-desk1.ccr.corp.intel.com) ([10.239.13.1]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Apr 2021 19:54:31 -0700 From: "Huang, Ying" To: Miaohe Lin Cc: , , , , , , , , , , , , Subject: Re: [PATCH v2 2/5] mm/swapfile: use percpu_ref to serialize against concurrent swapoff References: <20210417094039.51711-1-linmiaohe@huawei.com> <20210417094039.51711-3-linmiaohe@huawei.com> Date: Mon, 19 Apr 2021 10:54:29 +0800 In-Reply-To: <20210417094039.51711-3-linmiaohe@huawei.com> (Miaohe Lin's message of "Sat, 17 Apr 2021 05:40:36 -0400") Message-ID: <87a6pvkmqi.fsf@yhuang6-desk1.ccr.corp.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=ascii Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Miaohe Lin writes: > Use percpu_ref to serialize against concurrent swapoff. Also remove the > SWP_VALID flag because it's used together with RCU solution. > > Signed-off-by: Miaohe Lin > --- > include/linux/swap.h | 3 +-- > mm/swapfile.c | 43 +++++++++++++++++-------------------------- > 2 files changed, 18 insertions(+), 28 deletions(-) > > diff --git a/include/linux/swap.h b/include/linux/swap.h > index 8be36eb58b7a..993693b38109 100644 > --- a/include/linux/swap.h > +++ b/include/linux/swap.h > @@ -177,7 +177,6 @@ enum { > SWP_PAGE_DISCARD = (1 << 10), /* freed swap page-cluster discards */ > SWP_STABLE_WRITES = (1 << 11), /* no overwrite PG_writeback pages */ > SWP_SYNCHRONOUS_IO = (1 << 12), /* synchronous IO is efficient */ > - SWP_VALID = (1 << 13), /* swap is valid to be operated on? */ > /* add others here before... */ > SWP_SCANNING = (1 << 14), /* refcount in scan_swap_map */ > }; > @@ -514,7 +513,7 @@ sector_t swap_page_sector(struct page *page); > > static inline void put_swap_device(struct swap_info_struct *si) > { > - rcu_read_unlock(); > + percpu_ref_put(&si->users); > } > > #else /* CONFIG_SWAP */ > diff --git a/mm/swapfile.c b/mm/swapfile.c > index 66515a3a2824..90e197bc2eeb 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -1279,18 +1279,12 @@ static unsigned char __swap_entry_free_locked(struct swap_info_struct *p, > * via preventing the swap device from being swapoff, until > * put_swap_device() is called. Otherwise return NULL. > * > - * The entirety of the RCU read critical section must come before the > - * return from or after the call to synchronize_rcu() in > - * enable_swap_info() or swapoff(). So if "si->flags & SWP_VALID" is > - * true, the si->map, si->cluster_info, etc. must be valid in the > - * critical section. > - * > * Notice that swapoff or swapoff+swapon can still happen before the > - * rcu_read_lock() in get_swap_device() or after the rcu_read_unlock() > - * in put_swap_device() if there isn't any other way to prevent > - * swapoff, such as page lock, page table lock, etc. The caller must > - * be prepared for that. For example, the following situation is > - * possible. > + * percpu_ref_tryget_live() in get_swap_device() or after the > + * percpu_ref_put() in put_swap_device() if there isn't any other way > + * to prevent swapoff, such as page lock, page table lock, etc. The > + * caller must be prepared for that. For example, the following > + * situation is possible. > * > * CPU1 CPU2 > * do_swap_page() > @@ -1318,21 +1312,24 @@ struct swap_info_struct *get_swap_device(swp_entry_t entry) > si = swp_swap_info(entry); > if (!si) > goto bad_nofile; > - > - rcu_read_lock(); > - if (data_race(!(si->flags & SWP_VALID))) > - goto unlock_out; > + if (!percpu_ref_tryget_live(&si->users)) > + goto out; > + /* > + * Guarantee we will not reference uninitialized fields > + * of swap_info_struct. > + */ /* * Guarantee the si->users are checked before accessing other fields of * swap_info_struct. */ > + smp_rmb(); Usually, smp_rmb() need to be paired with smp_wmb(). Some comments are needed for that. Here smb_rmb() is paired with the spin_unlock() after setup_swap_info() in enable_swap_info(). > offset = swp_offset(entry); > if (offset >= si->max) > - goto unlock_out; > + goto put_out; > > return si; > bad_nofile: > pr_err("%s: %s%08lx\n", __func__, Bad_file, entry.val); > out: > return NULL; > -unlock_out: > - rcu_read_unlock(); > +put_out: > + percpu_ref_put(&si->users); > return NULL; > } > > @@ -2475,7 +2472,7 @@ static void setup_swap_info(struct swap_info_struct *p, int prio, > > static void _enable_swap_info(struct swap_info_struct *p) > { > - p->flags |= SWP_WRITEOK | SWP_VALID; > + p->flags |= SWP_WRITEOK; > atomic_long_add(p->pages, &nr_swap_pages); > total_swap_pages += p->pages; > > @@ -2507,7 +2504,7 @@ static void enable_swap_info(struct swap_info_struct *p, int prio, > spin_unlock(&swap_lock); > /* > * Guarantee swap_map, cluster_info, etc. fields are valid > - * between get/put_swap_device() if SWP_VALID bit is set > + * between get/put_swap_device(). > */ The comments need to be revised. Something likes below? /* Finished initialized swap device, now it's safe to reference it */ Best Regards, Huang, Ying > percpu_ref_resurrect(&p->users); > spin_lock(&swap_lock); > @@ -2625,12 +2622,6 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile) > > reenable_swap_slots_cache_unlock(); > > - spin_lock(&swap_lock); > - spin_lock(&p->lock); > - p->flags &= ~SWP_VALID; /* mark swap device as invalid */ > - spin_unlock(&p->lock); > - spin_unlock(&swap_lock); > - > percpu_ref_kill(&p->users); > /* > * We need synchronize_rcu() here to protect the accessing From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4482BC433B4 for ; Mon, 19 Apr 2021 02:54:41 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id C77A461008 for ; Mon, 19 Apr 2021 02:54:40 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C77A461008 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 4FEE96B0036; Sun, 18 Apr 2021 22:54:40 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4AEF26B006E; Sun, 18 Apr 2021 22:54:40 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 39D996B0070; Sun, 18 Apr 2021 22:54:40 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0146.hostedemail.com [216.40.44.146]) by kanga.kvack.org (Postfix) with ESMTP id 1A86D6B0036 for ; Sun, 18 Apr 2021 22:54:40 -0400 (EDT) Received: from smtpin16.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id BD1868249980 for ; Mon, 19 Apr 2021 02:54:39 +0000 (UTC) X-FDA: 78047598678.16.D535833 Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by imf19.hostedemail.com (Postfix) with ESMTP id 9073890009F1 for ; Mon, 19 Apr 2021 02:54:18 +0000 (UTC) IronPort-SDR: 9KN4S2qo1AWhAM1pmhslZIBJr2/bSIfN11B06XhXah+O00eyw/zUr9TbsOUOAOj2AkK776kUC7 +ajDKuz895bw== X-IronPort-AV: E=McAfee;i="6200,9189,9958"; a="259218146" X-IronPort-AV: E=Sophos;i="5.82,233,1613462400"; d="scan'208";a="259218146" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Apr 2021 19:54:35 -0700 IronPort-SDR: eAzKJsHcHzyNdV5oBsYqLNL0tkbWI3w8Zv77hHvCohRtf/KK6AxZxuyTxFPWfsxV29YBauFXw3 cK2WURUxjbfw== X-IronPort-AV: E=Sophos;i="5.82,233,1613462400"; d="scan'208";a="419831701" Received: from yhuang6-desk1.sh.intel.com (HELO yhuang6-desk1.ccr.corp.intel.com) ([10.239.13.1]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Apr 2021 19:54:31 -0700 From: "Huang, Ying" To: Miaohe Lin Cc: , , , , , , , , , , , , Subject: Re: [PATCH v2 2/5] mm/swapfile: use percpu_ref to serialize against concurrent swapoff References: <20210417094039.51711-1-linmiaohe@huawei.com> <20210417094039.51711-3-linmiaohe@huawei.com> Date: Mon, 19 Apr 2021 10:54:29 +0800 In-Reply-To: <20210417094039.51711-3-linmiaohe@huawei.com> (Miaohe Lin's message of "Sat, 17 Apr 2021 05:40:36 -0400") Message-ID: <87a6pvkmqi.fsf@yhuang6-desk1.ccr.corp.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=ascii X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: 9073890009F1 X-Stat-Signature: bnakm44egus3ise9f1menzgyancmfr9g Received-SPF: none (intel.com>: No applicable sender policy available) receiver=imf19; identity=mailfrom; envelope-from=""; helo=mga07.intel.com; client-ip=134.134.136.100 X-HE-DKIM-Result: none/none X-HE-Tag: 1618800858-273226 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Miaohe Lin writes: > Use percpu_ref to serialize against concurrent swapoff. Also remove the > SWP_VALID flag because it's used together with RCU solution. > > Signed-off-by: Miaohe Lin > --- > include/linux/swap.h | 3 +-- > mm/swapfile.c | 43 +++++++++++++++++-------------------------- > 2 files changed, 18 insertions(+), 28 deletions(-) > > diff --git a/include/linux/swap.h b/include/linux/swap.h > index 8be36eb58b7a..993693b38109 100644 > --- a/include/linux/swap.h > +++ b/include/linux/swap.h > @@ -177,7 +177,6 @@ enum { > SWP_PAGE_DISCARD = (1 << 10), /* freed swap page-cluster discards */ > SWP_STABLE_WRITES = (1 << 11), /* no overwrite PG_writeback pages */ > SWP_SYNCHRONOUS_IO = (1 << 12), /* synchronous IO is efficient */ > - SWP_VALID = (1 << 13), /* swap is valid to be operated on? */ > /* add others here before... */ > SWP_SCANNING = (1 << 14), /* refcount in scan_swap_map */ > }; > @@ -514,7 +513,7 @@ sector_t swap_page_sector(struct page *page); > > static inline void put_swap_device(struct swap_info_struct *si) > { > - rcu_read_unlock(); > + percpu_ref_put(&si->users); > } > > #else /* CONFIG_SWAP */ > diff --git a/mm/swapfile.c b/mm/swapfile.c > index 66515a3a2824..90e197bc2eeb 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -1279,18 +1279,12 @@ static unsigned char __swap_entry_free_locked(struct swap_info_struct *p, > * via preventing the swap device from being swapoff, until > * put_swap_device() is called. Otherwise return NULL. > * > - * The entirety of the RCU read critical section must come before the > - * return from or after the call to synchronize_rcu() in > - * enable_swap_info() or swapoff(). So if "si->flags & SWP_VALID" is > - * true, the si->map, si->cluster_info, etc. must be valid in the > - * critical section. > - * > * Notice that swapoff or swapoff+swapon can still happen before the > - * rcu_read_lock() in get_swap_device() or after the rcu_read_unlock() > - * in put_swap_device() if there isn't any other way to prevent > - * swapoff, such as page lock, page table lock, etc. The caller must > - * be prepared for that. For example, the following situation is > - * possible. > + * percpu_ref_tryget_live() in get_swap_device() or after the > + * percpu_ref_put() in put_swap_device() if there isn't any other way > + * to prevent swapoff, such as page lock, page table lock, etc. The > + * caller must be prepared for that. For example, the following > + * situation is possible. > * > * CPU1 CPU2 > * do_swap_page() > @@ -1318,21 +1312,24 @@ struct swap_info_struct *get_swap_device(swp_entry_t entry) > si = swp_swap_info(entry); > if (!si) > goto bad_nofile; > - > - rcu_read_lock(); > - if (data_race(!(si->flags & SWP_VALID))) > - goto unlock_out; > + if (!percpu_ref_tryget_live(&si->users)) > + goto out; > + /* > + * Guarantee we will not reference uninitialized fields > + * of swap_info_struct. > + */ /* * Guarantee the si->users are checked before accessing other fields of * swap_info_struct. */ > + smp_rmb(); Usually, smp_rmb() need to be paired with smp_wmb(). Some comments are needed for that. Here smb_rmb() is paired with the spin_unlock() after setup_swap_info() in enable_swap_info(). > offset = swp_offset(entry); > if (offset >= si->max) > - goto unlock_out; > + goto put_out; > > return si; > bad_nofile: > pr_err("%s: %s%08lx\n", __func__, Bad_file, entry.val); > out: > return NULL; > -unlock_out: > - rcu_read_unlock(); > +put_out: > + percpu_ref_put(&si->users); > return NULL; > } > > @@ -2475,7 +2472,7 @@ static void setup_swap_info(struct swap_info_struct *p, int prio, > > static void _enable_swap_info(struct swap_info_struct *p) > { > - p->flags |= SWP_WRITEOK | SWP_VALID; > + p->flags |= SWP_WRITEOK; > atomic_long_add(p->pages, &nr_swap_pages); > total_swap_pages += p->pages; > > @@ -2507,7 +2504,7 @@ static void enable_swap_info(struct swap_info_struct *p, int prio, > spin_unlock(&swap_lock); > /* > * Guarantee swap_map, cluster_info, etc. fields are valid > - * between get/put_swap_device() if SWP_VALID bit is set > + * between get/put_swap_device(). > */ The comments need to be revised. Something likes below? /* Finished initialized swap device, now it's safe to reference it */ Best Regards, Huang, Ying > percpu_ref_resurrect(&p->users); > spin_lock(&swap_lock); > @@ -2625,12 +2622,6 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile) > > reenable_swap_slots_cache_unlock(); > > - spin_lock(&swap_lock); > - spin_lock(&p->lock); > - p->flags &= ~SWP_VALID; /* mark swap device as invalid */ > - spin_unlock(&p->lock); > - spin_unlock(&swap_lock); > - > percpu_ref_kill(&p->users); > /* > * We need synchronize_rcu() here to protect the accessing