-by: "Huang, Ying"
Suggested-by: Dave Hansen
Reviewed-by: Daniel Jordan
Cc: Michal Hocko
Cc: Johannes Weiner
Cc: Shaohua Li
Cc: Hugh Dickins
Cc: Minchan Kim
Cc: Rik van Riel
Cc: Dan Williams
---
mm/swapfile.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/mm/
The part of __swap_entry_free() with lock held is separated into a new
function __swap_entry_free_locked(). Because we want to reuse that
piece of code in some other places.
Just mechanical code refactoring, there is no any functional change in
this function.
Signed-off-by: "Huang,
via merging huge/normal
code path/functions if possible.
One concern is that this may cause code size to dilate when
!CONFIG_TRANSPARENT_HUGEPAGE. The data shows that most refactoring
will only cause quite slight code size increase.
Best Regards,
Huang, Ying
out by Daniel, it is better to use "swap_count(map[i])"
here, because it works for "map[i] == 0" case too.
And this makes the implementation more consistent between normal and
huge swap entry.
Signed-off-by: "Huang, Ying"
Suggested-and-reviewed-by: Daniel Jordan
Cc:
via merging huge/normal
code path/functions if possible.
One concern is that this may cause code size to dilate when
!CONFIG_TRANSPARENT_HUGEPAGE. The data shows that most refactoring
will only cause quite slight code size increase.
Best Regards,
Huang, Ying
out by Daniel, it is better to use "swap_count(map[i])"
here, because it works for "map[i] == 0" case too.
And this makes the implementation more consistent between normal and
huge swap entry.
Signed-off-by: "Huang, Ying"
Suggested-and-reviewed-by: Daniel Jordan
Cc:
To improve the code readability.
Signed-off-by: "Huang, Ying"
Suggested-by: Dave Hansen
Reviewed-by: Daniel Jordan
Cc: Michal Hocko
Cc: Johannes Weiner
Cc: Shaohua Li
Cc: Hugh Dickins
Cc: Minchan Kim
Cc: Rik van Riel
Cc: Dan Williams
---
mm/swapfile.c | 6 ++
1 file
=n.
Signed-off-by: "Huang, Ying"
Suggested-by: Dave Hansen
Reviewed-by: Daniel Jordan
Cc: Michal Hocko
Cc: Johannes Weiner
Cc: Shaohua Li
Cc: Hugh Dickins
Cc: Minchan Kim
Cc: Rik van Riel
Cc: Dan Williams
---
mm/swapf
-by: "Huang, Ying"
Suggested-by: Dave Hansen
Reviewed-by: Daniel Jordan
Cc: Michal Hocko
Cc: Johannes Weiner
Cc: Shaohua Li
Cc: Hugh Dickins
Cc: Minchan Kim
Cc: Rik van Riel
Cc: Dan Williams
---
mm/swapfile.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/mm/
The part of __swap_entry_free() with lock held is separated into a new
function __swap_entry_free_locked(). Because we want to reuse that
piece of code in some other places.
Just mechanical code refactoring, there is no any functional change in
this function.
Signed-off-by: "Huang,
it is a public
function with a stub implementation for CONFIG_THP_SWAP=n in swap.h.
Signed-off-by: "Huang, Ying"
Suggested-by: Dave Hansen
Reviewed-by: Daniel Jordan
Cc: Michal Hocko
Cc: Johannes Weiner
Cc: Shaohua Li
Cc: Hugh Dickins
Cc: Minchan Kim
Cc: Rik van Riel
Cc: Dan William
filename
base 242152028 340 2658367d7 mm/swapfile.o
head 241232004 340 264676763 mm/swapfile.o
Signed-off-by: "Huang, Ying"
Cc: Daniel Jordan
Cc: Dave Hansen
Cc: Michal Hocko
Cc: Johannes Weiner
Cc: Shaohua Li
Cc: Hugh Dickins
Cc: Minchan Kim
it is a public
function with a stub implementation for CONFIG_THP_SWAP=n in swap.h.
Signed-off-by: "Huang, Ying"
Suggested-by: Dave Hansen
Reviewed-by: Daniel Jordan
Cc: Michal Hocko
Cc: Johannes Weiner
Cc: Shaohua Li
Cc: Hugh Dickins
Cc: Minchan Kim
Cc: Rik van Riel
Cc: Dan William
filename
base 242152028 340 2658367d7 mm/swapfile.o
head 241232004 340 264676763 mm/swapfile.o
Signed-off-by: "Huang, Ying"
Cc: Daniel Jordan
Cc: Dave Hansen
Cc: Michal Hocko
Cc: Johannes Weiner
Cc: Shaohua Li
Cc: Hugh Dickins
Cc: Minchan Kim
Dave Hansen writes:
> On 07/17/2018 08:25 PM, Huang, Ying wrote:
>>> Seriously, though, does it hurt us to add a comment or two to say
>>> something like:
>>>
>>> /*
>>> * Should not even be attempting cluster allocations when
>&
Dave Hansen writes:
> On 07/17/2018 08:25 PM, Huang, Ying wrote:
>>> Seriously, though, does it hurt us to add a comment or two to say
>>> something like:
>>>
>>> /*
>>> * Should not even be attempting cluster allocations when
>&
velopment. Do we really need comments here?
I will try to add more comments for other places in code regardless this
one.
Best Regards,
Huang, Ying
velopment. Do we really need comments here?
I will try to add more comments for other places in code regardless this
one.
Best Regards,
Huang, Ying
Dave Hansen writes:
> On 07/16/2018 05:55 PM, Huang, Ying wrote:
>> +/*
>> + * For non-HDD swap devices, the fine grained cluster lock is used to
>> + * protect si->swap_map. But cluster and cluster locks isn't
>> + * available for HDD, so coarse grained
Dave Hansen writes:
> On 07/16/2018 05:55 PM, Huang, Ying wrote:
>> +/*
>> + * For non-HDD swap devices, the fine grained cluster lock is used to
>> + * protect si->swap_map. But cluster and cluster locks isn't
>> + * available for HDD, so coarse grained
Daniel Jordan writes:
> On Tue, Jul 17, 2018 at 08:55:49AM +0800, Huang, Ying wrote:
>> This patchset is based on 2018-07-13 head of mmotm tree.
>
> Looks good.
>
> Still think patch 7 would be easier to review if split into two logical
> changes. Either way, th
Daniel Jordan writes:
> On Tue, Jul 17, 2018 at 08:55:49AM +0800, Huang, Ying wrote:
>> This patchset is based on 2018-07-13 head of mmotm tree.
>
> Looks good.
>
> Still think patch 7 would be easier to review if split into two logical
> changes. Either way, th
Dave Hansen writes:
> On 07/16/2018 05:55 PM, Huang, Ying wrote:
>> text data bss dec hex filename
>> base: 24215 2028 340 2658367d7 mm/swapfile.o
>> unified: 245772028 340 269456941 mm/swapfi
Dave Hansen writes:
> On 07/16/2018 05:55 PM, Huang, Ying wrote:
>> text data bss dec hex filename
>> base: 24215 2028 340 2658367d7 mm/swapfile.o
>> unified: 245772028 340 269456941 mm/swapfi
=n.
Signed-off-by: "Huang, Ying"
Suggested-by: Dave Hansen
Cc: Michal Hocko
Cc: Johannes Weiner
Cc: Shaohua Li
Cc: Hugh Dickins
Cc: Minchan Kim
Cc: Rik van Riel
Cc: Daniel Jordan
Cc: Dan Williams
---
mm/swapfile.c | 83 ++---
The part of __swap_entry_free() with lock held is separated into a new
function __swap_entry_free_locked(). Because we want to reuse that
piece of code in some other places.
Just mechanical code refactoring, there is no any functional change in
this function.
Signed-off-by: "Huang, Ying
=n.
Signed-off-by: "Huang, Ying"
Suggested-by: Dave Hansen
Cc: Michal Hocko
Cc: Johannes Weiner
Cc: Shaohua Li
Cc: Hugh Dickins
Cc: Minchan Kim
Cc: Rik van Riel
Cc: Daniel Jordan
Cc: Dan Williams
---
mm/swapfile.c | 83 ++---
The part of __swap_entry_free() with lock held is separated into a new
function __swap_entry_free_locked(). Because we want to reuse that
piece of code in some other places.
Just mechanical code refactoring, there is no any functional change in
this function.
Signed-off-by: "Huang, Ying
-by: "Huang, Ying"
Suggested-by: Dave Hansen
Cc: Michal Hocko
Cc: Johannes Weiner
Cc: Shaohua Li
Cc: Hugh Dickins
Cc: Minchan Kim
Cc: Rik van Riel
Cc: Daniel Jordan
Cc: Dan Williams
---
mm/swapfile.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/mm/swapf
via merging huge/normal
code path/functions if possible.
One concern is that this may cause code size to dilate when
!CONFIG_TRANSPARENT_HUGEPAGE. The data shows that most refactoring
will only cause quite slight code size increase.
Best Regards,
Huang, Ying
-off-by: "Huang, Ying"
Cc: Dave Hansen
Cc: Michal Hocko
Cc: Johannes Weiner
Cc: Shaohua Li
Cc: Hugh Dickins
Cc: Minchan Kim
Cc: Rik van Riel
Cc: Daniel Jordan
Cc: Dan Williams
---
mm/swapfile.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff
-by: "Huang, Ying"
Suggested-by: Dave Hansen
Cc: Michal Hocko
Cc: Johannes Weiner
Cc: Shaohua Li
Cc: Hugh Dickins
Cc: Minchan Kim
Cc: Rik van Riel
Cc: Daniel Jordan
Cc: Dan Williams
---
mm/swapfile.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/mm/swapf
via merging huge/normal
code path/functions if possible.
One concern is that this may cause code size to dilate when
!CONFIG_TRANSPARENT_HUGEPAGE. The data shows that most refactoring
will only cause quite slight code size increase.
Best Regards,
Huang, Ying
-off-by: "Huang, Ying"
Cc: Dave Hansen
Cc: Michal Hocko
Cc: Johannes Weiner
Cc: Shaohua Li
Cc: Hugh Dickins
Cc: Minchan Kim
Cc: Rik van Riel
Cc: Daniel Jordan
Cc: Dan Williams
---
mm/swapfile.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff
it is a public
function with a stub implementation for CONFIG_THP_SWAP=n in swap.h.
Signed-off-by: "Huang, Ying"
Suggested-by: Dave Hansen
Cc: Michal Hocko
Cc: Johannes Weiner
Cc: Shaohua Li
Cc: Hugh Dickins
Cc: Minchan Kim
Cc: Rik van Riel
Cc: Daniel Jordan
Cc: Dan Williams
---
mm/swapf
out by Daniel, it is better to use "swap_count(map[i])"
here, because it works for "map[i] == 0" case too.
And this makes the implementation more consistent between normal and
huge swap entry.
Signed-off-by: "Huang, Ying"
Suggested-by: Daniel Jordan
Cc: Dave Hansen
To improve the code readability.
Signed-off-by: "Huang, Ying"
Suggested-by: Dave Hansen
Cc: Michal Hocko
Cc: Johannes Weiner
Cc: Shaohua Li
Cc: Hugh Dickins
Cc: Minchan Kim
Cc: Rik van Riel
Cc: Daniel Jordan
Cc: Dan Williams
---
mm/swapfile.c | 6 ++
1 file changed, 6
it is a public
function with a stub implementation for CONFIG_THP_SWAP=n in swap.h.
Signed-off-by: "Huang, Ying"
Suggested-by: Dave Hansen
Cc: Michal Hocko
Cc: Johannes Weiner
Cc: Shaohua Li
Cc: Hugh Dickins
Cc: Minchan Kim
Cc: Rik van Riel
Cc: Daniel Jordan
Cc: Dan Williams
---
mm/swapf
out by Daniel, it is better to use "swap_count(map[i])"
here, because it works for "map[i] == 0" case too.
And this makes the implementation more consistent between normal and
huge swap entry.
Signed-off-by: "Huang, Ying"
Suggested-by: Daniel Jordan
Cc: Dave Hansen
To improve the code readability.
Signed-off-by: "Huang, Ying"
Suggested-by: Dave Hansen
Cc: Michal Hocko
Cc: Johannes Weiner
Cc: Shaohua Li
Cc: Hugh Dickins
Cc: Minchan Kim
Cc: Rik van Riel
Cc: Daniel Jordan
Cc: Dan Williams
---
mm/swapfile.c | 6 ++
1 file changed, 6
via merging huge/normal
code path/functions if possible.
One concern is that this may cause code size to dilate when
!CONFIG_TRANSPARENT_HUGEPAGE. The data shows that most refactoring
will only cause quite slight code size increase.
Best Regards,
Huang, Ying
via merging huge/normal
code path/functions if possible.
One concern is that this may cause code size to dilate when
!CONFIG_TRANSPARENT_HUGEPAGE. The data shows that most refactoring
will only cause quite slight code size increase.
Best Regards,
Huang, Ying
Daniel Jordan writes:
> On Fri, Jul 13, 2018 at 07:36:36AM +0800, Huang, Ying wrote:
>> From: Huang Ying
>>
>> In this patch, locking related code is shared between huge/normal code
>> path in put_swap_page() to reduce code duplication. And `free_entries
>&g
Daniel Jordan writes:
> On Fri, Jul 13, 2018 at 07:36:36AM +0800, Huang, Ying wrote:
>> From: Huang Ying
>>
>> In this patch, locking related code is shared between huge/normal code
>> path in put_swap_page() to reduce code duplication. And `free_entries
>&g
Daniel Jordan writes:
> On Fri, Jul 13, 2018 at 07:36:33AM +0800, Huang, Ying wrote:
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index 75c84aa763a3..160f78072667 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -270,7 +270,10 @@ static in
Daniel Jordan writes:
> On Fri, Jul 13, 2018 at 07:36:33AM +0800, Huang, Ying wrote:
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index 75c84aa763a3..160f78072667 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -270,7 +270,10 @@ static in
is is described in comments of struct
swap_cluster_info. si->lock is used to protect other fields of si. If
two locks need to be held, hold si->lock first. This is for non-HDD.
For HDD, there are no cluster, so si->lock is used to protect
si->swap_map[].
Best Regards,
Huang, Ying
is is described in comments of struct
swap_cluster_info. si->lock is used to protect other fields of si. If
two locks need to be held, hold si->lock first. This is for non-HDD.
For HDD, there are no cluster, so si->lock is used to protect
si->swap_map[].
Best Regards,
Huang, Ying
From: Huang Ying
CONFIG_THP_SWAP should depend on CONFIG_SWAP, because it's
unreasonable to optimize swapping for THP (Transparent Huge Page)
without basic swapping support.
In original code, when CONFIG_SWAP=n and CONFIG_THP_SWAP=y,
split_swap_cluster() will not be built because
From: Huang Ying
CONFIG_THP_SWAP should depend on CONFIG_SWAP, because it's
unreasonable to optimize swapping for THP (Transparent Huge Page)
without basic swapping support.
In original code, when CONFIG_SWAP=n and CONFIG_THP_SWAP=y,
split_swap_cluster() will not be built because
From: Huang Ying
The part of __swap_entry_free() with lock held is separated into a new
function __swap_entry_free_locked(). Because we want to reuse that
piece of code in some other places.
Just mechanical code refactoring, there is no any functional change in
this function.
Signed-off
From: Huang Ying
The part of __swap_entry_free() with lock held is separated into a new
function __swap_entry_free_locked(). Because we want to reuse that
piece of code in some other places.
Just mechanical code refactoring, there is no any functional change in
this function.
Signed-off
From: Huang Ying
As suggested by Dave, we should unify the code path for normal and
huge swap support if possible to avoid duplicated code, bugs, etc. and
make it easier to review code.
In this patch, the normal/huge code path in swap_page_trans_huge_swapped()
is unified, the added and removed
From: Huang Ying
In this patch, the normal/huge code path in put_swap_page() and
several helper functions are unified to avoid duplicated code, bugs,
etc. and make it easier to review the code.
The removed lines are more than added lines. And the binary size is
kept exactly same when
From: Huang Ying
In mm/swapfile.c, THP (Transparent Huge Page) swap specific code is
enclosed by #ifdef CONFIG_THP_SWAP/#endif to avoid code dilating when
THP isn't enabled. But #ifdef/#endif in .c file hurt the code
readability, so Dave suggested to use IS_ENABLED(CONFIG_THP_SWAP)
instead
From: Huang Ying
To improve the code readability.
Signed-off-by: "Huang, Ying"
Suggested-by: Dave Hansen
Cc: Michal Hocko
Cc: Johannes Weiner
Cc: Shaohua Li
Cc: Hugh Dickins
Cc: Minchan Kim
Cc: Rik van Riel
Cc: Daniel Jordan
Cc: Dan Williams
---
mm/swapfile.c | 6
From: Huang Ying
In mm/swapfile.c, THP (Transparent Huge Page) swap specific code is
enclosed by #ifdef CONFIG_THP_SWAP/#endif to avoid code dilating when
THP isn't enabled. But #ifdef/#endif in .c file hurt the code
readability, so Dave suggested to use IS_ENABLED(CONFIG_THP_SWAP)
instead
From: Huang Ying
To improve the code readability.
Signed-off-by: "Huang, Ying"
Suggested-by: Dave Hansen
Cc: Michal Hocko
Cc: Johannes Weiner
Cc: Shaohua Li
Cc: Hugh Dickins
Cc: Minchan Kim
Cc: Rik van Riel
Cc: Daniel Jordan
Cc: Dan Williams
---
mm/swapfile.c | 6
From: Huang Ying
As suggested by Dave, we should unify the code path for normal and
huge swap support if possible to avoid duplicated code, bugs, etc. and
make it easier to review code.
In this patch, the normal/huge code path in swap_page_trans_huge_swapped()
is unified, the added and removed
From: Huang Ying
In this patch, the normal/huge code path in put_swap_page() and
several helper functions are unified to avoid duplicated code, bugs,
etc. and make it easier to review the code.
The removed lines are more than added lines. And the binary size is
kept exactly same when
From: Huang Ying
In this patch, locking related code is shared between huge/normal code
path in put_swap_page() to reduce code duplication. And `free_entries
== 0` case is merged into more general `free_entries !=
SWAPFILE_CLUSTER` case, because the new locking method makes it easy.
The added
via merging huge/normal
code path/functions if possible.
One concern is that this may cause code size to dilate when
!CONFIG_TRANSPARENT_HUGEPAGE. The data shows that most refactoring
will only cause quite slight code size increase.
Best Regards,
Huang, Ying
From: Huang Ying
In this patch, locking related code is shared between huge/normal code
path in put_swap_page() to reduce code duplication. And `free_entries
== 0` case is merged into more general `free_entries !=
SWAPFILE_CLUSTER` case, because the new locking method makes it easy.
The added
via merging huge/normal
code path/functions if possible.
One concern is that this may cause code size to dilate when
!CONFIG_TRANSPARENT_HUGEPAGE. The data shows that most refactoring
will only cause quite slight code size increase.
Best Regards,
Huang, Ying
Dave Hansen writes:
> On 07/10/2018 12:13 AM, Huang, Ying wrote:
>> Dave Hansen writes:
>>> The code non-resuse was, and continues to be, IMNHO, one of the largest
>>> sources of bugs with the original THP implementation. It might be
>>> infeasible
Dave Hansen writes:
> On 07/10/2018 12:13 AM, Huang, Ying wrote:
>> Dave Hansen writes:
>>> The code non-resuse was, and continues to be, IMNHO, one of the largest
>>> sources of bugs with the original THP implementation. It might be
>>> infeasible
Dave Hansen writes:
> On 07/09/2018 11:53 PM, Huang, Ying wrote:
>> Dave Hansen writes:
>>>> +#ifdef CONFIG_THP_SWAP
>>>> +static inline int cluster_swapcount(struct swap_cluster_info *ci)
>>>> +{
>>>> + if (!ci || !cluster_is_
Dave Hansen writes:
> On 07/09/2018 11:53 PM, Huang, Ying wrote:
>> Dave Hansen writes:
>>>> +#ifdef CONFIG_THP_SWAP
>>>> +static inline int cluster_swapcount(struct swap_cluster_info *ci)
>>>> +{
>>>> + if (!ci || !cluster_is_
ine patch, adding arguments
>>> to a existing function would not be something I'd try to include in the
>>> patch. I'd break it out separately unless absolutely necessary.
>>
>> You mean add another patch, which only adds arguments to the function,
>> but not change the body of the function?
>
> Yes. Or, just add the non-THP-swap version first.
OK, will do this.
Best Regards,
Huang, Ying
ine patch, adding arguments
>>> to a existing function would not be something I'd try to include in the
>>> patch. I'd break it out separately unless absolutely necessary.
>>
>> You mean add another patch, which only adds arguments to the function,
>> but not change the body of the function?
>
> Yes. Or, just add the non-THP-swap version first.
OK, will do this.
Best Regards,
Huang, Ying
Daniel Jordan writes:
> On Fri, Jun 22, 2018 at 11:51:44AM +0800, Huang, Ying wrote:
>> Because there is no way to prevent a huge swap cluster from being
>> split except when it has SWAP_HAS_CACHE flag set.
>
> What about making get_mctgt_type_thp take the cluster lock?
Daniel Jordan writes:
> On Fri, Jun 22, 2018 at 11:51:44AM +0800, Huang, Ying wrote:
>> Because there is no way to prevent a huge swap cluster from being
>> split except when it has SWAP_HAS_CACHE flag set.
>
> What about making get_mctgt_type_thp take the cluster lock?
here code can not be shared, call that out in the changelog.
Will do that if we resolve the code size concern.
> This is a *really* hard-to-review set at the moment. Doing those things
> will make it much easier to review and hopefully give us more
> maintainable code going forward.
>
> My apologies for not having done this review sooner.
Thanks a lot for your comments!
Best Regards,
Huang, Ying
here code can not be shared, call that out in the changelog.
Will do that if we resolve the code size concern.
> This is a *really* hard-to-review set at the moment. Doing those things
> will make it much easier to review and hopefully give us more
> maintainable code going forward.
>
> My apologies for not having done this review sooner.
Thanks a lot for your comments!
Best Regards,
Huang, Ying
cluster_clear_huge(ci);
>> }
>
> Also, I'll point out that cluster_swapcount() continues the horrific
> naming of cluster_couunt(), not saying what the count is *of*. The
> return value doesn't help much:
>
> return cluster_count(ci) - SWAPFILE_CLUSTER;
We have page_swapcount() for page, swp_swapcount() for swap entry.
cluster_swapcount() tries to mimic them for swap cluster. But I am not
good at naming in general. What's your suggestion?
Best Regards,
Huang, Ying
cluster_clear_huge(ci);
>> }
>
> Also, I'll point out that cluster_swapcount() continues the horrific
> naming of cluster_couunt(), not saying what the count is *of*. The
> return value doesn't help much:
>
> return cluster_count(ci) - SWAPFILE_CLUSTER;
We have page_swapcount() for page, swp_swapcount() for swap entry.
cluster_swapcount() tries to mimic them for swap cluster. But I am not
good at naming in general. What's your suggestion?
Best Regards,
Huang, Ying
t;> * might occur if a page table entry has got corrupted.
>> */
>> -int swap_duplicate(swp_entry_t entry)
>> +int swap_duplicate(swp_entry_t *entry, bool cluster)
>> {
>> int err = 0;
>>
>> -while (!err && __swap_duplicate(entry, 1) == -ENOMEM)
>> -err = add_swap_count_continuation(entry, GFP_ATOMIC);
>> +if (thp_swap_supported() && cluster)
>> +return __swap_duplicate_cluster(entry, 1);
>> +
>> +while (!err && __swap_duplicate(*entry, 1) == -ENOMEM)
>> +err = add_swap_count_continuation(*entry, GFP_ATOMIC);
>> return err;
>> }
>
> Reading this, I wonder whether this has been refactored as much as
> possible. Both add_swap_count_continuation() and
> __swap_duplciate_cluster() start off with the same get_swap_device() dance.
Yes. There's some duplicated code logic. Will think about how to
improve it.
>> @@ -3558,9 +3660,12 @@ int swap_duplicate(swp_entry_t entry)
>> * -EBUSY means there is a swap cache.
>> * Note: return code is different from swap_duplicate().
>> */
>> -int swapcache_prepare(swp_entry_t entry)
>> +int swapcache_prepare(swp_entry_t entry, bool cluster)
>> {
>> -return __swap_duplicate(entry, SWAP_HAS_CACHE);
>> +if (thp_swap_supported() && cluster)
>> +return __swap_duplicate_cluster(, SWAP_HAS_CACHE);
>> +else
>> +return __swap_duplicate(entry, SWAP_HAS_CACHE);
>> }
>>
>> struct swap_info_struct *swp_swap_info(swp_entry_t entry)
>> @@ -3590,51 +3695,13 @@ pgoff_t __page_file_index(struct page *page)
>> }
>> EXPORT_SYMBOL_GPL(__page_file_index);
>>
>> -/*
>> - * add_swap_count_continuation - called when a swap count is duplicated
>> - * beyond SWAP_MAP_MAX, it allocates a new page and links that to the
>> entry's
>> - * page of the original vmalloc'ed swap_map, to hold the continuation count
>> - * (for that entry and for its neighbouring PAGE_SIZE swap entries). Called
>> - * again when count is duplicated beyond SWAP_MAP_MAX * SWAP_CONT_MAX, etc.
>
> This closes out with a lot of refactoring noise. Any chance that can be
> isolated into another patch?
Sure. Will do that.
Best Regards,
Huang, Ying
t;> * might occur if a page table entry has got corrupted.
>> */
>> -int swap_duplicate(swp_entry_t entry)
>> +int swap_duplicate(swp_entry_t *entry, bool cluster)
>> {
>> int err = 0;
>>
>> -while (!err && __swap_duplicate(entry, 1) == -ENOMEM)
>> -err = add_swap_count_continuation(entry, GFP_ATOMIC);
>> +if (thp_swap_supported() && cluster)
>> +return __swap_duplicate_cluster(entry, 1);
>> +
>> +while (!err && __swap_duplicate(*entry, 1) == -ENOMEM)
>> +err = add_swap_count_continuation(*entry, GFP_ATOMIC);
>> return err;
>> }
>
> Reading this, I wonder whether this has been refactored as much as
> possible. Both add_swap_count_continuation() and
> __swap_duplciate_cluster() start off with the same get_swap_device() dance.
Yes. There's some duplicated code logic. Will think about how to
improve it.
>> @@ -3558,9 +3660,12 @@ int swap_duplicate(swp_entry_t entry)
>> * -EBUSY means there is a swap cache.
>> * Note: return code is different from swap_duplicate().
>> */
>> -int swapcache_prepare(swp_entry_t entry)
>> +int swapcache_prepare(swp_entry_t entry, bool cluster)
>> {
>> -return __swap_duplicate(entry, SWAP_HAS_CACHE);
>> +if (thp_swap_supported() && cluster)
>> +return __swap_duplicate_cluster(, SWAP_HAS_CACHE);
>> +else
>> +return __swap_duplicate(entry, SWAP_HAS_CACHE);
>> }
>>
>> struct swap_info_struct *swp_swap_info(swp_entry_t entry)
>> @@ -3590,51 +3695,13 @@ pgoff_t __page_file_index(struct page *page)
>> }
>> EXPORT_SYMBOL_GPL(__page_file_index);
>>
>> -/*
>> - * add_swap_count_continuation - called when a swap count is duplicated
>> - * beyond SWAP_MAP_MAX, it allocates a new page and links that to the
>> entry's
>> - * page of the original vmalloc'ed swap_map, to hold the continuation count
>> - * (for that entry and for its neighbouring PAGE_SIZE swap entries). Called
>> - * again when count is duplicated beyond SWAP_MAP_MAX * SWAP_CONT_MAX, etc.
>
> This closes out with a lot of refactoring noise. Any chance that can be
> isolated into another patch?
Sure. Will do that.
Best Regards,
Huang, Ying
Dave Hansen writes:
> On 07/09/2018 06:19 PM, Huang, Ying wrote:
>> Dave Hansen writes:
>>
>>>> config THP_SWAP
>>>>def_bool y
>>>> - depends on TRANSPARENT_HUGEPAGE && ARCH_WANTS_THP_SWAP
>>>> + depe
Dave Hansen writes:
> On 07/09/2018 06:19 PM, Huang, Ying wrote:
>> Dave Hansen writes:
>>
>>>> config THP_SWAP
>>>>def_bool y
>>>> - depends on TRANSPARENT_HUGEPAGE && ARCH_WANTS_THP_SWAP
>>>> + depe
-fix. Is there a reason this didn't cause problems
> up to now?
Yes. The original code has some problem in theory, but not in practice
because all code enclosed by
#ifdef CONFIG_THP_SWAP
#endif
are in swapfile.c. But that will be not true in this patchset.
Best Regards,
Huang, Ying
-fix. Is there a reason this didn't cause problems
> up to now?
Yes. The original code has some problem in theory, but not in practice
because all code enclosed by
#ifdef CONFIG_THP_SWAP
#endif
are in swapfile.c. But that will be not true in this patchset.
Best Regards,
Huang, Ying
Dave Hansen writes:
> On 06/21/2018 08:51 PM, Huang, Ying wrote:
>> From: Huang Ying
>>
>> Previously, the PMD swap operations are only enabled for
>> CONFIG_ARCH_ENABLE_THP_MIGRATION. Because they are only used by the
>> THP migration support. We will sup
Dave Hansen writes:
> On 06/21/2018 08:51 PM, Huang, Ying wrote:
>> From: Huang Ying
>>
>> Previously, the PMD swap operations are only enabled for
>> CONFIG_ARCH_ENABLE_THP_MIGRATION. Because they are only used by the
>> THP migration support. We will sup
Dan Williams writes:
> On Thu, Jun 21, 2018 at 8:55 PM Huang, Ying wrote:
>>
>> From: Huang Ying
>>
>> To support to swapin the THP as a whole, we need to create PMD swap
>> mapping during swapout, and maintain PMD swap mapping count. This
>> patch imp
Dan Williams writes:
> On Thu, Jun 21, 2018 at 8:55 PM Huang, Ying wrote:
>>
>> From: Huang Ying
>>
>> To support to swapin the THP as a whole, we need to create PMD swap
>> mapping during swapout, and maintain PMD swap mapping count. This
>> patch imp
Dan Williams writes:
> On Thu, Jun 21, 2018 at 8:55 PM Huang, Ying wrote:
>>
>> From: Huang Ying
>>
>> It's unreasonable to optimize swapping for THP without basic swapping
>> support. And this will cause build errors when THP_SWAP functions are
>> def
Dan Williams writes:
> On Sun, Jul 8, 2018 at 10:40 PM, Huang, Ying wrote:
>> Dan Williams writes:
>>> Would that also allow us to clean up the usage of
>>> CONFIG_ARCH_ENABLE_THP_MIGRATION in fs/proc/task_mmu.c? In other
>>> words, what's the point
Dan Williams writes:
> On Sun, Jul 8, 2018 at 10:40 PM, Huang, Ying wrote:
>> Dan Williams writes:
>>> Would that also allow us to clean up the usage of
>>> CONFIG_ARCH_ENABLE_THP_MIGRATION in fs/proc/task_mmu.c? In other
>>> words, what's the point
Dan Williams writes:
> On Thu, Jun 21, 2018 at 8:55 PM Huang, Ying wrote:
>>
>> From: Huang Ying
>>
>> It's unreasonable to optimize swapping for THP without basic swapping
>> support. And this will cause build errors when THP_SWAP functions are
>> def
Dan Williams writes:
> On Thu, Jun 21, 2018 at 8:55 PM Huang, Ying wrote:
>>
>> From: Huang Ying
>>
>> Previously, the PMD swap operations are only enabled for
>> CONFIG_ARCH_ENABLE_THP_MIGRATION. Because they are only used by the
>> THP migration sup
Dan Williams writes:
> On Thu, Jun 21, 2018 at 8:55 PM Huang, Ying wrote:
>>
>> From: Huang Ying
>>
>> Previously, the PMD swap operations are only enabled for
>> CONFIG_ARCH_ENABLE_THP_MIGRATION. Because they are only used by the
>> THP migration sup
Daniel Jordan writes:
> On Fri, Jun 22, 2018 at 11:51:35AM +0800, Huang, Ying wrote:
>> +static unsigned char swap_free_cluster(struct swap_info_struct *si,
>> + swp_entry_t entry)
> ...
>> +/* Cluster has been split, free each
Daniel Jordan writes:
> On Fri, Jun 22, 2018 at 11:51:35AM +0800, Huang, Ying wrote:
>> +static unsigned char swap_free_cluster(struct swap_info_struct *si,
>> + swp_entry_t entry)
> ...
>> +/* Cluster has been split, free each
Sergey Senozhatsky writes:
> On (07/04/18 10:20), Huang, Ying wrote:
>> > On (06/27/18 21:51), Andrew Morton wrote:
>> >> On Fri, 22 Jun 2018 11:51:30 +0800 "Huang, Ying"
>> >> wrote:
>> >>
>> >> > This is the fi
Sergey Senozhatsky writes:
> On (07/04/18 10:20), Huang, Ying wrote:
>> > On (06/27/18 21:51), Andrew Morton wrote:
>> >> On Fri, 22 Jun 2018 11:51:30 +0800 "Huang, Ying"
>> >> wrote:
>> >>
>> >> > This is the fi
Daniel Jordan writes:
> On Fri, Jun 22, 2018 at 11:51:38AM +0800, Huang, Ying wrote:
>> @@ -411,14 +414,32 @@ struct page *__read_swap_cache_async(swp_entry_t
>> entry, gfp_t gfp_mask,
> ...
>> +if (thp_swap_sup
Daniel Jordan writes:
> On Fri, Jun 22, 2018 at 11:51:38AM +0800, Huang, Ying wrote:
>> @@ -411,14 +414,32 @@ struct page *__read_swap_cache_async(swp_entry_t
>> entry, gfp_t gfp_mask,
> ...
>> +if (thp_swap_sup
Sergey Senozhatsky writes:
> On (06/27/18 21:51), Andrew Morton wrote:
>> On Fri, 22 Jun 2018 11:51:30 +0800 "Huang, Ying"
>> wrote:
>>
>> > This is the final step of THP (Transparent Huge Page) swap
>> > optimization. After the first and sec
Sergey Senozhatsky writes:
> On (06/27/18 21:51), Andrew Morton wrote:
>> On Fri, 22 Jun 2018 11:51:30 +0800 "Huang, Ying"
>> wrote:
>>
>> > This is the final step of THP (Transparent Huge Page) swap
>> > optimization. After the first and sec
701 - 800 of 3349 matches
Mail list logo
