From: "Huang\, Ying" <ying.huang@intel.com> To: Tim Chen <tim.c.chen@linux.intel.com> Cc: "Huang\, Ying" <ying.huang@intel.com>, Minchan Kim <minchan@kernel.org>, Andrew Morton <akpm@linux-foundation.org>, <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>, Hugh Dickins <hughd@google.com>, Shaohua Li <shli@kernel.org>, Rik van Riel <riel@redhat.com> Subject: Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free Date: Thu, 27 Apr 2017 09:21:51 +0800 [thread overview] Message-ID: <87efwe3as0.fsf@yhuang-dev.intel.com> (raw) In-Reply-To: <1493237623.3209.142.camel@linux.intel.com> (Tim Chen's message of "Wed, 26 Apr 2017 13:13:43 -0700") Tim Chen <tim.c.chen@linux.intel.com> writes: >> >> From 7bd903c42749c448ef6acbbdee8dcbc1c5b498b9 Mon Sep 17 00:00:00 2001 >> From: Huang Ying <ying.huang@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. 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
WARNING: multiple messages have this Message-ID (diff)
From: "Huang\, Ying" <ying.huang@intel.com> To: Tim Chen <tim.c.chen@linux.intel.com> Cc: "Huang, Ying" <ying.huang@intel.com>, Minchan Kim <minchan@kernel.org>, Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org, linux-kernel@vger.kernel.org, Hugh Dickins <hughd@google.com>, Shaohua Li <shli@kernel.org>, Rik van Riel <riel@redhat.com> Subject: Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free Date: Thu, 27 Apr 2017 09:21:51 +0800 [thread overview] Message-ID: <87efwe3as0.fsf@yhuang-dev.intel.com> (raw) In-Reply-To: <1493237623.3209.142.camel@linux.intel.com> (Tim Chen's message of "Wed, 26 Apr 2017 13:13:43 -0700") Tim Chen <tim.c.chen@linux.intel.com> writes: >> >> From 7bd903c42749c448ef6acbbdee8dcbc1c5b498b9 Mon Sep 17 00:00:00 2001 >> From: Huang Ying <ying.huang@intel.com> >> Date: Thu, 23 Feb 2017 13:05:20 +0800 >> Subject: [PATCH -v5] mm, swap: Sort swap entries before free >> >>A >> --- >> A mm/swapfile.c | 43 ++++++++++++++++++++++++++++++++++++++----- >> A 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 @@ >> A #include <linux/swapfile.h> >> A #include <linux/export.h> >> A #include <linux/swap_slots.h> >> +#include <linux/sort.h> >> A >> A #include <asm/pgtable.h> >> A #include <asm/tlbflush.h> >> @@ -1065,20 +1066,52 @@ void swapcache_free(swp_entry_t entry) >> A } >> A } >> A >> +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)); >> +} >> + >> A void swapcache_free_entries(swp_entry_t *entries, int n) >> A { >> A 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. >> A >> A if (n <= 0) >> A return; >> A >> A prev = NULL; >> A 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. A 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. A 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); >> A prev = p; >> A } >> A if (p) > > Thanks. > > Tim -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2017-04-27 1:22 UTC|newest] Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-04-07 6:49 [PATCH -mm -v3] mm, swap: Sort swap entries before free Huang, Ying 2017-04-07 6:49 ` Huang, Ying 2017-04-07 13:05 ` Rik van Riel 2017-04-07 13:05 ` Rik van Riel 2017-04-07 21:43 ` Andrew Morton 2017-04-07 21:43 ` Andrew Morton 2017-04-11 7:03 ` Huang, Ying 2017-04-11 7:03 ` Huang, Ying 2017-04-14 1:36 ` Huang, Ying 2017-04-14 1:36 ` Huang, Ying 2017-04-14 1:41 ` Huang, Ying 2017-04-18 4:59 ` Minchan Kim 2017-04-18 4:59 ` Minchan Kim 2017-04-19 8:14 ` Huang, Ying 2017-04-19 8:14 ` Huang, Ying 2017-04-20 6:38 ` Minchan Kim 2017-04-20 6:38 ` Minchan Kim 2017-04-20 7:15 ` Huang, Ying 2017-04-20 7:15 ` Huang, Ying 2017-04-21 12:29 ` Huang, Ying 2017-04-21 12:29 ` Huang, Ying 2017-04-21 23:29 ` Tim Chen 2017-04-21 23:29 ` Tim Chen 2017-04-23 13:16 ` Huang, Ying 2017-04-23 13:16 ` Huang, Ying 2017-04-24 16:03 ` Tim Chen 2017-04-24 16:03 ` Tim Chen 2017-04-24 4:52 ` Minchan Kim 2017-04-24 4:52 ` Minchan Kim 2017-04-24 6:47 ` Huang, Ying 2017-04-24 6:47 ` Huang, Ying 2017-04-26 12:42 ` Huang, Ying 2017-04-26 12:42 ` Huang, Ying 2017-04-26 20:13 ` Tim Chen 2017-04-26 20:13 ` Tim Chen 2017-04-27 1:21 ` Huang, Ying [this message] 2017-04-27 1:21 ` Huang, Ying 2017-04-27 16:48 ` Tim Chen 2017-04-27 16:48 ` Tim Chen 2017-04-27 4:35 ` Minchan Kim 2017-04-27 4:35 ` Minchan Kim 2017-04-28 1:09 ` Huang, Ying 2017-04-28 1:09 ` Huang, Ying 2017-04-28 7:42 ` Minchan Kim 2017-04-28 7:42 ` Minchan Kim 2017-04-28 8:05 ` Huang, Ying 2017-04-28 8:05 ` Huang, Ying 2017-04-28 9:00 ` Minchan Kim 2017-04-28 9:00 ` Minchan Kim 2017-04-28 11:48 ` Huang, Ying 2017-04-28 11:48 ` Huang, Ying 2017-04-28 13:35 ` Huang, Ying 2017-04-28 13:35 ` Huang, Ying 2017-05-02 5:02 ` Minchan Kim 2017-05-02 5:02 ` Minchan Kim 2017-05-02 5:35 ` Huang, Ying 2017-05-02 5:35 ` Huang, Ying 2017-05-02 5:48 ` Minchan Kim 2017-05-02 5:48 ` Minchan Kim 2017-05-02 6:08 ` Huang, Ying 2017-05-02 6:08 ` Huang, Ying
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87efwe3as0.fsf@yhuang-dev.intel.com \ --to=ying.huang@intel.com \ --cc=akpm@linux-foundation.org \ --cc=hughd@google.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=minchan@kernel.org \ --cc=riel@redhat.com \ --cc=shli@kernel.org \ --cc=tim.c.chen@linux.intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.