* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
@ 2017-04-24 4:52 ` Minchan Kim
0 siblings, 0 replies; 61+ messages in thread
From: Minchan Kim @ 2017-04-24 4:52 UTC (permalink / raw)
To: Huang, Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Hugh Dickins, Shaohua Li,
Rik van Riel
On Fri, Apr 21, 2017 at 08:29:30PM +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). I think this is the
> worse case because there is no lock contention. The memory freeing time
> increased from 1.94s to 2.12s (increase ~9.2%). So there is some
> overhead for some cases. I change the algorithm to something like
> below,
>
> void swapcache_free_entries(swp_entry_t *entries, int n)
> {
> struct swap_info_struct *p, *prev;
> int i;
> + swp_entry_t entry;
> + unsigned int prev_swp_type;
>
> if (n <= 0)
> return;
>
> + prev_swp_type = swp_type(entries[0]);
> + for (i = n - 1; i > 0; i--) {
> + if (swp_type(entries[i]) != prev_swp_type)
> + break;
> + }
That's really what I want to avoid. For many swap usecases,
it adds unnecessary overhead.
> +
> + /* Sort swap entries by swap device, so each lock is only taken once. */
> + if (i)
> + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
> prev = NULL;
> p = NULL;
> for (i = 0; i < n; ++i) {
> - p = swap_info_get_cont(entries[i], prev);
> + entry = entries[i];
> + p = swap_info_get_cont(entry, prev);
> if (p)
> - swap_entry_free(p, entries[i]);
> + swap_entry_free(p, entry);
> prev = p;
> }
> if (p)
>
> With this patch, the memory freeing time increased from 1.94s to 1.97s.
> I think this is good enough. Do you think so?
What I mean is as follows(I didn't test it at all):
With this, sort entries if we found multiple entries in current
entries. It adds some condition checks for non-multiple swap
usecase but it would be more cheaper than the sorting.
And it adds a [un]lock overhead for multiple swap usecase but
it should be a compromise for single-swap usecase which is more
popular.
diff --git a/mm/swapfile.c b/mm/swapfile.c
index f23c56e9be39..0d76a492786f 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1073,30 +1073,40 @@ static int swp_entry_cmp(const void *ent1, const void *ent2)
return (long)(swp_type(*e1) - swp_type(*e2));
}
-void swapcache_free_entries(swp_entry_t *entries, int n)
+void swapcache_free_entries(swp_entry_t *entries, int nr)
{
- struct swap_info_struct *p, *prev;
int i;
+ struct swap_info_struct *cur, *prev = NULL;
+ bool sorted = false;
- if (n <= 0)
+ if (nr <= 0)
return;
- 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);
- for (i = 0; i < n; ++i) {
- p = swap_info_get_cont(entries[i], prev);
- if (p)
- swap_entry_free(p, entries[i]);
- else
+ for (i = 0; i < nr; i++) {
+ cur = swap_info_get_cont(entries[i], prev);
+ if (!cur)
break;
- prev = p;
+ if (cur != prev && !sorted && prev) {
+ spin_unlock(&cur->lock);
+ /*
+ * Sort swap entries by swap device,
+ * so each lock is only taken once.
+ */
+ sort(entries + i, nr - i,
+ sizeof(swp_entry_t),
+ swp_entry_cmp, NULL);
+ sorted = true;
+ prev = NULL;
+ i--;
+ continue;
+ }
+
+ swap_entry_free(cur, entries[i]);
+ prev = cur;
}
- if (p)
- spin_unlock(&p->lock);
+
+ if (cur)
+ spin_unlock(&cur->lock);
}
/*
--
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>
^ permalink raw reply related [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
2017-04-24 4:52 ` Minchan Kim
@ 2017-04-24 6:47 ` Huang, Ying
-1 siblings, 0 replies; 61+ messages in thread
From: Huang, Ying @ 2017-04-24 6:47 UTC (permalink / raw)
To: Minchan Kim
Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
Shaohua Li, Rik van Riel
Minchan Kim <minchan@kernel.org> writes:
> On Fri, Apr 21, 2017 at 08:29:30PM +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). I think this is the
>> worse case because there is no lock contention. The memory freeing time
>> increased from 1.94s to 2.12s (increase ~9.2%). So there is some
>> overhead for some cases. I change the algorithm to something like
>> below,
>>
>> void swapcache_free_entries(swp_entry_t *entries, int n)
>> {
>> struct swap_info_struct *p, *prev;
>> int i;
>> + swp_entry_t entry;
>> + unsigned int prev_swp_type;
>>
>> if (n <= 0)
>> return;
>>
>> + prev_swp_type = swp_type(entries[0]);
>> + for (i = n - 1; i > 0; i--) {
>> + if (swp_type(entries[i]) != prev_swp_type)
>> + break;
>> + }
>
> That's really what I want to avoid. For many swap usecases,
> it adds unnecessary overhead.
>
>> +
>> + /* Sort swap entries by swap device, so each lock is only taken once. */
>> + if (i)
>> + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
>> prev = NULL;
>> p = NULL;
>> for (i = 0; i < n; ++i) {
>> - p = swap_info_get_cont(entries[i], prev);
>> + entry = entries[i];
>> + p = swap_info_get_cont(entry, prev);
>> if (p)
>> - swap_entry_free(p, entries[i]);
>> + swap_entry_free(p, entry);
>> prev = p;
>> }
>> if (p)
>>
>> With this patch, the memory freeing time increased from 1.94s to 1.97s.
>> I think this is good enough. Do you think so?
>
> What I mean is as follows(I didn't test it at all):
>
> With this, sort entries if we found multiple entries in current
> entries. It adds some condition checks for non-multiple swap
> usecase but it would be more cheaper than the sorting.
> And it adds a [un]lock overhead for multiple swap usecase but
> it should be a compromise for single-swap usecase which is more
> popular.
Yes. What I concerned is that one swap device may be locked twice
instead of once during the freeing. I will give it some test.
Best Regards,
Huang, Ying
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index f23c56e9be39..0d76a492786f 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1073,30 +1073,40 @@ static int swp_entry_cmp(const void *ent1, const void *ent2)
> return (long)(swp_type(*e1) - swp_type(*e2));
> }
>
> -void swapcache_free_entries(swp_entry_t *entries, int n)
> +void swapcache_free_entries(swp_entry_t *entries, int nr)
> {
> - struct swap_info_struct *p, *prev;
> int i;
> + struct swap_info_struct *cur, *prev = NULL;
> + bool sorted = false;
>
> - if (n <= 0)
> + if (nr <= 0)
> return;
>
> - 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);
> - for (i = 0; i < n; ++i) {
> - p = swap_info_get_cont(entries[i], prev);
> - if (p)
> - swap_entry_free(p, entries[i]);
> - else
> + for (i = 0; i < nr; i++) {
> + cur = swap_info_get_cont(entries[i], prev);
> + if (!cur)
> break;
> - prev = p;
> + if (cur != prev && !sorted && prev) {
> + spin_unlock(&cur->lock);
> + /*
> + * Sort swap entries by swap device,
> + * so each lock is only taken once.
> + */
> + sort(entries + i, nr - i,
> + sizeof(swp_entry_t),
> + swp_entry_cmp, NULL);
> + sorted = true;
> + prev = NULL;
> + i--;
> + continue;
> + }
> +
> + swap_entry_free(cur, entries[i]);
> + prev = cur;
> }
> - if (p)
> - spin_unlock(&p->lock);
> +
> + if (cur)
> + spin_unlock(&cur->lock);
> }
>
> /*
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
@ 2017-04-24 6:47 ` Huang, Ying
0 siblings, 0 replies; 61+ messages in thread
From: Huang, Ying @ 2017-04-24 6:47 UTC (permalink / raw)
To: Minchan Kim
Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
Shaohua Li, Rik van Riel
Minchan Kim <minchan@kernel.org> writes:
> On Fri, Apr 21, 2017 at 08:29:30PM +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). I think this is the
>> worse case because there is no lock contention. The memory freeing time
>> increased from 1.94s to 2.12s (increase ~9.2%). So there is some
>> overhead for some cases. I change the algorithm to something like
>> below,
>>
>> void swapcache_free_entries(swp_entry_t *entries, int n)
>> {
>> struct swap_info_struct *p, *prev;
>> int i;
>> + swp_entry_t entry;
>> + unsigned int prev_swp_type;
>>
>> if (n <= 0)
>> return;
>>
>> + prev_swp_type = swp_type(entries[0]);
>> + for (i = n - 1; i > 0; i--) {
>> + if (swp_type(entries[i]) != prev_swp_type)
>> + break;
>> + }
>
> That's really what I want to avoid. For many swap usecases,
> it adds unnecessary overhead.
>
>> +
>> + /* Sort swap entries by swap device, so each lock is only taken once. */
>> + if (i)
>> + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
>> prev = NULL;
>> p = NULL;
>> for (i = 0; i < n; ++i) {
>> - p = swap_info_get_cont(entries[i], prev);
>> + entry = entries[i];
>> + p = swap_info_get_cont(entry, prev);
>> if (p)
>> - swap_entry_free(p, entries[i]);
>> + swap_entry_free(p, entry);
>> prev = p;
>> }
>> if (p)
>>
>> With this patch, the memory freeing time increased from 1.94s to 1.97s.
>> I think this is good enough. Do you think so?
>
> What I mean is as follows(I didn't test it at all):
>
> With this, sort entries if we found multiple entries in current
> entries. It adds some condition checks for non-multiple swap
> usecase but it would be more cheaper than the sorting.
> And it adds a [un]lock overhead for multiple swap usecase but
> it should be a compromise for single-swap usecase which is more
> popular.
Yes. What I concerned is that one swap device may be locked twice
instead of once during the freeing. I will give it some test.
Best Regards,
Huang, Ying
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index f23c56e9be39..0d76a492786f 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1073,30 +1073,40 @@ static int swp_entry_cmp(const void *ent1, const void *ent2)
> return (long)(swp_type(*e1) - swp_type(*e2));
> }
>
> -void swapcache_free_entries(swp_entry_t *entries, int n)
> +void swapcache_free_entries(swp_entry_t *entries, int nr)
> {
> - struct swap_info_struct *p, *prev;
> int i;
> + struct swap_info_struct *cur, *prev = NULL;
> + bool sorted = false;
>
> - if (n <= 0)
> + if (nr <= 0)
> return;
>
> - 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);
> - for (i = 0; i < n; ++i) {
> - p = swap_info_get_cont(entries[i], prev);
> - if (p)
> - swap_entry_free(p, entries[i]);
> - else
> + for (i = 0; i < nr; i++) {
> + cur = swap_info_get_cont(entries[i], prev);
> + if (!cur)
> break;
> - prev = p;
> + if (cur != prev && !sorted && prev) {
> + spin_unlock(&cur->lock);
> + /*
> + * Sort swap entries by swap device,
> + * so each lock is only taken once.
> + */
> + sort(entries + i, nr - i,
> + sizeof(swp_entry_t),
> + swp_entry_cmp, NULL);
> + sorted = true;
> + prev = NULL;
> + i--;
> + continue;
> + }
> +
> + swap_entry_free(cur, entries[i]);
> + prev = cur;
> }
> - if (p)
> - spin_unlock(&p->lock);
> +
> + if (cur)
> + spin_unlock(&cur->lock);
> }
>
> /*
--
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>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
2017-04-24 4:52 ` Minchan Kim
@ 2017-04-26 12:42 ` Huang, Ying
-1 siblings, 0 replies; 61+ messages in thread
From: Huang, Ying @ 2017-04-26 12:42 UTC (permalink / raw)
To: Minchan Kim
Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
Shaohua Li, Rik van Riel
Minchan Kim <minchan@kernel.org> writes:
> On Fri, Apr 21, 2017 at 08:29:30PM +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). I think this is the
>> worse case because there is no lock contention. The memory freeing time
>> increased from 1.94s to 2.12s (increase ~9.2%). So there is some
>> overhead for some cases. I change the algorithm to something like
>> below,
>>
>> void swapcache_free_entries(swp_entry_t *entries, int n)
>> {
>> struct swap_info_struct *p, *prev;
>> int i;
>> + swp_entry_t entry;
>> + unsigned int prev_swp_type;
>>
>> if (n <= 0)
>> return;
>>
>> + prev_swp_type = swp_type(entries[0]);
>> + for (i = n - 1; i > 0; i--) {
>> + if (swp_type(entries[i]) != prev_swp_type)
>> + break;
>> + }
>
> That's really what I want to avoid. For many swap usecases,
> it adds unnecessary overhead.
>
>> +
>> + /* Sort swap entries by swap device, so each lock is only taken once. */
>> + if (i)
>> + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
>> prev = NULL;
>> p = NULL;
>> for (i = 0; i < n; ++i) {
>> - p = swap_info_get_cont(entries[i], prev);
>> + entry = entries[i];
>> + p = swap_info_get_cont(entry, prev);
>> if (p)
>> - swap_entry_free(p, entries[i]);
>> + swap_entry_free(p, entry);
>> prev = p;
>> }
>> if (p)
>>
>> With this patch, the memory freeing time increased from 1.94s to 1.97s.
>> I think this is good enough. Do you think so?
>
> What I mean is as follows(I didn't test it at all):
>
> With this, sort entries if we found multiple entries in current
> entries. It adds some condition checks for non-multiple swap
> usecase but it would be more cheaper than the sorting.
> And it adds a [un]lock overhead for multiple swap usecase but
> it should be a compromise for single-swap usecase which is more
> popular.
>
How about the following solution? It can avoid [un]lock overhead and
double lock issue for multiple swap user case and has good performance
for one swap user case too.
Best Regards,
Huang, Ying
>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
To reduce the lock contention of swap_info_struct->lock when freeing
swap entry. The freed swap entries will be collected in a per-CPU
buffer firstly, and be really freed later in batch. During the batch
freeing, if the consecutive swap entries in the per-CPU buffer belongs
to same swap device, the swap_info_struct->lock needs to be
acquired/released only once, so that the lock contention could be
reduced greatly. But if there are multiple swap devices, it is
possible that the lock may be unnecessarily released/acquired because
the swap entries belong to the same swap device are non-consecutive in
the per-CPU buffer.
To solve the issue, the per-CPU buffer is sorted according to the swap
device before freeing the swap entries. Test shows that the time
spent by swapcache_free_entries() could be reduced after the patch.
With the patch, the memory (some swapped out) free time reduced
13.6% (from 2.59s to 2.28s) in the vm-scalability swap-w-rand test
case with 16 processes. The test is done on a Xeon E5 v3 system. The
swap device used is a RAM simulated PMEM (persistent memory) device.
To test swapping, the test case creates 16 processes, which allocate
and write to the anonymous pages until the RAM and part of the swap
device is used up, finally the memory (some swapped out) is freed
before exit.
Signed-off-by: Huang Ying <ying.huang@intel.com>
Acked-by: Tim Chen <tim.c.chen@intel.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Shaohua Li <shli@kernel.org>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Rik van Riel <riel@redhat.com>
v5:
- Use a smarter way to determine whether sort is necessary.
v4:
- Avoid unnecessary sort if all entries are from one swap device.
v3:
- Add some comments in code per Rik's suggestion.
v2:
- Avoid sort swap entries if there is only one swap device.
---
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;
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;
+
+ /* 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;
+ for (i = m; i < n; i++) {
+ entry = entries[i];
+ if (swp_type(entry) == prev_swp_type)
+ continue;
+ p = swap_info_get_cont(entry, prev);
+ if (likely(p))
+ swap_entry_free(p, entry);
prev = p;
}
if (p)
--
2.11.0
^ permalink raw reply related [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
@ 2017-04-26 12:42 ` Huang, Ying
0 siblings, 0 replies; 61+ messages in thread
From: Huang, Ying @ 2017-04-26 12:42 UTC (permalink / raw)
To: Minchan Kim
Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
Shaohua Li, Rik van Riel
Minchan Kim <minchan@kernel.org> writes:
> On Fri, Apr 21, 2017 at 08:29:30PM +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). I think this is the
>> worse case because there is no lock contention. The memory freeing time
>> increased from 1.94s to 2.12s (increase ~9.2%). So there is some
>> overhead for some cases. I change the algorithm to something like
>> below,
>>
>> void swapcache_free_entries(swp_entry_t *entries, int n)
>> {
>> struct swap_info_struct *p, *prev;
>> int i;
>> + swp_entry_t entry;
>> + unsigned int prev_swp_type;
>>
>> if (n <= 0)
>> return;
>>
>> + prev_swp_type = swp_type(entries[0]);
>> + for (i = n - 1; i > 0; i--) {
>> + if (swp_type(entries[i]) != prev_swp_type)
>> + break;
>> + }
>
> That's really what I want to avoid. For many swap usecases,
> it adds unnecessary overhead.
>
>> +
>> + /* Sort swap entries by swap device, so each lock is only taken once. */
>> + if (i)
>> + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
>> prev = NULL;
>> p = NULL;
>> for (i = 0; i < n; ++i) {
>> - p = swap_info_get_cont(entries[i], prev);
>> + entry = entries[i];
>> + p = swap_info_get_cont(entry, prev);
>> if (p)
>> - swap_entry_free(p, entries[i]);
>> + swap_entry_free(p, entry);
>> prev = p;
>> }
>> if (p)
>>
>> With this patch, the memory freeing time increased from 1.94s to 1.97s.
>> I think this is good enough. Do you think so?
>
> What I mean is as follows(I didn't test it at all):
>
> With this, sort entries if we found multiple entries in current
> entries. It adds some condition checks for non-multiple swap
> usecase but it would be more cheaper than the sorting.
> And it adds a [un]lock overhead for multiple swap usecase but
> it should be a compromise for single-swap usecase which is more
> popular.
>
How about the following solution? It can avoid [un]lock overhead and
double lock issue for multiple swap user case and has good performance
for one swap user case too.
Best Regards,
Huang, Ying
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
2017-04-26 12:42 ` Huang, Ying
@ 2017-04-26 20:13 ` Tim Chen
-1 siblings, 0 replies; 61+ messages in thread
From: Tim Chen @ 2017-04-26 20:13 UTC (permalink / raw)
To: Huang, Ying, Minchan Kim
Cc: Andrew Morton, linux-mm, linux-kernel, Hugh Dickins, Shaohua Li,
Rik van Riel
>
> 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.
>
> 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
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
@ 2017-04-26 20:13 ` Tim Chen
0 siblings, 0 replies; 61+ messages in thread
From: Tim Chen @ 2017-04-26 20:13 UTC (permalink / raw)
To: Huang, Ying, Minchan Kim
Cc: Andrew Morton, linux-mm, linux-kernel, Hugh Dickins, Shaohua Li,
Rik van Riel
>
> 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.
> 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.
> + 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>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
2017-04-26 20:13 ` Tim Chen
@ 2017-04-27 1:21 ` Huang, Ying
-1 siblings, 0 replies; 61+ messages in thread
From: Huang, Ying @ 2017-04-27 1:21 UTC (permalink / raw)
To: Tim Chen
Cc: Huang, Ying, Minchan Kim, Andrew Morton, linux-mm, linux-kernel,
Hugh Dickins, Shaohua Li, Rik van Riel
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
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
@ 2017-04-27 1:21 ` Huang, Ying
0 siblings, 0 replies; 61+ messages in thread
From: Huang, Ying @ 2017-04-27 1:21 UTC (permalink / raw)
To: Tim Chen
Cc: Huang, Ying, Minchan Kim, Andrew Morton, linux-mm, linux-kernel,
Hugh Dickins, Shaohua Li, Rik van Riel
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>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
2017-04-27 1:21 ` Huang, Ying
@ 2017-04-27 16:48 ` Tim Chen
-1 siblings, 0 replies; 61+ messages in thread
From: Tim Chen @ 2017-04-27 16:48 UTC (permalink / raw)
To: Huang, Ying
Cc: Minchan Kim, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
Shaohua Li, Rik van Riel
On Thu, 2017-04-27 at 09:21 +0800, Huang, Ying wrote:
> 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).
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
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
@ 2017-04-27 16:48 ` Tim Chen
0 siblings, 0 replies; 61+ messages in thread
From: Tim Chen @ 2017-04-27 16:48 UTC (permalink / raw)
To: Huang, Ying
Cc: Minchan Kim, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
Shaohua Li, Rik van Riel
On Thu, 2017-04-27 at 09:21 +0800, Huang, Ying wrote:
> 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.A A That is better!A A 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.A A Then the the
> second and the third entry will be sorted.A A 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. A 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);
> > > 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>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
2017-04-26 12:42 ` Huang, Ying
@ 2017-04-27 4:35 ` Minchan Kim
-1 siblings, 0 replies; 61+ messages in thread
From: Minchan Kim @ 2017-04-27 4:35 UTC (permalink / raw)
To: Huang, Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Hugh Dickins, Shaohua Li,
Rik van Riel
On Wed, Apr 26, 2017 at 08:42:10PM +0800, Huang, Ying wrote:
> Minchan Kim <minchan@kernel.org> writes:
>
> > On Fri, Apr 21, 2017 at 08:29:30PM +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). I think this is the
> >> worse case because there is no lock contention. The memory freeing time
> >> increased from 1.94s to 2.12s (increase ~9.2%). So there is some
> >> overhead for some cases. I change the algorithm to something like
> >> below,
> >>
> >> void swapcache_free_entries(swp_entry_t *entries, int n)
> >> {
> >> struct swap_info_struct *p, *prev;
> >> int i;
> >> + swp_entry_t entry;
> >> + unsigned int prev_swp_type;
> >>
> >> if (n <= 0)
> >> return;
> >>
> >> + prev_swp_type = swp_type(entries[0]);
> >> + for (i = n - 1; i > 0; i--) {
> >> + if (swp_type(entries[i]) != prev_swp_type)
> >> + break;
> >> + }
> >
> > That's really what I want to avoid. For many swap usecases,
> > it adds unnecessary overhead.
> >
> >> +
> >> + /* Sort swap entries by swap device, so each lock is only taken once. */
> >> + if (i)
> >> + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
> >> prev = NULL;
> >> p = NULL;
> >> for (i = 0; i < n; ++i) {
> >> - p = swap_info_get_cont(entries[i], prev);
> >> + entry = entries[i];
> >> + p = swap_info_get_cont(entry, prev);
> >> if (p)
> >> - swap_entry_free(p, entries[i]);
> >> + swap_entry_free(p, entry);
> >> prev = p;
> >> }
> >> if (p)
> >>
> >> With this patch, the memory freeing time increased from 1.94s to 1.97s.
> >> I think this is good enough. Do you think so?
> >
> > What I mean is as follows(I didn't test it at all):
> >
> > With this, sort entries if we found multiple entries in current
> > entries. It adds some condition checks for non-multiple swap
> > usecase but it would be more cheaper than the sorting.
> > And it adds a [un]lock overhead for multiple swap usecase but
> > it should be a compromise for single-swap usecase which is more
> > popular.
> >
>
> How about the following solution? It can avoid [un]lock overhead and
> double lock issue for multiple swap user case and has good performance
> for one swap user case too.
How worse with approach I suggested compared to as-is?
Unless it's too bad, let's not add more complicated thing to just
enhance the minor usecase in such even *slow* path.
It adds code size/maintainance overead.
With your suggestion, it might enhance a bit with speicific benchmark
but not sure it's really worth for real practice.
>
> Best Regards,
> Huang, Ying
>
> 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
>
> To reduce the lock contention of swap_info_struct->lock when freeing
> swap entry. The freed swap entries will be collected in a per-CPU
> buffer firstly, and be really freed later in batch. During the batch
> freeing, if the consecutive swap entries in the per-CPU buffer belongs
> to same swap device, the swap_info_struct->lock needs to be
> acquired/released only once, so that the lock contention could be
> reduced greatly. But if there are multiple swap devices, it is
> possible that the lock may be unnecessarily released/acquired because
> the swap entries belong to the same swap device are non-consecutive in
> the per-CPU buffer.
>
> To solve the issue, the per-CPU buffer is sorted according to the swap
> device before freeing the swap entries. Test shows that the time
> spent by swapcache_free_entries() could be reduced after the patch.
>
> With the patch, the memory (some swapped out) free time reduced
> 13.6% (from 2.59s to 2.28s) in the vm-scalability swap-w-rand test
> case with 16 processes. The test is done on a Xeon E5 v3 system. The
> swap device used is a RAM simulated PMEM (persistent memory) device.
> To test swapping, the test case creates 16 processes, which allocate
> and write to the anonymous pages until the RAM and part of the swap
> device is used up, finally the memory (some swapped out) is freed
> before exit.
>
> Signed-off-by: Huang Ying <ying.huang@intel.com>
> Acked-by: Tim Chen <tim.c.chen@intel.com>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Shaohua Li <shli@kernel.org>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Rik van Riel <riel@redhat.com>
>
> v5:
>
> - Use a smarter way to determine whether sort is necessary.
>
> v4:
>
> - Avoid unnecessary sort if all entries are from one swap device.
>
> v3:
>
> - Add some comments in code per Rik's suggestion.
>
> v2:
>
> - Avoid sort swap entries if there is only one swap device.
> ---
> 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;
>
> 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;
> +
> + /* 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;
> + for (i = m; i < n; i++) {
> + entry = entries[i];
> + if (swp_type(entry) == prev_swp_type)
> + continue;
> + p = swap_info_get_cont(entry, prev);
> + if (likely(p))
> + swap_entry_free(p, entry);
> prev = p;
> }
> if (p)
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
@ 2017-04-27 4:35 ` Minchan Kim
0 siblings, 0 replies; 61+ messages in thread
From: Minchan Kim @ 2017-04-27 4:35 UTC (permalink / raw)
To: Huang, Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Hugh Dickins, Shaohua Li,
Rik van Riel
On Wed, Apr 26, 2017 at 08:42:10PM +0800, Huang, Ying wrote:
> Minchan Kim <minchan@kernel.org> writes:
>
> > On Fri, Apr 21, 2017 at 08:29:30PM +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). I think this is the
> >> worse case because there is no lock contention. The memory freeing time
> >> increased from 1.94s to 2.12s (increase ~9.2%). So there is some
> >> overhead for some cases. I change the algorithm to something like
> >> below,
> >>
> >> void swapcache_free_entries(swp_entry_t *entries, int n)
> >> {
> >> struct swap_info_struct *p, *prev;
> >> int i;
> >> + swp_entry_t entry;
> >> + unsigned int prev_swp_type;
> >>
> >> if (n <= 0)
> >> return;
> >>
> >> + prev_swp_type = swp_type(entries[0]);
> >> + for (i = n - 1; i > 0; i--) {
> >> + if (swp_type(entries[i]) != prev_swp_type)
> >> + break;
> >> + }
> >
> > That's really what I want to avoid. For many swap usecases,
> > it adds unnecessary overhead.
> >
> >> +
> >> + /* Sort swap entries by swap device, so each lock is only taken once. */
> >> + if (i)
> >> + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
> >> prev = NULL;
> >> p = NULL;
> >> for (i = 0; i < n; ++i) {
> >> - p = swap_info_get_cont(entries[i], prev);
> >> + entry = entries[i];
> >> + p = swap_info_get_cont(entry, prev);
> >> if (p)
> >> - swap_entry_free(p, entries[i]);
> >> + swap_entry_free(p, entry);
> >> prev = p;
> >> }
> >> if (p)
> >>
> >> With this patch, the memory freeing time increased from 1.94s to 1.97s.
> >> I think this is good enough. Do you think so?
> >
> > What I mean is as follows(I didn't test it at all):
> >
> > With this, sort entries if we found multiple entries in current
> > entries. It adds some condition checks for non-multiple swap
> > usecase but it would be more cheaper than the sorting.
> > And it adds a [un]lock overhead for multiple swap usecase but
> > it should be a compromise for single-swap usecase which is more
> > popular.
> >
>
> How about the following solution? It can avoid [un]lock overhead and
> double lock issue for multiple swap user case and has good performance
> for one swap user case too.
How worse with approach I suggested compared to as-is?
Unless it's too bad, let's not add more complicated thing to just
enhance the minor usecase in such even *slow* path.
It adds code size/maintainance overead.
With your suggestion, it might enhance a bit with speicific benchmark
but not sure it's really worth for real practice.
>
> Best Regards,
> Huang, Ying
>
> 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
>
> To reduce the lock contention of swap_info_struct->lock when freeing
> swap entry. The freed swap entries will be collected in a per-CPU
> buffer firstly, and be really freed later in batch. During the batch
> freeing, if the consecutive swap entries in the per-CPU buffer belongs
> to same swap device, the swap_info_struct->lock needs to be
> acquired/released only once, so that the lock contention could be
> reduced greatly. But if there are multiple swap devices, it is
> possible that the lock may be unnecessarily released/acquired because
> the swap entries belong to the same swap device are non-consecutive in
> the per-CPU buffer.
>
> To solve the issue, the per-CPU buffer is sorted according to the swap
> device before freeing the swap entries. Test shows that the time
> spent by swapcache_free_entries() could be reduced after the patch.
>
> With the patch, the memory (some swapped out) free time reduced
> 13.6% (from 2.59s to 2.28s) in the vm-scalability swap-w-rand test
> case with 16 processes. The test is done on a Xeon E5 v3 system. The
> swap device used is a RAM simulated PMEM (persistent memory) device.
> To test swapping, the test case creates 16 processes, which allocate
> and write to the anonymous pages until the RAM and part of the swap
> device is used up, finally the memory (some swapped out) is freed
> before exit.
>
> Signed-off-by: Huang Ying <ying.huang@intel.com>
> Acked-by: Tim Chen <tim.c.chen@intel.com>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Shaohua Li <shli@kernel.org>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Rik van Riel <riel@redhat.com>
>
> v5:
>
> - Use a smarter way to determine whether sort is necessary.
>
> v4:
>
> - Avoid unnecessary sort if all entries are from one swap device.
>
> v3:
>
> - Add some comments in code per Rik's suggestion.
>
> v2:
>
> - Avoid sort swap entries if there is only one swap device.
> ---
> 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;
>
> 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;
> +
> + /* 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;
> + for (i = m; i < n; i++) {
> + entry = entries[i];
> + if (swp_type(entry) == prev_swp_type)
> + continue;
> + p = swap_info_get_cont(entry, prev);
> + if (likely(p))
> + swap_entry_free(p, entry);
> prev = p;
> }
> if (p)
> --
> 2.11.0
>
--
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>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
2017-04-27 4:35 ` Minchan Kim
@ 2017-04-28 1:09 ` Huang, Ying
-1 siblings, 0 replies; 61+ messages in thread
From: Huang, Ying @ 2017-04-28 1:09 UTC (permalink / raw)
To: Minchan Kim
Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
Shaohua Li, Rik van Riel
Minchan Kim <minchan@kernel.org> writes:
> On Wed, Apr 26, 2017 at 08:42:10PM +0800, Huang, Ying wrote:
>> Minchan Kim <minchan@kernel.org> writes:
>>
>> > On Fri, Apr 21, 2017 at 08:29:30PM +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). I think this is the
>> >> worse case because there is no lock contention. The memory freeing time
>> >> increased from 1.94s to 2.12s (increase ~9.2%). So there is some
>> >> overhead for some cases. I change the algorithm to something like
>> >> below,
>> >>
>> >> void swapcache_free_entries(swp_entry_t *entries, int n)
>> >> {
>> >> struct swap_info_struct *p, *prev;
>> >> int i;
>> >> + swp_entry_t entry;
>> >> + unsigned int prev_swp_type;
>> >>
>> >> if (n <= 0)
>> >> return;
>> >>
>> >> + prev_swp_type = swp_type(entries[0]);
>> >> + for (i = n - 1; i > 0; i--) {
>> >> + if (swp_type(entries[i]) != prev_swp_type)
>> >> + break;
>> >> + }
>> >
>> > That's really what I want to avoid. For many swap usecases,
>> > it adds unnecessary overhead.
>> >
>> >> +
>> >> + /* Sort swap entries by swap device, so each lock is only taken once. */
>> >> + if (i)
>> >> + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
>> >> prev = NULL;
>> >> p = NULL;
>> >> for (i = 0; i < n; ++i) {
>> >> - p = swap_info_get_cont(entries[i], prev);
>> >> + entry = entries[i];
>> >> + p = swap_info_get_cont(entry, prev);
>> >> if (p)
>> >> - swap_entry_free(p, entries[i]);
>> >> + swap_entry_free(p, entry);
>> >> prev = p;
>> >> }
>> >> if (p)
>> >>
>> >> With this patch, the memory freeing time increased from 1.94s to 1.97s.
>> >> I think this is good enough. Do you think so?
>> >
>> > What I mean is as follows(I didn't test it at all):
>> >
>> > With this, sort entries if we found multiple entries in current
>> > entries. It adds some condition checks for non-multiple swap
>> > usecase but it would be more cheaper than the sorting.
>> > And it adds a [un]lock overhead for multiple swap usecase but
>> > it should be a compromise for single-swap usecase which is more
>> > popular.
>> >
>>
>> How about the following solution? It can avoid [un]lock overhead and
>> double lock issue for multiple swap user case and has good performance
>> for one swap user case too.
>
> How worse with approach I suggested compared to as-is?
The performance difference between your version and my version is small
for my testing.
> Unless it's too bad, let's not add more complicated thing to just
> enhance the minor usecase in such even *slow* path.
> It adds code size/maintainance overead.
> With your suggestion, it might enhance a bit with speicific benchmark
> but not sure it's really worth for real practice.
I don't think the code complexity has much difference between our latest
versions. As for complexity, I think my original version which just
uses nr_swapfiles to avoid sort() for single swap device is simple and
good enough for this task. Maybe we can just improve the correctness of
swap device counting as Tim suggested.
Best Regards,
Huang, Ying
>>
>> Best Regards,
>> Huang, Ying
>>
>> 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
>>
>> To reduce the lock contention of swap_info_struct->lock when freeing
>> swap entry. The freed swap entries will be collected in a per-CPU
>> buffer firstly, and be really freed later in batch. During the batch
>> freeing, if the consecutive swap entries in the per-CPU buffer belongs
>> to same swap device, the swap_info_struct->lock needs to be
>> acquired/released only once, so that the lock contention could be
>> reduced greatly. But if there are multiple swap devices, it is
>> possible that the lock may be unnecessarily released/acquired because
>> the swap entries belong to the same swap device are non-consecutive in
>> the per-CPU buffer.
>>
>> To solve the issue, the per-CPU buffer is sorted according to the swap
>> device before freeing the swap entries. Test shows that the time
>> spent by swapcache_free_entries() could be reduced after the patch.
>>
>> With the patch, the memory (some swapped out) free time reduced
>> 13.6% (from 2.59s to 2.28s) in the vm-scalability swap-w-rand test
>> case with 16 processes. The test is done on a Xeon E5 v3 system. The
>> swap device used is a RAM simulated PMEM (persistent memory) device.
>> To test swapping, the test case creates 16 processes, which allocate
>> and write to the anonymous pages until the RAM and part of the swap
>> device is used up, finally the memory (some swapped out) is freed
>> before exit.
>>
>> Signed-off-by: Huang Ying <ying.huang@intel.com>
>> Acked-by: Tim Chen <tim.c.chen@intel.com>
>> Cc: Hugh Dickins <hughd@google.com>
>> Cc: Shaohua Li <shli@kernel.org>
>> Cc: Minchan Kim <minchan@kernel.org>
>> Cc: Rik van Riel <riel@redhat.com>
>>
>> v5:
>>
>> - Use a smarter way to determine whether sort is necessary.
>>
>> v4:
>>
>> - Avoid unnecessary sort if all entries are from one swap device.
>>
>> v3:
>>
>> - Add some comments in code per Rik's suggestion.
>>
>> v2:
>>
>> - Avoid sort swap entries if there is only one swap device.
>> ---
>> 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;
>>
>> 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;
>> +
>> + /* 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;
>> + for (i = m; i < n; i++) {
>> + entry = entries[i];
>> + if (swp_type(entry) == prev_swp_type)
>> + continue;
>> + p = swap_info_get_cont(entry, prev);
>> + if (likely(p))
>> + swap_entry_free(p, entry);
>> prev = p;
>> }
>> if (p)
>> --
>> 2.11.0
>>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
@ 2017-04-28 1:09 ` Huang, Ying
0 siblings, 0 replies; 61+ messages in thread
From: Huang, Ying @ 2017-04-28 1:09 UTC (permalink / raw)
To: Minchan Kim
Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
Shaohua Li, Rik van Riel
Minchan Kim <minchan@kernel.org> writes:
> On Wed, Apr 26, 2017 at 08:42:10PM +0800, Huang, Ying wrote:
>> Minchan Kim <minchan@kernel.org> writes:
>>
>> > On Fri, Apr 21, 2017 at 08:29:30PM +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). I think this is the
>> >> worse case because there is no lock contention. The memory freeing time
>> >> increased from 1.94s to 2.12s (increase ~9.2%). So there is some
>> >> overhead for some cases. I change the algorithm to something like
>> >> below,
>> >>
>> >> void swapcache_free_entries(swp_entry_t *entries, int n)
>> >> {
>> >> struct swap_info_struct *p, *prev;
>> >> int i;
>> >> + swp_entry_t entry;
>> >> + unsigned int prev_swp_type;
>> >>
>> >> if (n <= 0)
>> >> return;
>> >>
>> >> + prev_swp_type = swp_type(entries[0]);
>> >> + for (i = n - 1; i > 0; i--) {
>> >> + if (swp_type(entries[i]) != prev_swp_type)
>> >> + break;
>> >> + }
>> >
>> > That's really what I want to avoid. For many swap usecases,
>> > it adds unnecessary overhead.
>> >
>> >> +
>> >> + /* Sort swap entries by swap device, so each lock is only taken once. */
>> >> + if (i)
>> >> + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
>> >> prev = NULL;
>> >> p = NULL;
>> >> for (i = 0; i < n; ++i) {
>> >> - p = swap_info_get_cont(entries[i], prev);
>> >> + entry = entries[i];
>> >> + p = swap_info_get_cont(entry, prev);
>> >> if (p)
>> >> - swap_entry_free(p, entries[i]);
>> >> + swap_entry_free(p, entry);
>> >> prev = p;
>> >> }
>> >> if (p)
>> >>
>> >> With this patch, the memory freeing time increased from 1.94s to 1.97s.
>> >> I think this is good enough. Do you think so?
>> >
>> > What I mean is as follows(I didn't test it at all):
>> >
>> > With this, sort entries if we found multiple entries in current
>> > entries. It adds some condition checks for non-multiple swap
>> > usecase but it would be more cheaper than the sorting.
>> > And it adds a [un]lock overhead for multiple swap usecase but
>> > it should be a compromise for single-swap usecase which is more
>> > popular.
>> >
>>
>> How about the following solution? It can avoid [un]lock overhead and
>> double lock issue for multiple swap user case and has good performance
>> for one swap user case too.
>
> How worse with approach I suggested compared to as-is?
The performance difference between your version and my version is small
for my testing.
> Unless it's too bad, let's not add more complicated thing to just
> enhance the minor usecase in such even *slow* path.
> It adds code size/maintainance overead.
> With your suggestion, it might enhance a bit with speicific benchmark
> but not sure it's really worth for real practice.
I don't think the code complexity has much difference between our latest
versions. As for complexity, I think my original version which just
uses nr_swapfiles to avoid sort() for single swap device is simple and
good enough for this task. Maybe we can just improve the correctness of
swap device counting as Tim suggested.
Best Regards,
Huang, Ying
>>
>> Best Regards,
>> Huang, Ying
>>
>> 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
>>
>> To reduce the lock contention of swap_info_struct->lock when freeing
>> swap entry. The freed swap entries will be collected in a per-CPU
>> buffer firstly, and be really freed later in batch. During the batch
>> freeing, if the consecutive swap entries in the per-CPU buffer belongs
>> to same swap device, the swap_info_struct->lock needs to be
>> acquired/released only once, so that the lock contention could be
>> reduced greatly. But if there are multiple swap devices, it is
>> possible that the lock may be unnecessarily released/acquired because
>> the swap entries belong to the same swap device are non-consecutive in
>> the per-CPU buffer.
>>
>> To solve the issue, the per-CPU buffer is sorted according to the swap
>> device before freeing the swap entries. Test shows that the time
>> spent by swapcache_free_entries() could be reduced after the patch.
>>
>> With the patch, the memory (some swapped out) free time reduced
>> 13.6% (from 2.59s to 2.28s) in the vm-scalability swap-w-rand test
>> case with 16 processes. The test is done on a Xeon E5 v3 system. The
>> swap device used is a RAM simulated PMEM (persistent memory) device.
>> To test swapping, the test case creates 16 processes, which allocate
>> and write to the anonymous pages until the RAM and part of the swap
>> device is used up, finally the memory (some swapped out) is freed
>> before exit.
>>
>> Signed-off-by: Huang Ying <ying.huang@intel.com>
>> Acked-by: Tim Chen <tim.c.chen@intel.com>
>> Cc: Hugh Dickins <hughd@google.com>
>> Cc: Shaohua Li <shli@kernel.org>
>> Cc: Minchan Kim <minchan@kernel.org>
>> Cc: Rik van Riel <riel@redhat.com>
>>
>> v5:
>>
>> - Use a smarter way to determine whether sort is necessary.
>>
>> v4:
>>
>> - Avoid unnecessary sort if all entries are from one swap device.
>>
>> v3:
>>
>> - Add some comments in code per Rik's suggestion.
>>
>> v2:
>>
>> - Avoid sort swap entries if there is only one swap device.
>> ---
>> 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;
>>
>> 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;
>> +
>> + /* 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;
>> + for (i = m; i < n; i++) {
>> + entry = entries[i];
>> + if (swp_type(entry) == prev_swp_type)
>> + continue;
>> + p = swap_info_get_cont(entry, prev);
>> + if (likely(p))
>> + swap_entry_free(p, entry);
>> prev = p;
>> }
>> if (p)
>> --
>> 2.11.0
>>
--
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>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
2017-04-28 1:09 ` Huang, Ying
@ 2017-04-28 7:42 ` Minchan Kim
-1 siblings, 0 replies; 61+ messages in thread
From: Minchan Kim @ 2017-04-28 7:42 UTC (permalink / raw)
To: Huang, Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Hugh Dickins, Shaohua Li,
Rik van Riel
On Fri, Apr 28, 2017 at 09:09:53AM +0800, Huang, Ying wrote:
> Minchan Kim <minchan@kernel.org> writes:
>
> > On Wed, Apr 26, 2017 at 08:42:10PM +0800, Huang, Ying wrote:
> >> Minchan Kim <minchan@kernel.org> writes:
> >>
> >> > On Fri, Apr 21, 2017 at 08:29:30PM +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). I think this is the
> >> >> worse case because there is no lock contention. The memory freeing time
> >> >> increased from 1.94s to 2.12s (increase ~9.2%). So there is some
> >> >> overhead for some cases. I change the algorithm to something like
> >> >> below,
> >> >>
> >> >> void swapcache_free_entries(swp_entry_t *entries, int n)
> >> >> {
> >> >> struct swap_info_struct *p, *prev;
> >> >> int i;
> >> >> + swp_entry_t entry;
> >> >> + unsigned int prev_swp_type;
> >> >>
> >> >> if (n <= 0)
> >> >> return;
> >> >>
> >> >> + prev_swp_type = swp_type(entries[0]);
> >> >> + for (i = n - 1; i > 0; i--) {
> >> >> + if (swp_type(entries[i]) != prev_swp_type)
> >> >> + break;
> >> >> + }
> >> >
> >> > That's really what I want to avoid. For many swap usecases,
> >> > it adds unnecessary overhead.
> >> >
> >> >> +
> >> >> + /* Sort swap entries by swap device, so each lock is only taken once. */
> >> >> + if (i)
> >> >> + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
> >> >> prev = NULL;
> >> >> p = NULL;
> >> >> for (i = 0; i < n; ++i) {
> >> >> - p = swap_info_get_cont(entries[i], prev);
> >> >> + entry = entries[i];
> >> >> + p = swap_info_get_cont(entry, prev);
> >> >> if (p)
> >> >> - swap_entry_free(p, entries[i]);
> >> >> + swap_entry_free(p, entry);
> >> >> prev = p;
> >> >> }
> >> >> if (p)
> >> >>
> >> >> With this patch, the memory freeing time increased from 1.94s to 1.97s.
> >> >> I think this is good enough. Do you think so?
> >> >
> >> > What I mean is as follows(I didn't test it at all):
> >> >
> >> > With this, sort entries if we found multiple entries in current
> >> > entries. It adds some condition checks for non-multiple swap
> >> > usecase but it would be more cheaper than the sorting.
> >> > And it adds a [un]lock overhead for multiple swap usecase but
> >> > it should be a compromise for single-swap usecase which is more
> >> > popular.
> >> >
> >>
> >> How about the following solution? It can avoid [un]lock overhead and
> >> double lock issue for multiple swap user case and has good performance
> >> for one swap user case too.
> >
> > How worse with approach I suggested compared to as-is?
>
> The performance difference between your version and my version is small
> for my testing.
If so, why should we add code to optimize further?
>
> > Unless it's too bad, let's not add more complicated thing to just
> > enhance the minor usecase in such even *slow* path.
> > It adds code size/maintainance overead.
> > With your suggestion, it might enhance a bit with speicific benchmark
> > but not sure it's really worth for real practice.
>
> I don't think the code complexity has much difference between our latest
> versions. As for complexity, I think my original version which just
What I suggested is to avoid pointless overhead for *major* usecase
and the code you are adding now is to optimize further for *minor*
usecase. And now I dobut the code you are adding is really worth
unless it makes a meaningful output.
If it doesn't, it adds just overhead(code size, maintainance, power and
performance). You might argue it's really *small* so it would be okay
but think about that you would be not only one in the community so
kernel bloats day by day with code to handle corner cases.
> uses nr_swapfiles to avoid sort() for single swap device is simple and
> good enough for this task. Maybe we can just improve the correctness of
But it hurts *major* usecase.
> swap device counting as Tim suggested.
I don't know what Tim suggested. Anyway, my point is that minor
usecase doesn't hurt major usecase and justify the benefit
if you want to put more. So I'm okay with either solution to
meet it.
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
@ 2017-04-28 7:42 ` Minchan Kim
0 siblings, 0 replies; 61+ messages in thread
From: Minchan Kim @ 2017-04-28 7:42 UTC (permalink / raw)
To: Huang, Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Hugh Dickins, Shaohua Li,
Rik van Riel
On Fri, Apr 28, 2017 at 09:09:53AM +0800, Huang, Ying wrote:
> Minchan Kim <minchan@kernel.org> writes:
>
> > On Wed, Apr 26, 2017 at 08:42:10PM +0800, Huang, Ying wrote:
> >> Minchan Kim <minchan@kernel.org> writes:
> >>
> >> > On Fri, Apr 21, 2017 at 08:29:30PM +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). I think this is the
> >> >> worse case because there is no lock contention. The memory freeing time
> >> >> increased from 1.94s to 2.12s (increase ~9.2%). So there is some
> >> >> overhead for some cases. I change the algorithm to something like
> >> >> below,
> >> >>
> >> >> void swapcache_free_entries(swp_entry_t *entries, int n)
> >> >> {
> >> >> struct swap_info_struct *p, *prev;
> >> >> int i;
> >> >> + swp_entry_t entry;
> >> >> + unsigned int prev_swp_type;
> >> >>
> >> >> if (n <= 0)
> >> >> return;
> >> >>
> >> >> + prev_swp_type = swp_type(entries[0]);
> >> >> + for (i = n - 1; i > 0; i--) {
> >> >> + if (swp_type(entries[i]) != prev_swp_type)
> >> >> + break;
> >> >> + }
> >> >
> >> > That's really what I want to avoid. For many swap usecases,
> >> > it adds unnecessary overhead.
> >> >
> >> >> +
> >> >> + /* Sort swap entries by swap device, so each lock is only taken once. */
> >> >> + if (i)
> >> >> + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
> >> >> prev = NULL;
> >> >> p = NULL;
> >> >> for (i = 0; i < n; ++i) {
> >> >> - p = swap_info_get_cont(entries[i], prev);
> >> >> + entry = entries[i];
> >> >> + p = swap_info_get_cont(entry, prev);
> >> >> if (p)
> >> >> - swap_entry_free(p, entries[i]);
> >> >> + swap_entry_free(p, entry);
> >> >> prev = p;
> >> >> }
> >> >> if (p)
> >> >>
> >> >> With this patch, the memory freeing time increased from 1.94s to 1.97s.
> >> >> I think this is good enough. Do you think so?
> >> >
> >> > What I mean is as follows(I didn't test it at all):
> >> >
> >> > With this, sort entries if we found multiple entries in current
> >> > entries. It adds some condition checks for non-multiple swap
> >> > usecase but it would be more cheaper than the sorting.
> >> > And it adds a [un]lock overhead for multiple swap usecase but
> >> > it should be a compromise for single-swap usecase which is more
> >> > popular.
> >> >
> >>
> >> How about the following solution? It can avoid [un]lock overhead and
> >> double lock issue for multiple swap user case and has good performance
> >> for one swap user case too.
> >
> > How worse with approach I suggested compared to as-is?
>
> The performance difference between your version and my version is small
> for my testing.
If so, why should we add code to optimize further?
>
> > Unless it's too bad, let's not add more complicated thing to just
> > enhance the minor usecase in such even *slow* path.
> > It adds code size/maintainance overead.
> > With your suggestion, it might enhance a bit with speicific benchmark
> > but not sure it's really worth for real practice.
>
> I don't think the code complexity has much difference between our latest
> versions. As for complexity, I think my original version which just
What I suggested is to avoid pointless overhead for *major* usecase
and the code you are adding now is to optimize further for *minor*
usecase. And now I dobut the code you are adding is really worth
unless it makes a meaningful output.
If it doesn't, it adds just overhead(code size, maintainance, power and
performance). You might argue it's really *small* so it would be okay
but think about that you would be not only one in the community so
kernel bloats day by day with code to handle corner cases.
> uses nr_swapfiles to avoid sort() for single swap device is simple and
> good enough for this task. Maybe we can just improve the correctness of
But it hurts *major* usecase.
> swap device counting as Tim suggested.
I don't know what Tim suggested. Anyway, my point is that minor
usecase doesn't hurt major usecase and justify the benefit
if you want to put more. So I'm okay with either solution to
meet it.
--
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>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
2017-04-28 7:42 ` Minchan Kim
@ 2017-04-28 8:05 ` Huang, Ying
-1 siblings, 0 replies; 61+ messages in thread
From: Huang, Ying @ 2017-04-28 8:05 UTC (permalink / raw)
To: Minchan Kim
Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
Shaohua Li, Rik van Riel
Minchan Kim <minchan@kernel.org> writes:
> On Fri, Apr 28, 2017 at 09:09:53AM +0800, Huang, Ying wrote:
>> Minchan Kim <minchan@kernel.org> writes:
>>
>> > On Wed, Apr 26, 2017 at 08:42:10PM +0800, Huang, Ying wrote:
>> >> Minchan Kim <minchan@kernel.org> writes:
>> >>
>> >> > On Fri, Apr 21, 2017 at 08:29:30PM +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). I think this is the
>> >> >> worse case because there is no lock contention. The memory freeing time
>> >> >> increased from 1.94s to 2.12s (increase ~9.2%). So there is some
>> >> >> overhead for some cases. I change the algorithm to something like
>> >> >> below,
>> >> >>
>> >> >> void swapcache_free_entries(swp_entry_t *entries, int n)
>> >> >> {
>> >> >> struct swap_info_struct *p, *prev;
>> >> >> int i;
>> >> >> + swp_entry_t entry;
>> >> >> + unsigned int prev_swp_type;
>> >> >>
>> >> >> if (n <= 0)
>> >> >> return;
>> >> >>
>> >> >> + prev_swp_type = swp_type(entries[0]);
>> >> >> + for (i = n - 1; i > 0; i--) {
>> >> >> + if (swp_type(entries[i]) != prev_swp_type)
>> >> >> + break;
>> >> >> + }
>> >> >
>> >> > That's really what I want to avoid. For many swap usecases,
>> >> > it adds unnecessary overhead.
>> >> >
>> >> >> +
>> >> >> + /* Sort swap entries by swap device, so each lock is only taken once. */
>> >> >> + if (i)
>> >> >> + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
>> >> >> prev = NULL;
>> >> >> p = NULL;
>> >> >> for (i = 0; i < n; ++i) {
>> >> >> - p = swap_info_get_cont(entries[i], prev);
>> >> >> + entry = entries[i];
>> >> >> + p = swap_info_get_cont(entry, prev);
>> >> >> if (p)
>> >> >> - swap_entry_free(p, entries[i]);
>> >> >> + swap_entry_free(p, entry);
>> >> >> prev = p;
>> >> >> }
>> >> >> if (p)
>> >> >>
>> >> >> With this patch, the memory freeing time increased from 1.94s to 1.97s.
>> >> >> I think this is good enough. Do you think so?
>> >> >
>> >> > What I mean is as follows(I didn't test it at all):
>> >> >
>> >> > With this, sort entries if we found multiple entries in current
>> >> > entries. It adds some condition checks for non-multiple swap
>> >> > usecase but it would be more cheaper than the sorting.
>> >> > And it adds a [un]lock overhead for multiple swap usecase but
>> >> > it should be a compromise for single-swap usecase which is more
>> >> > popular.
>> >> >
>> >>
>> >> How about the following solution? It can avoid [un]lock overhead and
>> >> double lock issue for multiple swap user case and has good performance
>> >> for one swap user case too.
>> >
>> > How worse with approach I suggested compared to as-is?
>>
>> The performance difference between your version and my version is small
>> for my testing.
>
> If so, why should we add code to optimize further?
>
>>
>> > Unless it's too bad, let's not add more complicated thing to just
>> > enhance the minor usecase in such even *slow* path.
>> > It adds code size/maintainance overead.
>> > With your suggestion, it might enhance a bit with speicific benchmark
>> > but not sure it's really worth for real practice.
>>
>> I don't think the code complexity has much difference between our latest
>> versions. As for complexity, I think my original version which just
>
> What I suggested is to avoid pointless overhead for *major* usecase
> and the code you are adding now is to optimize further for *minor*
> usecase. And now I dobut the code you are adding is really worth
> unless it makes a meaningful output.
> If it doesn't, it adds just overhead(code size, maintainance, power and
> performance). You might argue it's really *small* so it would be okay
> but think about that you would be not only one in the community so
> kernel bloats day by day with code to handle corner cases.
>
>> uses nr_swapfiles to avoid sort() for single swap device is simple and
>> good enough for this task. Maybe we can just improve the correctness of
>
> But it hurts *major* usecase.
>
>> swap device counting as Tim suggested.
>
> I don't know what Tim suggested. Anyway, my point is that minor
> usecase doesn't hurt major usecase and justify the benefit
> if you want to put more. So I'm okay with either solution to
> meet it.
Tim suggested to add a mechanism to correctly track how many swap
devices are in use in swapon/swapoff. So we only sort if the number of
the swap device > 1. This will not cover multiple swap devices with
different priorities, but will cover the major usecases. The code
should be simpler.
Best Regards,
Huang, Ying
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
@ 2017-04-28 8:05 ` Huang, Ying
0 siblings, 0 replies; 61+ messages in thread
From: Huang, Ying @ 2017-04-28 8:05 UTC (permalink / raw)
To: Minchan Kim
Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
Shaohua Li, Rik van Riel
Minchan Kim <minchan@kernel.org> writes:
> On Fri, Apr 28, 2017 at 09:09:53AM +0800, Huang, Ying wrote:
>> Minchan Kim <minchan@kernel.org> writes:
>>
>> > On Wed, Apr 26, 2017 at 08:42:10PM +0800, Huang, Ying wrote:
>> >> Minchan Kim <minchan@kernel.org> writes:
>> >>
>> >> > On Fri, Apr 21, 2017 at 08:29:30PM +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). I think this is the
>> >> >> worse case because there is no lock contention. The memory freeing time
>> >> >> increased from 1.94s to 2.12s (increase ~9.2%). So there is some
>> >> >> overhead for some cases. I change the algorithm to something like
>> >> >> below,
>> >> >>
>> >> >> void swapcache_free_entries(swp_entry_t *entries, int n)
>> >> >> {
>> >> >> struct swap_info_struct *p, *prev;
>> >> >> int i;
>> >> >> + swp_entry_t entry;
>> >> >> + unsigned int prev_swp_type;
>> >> >>
>> >> >> if (n <= 0)
>> >> >> return;
>> >> >>
>> >> >> + prev_swp_type = swp_type(entries[0]);
>> >> >> + for (i = n - 1; i > 0; i--) {
>> >> >> + if (swp_type(entries[i]) != prev_swp_type)
>> >> >> + break;
>> >> >> + }
>> >> >
>> >> > That's really what I want to avoid. For many swap usecases,
>> >> > it adds unnecessary overhead.
>> >> >
>> >> >> +
>> >> >> + /* Sort swap entries by swap device, so each lock is only taken once. */
>> >> >> + if (i)
>> >> >> + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
>> >> >> prev = NULL;
>> >> >> p = NULL;
>> >> >> for (i = 0; i < n; ++i) {
>> >> >> - p = swap_info_get_cont(entries[i], prev);
>> >> >> + entry = entries[i];
>> >> >> + p = swap_info_get_cont(entry, prev);
>> >> >> if (p)
>> >> >> - swap_entry_free(p, entries[i]);
>> >> >> + swap_entry_free(p, entry);
>> >> >> prev = p;
>> >> >> }
>> >> >> if (p)
>> >> >>
>> >> >> With this patch, the memory freeing time increased from 1.94s to 1.97s.
>> >> >> I think this is good enough. Do you think so?
>> >> >
>> >> > What I mean is as follows(I didn't test it at all):
>> >> >
>> >> > With this, sort entries if we found multiple entries in current
>> >> > entries. It adds some condition checks for non-multiple swap
>> >> > usecase but it would be more cheaper than the sorting.
>> >> > And it adds a [un]lock overhead for multiple swap usecase but
>> >> > it should be a compromise for single-swap usecase which is more
>> >> > popular.
>> >> >
>> >>
>> >> How about the following solution? It can avoid [un]lock overhead and
>> >> double lock issue for multiple swap user case and has good performance
>> >> for one swap user case too.
>> >
>> > How worse with approach I suggested compared to as-is?
>>
>> The performance difference between your version and my version is small
>> for my testing.
>
> If so, why should we add code to optimize further?
>
>>
>> > Unless it's too bad, let's not add more complicated thing to just
>> > enhance the minor usecase in such even *slow* path.
>> > It adds code size/maintainance overead.
>> > With your suggestion, it might enhance a bit with speicific benchmark
>> > but not sure it's really worth for real practice.
>>
>> I don't think the code complexity has much difference between our latest
>> versions. As for complexity, I think my original version which just
>
> What I suggested is to avoid pointless overhead for *major* usecase
> and the code you are adding now is to optimize further for *minor*
> usecase. And now I dobut the code you are adding is really worth
> unless it makes a meaningful output.
> If it doesn't, it adds just overhead(code size, maintainance, power and
> performance). You might argue it's really *small* so it would be okay
> but think about that you would be not only one in the community so
> kernel bloats day by day with code to handle corner cases.
>
>> uses nr_swapfiles to avoid sort() for single swap device is simple and
>> good enough for this task. Maybe we can just improve the correctness of
>
> But it hurts *major* usecase.
>
>> swap device counting as Tim suggested.
>
> I don't know what Tim suggested. Anyway, my point is that minor
> usecase doesn't hurt major usecase and justify the benefit
> if you want to put more. So I'm okay with either solution to
> meet it.
Tim suggested to add a mechanism to correctly track how many swap
devices are in use in swapon/swapoff. So we only sort if the number of
the swap device > 1. This will not cover multiple swap devices with
different priorities, but will cover the major usecases. The code
should be simpler.
Best Regards,
Huang, Ying
--
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>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
2017-04-28 8:05 ` Huang, Ying
@ 2017-04-28 9:00 ` Minchan Kim
-1 siblings, 0 replies; 61+ messages in thread
From: Minchan Kim @ 2017-04-28 9:00 UTC (permalink / raw)
To: Huang, Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Hugh Dickins, Shaohua Li,
Rik van Riel
On Fri, Apr 28, 2017 at 04:05:26PM +0800, Huang, Ying wrote:
> Minchan Kim <minchan@kernel.org> writes:
>
> > On Fri, Apr 28, 2017 at 09:09:53AM +0800, Huang, Ying wrote:
> >> Minchan Kim <minchan@kernel.org> writes:
> >>
> >> > On Wed, Apr 26, 2017 at 08:42:10PM +0800, Huang, Ying wrote:
> >> >> Minchan Kim <minchan@kernel.org> writes:
> >> >>
> >> >> > On Fri, Apr 21, 2017 at 08:29:30PM +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). I think this is the
> >> >> >> worse case because there is no lock contention. The memory freeing time
> >> >> >> increased from 1.94s to 2.12s (increase ~9.2%). So there is some
> >> >> >> overhead for some cases. I change the algorithm to something like
> >> >> >> below,
> >> >> >>
> >> >> >> void swapcache_free_entries(swp_entry_t *entries, int n)
> >> >> >> {
> >> >> >> struct swap_info_struct *p, *prev;
> >> >> >> int i;
> >> >> >> + swp_entry_t entry;
> >> >> >> + unsigned int prev_swp_type;
> >> >> >>
> >> >> >> if (n <= 0)
> >> >> >> return;
> >> >> >>
> >> >> >> + prev_swp_type = swp_type(entries[0]);
> >> >> >> + for (i = n - 1; i > 0; i--) {
> >> >> >> + if (swp_type(entries[i]) != prev_swp_type)
> >> >> >> + break;
> >> >> >> + }
> >> >> >
> >> >> > That's really what I want to avoid. For many swap usecases,
> >> >> > it adds unnecessary overhead.
> >> >> >
> >> >> >> +
> >> >> >> + /* Sort swap entries by swap device, so each lock is only taken once. */
> >> >> >> + if (i)
> >> >> >> + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
> >> >> >> prev = NULL;
> >> >> >> p = NULL;
> >> >> >> for (i = 0; i < n; ++i) {
> >> >> >> - p = swap_info_get_cont(entries[i], prev);
> >> >> >> + entry = entries[i];
> >> >> >> + p = swap_info_get_cont(entry, prev);
> >> >> >> if (p)
> >> >> >> - swap_entry_free(p, entries[i]);
> >> >> >> + swap_entry_free(p, entry);
> >> >> >> prev = p;
> >> >> >> }
> >> >> >> if (p)
> >> >> >>
> >> >> >> With this patch, the memory freeing time increased from 1.94s to 1.97s.
> >> >> >> I think this is good enough. Do you think so?
> >> >> >
> >> >> > What I mean is as follows(I didn't test it at all):
> >> >> >
> >> >> > With this, sort entries if we found multiple entries in current
> >> >> > entries. It adds some condition checks for non-multiple swap
> >> >> > usecase but it would be more cheaper than the sorting.
> >> >> > And it adds a [un]lock overhead for multiple swap usecase but
> >> >> > it should be a compromise for single-swap usecase which is more
> >> >> > popular.
> >> >> >
> >> >>
> >> >> How about the following solution? It can avoid [un]lock overhead and
> >> >> double lock issue for multiple swap user case and has good performance
> >> >> for one swap user case too.
> >> >
> >> > How worse with approach I suggested compared to as-is?
> >>
> >> The performance difference between your version and my version is small
> >> for my testing.
> >
> > If so, why should we add code to optimize further?
> >
> >>
> >> > Unless it's too bad, let's not add more complicated thing to just
> >> > enhance the minor usecase in such even *slow* path.
> >> > It adds code size/maintainance overead.
> >> > With your suggestion, it might enhance a bit with speicific benchmark
> >> > but not sure it's really worth for real practice.
> >>
> >> I don't think the code complexity has much difference between our latest
> >> versions. As for complexity, I think my original version which just
> >
> > What I suggested is to avoid pointless overhead for *major* usecase
> > and the code you are adding now is to optimize further for *minor*
> > usecase. And now I dobut the code you are adding is really worth
> > unless it makes a meaningful output.
> > If it doesn't, it adds just overhead(code size, maintainance, power and
> > performance). You might argue it's really *small* so it would be okay
> > but think about that you would be not only one in the community so
> > kernel bloats day by day with code to handle corner cases.
> >
> >> uses nr_swapfiles to avoid sort() for single swap device is simple and
> >> good enough for this task. Maybe we can just improve the correctness of
> >
> > But it hurts *major* usecase.
> >
> >> swap device counting as Tim suggested.
> >
> > I don't know what Tim suggested. Anyway, my point is that minor
> > usecase doesn't hurt major usecase and justify the benefit
> > if you want to put more. So I'm okay with either solution to
> > meet it.
>
> Tim suggested to add a mechanism to correctly track how many swap
> devices are in use in swapon/swapoff. So we only sort if the number of
> the swap device > 1. This will not cover multiple swap devices with
> different priorities, but will cover the major usecases. The code
> should be simpler.
As you know, it doesn't solve multiple swaps by priority.
Even, there are cases full with entries same swap device
although multiple swap devices are used.
So, I think runtime sorting by judging need to be sored is still
better.
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
@ 2017-04-28 9:00 ` Minchan Kim
0 siblings, 0 replies; 61+ messages in thread
From: Minchan Kim @ 2017-04-28 9:00 UTC (permalink / raw)
To: Huang, Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Hugh Dickins, Shaohua Li,
Rik van Riel
On Fri, Apr 28, 2017 at 04:05:26PM +0800, Huang, Ying wrote:
> Minchan Kim <minchan@kernel.org> writes:
>
> > On Fri, Apr 28, 2017 at 09:09:53AM +0800, Huang, Ying wrote:
> >> Minchan Kim <minchan@kernel.org> writes:
> >>
> >> > On Wed, Apr 26, 2017 at 08:42:10PM +0800, Huang, Ying wrote:
> >> >> Minchan Kim <minchan@kernel.org> writes:
> >> >>
> >> >> > On Fri, Apr 21, 2017 at 08:29:30PM +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). I think this is the
> >> >> >> worse case because there is no lock contention. The memory freeing time
> >> >> >> increased from 1.94s to 2.12s (increase ~9.2%). So there is some
> >> >> >> overhead for some cases. I change the algorithm to something like
> >> >> >> below,
> >> >> >>
> >> >> >> void swapcache_free_entries(swp_entry_t *entries, int n)
> >> >> >> {
> >> >> >> struct swap_info_struct *p, *prev;
> >> >> >> int i;
> >> >> >> + swp_entry_t entry;
> >> >> >> + unsigned int prev_swp_type;
> >> >> >>
> >> >> >> if (n <= 0)
> >> >> >> return;
> >> >> >>
> >> >> >> + prev_swp_type = swp_type(entries[0]);
> >> >> >> + for (i = n - 1; i > 0; i--) {
> >> >> >> + if (swp_type(entries[i]) != prev_swp_type)
> >> >> >> + break;
> >> >> >> + }
> >> >> >
> >> >> > That's really what I want to avoid. For many swap usecases,
> >> >> > it adds unnecessary overhead.
> >> >> >
> >> >> >> +
> >> >> >> + /* Sort swap entries by swap device, so each lock is only taken once. */
> >> >> >> + if (i)
> >> >> >> + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
> >> >> >> prev = NULL;
> >> >> >> p = NULL;
> >> >> >> for (i = 0; i < n; ++i) {
> >> >> >> - p = swap_info_get_cont(entries[i], prev);
> >> >> >> + entry = entries[i];
> >> >> >> + p = swap_info_get_cont(entry, prev);
> >> >> >> if (p)
> >> >> >> - swap_entry_free(p, entries[i]);
> >> >> >> + swap_entry_free(p, entry);
> >> >> >> prev = p;
> >> >> >> }
> >> >> >> if (p)
> >> >> >>
> >> >> >> With this patch, the memory freeing time increased from 1.94s to 1.97s.
> >> >> >> I think this is good enough. Do you think so?
> >> >> >
> >> >> > What I mean is as follows(I didn't test it at all):
> >> >> >
> >> >> > With this, sort entries if we found multiple entries in current
> >> >> > entries. It adds some condition checks for non-multiple swap
> >> >> > usecase but it would be more cheaper than the sorting.
> >> >> > And it adds a [un]lock overhead for multiple swap usecase but
> >> >> > it should be a compromise for single-swap usecase which is more
> >> >> > popular.
> >> >> >
> >> >>
> >> >> How about the following solution? It can avoid [un]lock overhead and
> >> >> double lock issue for multiple swap user case and has good performance
> >> >> for one swap user case too.
> >> >
> >> > How worse with approach I suggested compared to as-is?
> >>
> >> The performance difference between your version and my version is small
> >> for my testing.
> >
> > If so, why should we add code to optimize further?
> >
> >>
> >> > Unless it's too bad, let's not add more complicated thing to just
> >> > enhance the minor usecase in such even *slow* path.
> >> > It adds code size/maintainance overead.
> >> > With your suggestion, it might enhance a bit with speicific benchmark
> >> > but not sure it's really worth for real practice.
> >>
> >> I don't think the code complexity has much difference between our latest
> >> versions. As for complexity, I think my original version which just
> >
> > What I suggested is to avoid pointless overhead for *major* usecase
> > and the code you are adding now is to optimize further for *minor*
> > usecase. And now I dobut the code you are adding is really worth
> > unless it makes a meaningful output.
> > If it doesn't, it adds just overhead(code size, maintainance, power and
> > performance). You might argue it's really *small* so it would be okay
> > but think about that you would be not only one in the community so
> > kernel bloats day by day with code to handle corner cases.
> >
> >> uses nr_swapfiles to avoid sort() for single swap device is simple and
> >> good enough for this task. Maybe we can just improve the correctness of
> >
> > But it hurts *major* usecase.
> >
> >> swap device counting as Tim suggested.
> >
> > I don't know what Tim suggested. Anyway, my point is that minor
> > usecase doesn't hurt major usecase and justify the benefit
> > if you want to put more. So I'm okay with either solution to
> > meet it.
>
> Tim suggested to add a mechanism to correctly track how many swap
> devices are in use in swapon/swapoff. So we only sort if the number of
> the swap device > 1. This will not cover multiple swap devices with
> different priorities, but will cover the major usecases. The code
> should be simpler.
As you know, it doesn't solve multiple swaps by priority.
Even, there are cases full with entries same swap device
although multiple swap devices are used.
So, I think runtime sorting by judging need to be sored is still
better.
--
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>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
2017-04-28 9:00 ` Minchan Kim
@ 2017-04-28 11:48 ` Huang, Ying
-1 siblings, 0 replies; 61+ messages in thread
From: Huang, Ying @ 2017-04-28 11:48 UTC (permalink / raw)
To: Minchan Kim
Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
Shaohua Li, Rik van Riel
Minchan Kim <minchan@kernel.org> writes:
> On Fri, Apr 28, 2017 at 04:05:26PM +0800, Huang, Ying wrote:
>> Minchan Kim <minchan@kernel.org> writes:
>>
>> > On Fri, Apr 28, 2017 at 09:09:53AM +0800, Huang, Ying wrote:
>> >> Minchan Kim <minchan@kernel.org> writes:
>> >>
>> >> > On Wed, Apr 26, 2017 at 08:42:10PM +0800, Huang, Ying wrote:
>> >> >> Minchan Kim <minchan@kernel.org> writes:
>> >> >>
>> >> >> > On Fri, Apr 21, 2017 at 08:29:30PM +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). I think this is the
>> >> >> >> worse case because there is no lock contention. The memory freeing time
>> >> >> >> increased from 1.94s to 2.12s (increase ~9.2%). So there is some
>> >> >> >> overhead for some cases. I change the algorithm to something like
>> >> >> >> below,
>> >> >> >>
>> >> >> >> void swapcache_free_entries(swp_entry_t *entries, int n)
>> >> >> >> {
>> >> >> >> struct swap_info_struct *p, *prev;
>> >> >> >> int i;
>> >> >> >> + swp_entry_t entry;
>> >> >> >> + unsigned int prev_swp_type;
>> >> >> >>
>> >> >> >> if (n <= 0)
>> >> >> >> return;
>> >> >> >>
>> >> >> >> + prev_swp_type = swp_type(entries[0]);
>> >> >> >> + for (i = n - 1; i > 0; i--) {
>> >> >> >> + if (swp_type(entries[i]) != prev_swp_type)
>> >> >> >> + break;
>> >> >> >> + }
>> >> >> >
>> >> >> > That's really what I want to avoid. For many swap usecases,
>> >> >> > it adds unnecessary overhead.
>> >> >> >
>> >> >> >> +
>> >> >> >> + /* Sort swap entries by swap device, so each lock is only taken once. */
>> >> >> >> + if (i)
>> >> >> >> + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
>> >> >> >> prev = NULL;
>> >> >> >> p = NULL;
>> >> >> >> for (i = 0; i < n; ++i) {
>> >> >> >> - p = swap_info_get_cont(entries[i], prev);
>> >> >> >> + entry = entries[i];
>> >> >> >> + p = swap_info_get_cont(entry, prev);
>> >> >> >> if (p)
>> >> >> >> - swap_entry_free(p, entries[i]);
>> >> >> >> + swap_entry_free(p, entry);
>> >> >> >> prev = p;
>> >> >> >> }
>> >> >> >> if (p)
>> >> >> >>
>> >> >> >> With this patch, the memory freeing time increased from 1.94s to 1.97s.
>> >> >> >> I think this is good enough. Do you think so?
>> >> >> >
>> >> >> > What I mean is as follows(I didn't test it at all):
>> >> >> >
>> >> >> > With this, sort entries if we found multiple entries in current
>> >> >> > entries. It adds some condition checks for non-multiple swap
>> >> >> > usecase but it would be more cheaper than the sorting.
>> >> >> > And it adds a [un]lock overhead for multiple swap usecase but
>> >> >> > it should be a compromise for single-swap usecase which is more
>> >> >> > popular.
>> >> >> >
>> >> >>
>> >> >> How about the following solution? It can avoid [un]lock overhead and
>> >> >> double lock issue for multiple swap user case and has good performance
>> >> >> for one swap user case too.
>> >> >
>> >> > How worse with approach I suggested compared to as-is?
>> >>
>> >> The performance difference between your version and my version is small
>> >> for my testing.
>> >
>> > If so, why should we add code to optimize further?
>> >
>> >>
>> >> > Unless it's too bad, let's not add more complicated thing to just
>> >> > enhance the minor usecase in such even *slow* path.
>> >> > It adds code size/maintainance overead.
>> >> > With your suggestion, it might enhance a bit with speicific benchmark
>> >> > but not sure it's really worth for real practice.
>> >>
>> >> I don't think the code complexity has much difference between our latest
>> >> versions. As for complexity, I think my original version which just
>> >
>> > What I suggested is to avoid pointless overhead for *major* usecase
>> > and the code you are adding now is to optimize further for *minor*
>> > usecase. And now I dobut the code you are adding is really worth
>> > unless it makes a meaningful output.
>> > If it doesn't, it adds just overhead(code size, maintainance, power and
>> > performance). You might argue it's really *small* so it would be okay
>> > but think about that you would be not only one in the community so
>> > kernel bloats day by day with code to handle corner cases.
>> >
>> >> uses nr_swapfiles to avoid sort() for single swap device is simple and
>> >> good enough for this task. Maybe we can just improve the correctness of
>> >
>> > But it hurts *major* usecase.
>> >
>> >> swap device counting as Tim suggested.
>> >
>> > I don't know what Tim suggested. Anyway, my point is that minor
>> > usecase doesn't hurt major usecase and justify the benefit
>> > if you want to put more. So I'm okay with either solution to
>> > meet it.
>>
>> Tim suggested to add a mechanism to correctly track how many swap
>> devices are in use in swapon/swapoff. So we only sort if the number of
>> the swap device > 1. This will not cover multiple swap devices with
>> different priorities, but will cover the major usecases. The code
>> should be simpler.
>
> As you know, it doesn't solve multiple swaps by priority.
I don't think this is *major* usecase.
> Even, there are cases full with entries same swap device
> although multiple swap devices are used.
Why, if you have multiple swap device, every time you will allocate from
different swap device. Although there are swap alloc slots cache, the
possibility of full alignment is low.
Even if there are cases all entries come from one swap device, the
sorting is fast in fact because the array is short and the elements are
sorted (same swap type) already. So it is not necessary to worry about
that too much.
Best Regards,
Huang, Ying
> So, I think runtime sorting by judging need to be sored is still
> better.
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
@ 2017-04-28 11:48 ` Huang, Ying
0 siblings, 0 replies; 61+ messages in thread
From: Huang, Ying @ 2017-04-28 11:48 UTC (permalink / raw)
To: Minchan Kim
Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
Shaohua Li, Rik van Riel
Minchan Kim <minchan@kernel.org> writes:
> On Fri, Apr 28, 2017 at 04:05:26PM +0800, Huang, Ying wrote:
>> Minchan Kim <minchan@kernel.org> writes:
>>
>> > On Fri, Apr 28, 2017 at 09:09:53AM +0800, Huang, Ying wrote:
>> >> Minchan Kim <minchan@kernel.org> writes:
>> >>
>> >> > On Wed, Apr 26, 2017 at 08:42:10PM +0800, Huang, Ying wrote:
>> >> >> Minchan Kim <minchan@kernel.org> writes:
>> >> >>
>> >> >> > On Fri, Apr 21, 2017 at 08:29:30PM +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). I think this is the
>> >> >> >> worse case because there is no lock contention. The memory freeing time
>> >> >> >> increased from 1.94s to 2.12s (increase ~9.2%). So there is some
>> >> >> >> overhead for some cases. I change the algorithm to something like
>> >> >> >> below,
>> >> >> >>
>> >> >> >> void swapcache_free_entries(swp_entry_t *entries, int n)
>> >> >> >> {
>> >> >> >> struct swap_info_struct *p, *prev;
>> >> >> >> int i;
>> >> >> >> + swp_entry_t entry;
>> >> >> >> + unsigned int prev_swp_type;
>> >> >> >>
>> >> >> >> if (n <= 0)
>> >> >> >> return;
>> >> >> >>
>> >> >> >> + prev_swp_type = swp_type(entries[0]);
>> >> >> >> + for (i = n - 1; i > 0; i--) {
>> >> >> >> + if (swp_type(entries[i]) != prev_swp_type)
>> >> >> >> + break;
>> >> >> >> + }
>> >> >> >
>> >> >> > That's really what I want to avoid. For many swap usecases,
>> >> >> > it adds unnecessary overhead.
>> >> >> >
>> >> >> >> +
>> >> >> >> + /* Sort swap entries by swap device, so each lock is only taken once. */
>> >> >> >> + if (i)
>> >> >> >> + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
>> >> >> >> prev = NULL;
>> >> >> >> p = NULL;
>> >> >> >> for (i = 0; i < n; ++i) {
>> >> >> >> - p = swap_info_get_cont(entries[i], prev);
>> >> >> >> + entry = entries[i];
>> >> >> >> + p = swap_info_get_cont(entry, prev);
>> >> >> >> if (p)
>> >> >> >> - swap_entry_free(p, entries[i]);
>> >> >> >> + swap_entry_free(p, entry);
>> >> >> >> prev = p;
>> >> >> >> }
>> >> >> >> if (p)
>> >> >> >>
>> >> >> >> With this patch, the memory freeing time increased from 1.94s to 1.97s.
>> >> >> >> I think this is good enough. Do you think so?
>> >> >> >
>> >> >> > What I mean is as follows(I didn't test it at all):
>> >> >> >
>> >> >> > With this, sort entries if we found multiple entries in current
>> >> >> > entries. It adds some condition checks for non-multiple swap
>> >> >> > usecase but it would be more cheaper than the sorting.
>> >> >> > And it adds a [un]lock overhead for multiple swap usecase but
>> >> >> > it should be a compromise for single-swap usecase which is more
>> >> >> > popular.
>> >> >> >
>> >> >>
>> >> >> How about the following solution? It can avoid [un]lock overhead and
>> >> >> double lock issue for multiple swap user case and has good performance
>> >> >> for one swap user case too.
>> >> >
>> >> > How worse with approach I suggested compared to as-is?
>> >>
>> >> The performance difference between your version and my version is small
>> >> for my testing.
>> >
>> > If so, why should we add code to optimize further?
>> >
>> >>
>> >> > Unless it's too bad, let's not add more complicated thing to just
>> >> > enhance the minor usecase in such even *slow* path.
>> >> > It adds code size/maintainance overead.
>> >> > With your suggestion, it might enhance a bit with speicific benchmark
>> >> > but not sure it's really worth for real practice.
>> >>
>> >> I don't think the code complexity has much difference between our latest
>> >> versions. As for complexity, I think my original version which just
>> >
>> > What I suggested is to avoid pointless overhead for *major* usecase
>> > and the code you are adding now is to optimize further for *minor*
>> > usecase. And now I dobut the code you are adding is really worth
>> > unless it makes a meaningful output.
>> > If it doesn't, it adds just overhead(code size, maintainance, power and
>> > performance). You might argue it's really *small* so it would be okay
>> > but think about that you would be not only one in the community so
>> > kernel bloats day by day with code to handle corner cases.
>> >
>> >> uses nr_swapfiles to avoid sort() for single swap device is simple and
>> >> good enough for this task. Maybe we can just improve the correctness of
>> >
>> > But it hurts *major* usecase.
>> >
>> >> swap device counting as Tim suggested.
>> >
>> > I don't know what Tim suggested. Anyway, my point is that minor
>> > usecase doesn't hurt major usecase and justify the benefit
>> > if you want to put more. So I'm okay with either solution to
>> > meet it.
>>
>> Tim suggested to add a mechanism to correctly track how many swap
>> devices are in use in swapon/swapoff. So we only sort if the number of
>> the swap device > 1. This will not cover multiple swap devices with
>> different priorities, but will cover the major usecases. The code
>> should be simpler.
>
> As you know, it doesn't solve multiple swaps by priority.
I don't think this is *major* usecase.
> Even, there are cases full with entries same swap device
> although multiple swap devices are used.
Why, if you have multiple swap device, every time you will allocate from
different swap device. Although there are swap alloc slots cache, the
possibility of full alignment is low.
Even if there are cases all entries come from one swap device, the
sorting is fast in fact because the array is short and the elements are
sorted (same swap type) already. So it is not necessary to worry about
that too much.
Best Regards,
Huang, Ying
> So, I think runtime sorting by judging need to be sored is still
> better.
--
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>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
2017-04-28 11:48 ` Huang, Ying
@ 2017-04-28 13:35 ` Huang, Ying
-1 siblings, 0 replies; 61+ messages in thread
From: Huang, Ying @ 2017-04-28 13:35 UTC (permalink / raw)
To: Huang, Ying
Cc: Minchan Kim, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
Shaohua Li, Rik van Riel
"Huang, Ying" <ying.huang@intel.com> writes:
> Minchan Kim <minchan@kernel.org> writes:
>
>> On Fri, Apr 28, 2017 at 04:05:26PM +0800, Huang, Ying wrote:
>>> Minchan Kim <minchan@kernel.org> writes:
>>>
>>> > On Fri, Apr 28, 2017 at 09:09:53AM +0800, Huang, Ying wrote:
>>> >> Minchan Kim <minchan@kernel.org> writes:
>>> >>
>>> >> > On Wed, Apr 26, 2017 at 08:42:10PM +0800, Huang, Ying wrote:
>>> >> >> Minchan Kim <minchan@kernel.org> writes:
>>> >> >>
>>> >> >> > On Fri, Apr 21, 2017 at 08:29:30PM +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). I think this is the
>>> >> >> >> worse case because there is no lock contention. The memory freeing time
>>> >> >> >> increased from 1.94s to 2.12s (increase ~9.2%). So there is some
>>> >> >> >> overhead for some cases. I change the algorithm to something like
>>> >> >> >> below,
>>> >> >> >>
>>> >> >> >> void swapcache_free_entries(swp_entry_t *entries, int n)
>>> >> >> >> {
>>> >> >> >> struct swap_info_struct *p, *prev;
>>> >> >> >> int i;
>>> >> >> >> + swp_entry_t entry;
>>> >> >> >> + unsigned int prev_swp_type;
>>> >> >> >>
>>> >> >> >> if (n <= 0)
>>> >> >> >> return;
>>> >> >> >>
>>> >> >> >> + prev_swp_type = swp_type(entries[0]);
>>> >> >> >> + for (i = n - 1; i > 0; i--) {
>>> >> >> >> + if (swp_type(entries[i]) != prev_swp_type)
>>> >> >> >> + break;
>>> >> >> >> + }
>>> >> >> >
>>> >> >> > That's really what I want to avoid. For many swap usecases,
>>> >> >> > it adds unnecessary overhead.
>>> >> >> >
>>> >> >> >> +
>>> >> >> >> + /* Sort swap entries by swap device, so each lock is only taken once. */
>>> >> >> >> + if (i)
>>> >> >> >> + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
>>> >> >> >> prev = NULL;
>>> >> >> >> p = NULL;
>>> >> >> >> for (i = 0; i < n; ++i) {
>>> >> >> >> - p = swap_info_get_cont(entries[i], prev);
>>> >> >> >> + entry = entries[i];
>>> >> >> >> + p = swap_info_get_cont(entry, prev);
>>> >> >> >> if (p)
>>> >> >> >> - swap_entry_free(p, entries[i]);
>>> >> >> >> + swap_entry_free(p, entry);
>>> >> >> >> prev = p;
>>> >> >> >> }
>>> >> >> >> if (p)
>>> >> >> >>
>>> >> >> >> With this patch, the memory freeing time increased from 1.94s to 1.97s.
>>> >> >> >> I think this is good enough. Do you think so?
>>> >> >> >
>>> >> >> > What I mean is as follows(I didn't test it at all):
>>> >> >> >
>>> >> >> > With this, sort entries if we found multiple entries in current
>>> >> >> > entries. It adds some condition checks for non-multiple swap
>>> >> >> > usecase but it would be more cheaper than the sorting.
>>> >> >> > And it adds a [un]lock overhead for multiple swap usecase but
>>> >> >> > it should be a compromise for single-swap usecase which is more
>>> >> >> > popular.
>>> >> >> >
>>> >> >>
>>> >> >> How about the following solution? It can avoid [un]lock overhead and
>>> >> >> double lock issue for multiple swap user case and has good performance
>>> >> >> for one swap user case too.
>>> >> >
>>> >> > How worse with approach I suggested compared to as-is?
>>> >>
>>> >> The performance difference between your version and my version is small
>>> >> for my testing.
>>> >
>>> > If so, why should we add code to optimize further?
>>> >
>>> >>
>>> >> > Unless it's too bad, let's not add more complicated thing to just
>>> >> > enhance the minor usecase in such even *slow* path.
>>> >> > It adds code size/maintainance overead.
>>> >> > With your suggestion, it might enhance a bit with speicific benchmark
>>> >> > but not sure it's really worth for real practice.
>>> >>
>>> >> I don't think the code complexity has much difference between our latest
>>> >> versions. As for complexity, I think my original version which just
>>> >
>>> > What I suggested is to avoid pointless overhead for *major* usecase
>>> > and the code you are adding now is to optimize further for *minor*
>>> > usecase. And now I dobut the code you are adding is really worth
>>> > unless it makes a meaningful output.
>>> > If it doesn't, it adds just overhead(code size, maintainance, power and
>>> > performance). You might argue it's really *small* so it would be okay
>>> > but think about that you would be not only one in the community so
>>> > kernel bloats day by day with code to handle corner cases.
>>> >
>>> >> uses nr_swapfiles to avoid sort() for single swap device is simple and
>>> >> good enough for this task. Maybe we can just improve the correctness of
>>> >
>>> > But it hurts *major* usecase.
>>> >
>>> >> swap device counting as Tim suggested.
>>> >
>>> > I don't know what Tim suggested. Anyway, my point is that minor
>>> > usecase doesn't hurt major usecase and justify the benefit
>>> > if you want to put more. So I'm okay with either solution to
>>> > meet it.
>>>
>>> Tim suggested to add a mechanism to correctly track how many swap
>>> devices are in use in swapon/swapoff. So we only sort if the number of
>>> the swap device > 1. This will not cover multiple swap devices with
>>> different priorities, but will cover the major usecases. The code
>>> should be simpler.
>>
>> As you know, it doesn't solve multiple swaps by priority.
>
> I don't think this is *major* usecase.
>
>> Even, there are cases full with entries same swap device
>> although multiple swap devices are used.
>
> Why, if you have multiple swap device, every time you will allocate from
> different swap device. Although there are swap alloc slots cache, the
> possibility of full alignment is low.
>
> Even if there are cases all entries come from one swap device, the
> sorting is fast in fact because the array is short and the elements are
> sorted (same swap type) already. So it is not necessary to worry about
> that too much.
In fact, during the test, I found the overhead of sort() is comparable
with the performance difference of adding likely()/unlikely() to the
"if" in the function.
Best Regards,
Huang, Ying
> Best Regards,
> Huang, Ying
>
>> So, I think runtime sorting by judging need to be sored is still
>> better.
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
@ 2017-04-28 13:35 ` Huang, Ying
0 siblings, 0 replies; 61+ messages in thread
From: Huang, Ying @ 2017-04-28 13:35 UTC (permalink / raw)
To: Huang, Ying
Cc: Minchan Kim, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
Shaohua Li, Rik van Riel
"Huang, Ying" <ying.huang@intel.com> writes:
> Minchan Kim <minchan@kernel.org> writes:
>
>> On Fri, Apr 28, 2017 at 04:05:26PM +0800, Huang, Ying wrote:
>>> Minchan Kim <minchan@kernel.org> writes:
>>>
>>> > On Fri, Apr 28, 2017 at 09:09:53AM +0800, Huang, Ying wrote:
>>> >> Minchan Kim <minchan@kernel.org> writes:
>>> >>
>>> >> > On Wed, Apr 26, 2017 at 08:42:10PM +0800, Huang, Ying wrote:
>>> >> >> Minchan Kim <minchan@kernel.org> writes:
>>> >> >>
>>> >> >> > On Fri, Apr 21, 2017 at 08:29:30PM +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). I think this is the
>>> >> >> >> worse case because there is no lock contention. The memory freeing time
>>> >> >> >> increased from 1.94s to 2.12s (increase ~9.2%). So there is some
>>> >> >> >> overhead for some cases. I change the algorithm to something like
>>> >> >> >> below,
>>> >> >> >>
>>> >> >> >> void swapcache_free_entries(swp_entry_t *entries, int n)
>>> >> >> >> {
>>> >> >> >> struct swap_info_struct *p, *prev;
>>> >> >> >> int i;
>>> >> >> >> + swp_entry_t entry;
>>> >> >> >> + unsigned int prev_swp_type;
>>> >> >> >>
>>> >> >> >> if (n <= 0)
>>> >> >> >> return;
>>> >> >> >>
>>> >> >> >> + prev_swp_type = swp_type(entries[0]);
>>> >> >> >> + for (i = n - 1; i > 0; i--) {
>>> >> >> >> + if (swp_type(entries[i]) != prev_swp_type)
>>> >> >> >> + break;
>>> >> >> >> + }
>>> >> >> >
>>> >> >> > That's really what I want to avoid. For many swap usecases,
>>> >> >> > it adds unnecessary overhead.
>>> >> >> >
>>> >> >> >> +
>>> >> >> >> + /* Sort swap entries by swap device, so each lock is only taken once. */
>>> >> >> >> + if (i)
>>> >> >> >> + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
>>> >> >> >> prev = NULL;
>>> >> >> >> p = NULL;
>>> >> >> >> for (i = 0; i < n; ++i) {
>>> >> >> >> - p = swap_info_get_cont(entries[i], prev);
>>> >> >> >> + entry = entries[i];
>>> >> >> >> + p = swap_info_get_cont(entry, prev);
>>> >> >> >> if (p)
>>> >> >> >> - swap_entry_free(p, entries[i]);
>>> >> >> >> + swap_entry_free(p, entry);
>>> >> >> >> prev = p;
>>> >> >> >> }
>>> >> >> >> if (p)
>>> >> >> >>
>>> >> >> >> With this patch, the memory freeing time increased from 1.94s to 1.97s.
>>> >> >> >> I think this is good enough. Do you think so?
>>> >> >> >
>>> >> >> > What I mean is as follows(I didn't test it at all):
>>> >> >> >
>>> >> >> > With this, sort entries if we found multiple entries in current
>>> >> >> > entries. It adds some condition checks for non-multiple swap
>>> >> >> > usecase but it would be more cheaper than the sorting.
>>> >> >> > And it adds a [un]lock overhead for multiple swap usecase but
>>> >> >> > it should be a compromise for single-swap usecase which is more
>>> >> >> > popular.
>>> >> >> >
>>> >> >>
>>> >> >> How about the following solution? It can avoid [un]lock overhead and
>>> >> >> double lock issue for multiple swap user case and has good performance
>>> >> >> for one swap user case too.
>>> >> >
>>> >> > How worse with approach I suggested compared to as-is?
>>> >>
>>> >> The performance difference between your version and my version is small
>>> >> for my testing.
>>> >
>>> > If so, why should we add code to optimize further?
>>> >
>>> >>
>>> >> > Unless it's too bad, let's not add more complicated thing to just
>>> >> > enhance the minor usecase in such even *slow* path.
>>> >> > It adds code size/maintainance overead.
>>> >> > With your suggestion, it might enhance a bit with speicific benchmark
>>> >> > but not sure it's really worth for real practice.
>>> >>
>>> >> I don't think the code complexity has much difference between our latest
>>> >> versions. As for complexity, I think my original version which just
>>> >
>>> > What I suggested is to avoid pointless overhead for *major* usecase
>>> > and the code you are adding now is to optimize further for *minor*
>>> > usecase. And now I dobut the code you are adding is really worth
>>> > unless it makes a meaningful output.
>>> > If it doesn't, it adds just overhead(code size, maintainance, power and
>>> > performance). You might argue it's really *small* so it would be okay
>>> > but think about that you would be not only one in the community so
>>> > kernel bloats day by day with code to handle corner cases.
>>> >
>>> >> uses nr_swapfiles to avoid sort() for single swap device is simple and
>>> >> good enough for this task. Maybe we can just improve the correctness of
>>> >
>>> > But it hurts *major* usecase.
>>> >
>>> >> swap device counting as Tim suggested.
>>> >
>>> > I don't know what Tim suggested. Anyway, my point is that minor
>>> > usecase doesn't hurt major usecase and justify the benefit
>>> > if you want to put more. So I'm okay with either solution to
>>> > meet it.
>>>
>>> Tim suggested to add a mechanism to correctly track how many swap
>>> devices are in use in swapon/swapoff. So we only sort if the number of
>>> the swap device > 1. This will not cover multiple swap devices with
>>> different priorities, but will cover the major usecases. The code
>>> should be simpler.
>>
>> As you know, it doesn't solve multiple swaps by priority.
>
> I don't think this is *major* usecase.
>
>> Even, there are cases full with entries same swap device
>> although multiple swap devices are used.
>
> Why, if you have multiple swap device, every time you will allocate from
> different swap device. Although there are swap alloc slots cache, the
> possibility of full alignment is low.
>
> Even if there are cases all entries come from one swap device, the
> sorting is fast in fact because the array is short and the elements are
> sorted (same swap type) already. So it is not necessary to worry about
> that too much.
In fact, during the test, I found the overhead of sort() is comparable
with the performance difference of adding likely()/unlikely() to the
"if" in the function.
Best Regards,
Huang, Ying
> Best Regards,
> Huang, Ying
>
>> So, I think runtime sorting by judging need to be sored is still
>> better.
--
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>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
2017-04-28 13:35 ` Huang, Ying
@ 2017-05-02 5:02 ` Minchan Kim
-1 siblings, 0 replies; 61+ messages in thread
From: Minchan Kim @ 2017-05-02 5:02 UTC (permalink / raw)
To: Huang, Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Hugh Dickins, Shaohua Li,
Rik van Riel
On Fri, Apr 28, 2017 at 09:35:37PM +0800, Huang, Ying wrote:
> In fact, during the test, I found the overhead of sort() is comparable
> with the performance difference of adding likely()/unlikely() to the
> "if" in the function.
Huang,
This discussion is started from your optimization code:
if (nr_swapfiles > 1)
sort();
I don't have such fast machine so cannot test it. However, you added
such optimization code in there so I guess it's *worth* to review so
with spending my time, I pointed out what you are missing and
suggested a idea to find a compromise.
Now you are saying sort is so fast so no worth to add more logics
to avoid the overhead?
Then, please just drop that if condition part and instead, sort
it unconditionally.
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
@ 2017-05-02 5:02 ` Minchan Kim
0 siblings, 0 replies; 61+ messages in thread
From: Minchan Kim @ 2017-05-02 5:02 UTC (permalink / raw)
To: Huang, Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Hugh Dickins, Shaohua Li,
Rik van Riel
On Fri, Apr 28, 2017 at 09:35:37PM +0800, Huang, Ying wrote:
> In fact, during the test, I found the overhead of sort() is comparable
> with the performance difference of adding likely()/unlikely() to the
> "if" in the function.
Huang,
This discussion is started from your optimization code:
if (nr_swapfiles > 1)
sort();
I don't have such fast machine so cannot test it. However, you added
such optimization code in there so I guess it's *worth* to review so
with spending my time, I pointed out what you are missing and
suggested a idea to find a compromise.
Now you are saying sort is so fast so no worth to add more logics
to avoid the overhead?
Then, please just drop that if condition part and instead, sort
it unconditionally.
--
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>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
2017-05-02 5:02 ` Minchan Kim
@ 2017-05-02 5:35 ` Huang, Ying
-1 siblings, 0 replies; 61+ messages in thread
From: Huang, Ying @ 2017-05-02 5:35 UTC (permalink / raw)
To: Minchan Kim
Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
Shaohua Li, Rik van Riel
Hi, Minchan,
Minchan Kim <minchan@kernel.org> writes:
> On Fri, Apr 28, 2017 at 09:35:37PM +0800, Huang, Ying wrote:
>> In fact, during the test, I found the overhead of sort() is comparable
>> with the performance difference of adding likely()/unlikely() to the
>> "if" in the function.
>
> Huang,
>
> This discussion is started from your optimization code:
>
> if (nr_swapfiles > 1)
> sort();
>
> I don't have such fast machine so cannot test it. However, you added
> such optimization code in there so I guess it's *worth* to review so
> with spending my time, I pointed out what you are missing and
> suggested a idea to find a compromise.
Sorry for wasting your time and Thanks a lot for your review and
suggestion!
When I started talking this with you, I found there is some measurable
overhead of sort(). But later when I done more tests, I found the
measurable overhead is at the same level of likely()/unlikely() compiler
notation. So you help me to find that, Thanks again!
> Now you are saying sort is so fast so no worth to add more logics
> to avoid the overhead?
> Then, please just drop that if condition part and instead, sort
> it unconditionally.
Now, because we found the overhead of sort() is low, I suggest to put
minimal effort to avoid it. Like the original implementation,
if (nr_swapfiles > 1)
sort();
Or, we can make nr_swapfiles more correct as Tim suggested (tracking
the number of the swap devices during swap on/off).
Best Regards,
Huang, Ying
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
@ 2017-05-02 5:35 ` Huang, Ying
0 siblings, 0 replies; 61+ messages in thread
From: Huang, Ying @ 2017-05-02 5:35 UTC (permalink / raw)
To: Minchan Kim
Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
Shaohua Li, Rik van Riel
Hi, Minchan,
Minchan Kim <minchan@kernel.org> writes:
> On Fri, Apr 28, 2017 at 09:35:37PM +0800, Huang, Ying wrote:
>> In fact, during the test, I found the overhead of sort() is comparable
>> with the performance difference of adding likely()/unlikely() to the
>> "if" in the function.
>
> Huang,
>
> This discussion is started from your optimization code:
>
> if (nr_swapfiles > 1)
> sort();
>
> I don't have such fast machine so cannot test it. However, you added
> such optimization code in there so I guess it's *worth* to review so
> with spending my time, I pointed out what you are missing and
> suggested a idea to find a compromise.
Sorry for wasting your time and Thanks a lot for your review and
suggestion!
When I started talking this with you, I found there is some measurable
overhead of sort(). But later when I done more tests, I found the
measurable overhead is at the same level of likely()/unlikely() compiler
notation. So you help me to find that, Thanks again!
> Now you are saying sort is so fast so no worth to add more logics
> to avoid the overhead?
> Then, please just drop that if condition part and instead, sort
> it unconditionally.
Now, because we found the overhead of sort() is low, I suggest to put
minimal effort to avoid it. Like the original implementation,
if (nr_swapfiles > 1)
sort();
Or, we can make nr_swapfiles more correct as Tim suggested (tracking
the number of the swap devices during swap on/off).
Best Regards,
Huang, Ying
--
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>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
2017-05-02 5:35 ` Huang, Ying
@ 2017-05-02 5:48 ` Minchan Kim
-1 siblings, 0 replies; 61+ messages in thread
From: Minchan Kim @ 2017-05-02 5:48 UTC (permalink / raw)
To: Huang, Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Hugh Dickins, Shaohua Li,
Rik van Riel
Hi Huang,
On Tue, May 02, 2017 at 01:35:24PM +0800, Huang, Ying wrote:
> Hi, Minchan,
>
> Minchan Kim <minchan@kernel.org> writes:
>
> > On Fri, Apr 28, 2017 at 09:35:37PM +0800, Huang, Ying wrote:
> >> In fact, during the test, I found the overhead of sort() is comparable
> >> with the performance difference of adding likely()/unlikely() to the
> >> "if" in the function.
> >
> > Huang,
> >
> > This discussion is started from your optimization code:
> >
> > if (nr_swapfiles > 1)
> > sort();
> >
> > I don't have such fast machine so cannot test it. However, you added
> > such optimization code in there so I guess it's *worth* to review so
> > with spending my time, I pointed out what you are missing and
> > suggested a idea to find a compromise.
>
> Sorry for wasting your time and Thanks a lot for your review and
> suggestion!
>
> When I started talking this with you, I found there is some measurable
> overhead of sort(). But later when I done more tests, I found the
> measurable overhead is at the same level of likely()/unlikely() compiler
> notation. So you help me to find that, Thanks again!
>
> > Now you are saying sort is so fast so no worth to add more logics
> > to avoid the overhead?
> > Then, please just drop that if condition part and instead, sort
> > it unconditionally.
>
> Now, because we found the overhead of sort() is low, I suggest to put
> minimal effort to avoid it. Like the original implementation,
>
> if (nr_swapfiles > 1)
> sort();
It might confuse someone in future and would make him/her send a patch
to fix like we discussed. If the logic is not clear and doesn't have
measureable overhead, just leave it which is more simple/clear.
>
> Or, we can make nr_swapfiles more correct as Tim suggested (tracking
> the number of the swap devices during swap on/off).
It might be better option but it's still hard to justify the patch
because you said it's hard to measure. Such optimiztion patch should
be from numbers.
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
@ 2017-05-02 5:48 ` Minchan Kim
0 siblings, 0 replies; 61+ messages in thread
From: Minchan Kim @ 2017-05-02 5:48 UTC (permalink / raw)
To: Huang, Ying
Cc: Andrew Morton, linux-mm, linux-kernel, Hugh Dickins, Shaohua Li,
Rik van Riel
Hi Huang,
On Tue, May 02, 2017 at 01:35:24PM +0800, Huang, Ying wrote:
> Hi, Minchan,
>
> Minchan Kim <minchan@kernel.org> writes:
>
> > On Fri, Apr 28, 2017 at 09:35:37PM +0800, Huang, Ying wrote:
> >> In fact, during the test, I found the overhead of sort() is comparable
> >> with the performance difference of adding likely()/unlikely() to the
> >> "if" in the function.
> >
> > Huang,
> >
> > This discussion is started from your optimization code:
> >
> > if (nr_swapfiles > 1)
> > sort();
> >
> > I don't have such fast machine so cannot test it. However, you added
> > such optimization code in there so I guess it's *worth* to review so
> > with spending my time, I pointed out what you are missing and
> > suggested a idea to find a compromise.
>
> Sorry for wasting your time and Thanks a lot for your review and
> suggestion!
>
> When I started talking this with you, I found there is some measurable
> overhead of sort(). But later when I done more tests, I found the
> measurable overhead is at the same level of likely()/unlikely() compiler
> notation. So you help me to find that, Thanks again!
>
> > Now you are saying sort is so fast so no worth to add more logics
> > to avoid the overhead?
> > Then, please just drop that if condition part and instead, sort
> > it unconditionally.
>
> Now, because we found the overhead of sort() is low, I suggest to put
> minimal effort to avoid it. Like the original implementation,
>
> if (nr_swapfiles > 1)
> sort();
It might confuse someone in future and would make him/her send a patch
to fix like we discussed. If the logic is not clear and doesn't have
measureable overhead, just leave it which is more simple/clear.
>
> Or, we can make nr_swapfiles more correct as Tim suggested (tracking
> the number of the swap devices during swap on/off).
It might be better option but it's still hard to justify the patch
because you said it's hard to measure. Such optimiztion patch should
be from numbers.
--
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>
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
2017-05-02 5:48 ` Minchan Kim
@ 2017-05-02 6:08 ` Huang, Ying
-1 siblings, 0 replies; 61+ messages in thread
From: Huang, Ying @ 2017-05-02 6:08 UTC (permalink / raw)
To: Minchan Kim
Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
Shaohua Li, Rik van Riel
Minchan Kim <minchan@kernel.org> writes:
> Hi Huang,
>
> On Tue, May 02, 2017 at 01:35:24PM +0800, Huang, Ying wrote:
>> Hi, Minchan,
>>
>> Minchan Kim <minchan@kernel.org> writes:
>>
>> > On Fri, Apr 28, 2017 at 09:35:37PM +0800, Huang, Ying wrote:
>> >> In fact, during the test, I found the overhead of sort() is comparable
>> >> with the performance difference of adding likely()/unlikely() to the
>> >> "if" in the function.
>> >
>> > Huang,
>> >
>> > This discussion is started from your optimization code:
>> >
>> > if (nr_swapfiles > 1)
>> > sort();
>> >
>> > I don't have such fast machine so cannot test it. However, you added
>> > such optimization code in there so I guess it's *worth* to review so
>> > with spending my time, I pointed out what you are missing and
>> > suggested a idea to find a compromise.
>>
>> Sorry for wasting your time and Thanks a lot for your review and
>> suggestion!
>>
>> When I started talking this with you, I found there is some measurable
>> overhead of sort(). But later when I done more tests, I found the
>> measurable overhead is at the same level of likely()/unlikely() compiler
>> notation. So you help me to find that, Thanks again!
>>
>> > Now you are saying sort is so fast so no worth to add more logics
>> > to avoid the overhead?
>> > Then, please just drop that if condition part and instead, sort
>> > it unconditionally.
>>
>> Now, because we found the overhead of sort() is low, I suggest to put
>> minimal effort to avoid it. Like the original implementation,
>>
>> if (nr_swapfiles > 1)
>> sort();
>
> It might confuse someone in future and would make him/her send a patch
> to fix like we discussed. If the logic is not clear and doesn't have
> measureable overhead, just leave it which is more simple/clear.
Because the added code is minimal and cheap, I tend to keep it and add
some comments to avoid confusion. For example,
/*
* Although nr_swapfiles isn't absolute correct, but the overhead of sort()
* is so low that it isn't necessary to optimize further.
*/
>>
>> Or, we can make nr_swapfiles more correct as Tim suggested (tracking
>> the number of the swap devices during swap on/off).
>
> It might be better option but it's still hard to justify the patch
> because you said it's hard to measure. Such optimiztion patch should
> be from numbers.
OK.
Best Regards,
Huang, Ying
^ permalink raw reply [flat|nested] 61+ messages in thread
* Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
@ 2017-05-02 6:08 ` Huang, Ying
0 siblings, 0 replies; 61+ messages in thread
From: Huang, Ying @ 2017-05-02 6:08 UTC (permalink / raw)
To: Minchan Kim
Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
Shaohua Li, Rik van Riel
Minchan Kim <minchan@kernel.org> writes:
> Hi Huang,
>
> On Tue, May 02, 2017 at 01:35:24PM +0800, Huang, Ying wrote:
>> Hi, Minchan,
>>
>> Minchan Kim <minchan@kernel.org> writes:
>>
>> > On Fri, Apr 28, 2017 at 09:35:37PM +0800, Huang, Ying wrote:
>> >> In fact, during the test, I found the overhead of sort() is comparable
>> >> with the performance difference of adding likely()/unlikely() to the
>> >> "if" in the function.
>> >
>> > Huang,
>> >
>> > This discussion is started from your optimization code:
>> >
>> > if (nr_swapfiles > 1)
>> > sort();
>> >
>> > I don't have such fast machine so cannot test it. However, you added
>> > such optimization code in there so I guess it's *worth* to review so
>> > with spending my time, I pointed out what you are missing and
>> > suggested a idea to find a compromise.
>>
>> Sorry for wasting your time and Thanks a lot for your review and
>> suggestion!
>>
>> When I started talking this with you, I found there is some measurable
>> overhead of sort(). But later when I done more tests, I found the
>> measurable overhead is at the same level of likely()/unlikely() compiler
>> notation. So you help me to find that, Thanks again!
>>
>> > Now you are saying sort is so fast so no worth to add more logics
>> > to avoid the overhead?
>> > Then, please just drop that if condition part and instead, sort
>> > it unconditionally.
>>
>> Now, because we found the overhead of sort() is low, I suggest to put
>> minimal effort to avoid it. Like the original implementation,
>>
>> if (nr_swapfiles > 1)
>> sort();
>
> It might confuse someone in future and would make him/her send a patch
> to fix like we discussed. If the logic is not clear and doesn't have
> measureable overhead, just leave it which is more simple/clear.
Because the added code is minimal and cheap, I tend to keep it and add
some comments to avoid confusion. For example,
/*
* Although nr_swapfiles isn't absolute correct, but the overhead of sort()
* is so low that it isn't necessary to optimize further.
*/
>>
>> Or, we can make nr_swapfiles more correct as Tim suggested (tracking
>> the number of the swap devices during swap on/off).
>
> It might be better option but it's still hard to justify the patch
> because you said it's hard to measure. Such optimiztion patch should
> be from numbers.
OK.
Best Regards,
Huang, Ying
--
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>
^ permalink raw reply [flat|nested] 61+ messages in thread