>
> From 7bd903c42749c448ef6acbbdee8dcbc1c5b498b9 Mon Sep 17 00:00:00 2001
> From: Huang Ying <ying.hu...@intel.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.
>
> 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.
> + p = swap_info_get_cont(entry, prev);
> + if (likely(p))
> + swap_entry_free(p, entry);
> prev = p;
> }
> if (p)
Thanks.
Tim
