On Thu, 2017-04-27 at 09:21 +0800, Huang, Ying wrote:
> Tim Chen <tim.c.chen@xxxxxxxxxxxxxxx> writes:
>
> >
> > >
> > >
> > > From 7bd903c42749c448ef6acbbdee8dcbc1c5b498b9 Mon Sep 17 00:00:00 2001
> > > From: Huang Ying <ying.huang@xxxxxxxxx>
> > > 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).
I was under the wrong impression that the code break from the first
loop when it finds a different swp type. Yes, we should skip the
free in the second loop if the first loop scan the whole list.
Thanks.
Tim
>
> 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
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
