All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH -mm] mm, swap: Simplify total_swapcache_pages() with get_swap_device()
@ 2019-05-27  8:27 Huang, Ying
  2019-05-27 10:15 ` Paul E. McKenney
  0 siblings, 1 reply; 4+ messages in thread
From: Huang, Ying @ 2019-05-27  8:27 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-mm, linux-kernel, Huang Ying, Hugh Dickins,
	Paul E . McKenney, Minchan Kim, Johannes Weiner, Tim Chen,
	Mel Gorman, Jérôme Glisse, Michal Hocko,
	Andrea Arcangeli, Yang Shi, David Rientjes, Rik van Riel,
	Jan Kara, Dave Jiang, Daniel Jordan, Andrea Parri

From: Huang Ying <ying.huang@intel.com>

total_swapcache_pages() may race with swapper_spaces[] allocation and
freeing.  Previously, this is protected with a swapper_spaces[]
specific RCU mechanism.  To simplify the logic/code complexity, it is
replaced with get/put_swap_device().  The code line number is reduced
too.  Although not so important, the swapoff() performance improves
too because one synchronize_rcu() call during swapoff() is deleted.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Jérôme Glisse <jglisse@redhat.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Yang Shi <yang.shi@linux.alibaba.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: Jan Kara <jack@suse.cz>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: Andrea Parri <andrea.parri@amarulasolutions.com>
---
 mm/swap_state.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/mm/swap_state.c b/mm/swap_state.c
index f509cdaa81b1..b84c58b572ca 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -73,23 +73,19 @@ unsigned long total_swapcache_pages(void)
 	unsigned int i, j, nr;
 	unsigned long ret = 0;
 	struct address_space *spaces;
+	struct swap_info_struct *si;
 
-	rcu_read_lock();
 	for (i = 0; i < MAX_SWAPFILES; i++) {
-		/*
-		 * The corresponding entries in nr_swapper_spaces and
-		 * swapper_spaces will be reused only after at least
-		 * one grace period.  So it is impossible for them
-		 * belongs to different usage.
-		 */
-		nr = nr_swapper_spaces[i];
-		spaces = rcu_dereference(swapper_spaces[i]);
-		if (!nr || !spaces)
+		/* Prevent swapoff to free swapper_spaces */
+		si = get_swap_device(swp_entry(i, 1));
+		if (!si)
 			continue;
+		nr = nr_swapper_spaces[i];
+		spaces = swapper_spaces[i];
 		for (j = 0; j < nr; j++)
 			ret += spaces[j].nrpages;
+		put_swap_device(si);
 	}
-	rcu_read_unlock();
 	return ret;
 }
 
@@ -611,20 +607,16 @@ int init_swap_address_space(unsigned int type, unsigned long nr_pages)
 		mapping_set_no_writeback_tags(space);
 	}
 	nr_swapper_spaces[type] = nr;
-	rcu_assign_pointer(swapper_spaces[type], spaces);
+	swapper_spaces[type] = spaces;
 
 	return 0;
 }
 
 void exit_swap_address_space(unsigned int type)
 {
-	struct address_space *spaces;
-
-	spaces = swapper_spaces[type];
+	kvfree(swapper_spaces[type]);
 	nr_swapper_spaces[type] = 0;
-	rcu_assign_pointer(swapper_spaces[type], NULL);
-	synchronize_rcu();
-	kvfree(spaces);
+	swapper_spaces[type] = NULL;
 }
 
 static inline void swap_ra_clamp_pfn(struct vm_area_struct *vma,
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH -mm] mm, swap: Simplify total_swapcache_pages() with get_swap_device()
  2019-05-27  8:27 [PATCH -mm] mm, swap: Simplify total_swapcache_pages() with get_swap_device() Huang, Ying
@ 2019-05-27 10:15 ` Paul E. McKenney
  2019-05-27 13:28   ` Andrea Parri
  2019-05-28  0:46   ` Huang, Ying
  0 siblings, 2 replies; 4+ messages in thread
From: Paul E. McKenney @ 2019-05-27 10:15 UTC (permalink / raw)
  To: Huang, Ying
  Cc: Andrew Morton, linux-mm, linux-kernel, Hugh Dickins, Minchan Kim,
	Johannes Weiner, Tim Chen, Mel Gorman, Jérôme Glisse,
	Michal Hocko, Andrea Arcangeli, Yang Shi, David Rientjes,
	Rik van Riel, Jan Kara, Dave Jiang, Daniel Jordan, Andrea Parri

On Mon, May 27, 2019 at 04:27:14PM +0800, Huang, Ying wrote:
> From: Huang Ying <ying.huang@intel.com>
> 
> total_swapcache_pages() may race with swapper_spaces[] allocation and
> freeing.  Previously, this is protected with a swapper_spaces[]
> specific RCU mechanism.  To simplify the logic/code complexity, it is
> replaced with get/put_swap_device().  The code line number is reduced
> too.  Although not so important, the swapoff() performance improves
> too because one synchronize_rcu() call during swapoff() is deleted.

I am guessing that total_swapcache_pages() is not used on any
fastpaths, but must defer to others on this.  Of course, if the
performance/scalability of total_swapcache_pages() is important,
benchmarking is needed.

But where do I find get_swap_device() and put_swap_device()?  I do not
see them in current mainline.

							Thanx, Paul

> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Tim Chen <tim.c.chen@linux.intel.com>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: Jérôme Glisse <jglisse@redhat.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Cc: Yang Shi <yang.shi@linux.alibaba.com>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Dave Jiang <dave.jiang@intel.com>
> Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
> Cc: Andrea Parri <andrea.parri@amarulasolutions.com>
> ---
>  mm/swap_state.c | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)
> 
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index f509cdaa81b1..b84c58b572ca 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -73,23 +73,19 @@ unsigned long total_swapcache_pages(void)
>  	unsigned int i, j, nr;
>  	unsigned long ret = 0;
>  	struct address_space *spaces;
> +	struct swap_info_struct *si;
> 
> -	rcu_read_lock();
>  	for (i = 0; i < MAX_SWAPFILES; i++) {
> -		/*
> -		 * The corresponding entries in nr_swapper_spaces and
> -		 * swapper_spaces will be reused only after at least
> -		 * one grace period.  So it is impossible for them
> -		 * belongs to different usage.
> -		 */
> -		nr = nr_swapper_spaces[i];
> -		spaces = rcu_dereference(swapper_spaces[i]);
> -		if (!nr || !spaces)
> +		/* Prevent swapoff to free swapper_spaces */
> +		si = get_swap_device(swp_entry(i, 1));
> +		if (!si)
>  			continue;
> +		nr = nr_swapper_spaces[i];
> +		spaces = swapper_spaces[i];
>  		for (j = 0; j < nr; j++)
>  			ret += spaces[j].nrpages;
> +		put_swap_device(si);
>  	}
> -	rcu_read_unlock();
>  	return ret;
>  }
> 
> @@ -611,20 +607,16 @@ int init_swap_address_space(unsigned int type, unsigned long nr_pages)
>  		mapping_set_no_writeback_tags(space);
>  	}
>  	nr_swapper_spaces[type] = nr;
> -	rcu_assign_pointer(swapper_spaces[type], spaces);
> +	swapper_spaces[type] = spaces;
> 
>  	return 0;
>  }
> 
>  void exit_swap_address_space(unsigned int type)
>  {
> -	struct address_space *spaces;
> -
> -	spaces = swapper_spaces[type];
> +	kvfree(swapper_spaces[type]);
>  	nr_swapper_spaces[type] = 0;
> -	rcu_assign_pointer(swapper_spaces[type], NULL);
> -	synchronize_rcu();
> -	kvfree(spaces);
> +	swapper_spaces[type] = NULL;
>  }
> 
>  static inline void swap_ra_clamp_pfn(struct vm_area_struct *vma,
> -- 
> 2.20.1
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH -mm] mm, swap: Simplify total_swapcache_pages() with get_swap_device()
  2019-05-27 10:15 ` Paul E. McKenney
@ 2019-05-27 13:28   ` Andrea Parri
  2019-05-28  0:46   ` Huang, Ying
  1 sibling, 0 replies; 4+ messages in thread
From: Andrea Parri @ 2019-05-27 13:28 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
	Minchan Kim, Johannes Weiner, Tim Chen, Mel Gorman,
	Jérôme Glisse, Michal Hocko, Andrea Arcangeli,
	Yang Shi, David Rientjes, Rik van Riel, Jan Kara, Dave Jiang,
	Daniel Jordan

> But where do I find get_swap_device() and put_swap_device()?  I do not
> see them in current mainline.

You should see them in the -mm tree:

  https://ozlabs.org/~akpm/mmots/broken-out/mm-swap-fix-race-between-swapoff-and-some-swap-operations.patch

or

  http://git.cmpxchg.org/cgit.cgi/linux-mmots.git/commit/?id=87efc56527b92a59d15c5d4e4b05f875b276a59a

Thanks,
  Andrea

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH -mm] mm, swap: Simplify total_swapcache_pages() with get_swap_device()
  2019-05-27 10:15 ` Paul E. McKenney
  2019-05-27 13:28   ` Andrea Parri
@ 2019-05-28  0:46   ` Huang, Ying
  1 sibling, 0 replies; 4+ messages in thread
From: Huang, Ying @ 2019-05-28  0:46 UTC (permalink / raw)
  To: Paul E. McKenney
  Cc: Andrew Morton, linux-mm, linux-kernel, Hugh Dickins, Minchan Kim,
	Johannes Weiner, Tim Chen, Mel Gorman, Jérôme Glisse,
	Michal Hocko, Andrea Arcangeli, Yang Shi, David Rientjes,
	Rik van Riel, Jan Kara, Dave Jiang, Daniel Jordan, Andrea Parri

Hi, Paul,

"Paul E. McKenney" <paulmck@linux.ibm.com> writes:

> On Mon, May 27, 2019 at 04:27:14PM +0800, Huang, Ying wrote:
>> From: Huang Ying <ying.huang@intel.com>
>> 
>> total_swapcache_pages() may race with swapper_spaces[] allocation and
>> freeing.  Previously, this is protected with a swapper_spaces[]
>> specific RCU mechanism.  To simplify the logic/code complexity, it is
>> replaced with get/put_swap_device().  The code line number is reduced
>> too.  Although not so important, the swapoff() performance improves
>> too because one synchronize_rcu() call during swapoff() is deleted.
>
> I am guessing that total_swapcache_pages() is not used on any
> fastpaths, but must defer to others on this.  Of course, if the
> performance/scalability of total_swapcache_pages() is important,
> benchmarking is needed.

This patch is mostly about code cleanup instead of performance.  That
is, to make the code easier to be understand.

> But where do I find get_swap_device() and put_swap_device()?  I do not
> see them in current mainline.

They are not in mainline, but in -mm tree.  I should have made it more
clear.  Sorry about that.

Best Regards,
Huang, Ying


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2019-05-28  0:46 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-27  8:27 [PATCH -mm] mm, swap: Simplify total_swapcache_pages() with get_swap_device() Huang, Ying
2019-05-27 10:15 ` Paul E. McKenney
2019-05-27 13:28   ` Andrea Parri
2019-05-28  0:46   ` Huang, Ying

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.