Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free

[Tim Chen <tim.c.chen@linux.intel.com>]

On Sun, 2017-04-23 at 21:16 +0800, Huang, Ying wrote:
> Tim Chen <tim.c.chen@linux.intel.com> writes:
> 
> > 
> > On Fri, 2017-04-21 at 20:29 +0800, Huang, Ying wrote:
> > > 
> > > "Huang, Ying" <ying.huang@intel.com> writes:
> > > 
> > > > 
> > > > 
> > > > Minchan Kim <minchan@kernel.org> writes:
> > > > 
> > > > > 
> > > > > 
> > > > > On Wed, Apr 19, 2017 at 04:14:43PM +0800, Huang, Ying wrote:
> > > > > > 
> > > > > > 
> > > > > > Minchan Kim <minchan@kernel.org> writes:
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > Hi Huang,
> > > > > > > 
> > > > > > > On Fri, Apr 07, 2017 at 02:49:01PM +0800, Huang, Ying wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > From: Huang Ying <ying.huang@intel.com>
> > > > > > > > 
> > > > > > > >  void swapcache_free_entries(swp_entry_t *entries, int n)
> > > > > > > >  {
> > > > > > > >  	struct swap_info_struct *p, *prev;
> > > > > > > > @@ -1075,6 +1083,10 @@ void swapcache_free_entries(swp_entry_t *entries, int n)
> > > > > > > >  
> > > > > > > >  	prev = NULL;
> > > > > > > >  	p = NULL;
> > > > > > > > +
> > > > > > > > +	/* Sort swap entries by swap device, so each lock is only taken once. */
> > > > > > > > +	if (nr_swapfiles > 1)
> > > > > > > > +		sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
> > > > > > > Let's think on other cases.
> > > > > > > 
> > > > > > > There are two swaps and they are configured by priority so a swap's usage
> > > > > > > would be zero unless other swap used up. In case of that, this sorting
> > > > > > > is pointless.
> > > > > > > 
> > > > > > > As well, nr_swapfiles is never decreased so if we enable multiple
> > > > > > > swaps and then disable until a swap is remained, this sorting is
> > > > > > > pointelss, too.
> > > > > > > 
> > > > > > > How about lazy sorting approach? IOW, if we found prev != p and,
> > > > > > > then we can sort it.
> > > > > > Yes.  That should be better.  I just don't know whether the added
> > > > > > complexity is necessary, given the array is short and sort is fast.
> > > > > Huh?
> > > > > 
> > > > > 1. swapon /dev/XXX1
> > > > > 2. swapon /dev/XXX2
> > > > > 3. swapoff /dev/XXX2
> > > > > 4. use only one swap
> > > > > 5. then, always pointless sort.
> > > > Yes.  In this situation we will do unnecessary sorting.  What I don't
> > > > know is whether the unnecessary sorting will hurt performance in real
> > > > life.  I can do some measurement.
> > > I tested the patch with 1 swap device and 1 process to eat memory
> > > (remove the "if (nr_swapfiles > 1)" for test).  
> > It is possible that nr_swapfiles > 1 when we have only 1 swapfile due
> > to swapoff.  The nr_swapfiles never decrement on swapoff.
> > We will need to use another counter in alloc_swap_info and
> > swapoff to track the true number of swapfiles in use to have a fast path
> > that avoid the search and sort for the 1 swap case.
> Yes.  That is a possible optimization.  But it doesn't cover another use
> cases raised by Minchan (two swap device with different priority).  So
> in general, we still need to check whether there are entries from
> multiple swap devices in the array.  Given the cost of the checking code
> is really low, I think maybe we can just always use the checking code.
> Do you think so?

The single swap case is very common. It will be better if we can bypass the
extra logic and cost for multiple swap.  Yes, we still need the proper
check to see if sort is necessary as you proposed for the multiple swap case.

Tim