Hi everyone,
This series contains a few patches to fix the comment, remove unneeded
return value, use some helpers and so on. More details can be found in
the respective changelogs. Thanks!
Miaohe Lin (15):
mm/swap: use helper is_swap_pte() in swap_vma_readahead
mm/swap: use helper macro __ATTR_RW
mm/swap: fold __swap_info_get() into its sole caller
mm/swap: remove unneeded return value of free_swap_slot
mm/swap: print bad swap offset entry in get_swap_device
mm/swap: remove buggy cache->nr check in refill_swap_slots_cache
mm/swap: remove unneeded p != NULL check in __swap_duplicate
mm/swap: make page_swapcount and __lru_add_drain_all
mm/swap: avoid calling swp_swap_info when try to check
SWP_STABLE_WRITES
mm/swap: break the loop if matching device is found
mm/swap: add helper swap_offset_available()
mm/swap: fix the obsolete comment for SWP_TYPE_SHIFT
mm/swap: clean up the comment of find_next_to_unuse
mm/swap: fix the comment of get_kernel_pages
mm/swap: fix comment about swap extent
include/linux/swap.h | 11 +----
include/linux/swap_slots.h | 2 +-
include/linux/swapops.h | 4 +-
mm/memory.c | 2 +-
mm/swap.c | 6 +--
mm/swap_slots.c | 6 +--
mm/swap_state.c | 8 +---
mm/swapfile.c | 86 ++++++++++++++++----------------------
8 files changed, 50 insertions(+), 75 deletions(-)
--
2.23.0
The return value of free_swap_slot is always 0 and also ignored now.
Remove it to clean up the code.
Signed-off-by: Miaohe Lin <[email protected]>
---
include/linux/swap_slots.h | 2 +-
mm/swap_slots.c | 4 +---
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/include/linux/swap_slots.h b/include/linux/swap_slots.h
index 347f1a304190..15adfb8c813a 100644
--- a/include/linux/swap_slots.h
+++ b/include/linux/swap_slots.h
@@ -24,7 +24,7 @@ struct swap_slots_cache {
void disable_swap_slots_cache_lock(void);
void reenable_swap_slots_cache_unlock(void);
void enable_swap_slots_cache(void);
-int free_swap_slot(swp_entry_t entry);
+void free_swap_slot(swp_entry_t entry);
extern bool swap_slot_cache_enabled;
diff --git a/mm/swap_slots.c b/mm/swap_slots.c
index 0218ec1cd24c..2f877e6f87d7 100644
--- a/mm/swap_slots.c
+++ b/mm/swap_slots.c
@@ -269,7 +269,7 @@ static int refill_swap_slots_cache(struct swap_slots_cache *cache)
return cache->nr;
}
-int free_swap_slot(swp_entry_t entry)
+void free_swap_slot(swp_entry_t entry)
{
struct swap_slots_cache *cache;
@@ -297,8 +297,6 @@ int free_swap_slot(swp_entry_t entry)
direct_free:
swapcache_free_entries(&entry, 1);
}
-
- return 0;
}
swp_entry_t folio_alloc_swap(struct folio *folio)
--
2.23.0
If offset exceeds the si->max, print bad swap offset entry to help debug
the unexpected case.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/swapfile.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 0aee6286d6a7..d4b81ca887c0 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1272,6 +1272,7 @@ struct swap_info_struct *get_swap_device(swp_entry_t entry)
out:
return NULL;
put_out:
+ pr_err("%s: %s%08lx\n", __func__, Bad_offset, entry.val);
percpu_ref_put(&si->users);
return NULL;
}
--
2.23.0
refill_swap_slots_cache is always called when cache->nr is 0. And if
cache->nr != 0, we should return cache->nr instead of 0. So remove
such buggy and confusing check.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/swap_slots.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/swap_slots.c b/mm/swap_slots.c
index 2f877e6f87d7..2a65a89b5b4d 100644
--- a/mm/swap_slots.c
+++ b/mm/swap_slots.c
@@ -258,7 +258,7 @@ void enable_swap_slots_cache(void)
/* called with swap slot cache's alloc lock held */
static int refill_swap_slots_cache(struct swap_slots_cache *cache)
{
- if (!use_swap_slot_cache || cache->nr)
+ if (!use_swap_slot_cache)
return 0;
cache->cur = 0;
--
2.23.0
If p is NULL, __swap_duplicate will already return -EINVAL. So if we
reach here, p must be non-NULL.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/swapfile.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/mm/swapfile.c b/mm/swapfile.c
index d4b81ca887c0..7b4c99ca2aea 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -3336,8 +3336,7 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage)
unlock_out:
unlock_cluster_or_swap_info(p, ci);
- if (p)
- put_swap_device(p);
+ put_swap_device(p);
return err;
}
--
2.23.0
We can break the loop if matching device is found to save some possible
cpu cycles because there should be only one matching device and there is
no need to continue if the matching one is already found.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/swapfile.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 133e03fea104..c90298a0561a 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1692,6 +1692,8 @@ int swap_type_of(dev_t device, sector_t offset)
spin_unlock(&swap_lock);
return type;
}
+
+ break;
}
}
spin_unlock(&swap_lock);
--
2.23.0
Add helper swap_offset_available() to remove some duplicated codes.
Minor readability improvement.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/swapfile.c | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/mm/swapfile.c b/mm/swapfile.c
index c90298a0561a..d5d3e2d03d28 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -776,6 +776,21 @@ static void set_cluster_next(struct swap_info_struct *si, unsigned long next)
this_cpu_write(*si->cluster_next_cpu, next);
}
+static inline bool swap_offset_available(struct swap_info_struct *si, unsigned long offset)
+{
+ if (data_race(!si->swap_map[offset])) {
+ spin_lock(&si->lock);
+ return true;
+ }
+
+ if (vm_swap_full() && READ_ONCE(si->swap_map[offset]) == SWAP_HAS_CACHE) {
+ spin_lock(&si->lock);
+ return true;
+ }
+
+ return false;
+}
+
static int scan_swap_map_slots(struct swap_info_struct *si,
unsigned char usage, int nr,
swp_entry_t slots[])
@@ -953,15 +968,8 @@ static int scan_swap_map_slots(struct swap_info_struct *si,
scan:
spin_unlock(&si->lock);
while (++offset <= READ_ONCE(si->highest_bit)) {
- if (data_race(!si->swap_map[offset])) {
- spin_lock(&si->lock);
+ if (swap_offset_available(si, offset))
goto checks;
- }
- if (vm_swap_full() &&
- READ_ONCE(si->swap_map[offset]) == SWAP_HAS_CACHE) {
- spin_lock(&si->lock);
- goto checks;
- }
if (unlikely(--latency_ration < 0)) {
cond_resched();
latency_ration = LATENCY_LIMIT;
@@ -970,15 +978,8 @@ static int scan_swap_map_slots(struct swap_info_struct *si,
}
offset = si->lowest_bit;
while (offset < scan_base) {
- if (data_race(!si->swap_map[offset])) {
- spin_lock(&si->lock);
+ if (swap_offset_available(si, offset))
goto checks;
- }
- if (vm_swap_full() &&
- READ_ONCE(si->swap_map[offset]) == SWAP_HAS_CACHE) {
- spin_lock(&si->lock);
- goto checks;
- }
if (unlikely(--latency_ration < 0)) {
cond_resched();
latency_ration = LATENCY_LIMIT;
--
2.23.0
Use flags of si directly to check SWP_STABLE_WRITES to avoid possible
READ_ONCE and thus save some cpu cycles.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/memory.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/memory.c b/mm/memory.c
index 9c3e7e6ac202..89dd15504f3d 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3892,7 +3892,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
*/
exclusive = true;
} else if (exclusive && PageWriteback(page) &&
- (swp_swap_info(entry)->flags & SWP_STABLE_WRITES)) {
+ (si->flags & SWP_STABLE_WRITES)) {
/*
* This is tricky: not all swap backends support
* concurrent page modifications while under writeback.
--
2.23.0
Since commit 10a9c496789f ("mm: simplify try_to_unuse"), frontswap
parameter is removed. Update the corresponding comment.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/swapfile.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/mm/swapfile.c b/mm/swapfile.c
index d5d3e2d03d28..7ead5fb96d9d 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -2007,9 +2007,9 @@ static int unuse_mm(struct mm_struct *mm, unsigned int type)
}
/*
- * Scan swap_map (or frontswap_map if frontswap parameter is true)
- * from current position to next entry still in use. Return 0
- * if there are no inuse entries after prev till end of the map.
+ * Scan swap_map from current position to next entry still in use.
+ * Return 0 if there are no inuse entries after prev till end of
+ * the map.
*/
static unsigned int find_next_to_unuse(struct swap_info_struct *si,
unsigned int prev)
--
2.23.0
On Mon, 9 May 2022 21:14:11 +0800 Miaohe Lin <[email protected]> wrote:
> We can break the loop if matching device is found to save some possible
> cpu cycles because there should be only one matching device and there is
> no need to continue if the matching one is already found.
>
> ...
>
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1692,6 +1692,8 @@ int swap_type_of(dev_t device, sector_t offset)
> spin_unlock(&swap_lock);
> return type;
> }
> +
> + break;
> }
> }
> spin_unlock(&swap_lock);
Are you sure? If we have two S_ISREG swapfiles on the same device,
don't they have the same sis->bdev?
If not, why bother passing `offset' into this function at all?
On Mon, 09 May 2022, Miaohe Lin wrote:
> Use flags of si directly to check SWP_STABLE_WRITES to avoid possible
> READ_ONCE and thus save some cpu cycles.
>
> Signed-off-by: Miaohe Lin <[email protected]>
> ---
> mm/memory.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 9c3e7e6ac202..89dd15504f3d 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3892,7 +3892,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
> */
> exclusive = true;
> } else if (exclusive && PageWriteback(page) &&
> - (swp_swap_info(entry)->flags & SWP_STABLE_WRITES)) {
> + (si->flags & SWP_STABLE_WRITES)) {
Should this have a data_race() annotation. Other bit-tests of si->flags
do because scan_swap_map_slots can update it asynchronously, but we know
that won't matter in practice.
Thanks,
NeilBrown
> /*
> * This is tricky: not all swap backends support
> * concurrent page modifications while under writeback.
> --
> 2.23.0
>
>
On Mon, 09 May 2022, Miaohe Lin wrote:
> Add helper swap_offset_available() to remove some duplicated codes.
> Minor readability improvement.
I don't think that putting the spin_lock() inside the inline helper is
good for readability.
If the function was called
swap_offset_available_and_locked()
it might be ok. Otherwise I would rather the spin_lock() was called
when the function returned true.
Thanks,
NeilBrown
>
> Signed-off-by: Miaohe Lin <[email protected]>
> ---
> mm/swapfile.c | 33 +++++++++++++++++----------------
> 1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index c90298a0561a..d5d3e2d03d28 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -776,6 +776,21 @@ static void set_cluster_next(struct swap_info_struct *si, unsigned long next)
> this_cpu_write(*si->cluster_next_cpu, next);
> }
>
> +static inline bool swap_offset_available(struct swap_info_struct *si, unsigned long offset)
> +{
> + if (data_race(!si->swap_map[offset])) {
> + spin_lock(&si->lock);
> + return true;
> + }
> +
> + if (vm_swap_full() && READ_ONCE(si->swap_map[offset]) == SWAP_HAS_CACHE) {
> + spin_lock(&si->lock);
> + return true;
> + }
> +
> + return false;
> +}
> +
> static int scan_swap_map_slots(struct swap_info_struct *si,
> unsigned char usage, int nr,
> swp_entry_t slots[])
> @@ -953,15 +968,8 @@ static int scan_swap_map_slots(struct swap_info_struct *si,
> scan:
> spin_unlock(&si->lock);
> while (++offset <= READ_ONCE(si->highest_bit)) {
> - if (data_race(!si->swap_map[offset])) {
> - spin_lock(&si->lock);
> + if (swap_offset_available(si, offset))
> goto checks;
> - }
> - if (vm_swap_full() &&
> - READ_ONCE(si->swap_map[offset]) == SWAP_HAS_CACHE) {
> - spin_lock(&si->lock);
> - goto checks;
> - }
> if (unlikely(--latency_ration < 0)) {
> cond_resched();
> latency_ration = LATENCY_LIMIT;
> @@ -970,15 +978,8 @@ static int scan_swap_map_slots(struct swap_info_struct *si,
> }
> offset = si->lowest_bit;
> while (offset < scan_base) {
> - if (data_race(!si->swap_map[offset])) {
> - spin_lock(&si->lock);
> + if (swap_offset_available(si, offset))
> goto checks;
> - }
> - if (vm_swap_full() &&
> - READ_ONCE(si->swap_map[offset]) == SWAP_HAS_CACHE) {
> - spin_lock(&si->lock);
> - goto checks;
> - }
> if (unlikely(--latency_ration < 0)) {
> cond_resched();
> latency_ration = LATENCY_LIMIT;
> --
> 2.23.0
>
>
On 2022/5/10 8:28, NeilBrown wrote:
> On Mon, 09 May 2022, Miaohe Lin wrote:
>> Use flags of si directly to check SWP_STABLE_WRITES to avoid possible
>> READ_ONCE and thus save some cpu cycles.
>>
>> Signed-off-by: Miaohe Lin <[email protected]>
>> ---
>> mm/memory.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 9c3e7e6ac202..89dd15504f3d 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -3892,7 +3892,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
>> */
>> exclusive = true;
>> } else if (exclusive && PageWriteback(page) &&
>> - (swp_swap_info(entry)->flags & SWP_STABLE_WRITES)) {
>> + (si->flags & SWP_STABLE_WRITES)) {
>
> Should this have a data_race() annotation. Other bit-tests of si->flags
> do because scan_swap_map_slots can update it asynchronously, but we know
> that won't matter in practice.
Yes, you're right. scan_swap_map_slots can update si->flags asynchronously while
do_swap_page tests SWP_STABLE_WRITES here. We know this is harmless because
SWP_STABLE_WRITES is only changed at swapon/swapoff.
Will add data_race() annotation in next version to avoid possible KCSAN data-race complaint.
Many thanks for pointing this out! :)
>
> Thanks,
> NeilBrown
>
>
>> /*
>> * This is tricky: not all swap backends support
>> * concurrent page modifications while under writeback.
>> --
>> 2.23.0
>>
>>
> .
>
On 2022/5/10 8:45, NeilBrown wrote:
> On Mon, 09 May 2022, Miaohe Lin wrote:
>> Add helper swap_offset_available() to remove some duplicated codes.
>> Minor readability improvement.
>
> I don't think that putting the spin_lock() inside the inline helper is
> good for readability.
> If the function was called
> swap_offset_available_and_locked()
Yes, swap_offset_available_and_locked should be more suitable as we do the spin_lock
inside it. Will do this in next version.
Thanks!
>
> it might be ok. Otherwise I would rather the spin_lock() was called
> when the function returned true.
>
> Thanks,
> NeilBrown
>
>>
>> Signed-off-by: Miaohe Lin <[email protected]>
>> ---
>> mm/swapfile.c | 33 +++++++++++++++++----------------
>> 1 file changed, 17 insertions(+), 16 deletions(-)
>>
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index c90298a0561a..d5d3e2d03d28 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -776,6 +776,21 @@ static void set_cluster_next(struct swap_info_struct *si, unsigned long next)
>> this_cpu_write(*si->cluster_next_cpu, next);
>> }
>>
>> +static inline bool swap_offset_available(struct swap_info_struct *si, unsigned long offset)
>> +{
>> + if (data_race(!si->swap_map[offset])) {
>> + spin_lock(&si->lock);
>> + return true;
>> + }
>> +
>> + if (vm_swap_full() && READ_ONCE(si->swap_map[offset]) == SWAP_HAS_CACHE) {
>> + spin_lock(&si->lock);
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> static int scan_swap_map_slots(struct swap_info_struct *si,
>> unsigned char usage, int nr,
>> swp_entry_t slots[])
>> @@ -953,15 +968,8 @@ static int scan_swap_map_slots(struct swap_info_struct *si,
>> scan:
>> spin_unlock(&si->lock);
>> while (++offset <= READ_ONCE(si->highest_bit)) {
>> - if (data_race(!si->swap_map[offset])) {
>> - spin_lock(&si->lock);
>> + if (swap_offset_available(si, offset))
>> goto checks;
>> - }
>> - if (vm_swap_full() &&
>> - READ_ONCE(si->swap_map[offset]) == SWAP_HAS_CACHE) {
>> - spin_lock(&si->lock);
>> - goto checks;
>> - }
>> if (unlikely(--latency_ration < 0)) {
>> cond_resched();
>> latency_ration = LATENCY_LIMIT;
>> @@ -970,15 +978,8 @@ static int scan_swap_map_slots(struct swap_info_struct *si,
>> }
>> offset = si->lowest_bit;
>> while (offset < scan_base) {
>> - if (data_race(!si->swap_map[offset])) {
>> - spin_lock(&si->lock);
>> + if (swap_offset_available(si, offset))
>> goto checks;
>> - }
>> - if (vm_swap_full() &&
>> - READ_ONCE(si->swap_map[offset]) == SWAP_HAS_CACHE) {
>> - spin_lock(&si->lock);
>> - goto checks;
>> - }
>> if (unlikely(--latency_ration < 0)) {
>> cond_resched();
>> latency_ration = LATENCY_LIMIT;
>> --
>> 2.23.0
>>
>>
> .
>
On 2022/5/10 5:16, Andrew Morton wrote:
> On Mon, 9 May 2022 21:14:11 +0800 Miaohe Lin <[email protected]> wrote:
>
>> We can break the loop if matching device is found to save some possible
>> cpu cycles because there should be only one matching device and there is
>> no need to continue if the matching one is already found.
>>
>> ...
>>
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -1692,6 +1692,8 @@ int swap_type_of(dev_t device, sector_t offset)
>> spin_unlock(&swap_lock);
>> return type;
>> }
>> +
>> + break;
>> }
>> }
>> spin_unlock(&swap_lock);
>
> Are you sure? If we have two S_ISREG swapfiles on the same device,
> don't they have the same sis->bdev?
Oh, I missed this use case. Sorry about it! :(
>
> If not, why bother passing `offset' into this function at all?
Yes, you're right. 'offset' indicates the swap header location. Will drop this patch.
Thanks!
> .
>
On 09.05.22 15:14, Miaohe Lin wrote:
> If offset exceeds the si->max, print bad swap offset entry to help debug
> the unexpected case.
>
> Signed-off-by: Miaohe Lin <[email protected]>
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
On 09.05.22 15:14, Miaohe Lin wrote:
> The return value of free_swap_slot is always 0 and also ignored now.
> Remove it to clean up the code.
>
> Signed-off-by: Miaohe Lin <[email protected]>
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
On 09.05.22 15:14, Miaohe Lin wrote:
> refill_swap_slots_cache is always called when cache->nr is 0. And if
> cache->nr != 0, we should return cache->nr instead of 0. So remove
> such buggy and confusing check.
Not sure about the "cache->nr != 0, we should return cache->nr instead
of 0" part, I'd just drop that from the patch description. We'd actually
end up overwriting cache->nr after your change, which doesn't sound
right and also different to what you describe here.
>
> Signed-off-by: Miaohe Lin <[email protected]>
> ---
> mm/swap_slots.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/swap_slots.c b/mm/swap_slots.c
> index 2f877e6f87d7..2a65a89b5b4d 100644
> --- a/mm/swap_slots.c
> +++ b/mm/swap_slots.c
> @@ -258,7 +258,7 @@ void enable_swap_slots_cache(void)
> /* called with swap slot cache's alloc lock held */
> static int refill_swap_slots_cache(struct swap_slots_cache *cache)
> {
> - if (!use_swap_slot_cache || cache->nr)
> + if (!use_swap_slot_cache)
> return 0;
>
> cache->cur = 0;
I feel like if this function would be called with cache->nr, it would be
a BUG. So I'm fine with removing it, but we could also think about
turning it into some sort of WARN/BG to make it clearer that this is
unexpected.
Anyhow,
Acked-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
On 09.05.22 15:14, Miaohe Lin wrote:
> Since commit 10a9c496789f ("mm: simplify try_to_unuse"), frontswap
> parameter is removed. Update the corresponding comment.
>
> Signed-off-by: Miaohe Lin <[email protected]>
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
On 09.05.22 15:14, Miaohe Lin wrote:
> If p is NULL, __swap_duplicate will already return -EINVAL. So if we
> reach here, p must be non-NULL.
>
> Signed-off-by: Miaohe Lin <[email protected]>
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
On 2022/5/12 21:37, David Hildenbrand wrote:
> On 09.05.22 15:14, Miaohe Lin wrote:
>> refill_swap_slots_cache is always called when cache->nr is 0. And if
>> cache->nr != 0, we should return cache->nr instead of 0. So remove
>> such buggy and confusing check.
>
> Not sure about the "cache->nr != 0, we should return cache->nr instead
> of 0" part, I'd just drop that from the patch description. We'd actually
> end up overwriting cache->nr after your change, which doesn't sound
> right and also different to what you describe here.
Will do.
>
>>
>> Signed-off-by: Miaohe Lin <[email protected]>
>> ---
>> mm/swap_slots.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/swap_slots.c b/mm/swap_slots.c
>> index 2f877e6f87d7..2a65a89b5b4d 100644
>> --- a/mm/swap_slots.c
>> +++ b/mm/swap_slots.c
>> @@ -258,7 +258,7 @@ void enable_swap_slots_cache(void)
>> /* called with swap slot cache's alloc lock held */
>> static int refill_swap_slots_cache(struct swap_slots_cache *cache)
>> {
>> - if (!use_swap_slot_cache || cache->nr)
>> + if (!use_swap_slot_cache)
>> return 0;
>>
>> cache->cur = 0;
>
> I feel like if this function would be called with cache->nr, it would be
> a BUG. So I'm fine with removing it, but we could also think about
> turning it into some sort of WARN/BG to make it clearer that this is
> unexpected.
Since refill_swap_slots_cache is only called by folio_alloc_swap when cache->nr == 0. I think
it might be too overkill to add a WARN/BG.
>
>
> Anyhow,
>
> Acked-by: David Hildenbrand <[email protected]>
Many thanks for comment and Acked-by tag!
>
On Mon, 9 May 2022 21:14:01 +0800 Miaohe Lin <[email protected]> wrote:
> This series contains a few patches to fix the comment, remove unneeded
> return value, use some helpers and so on. More details can be found in
> the respective changelogs.
After dropping [10/14] and with the four fixlets I just sent out, I
believe this series is ready to be moved into mm-stable. Is there
anything outstanding?
On Tue, 10 May 2022 10:21:21 +0800 Miaohe Lin <[email protected]> wrote:
> On 2022/5/10 8:28, NeilBrown wrote:
> > On Mon, 09 May 2022, Miaohe Lin wrote:
> >> Use flags of si directly to check SWP_STABLE_WRITES to avoid possible
> >> READ_ONCE and thus save some cpu cycles.
> >>
> >> Signed-off-by: Miaohe Lin <[email protected]>
> >> ---
> >> mm/memory.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/mm/memory.c b/mm/memory.c
> >> index 9c3e7e6ac202..89dd15504f3d 100644
> >> --- a/mm/memory.c
> >> +++ b/mm/memory.c
> >> @@ -3892,7 +3892,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
> >> */
> >> exclusive = true;
> >> } else if (exclusive && PageWriteback(page) &&
> >> - (swp_swap_info(entry)->flags & SWP_STABLE_WRITES)) {
> >> + (si->flags & SWP_STABLE_WRITES)) {
> >
> > Should this have a data_race() annotation. Other bit-tests of si->flags
> > do because scan_swap_map_slots can update it asynchronously, but we know
> > that won't matter in practice.
>
> Yes, you're right. scan_swap_map_slots can update si->flags asynchronously while
> do_swap_page tests SWP_STABLE_WRITES here. We know this is harmless because
> SWP_STABLE_WRITES is only changed at swapon/swapoff.
>
> Will add data_race() annotation in next version to avoid possible KCSAN data-race complaint.
>
I did this:
--- a/mm/memory.c~mm-swap-avoid-calling-swp_swap_info-when-try-to-check-swp_stable_writes-fix
+++ a/mm/memory.c
@@ -3889,7 +3889,7 @@ vm_fault_t do_swap_page(struct vm_fault
*/
exclusive = true;
} else if (exclusive && PageWriteback(page) &&
- (si->flags & SWP_STABLE_WRITES)) {
+ data_race(si->flags & SWP_STABLE_WRITES)) {
/*
* This is tricky: not all swap backends support
* concurrent page modifications while under writeback.
_
On Tue, 10 May 2022 10:03:19 +0800 Miaohe Lin <[email protected]> wrote:
> On 2022/5/10 8:45, NeilBrown wrote:
> > On Mon, 09 May 2022, Miaohe Lin wrote:
> >> Add helper swap_offset_available() to remove some duplicated codes.
> >> Minor readability improvement.
> >
> > I don't think that putting the spin_lock() inside the inline helper is
> > good for readability.
> > If the function was called
> > swap_offset_available_and_locked()
>
> Yes, swap_offset_available_and_locked should be more suitable as we do the spin_lock
> inside it. Will do this in next version.
>
--- a/mm/swapfile.c~mm-swap-add-helper-swap_offset_available-fix
+++ a/mm/swapfile.c
@@ -775,7 +775,8 @@ static void set_cluster_next(struct swap
this_cpu_write(*si->cluster_next_cpu, next);
}
-static inline bool swap_offset_available(struct swap_info_struct *si, unsigned long offset)
+static bool swap_offset_available_and_locked(struct swap_info_struct *si,
+ unsigned long offset)
{
if (data_race(!si->swap_map[offset])) {
spin_lock(&si->lock);
@@ -967,7 +968,7 @@ done:
scan:
spin_unlock(&si->lock);
while (++offset <= READ_ONCE(si->highest_bit)) {
- if (swap_offset_available(si, offset))
+ if (swap_offset_available_and_locked(si, offset))
goto checks;
if (unlikely(--latency_ration < 0)) {
cond_resched();
@@ -977,7 +978,7 @@ scan:
}
offset = si->lowest_bit;
while (offset < scan_base) {
- if (swap_offset_available(si, offset))
+ if (swap_offset_available_and_locked(si, offset))
goto checks;
if (unlikely(--latency_ration < 0)) {
cond_resched();
_
On Mon, 16 May 2022 10:00:43 +0800 Miaohe Lin <[email protected]> wrote:
> On 2022/5/12 21:37, David Hildenbrand wrote:
> > On 09.05.22 15:14, Miaohe Lin wrote:
> >> refill_swap_slots_cache is always called when cache->nr is 0. And if
> >> cache->nr != 0, we should return cache->nr instead of 0. So remove
> >> such buggy and confusing check.
> >
> > Not sure about the "cache->nr != 0, we should return cache->nr instead
> > of 0" part, I'd just drop that from the patch description. We'd actually
> > end up overwriting cache->nr after your change, which doesn't sound
> > right and also different to what you describe here.
>
> Will do.
I've updated the changleog to simply
: refill_swap_slots_cache is always called when cache->nr is 0. So remove
: such buggy and confusing check.
On Mon, May 09, 2022 at 09:14:05PM +0800, Miaohe Lin wrote:
> The return value of free_swap_slot is always 0 and also ignored now.
> Remove it to clean up the code.
>
> Signed-off-by: Miaohe Lin <[email protected]>
Reviewed-by: Oscar Salvador <[email protected]>
> ---
> include/linux/swap_slots.h | 2 +-
> mm/swap_slots.c | 4 +---
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/swap_slots.h b/include/linux/swap_slots.h
> index 347f1a304190..15adfb8c813a 100644
> --- a/include/linux/swap_slots.h
> +++ b/include/linux/swap_slots.h
> @@ -24,7 +24,7 @@ struct swap_slots_cache {
> void disable_swap_slots_cache_lock(void);
> void reenable_swap_slots_cache_unlock(void);
> void enable_swap_slots_cache(void);
> -int free_swap_slot(swp_entry_t entry);
> +void free_swap_slot(swp_entry_t entry);
>
> extern bool swap_slot_cache_enabled;
>
> diff --git a/mm/swap_slots.c b/mm/swap_slots.c
> index 0218ec1cd24c..2f877e6f87d7 100644
> --- a/mm/swap_slots.c
> +++ b/mm/swap_slots.c
> @@ -269,7 +269,7 @@ static int refill_swap_slots_cache(struct swap_slots_cache *cache)
> return cache->nr;
> }
>
> -int free_swap_slot(swp_entry_t entry)
> +void free_swap_slot(swp_entry_t entry)
> {
> struct swap_slots_cache *cache;
>
> @@ -297,8 +297,6 @@ int free_swap_slot(swp_entry_t entry)
> direct_free:
> swapcache_free_entries(&entry, 1);
> }
> -
> - return 0;
> }
>
> swp_entry_t folio_alloc_swap(struct folio *folio)
> --
> 2.23.0
>
>
--
Oscar Salvador
SUSE Labs
On Mon, May 09, 2022 at 09:14:06PM +0800, Miaohe Lin wrote:
> If offset exceeds the si->max, print bad swap offset entry to help debug
> the unexpected case.
>
> Signed-off-by: Miaohe Lin <[email protected]>
Reviewed-by: Oscar Salvador <[email protected]>
> ---
> mm/swapfile.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 0aee6286d6a7..d4b81ca887c0 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1272,6 +1272,7 @@ struct swap_info_struct *get_swap_device(swp_entry_t entry)
> out:
> return NULL;
> put_out:
> + pr_err("%s: %s%08lx\n", __func__, Bad_offset, entry.val);
> percpu_ref_put(&si->users);
> return NULL;
> }
> --
> 2.23.0
>
>
--
Oscar Salvador
SUSE Labs
On Mon, May 09, 2022 at 09:14:07PM +0800, Miaohe Lin wrote:
> refill_swap_slots_cache is always called when cache->nr is 0. And if
> cache->nr != 0, we should return cache->nr instead of 0. So remove
> such buggy and confusing check.
>
> Signed-off-by: Miaohe Lin <[email protected]>
With Andrew's ammendment:
Reviewed-by: Oscar Salvador <[email protected]>
> ---
> mm/swap_slots.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/swap_slots.c b/mm/swap_slots.c
> index 2f877e6f87d7..2a65a89b5b4d 100644
> --- a/mm/swap_slots.c
> +++ b/mm/swap_slots.c
> @@ -258,7 +258,7 @@ void enable_swap_slots_cache(void)
> /* called with swap slot cache's alloc lock held */
> static int refill_swap_slots_cache(struct swap_slots_cache *cache)
> {
> - if (!use_swap_slot_cache || cache->nr)
> + if (!use_swap_slot_cache)
> return 0;
>
> cache->cur = 0;
> --
> 2.23.0
>
>
--
Oscar Salvador
SUSE Labs
On Mon, May 09, 2022 at 09:14:08PM +0800, Miaohe Lin wrote:
> If p is NULL, __swap_duplicate will already return -EINVAL. So if we
> reach here, p must be non-NULL.
>
> Signed-off-by: Miaohe Lin <[email protected]>
Reviewed-by: Oscar Salvador <[email protected]>
> ---
> mm/swapfile.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index d4b81ca887c0..7b4c99ca2aea 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -3336,8 +3336,7 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage)
>
> unlock_out:
> unlock_cluster_or_swap_info(p, ci);
> - if (p)
> - put_swap_device(p);
> + put_swap_device(p);
> return err;
> }
>
> --
> 2.23.0
>
>
--
Oscar Salvador
SUSE Labs