Re: [PATCH -mm -v2] mm, swap: Sort swap entries before free
Rik van Rielwrites: > On Wed, 2017-04-05 at 15:10 +0800, Huang, Ying wrote: >> 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. > > That makes a lot of sense. > >> @@ -1075,6 +1083,8 @@ void swapcache_free_entries(swp_entry_t >> *entries, int n) >> >> prev = NULL; >> p = NULL; >> +if (nr_swapfiles > 1) >> +sort(entries, n, sizeof(entries[0]), swp_entry_cmp, >> NULL); > > But it really wants a comment in the code, so people > reading the code a few years from now can see why > we are sorting things we are about to free. > > Maybe something like: > /* Sort swap entries by swap device, so each lock is only taken > once. */ Good suggestion! I will add it in the next version. Best Regards, Huang, Ying >> for (i = 0; i < n; ++i) { >> p = swap_info_get_cont(entries[i], prev); >> if (p)
Re: [PATCH -mm -v2] mm, swap: Sort swap entries before free
Rik van Riel writes: > On Wed, 2017-04-05 at 15:10 +0800, Huang, Ying wrote: >> 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. > > That makes a lot of sense. > >> @@ -1075,6 +1083,8 @@ void swapcache_free_entries(swp_entry_t >> *entries, int n) >> >> prev = NULL; >> p = NULL; >> +if (nr_swapfiles > 1) >> +sort(entries, n, sizeof(entries[0]), swp_entry_cmp, >> NULL); > > But it really wants a comment in the code, so people > reading the code a few years from now can see why > we are sorting things we are about to free. > > Maybe something like: > /* Sort swap entries by swap device, so each lock is only taken > once. */ Good suggestion! I will add it in the next version. Best Regards, Huang, Ying >> for (i = 0; i < n; ++i) { >> p = swap_info_get_cont(entries[i], prev); >> if (p)
Re: [PATCH -mm -v2] mm, swap: Sort swap entries before free
On Wed, 2017-04-05 at 15:10 +0800, Huang, Ying wrote: > 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. That makes a lot of sense. > @@ -1075,6 +1083,8 @@ void swapcache_free_entries(swp_entry_t > *entries, int n) > > prev = NULL; > p = NULL; > + if (nr_swapfiles > 1) > + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, > NULL); But it really wants a comment in the code, so people reading the code a few years from now can see why we are sorting things we are about to free. Maybe something like: /* Sort swap entries by swap device, so each lock is only taken once. */ > for (i = 0; i < n; ++i) { > p = swap_info_get_cont(entries[i], prev); > if (p) -- All rights reversed signature.asc Description: This is a digitally signed message part
Re: [PATCH -mm -v2] mm, swap: Sort swap entries before free
On Wed, 2017-04-05 at 15:10 +0800, Huang, Ying wrote: > 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. That makes a lot of sense. > @@ -1075,6 +1083,8 @@ void swapcache_free_entries(swp_entry_t > *entries, int n) > > prev = NULL; > p = NULL; > + if (nr_swapfiles > 1) > + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, > NULL); But it really wants a comment in the code, so people reading the code a few years from now can see why we are sorting things we are about to free. Maybe something like: /* Sort swap entries by swap device, so each lock is only taken once. */ > for (i = 0; i < n; ++i) { > p = swap_info_get_cont(entries[i], prev); > if (p) -- All rights reversed signature.asc Description: This is a digitally signed message part
[PATCH -mm -v2] mm, swap: Sort swap entries before free
From: Huang YingTo 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. Test the patch via measuring the run time of swap_cache_free_entries() during the exit phase of the applications use much swap space. The results shows that the average run time of swap_cache_free_entries() reduced about 20% after applying the patch. Signed-off-by: Huang Ying Acked-by: Tim Chen Cc: Hugh Dickins Cc: Shaohua Li Cc: Minchan Kim Cc: Rik van Riel v2: - Avoid sort swap entries if there is only one swap device. --- mm/swapfile.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/mm/swapfile.c b/mm/swapfile.c index 90054f3c2cdc..b91b0b0088c5 100644 --- a/mm/swapfile.c +++ b/mm/swapfile.c @@ -37,6 +37,7 @@ #include #include #include +#include #include #include @@ -1065,6 +1066,13 @@ 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 (long)(swp_type(*e1) - swp_type(*e2)); +} + void swapcache_free_entries(swp_entry_t *entries, int n) { struct swap_info_struct *p, *prev; @@ -1075,6 +1083,8 @@ void swapcache_free_entries(swp_entry_t *entries, int n) prev = NULL; p = NULL; + 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) -- 2.11.0
[PATCH -mm -v2] mm, swap: Sort swap entries before free
From: Huang Ying 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. Test the patch via measuring the run time of swap_cache_free_entries() during the exit phase of the applications use much swap space. The results shows that the average run time of swap_cache_free_entries() reduced about 20% after applying the patch. Signed-off-by: Huang Ying Acked-by: Tim Chen Cc: Hugh Dickins Cc: Shaohua Li Cc: Minchan Kim Cc: Rik van Riel v2: - Avoid sort swap entries if there is only one swap device. --- mm/swapfile.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/mm/swapfile.c b/mm/swapfile.c index 90054f3c2cdc..b91b0b0088c5 100644 --- a/mm/swapfile.c +++ b/mm/swapfile.c @@ -37,6 +37,7 @@ #include #include #include +#include #include #include @@ -1065,6 +1066,13 @@ 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 (long)(swp_type(*e1) - swp_type(*e2)); +} + void swapcache_free_entries(swp_entry_t *entries, int n) { struct swap_info_struct *p, *prev; @@ -1075,6 +1083,8 @@ void swapcache_free_entries(swp_entry_t *entries, int n) prev = NULL; p = NULL; + 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) -- 2.11.0