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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87efwe3as0.fsf@yhuang-dev.intel.com>
Date:   Thu, 27 Apr 2017 09:21:51 +0800
From:   "Huang\, Ying" <ying.huang@...el.com>
To:     Tim Chen <tim.c.chen@...ux.intel.com>
Cc:     "Huang\, Ying" <ying.huang@...el.com>,
        Minchan Kim <minchan@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
        Hugh Dickins <hughd@...gle.com>, Shaohua Li <shli@...nel.org>,
        Rik van Riel <riel@...hat.com>
Subject: Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free

Tim Chen <tim.c.chen@...ux.intel.com> writes:

>> 
>> From 7bd903c42749c448ef6acbbdee8dcbc1c5b498b9 Mon Sep 17 00:00:00 2001
>> From: Huang Ying <ying.huang@...el.com>
>> Date: Thu, 23 Feb 2017 13:05:20 +0800
>> Subject: [PATCH -v5] mm, swap: Sort swap entries before free
>> 
>> 
>> ---
>>  mm/swapfile.c | 43 ++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 38 insertions(+), 5 deletions(-)
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index 71890061f653..10e75f9e8ac1 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -37,6 +37,7 @@
>>  #include <linux/swapfile.h>
>>  #include <linux/export.h>
>>  #include <linux/swap_slots.h>
>> +#include <linux/sort.h>
>>  
>>  #include <asm/pgtable.h>
>>  #include <asm/tlbflush.h>
>> @@ -1065,20 +1066,52 @@ void swapcache_free(swp_entry_t entry)
>>  	}
>>  }
>>  
>> +static int swp_entry_cmp(const void *ent1, const void *ent2)
>> +{
>> +	const swp_entry_t *e1 = ent1, *e2 = ent2;
>> +
>> +	return (int)(swp_type(*e1) - swp_type(*e2));
>> +}
>> +
>>  void swapcache_free_entries(swp_entry_t *entries, int n)
>>  {
>>  	struct swap_info_struct *p, *prev;
>> -	int i;
>> +	int i, m;
>> +	swp_entry_t entry;
>> +	unsigned int prev_swp_type;
>
> I think it will be clearer to name prev_swp_type as first_swp_type
> as this is the swp type of the first entry.

Yes.  That is better!  Will do that.

>>  
>>  	if (n <= 0)
>>  		return;
>>  
>>  	prev = NULL;
>>  	p = NULL;
>> -	for (i = 0; i < n; ++i) {
>> -		p = swap_info_get_cont(entries[i], prev);
>> -		if (p)
>> -			swap_entry_free(p, entries[i]);
>> +	m = 0;
>> +	prev_swp_type = swp_type(entries[0]);
>> +	for (i = 0; i < n; i++) {
>> +		entry = entries[i];
>> +		if (likely(swp_type(entry) == prev_swp_type)) {
>> +			p = swap_info_get_cont(entry, prev);
>> +			if (likely(p))
>> +				swap_entry_free(p, entry);
>> +			prev = p;
>> +		} else if (!m)
>> +			m = i;
>> +	}
>> +	if (p)
>> +		spin_unlock(&p->lock);
>> +	if (likely(!m))
>> +		return;
>> +
>
> We could still have prev_swp_type at the first entry after sorting.
> and we can avoid an unlock/relock for this case if we do this:
>
> 	if (likely(!m)) {
> 		if (p)
> 			spin_unlock(&p->lock);
> 		return;
> 	}
> 		
>> +	/* Sort swap entries by swap device, so each lock is only taken once. */
>> +	sort(entries + m, n - m, sizeof(entries[0]), swp_entry_cmp, NULL);
>> +	prev = NULL;
>
> Can eliminate prev=NULL if we adopt the above change.
>
>> +	for (i = m; i < n; i++) {
>> +		entry = entries[i];
>> +		if (swp_type(entry) == prev_swp_type)
>> +			continue;
>
> The if/continue statement seems incorrect. When swp_type(entry) == prev_swp_type
> we also need to free entry.  The if/continue statement should be deleted.
>
> Say we have 3 entries with swp_type
> 1,2,1
>
> We will get prev_swp_type as 1 and free the first entry
> and sort the remaining two.  The last entry with
> swp_type 1 will not be freed.

The first loop in the function will scan all elements of the array, so
the first and third entry will be freed in the first loop.  Then the the
second and the third entry will be sorted.  But all entries with the
same swap type (device) of the first entry needn't to be freed again.
The key point is that we will scan all elements of the array in the
first loop, record the first entry that has different swap type
(device).

Best Regards,
Huang, Ying

>> +		p = swap_info_get_cont(entry, prev);
>> +		if (likely(p))
>> +			swap_entry_free(p, entry);
>>  		prev = p;
>>  	}
>>  	if (p)
>
> Thanks.
>
> Tim