[PATCH -mm] mm, swap: Fix false error message in __swp_swapcount()

[PATCH -mm] mm, swap: Fix false error message in __swp_swapcount()

[Huang, Ying]

From: Ying Huang <ying.huang@intel.com>

__swp_swapcount() is used in __read_swap_cache_async().  Where the
invalid swap entry (offset > max) may be supplied during swap
readahead.  But __swp_swapcount() will print error message for these
expected invalid swap entry as below, which will make the users
confusing.

  swap_info_get: Bad swap offset entry 0200f8a7

So the swap entry checking code in __swp_swapcount() is changed to
avoid printing error message for it.  To avoid to duplicate code with
__swap_duplicate(), a new helper function named
__swap_info_get_silence() is added and invoked in both places.

Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: <stable@vger.kernel.org> # 4.11-4.13
Reported-by: Christian Kujau <lists@nerdbynature.de>
Fixes: e8c26ab60598 ("mm/swap: skip readahead for unreferenced swap slots")
Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
---
 mm/swapfile.c | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 3074b02eaa09..3193aa670c90 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1107,6 +1107,30 @@ static struct swap_info_struct *swap_info_get_cont(swp_entry_t entry,
 	return p;
 }
 
+static struct swap_info_struct *__swap_info_get_silence(swp_entry_t entry)
+{
+	struct swap_info_struct *p;
+	unsigned long offset, type;
+
+	if (non_swap_entry(entry))
+		goto out;
+
+	type = swp_type(entry);
+	if (type >= nr_swapfiles)
+		goto bad_file;
+	p = swap_info[type];
+	offset = swp_offset(entry);
+	if (unlikely(offset >= p->max))
+		goto out;
+
+	return p;
+
+bad_file:
+	pr_err("swap_info_get_silence: %s%08lx\n", Bad_file, entry.val);
+out:
+	return NULL;
+}
+
 static unsigned char __swap_entry_free(struct swap_info_struct *p,
 				       swp_entry_t entry, unsigned char usage)
 {
@@ -1357,7 +1381,7 @@ int __swp_swapcount(swp_entry_t entry)
 	int count = 0;
 	struct swap_info_struct *si;
 
-	si = __swap_info_get(entry);
+	si = __swap_info_get_silence(entry);
 	if (si)
 		count = swap_swapcount(si, entry);
 	return count;
@@ -3356,22 +3380,16 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage)
 {
 	struct swap_info_struct *p;
 	struct swap_cluster_info *ci;
-	unsigned long offset, type;
+	unsigned long offset;
 	unsigned char count;
 	unsigned char has_cache;
 	int err = -EINVAL;
 
-	if (non_swap_entry(entry))
+	p = __swap_info_get_silence(entry);
+	if (!p)
 		goto out;
 
-	type = swp_type(entry);
-	if (type >= nr_swapfiles)
-		goto bad_file;
-	p = swap_info[type];
 	offset = swp_offset(entry);
-	if (unlikely(offset >= p->max))
-		goto out;
-
 	ci = lock_cluster_or_swap_info(p, offset);
 
 	count = p->swap_map[offset];
@@ -3418,10 +3436,6 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage)
 	unlock_cluster_or_swap_info(p, ci);
 out:
 	return err;
-
-bad_file:
-	pr_err("swap_dup: %s%08lx\n", Bad_file, entry.val);
-	goto out;
 }
 
 /*
-- 
2.14.2

--
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>

Re: [PATCH -mm] mm, swap: Fix false error message in __swp_swapcount()

[Michal Hocko]

On Tue 24-10-17 10:47:00, Huang, Ying wrote:
> From: Ying Huang <ying.huang@intel.com>
> 
> __swp_swapcount() is used in __read_swap_cache_async().  Where the
> invalid swap entry (offset > max) may be supplied during swap
> readahead.  But __swp_swapcount() will print error message for these
> expected invalid swap entry as below, which will make the users
> confusing.
  ^^
confused... And I have to admit this changelog has left me confused as
well. What is an invalid swap entry in the readahead? Ohh, let me
re-real Fixes: commit. It didn't really help "We can avoid needlessly
allocating page for swap slots that are not used by anyone.  No pages
have to be read in for these slots."

Could you be more specific about when and how this happens please?
 
>   swap_info_get: Bad swap offset entry 0200f8a7
> 
> So the swap entry checking code in __swp_swapcount() is changed to
> avoid printing error message for it.  To avoid to duplicate code with
> __swap_duplicate(), a new helper function named
> __swap_info_get_silence() is added and invoked in both places.
> 
> Cc: Tim Chen <tim.c.chen@linux.intel.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: <stable@vger.kernel.org> # 4.11-4.13
> Reported-by: Christian Kujau <lists@nerdbynature.de>
> Fixes: e8c26ab60598 ("mm/swap: skip readahead for unreferenced swap slots")
> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
> ---
>  mm/swapfile.c | 42 ++++++++++++++++++++++++++++--------------
>  1 file changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 3074b02eaa09..3193aa670c90 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1107,6 +1107,30 @@ static struct swap_info_struct *swap_info_get_cont(swp_entry_t entry,
>  	return p;
>  }
>  
> +static struct swap_info_struct *__swap_info_get_silence(swp_entry_t entry)
> +{
> +	struct swap_info_struct *p;
> +	unsigned long offset, type;
> +
> +	if (non_swap_entry(entry))
> +		goto out;
> +
> +	type = swp_type(entry);
> +	if (type >= nr_swapfiles)
> +		goto bad_file;
> +	p = swap_info[type];
> +	offset = swp_offset(entry);
> +	if (unlikely(offset >= p->max))
> +		goto out;
> +
> +	return p;
> +
> +bad_file:
> +	pr_err("swap_info_get_silence: %s%08lx\n", Bad_file, entry.val);
> +out:
> +	return NULL;
> +}
> +
>  static unsigned char __swap_entry_free(struct swap_info_struct *p,
>  				       swp_entry_t entry, unsigned char usage)
>  {
> @@ -1357,7 +1381,7 @@ int __swp_swapcount(swp_entry_t entry)
>  	int count = 0;
>  	struct swap_info_struct *si;
>  
> -	si = __swap_info_get(entry);
> +	si = __swap_info_get_silence(entry);
>  	if (si)
>  		count = swap_swapcount(si, entry);
>  	return count;
> @@ -3356,22 +3380,16 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage)
>  {
>  	struct swap_info_struct *p;
>  	struct swap_cluster_info *ci;
> -	unsigned long offset, type;
> +	unsigned long offset;
>  	unsigned char count;
>  	unsigned char has_cache;
>  	int err = -EINVAL;
>  
> -	if (non_swap_entry(entry))
> +	p = __swap_info_get_silence(entry);
> +	if (!p)
>  		goto out;
>  
> -	type = swp_type(entry);
> -	if (type >= nr_swapfiles)
> -		goto bad_file;
> -	p = swap_info[type];
>  	offset = swp_offset(entry);
> -	if (unlikely(offset >= p->max))
> -		goto out;
> -
>  	ci = lock_cluster_or_swap_info(p, offset);
>  
>  	count = p->swap_map[offset];
> @@ -3418,10 +3436,6 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage)
>  	unlock_cluster_or_swap_info(p, ci);
>  out:
>  	return err;
> -
> -bad_file:
> -	pr_err("swap_dup: %s%08lx\n", Bad_file, entry.val);
> -	goto out;
>  }
>  
>  /*
> -- 
> 2.14.2
> 

-- 
Michal Hocko
SUSE Labs

--
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>

Re: [PATCH -mm] mm, swap: Fix false error message in __swp_swapcount()

[Huang, Ying]

Hi, Michal,

Michal Hocko <mhocko@kernel.org> writes:

> On Tue 24-10-17 10:47:00, Huang, Ying wrote:
>> From: Ying Huang <ying.huang@intel.com>
>> 
>> __swp_swapcount() is used in __read_swap_cache_async().  Where the
>> invalid swap entry (offset > max) may be supplied during swap
>> readahead.  But __swp_swapcount() will print error message for these
>> expected invalid swap entry as below, which will make the users
>> confusing.
>   ^^
> confused... And I have to admit this changelog has left me confused as
> well. What is an invalid swap entry in the readahead? Ohh, let me
> re-real Fixes: commit. It didn't really help "We can avoid needlessly
> allocating page for swap slots that are not used by anyone.  No pages
> have to be read in for these slots."
>
> Could you be more specific about when and how this happens please?

Sorry for confusing.

When page fault occurs for a swap entry, the original swap readahead
(not new VMA base swap readahead) may readahead several swap entries
after the fault swap entry.  The readahead algorithm calculates some of
the swap entries to readahead via increasing the offset of the fault
swap entry without checking whether they are beyond the end of the swap
device and it rely on the __swp_swapcount() and swapcache_prepare() to
check it.  Although __swp_swapcount() checks for the swap entry passed
in, it will complain with error message for the expected invalid swap
entry.  This makes the end user confusing.

Is this a little clearer.

Best Regards,
Huang, Ying

>>   swap_info_get: Bad swap offset entry 0200f8a7
>> 
>> So the swap entry checking code in __swp_swapcount() is changed to
>> avoid printing error message for it.  To avoid to duplicate code with
>> __swap_duplicate(), a new helper function named
>> __swap_info_get_silence() is added and invoked in both places.
>> 
>> Cc: Tim Chen <tim.c.chen@linux.intel.com>
>> Cc: Minchan Kim <minchan@kernel.org>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: <stable@vger.kernel.org> # 4.11-4.13
>> Reported-by: Christian Kujau <lists@nerdbynature.de>
>> Fixes: e8c26ab60598 ("mm/swap: skip readahead for unreferenced swap slots")
>> Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
>> ---
>>  mm/swapfile.c | 42 ++++++++++++++++++++++++++++--------------
>>  1 file changed, 28 insertions(+), 14 deletions(-)
>> 
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index 3074b02eaa09..3193aa670c90 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -1107,6 +1107,30 @@ static struct swap_info_struct *swap_info_get_cont(swp_entry_t entry,
>>  	return p;
>>  }
>>  
>> +static struct swap_info_struct *__swap_info_get_silence(swp_entry_t entry)
>> +{
>> +	struct swap_info_struct *p;
>> +	unsigned long offset, type;
>> +
>> +	if (non_swap_entry(entry))
>> +		goto out;
>> +
>> +	type = swp_type(entry);
>> +	if (type >= nr_swapfiles)
>> +		goto bad_file;
>> +	p = swap_info[type];
>> +	offset = swp_offset(entry);
>> +	if (unlikely(offset >= p->max))
>> +		goto out;
>> +
>> +	return p;
>> +
>> +bad_file:
>> +	pr_err("swap_info_get_silence: %s%08lx\n", Bad_file, entry.val);
>> +out:
>> +	return NULL;
>> +}
>> +
>>  static unsigned char __swap_entry_free(struct swap_info_struct *p,
>>  				       swp_entry_t entry, unsigned char usage)
>>  {
>> @@ -1357,7 +1381,7 @@ int __swp_swapcount(swp_entry_t entry)
>>  	int count = 0;
>>  	struct swap_info_struct *si;
>>  
>> -	si = __swap_info_get(entry);
>> +	si = __swap_info_get_silence(entry);
>>  	if (si)
>>  		count = swap_swapcount(si, entry);
>>  	return count;
>> @@ -3356,22 +3380,16 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage)
>>  {
>>  	struct swap_info_struct *p;
>>  	struct swap_cluster_info *ci;
>> -	unsigned long offset, type;
>> +	unsigned long offset;
>>  	unsigned char count;
>>  	unsigned char has_cache;
>>  	int err = -EINVAL;
>>  
>> -	if (non_swap_entry(entry))
>> +	p = __swap_info_get_silence(entry);
>> +	if (!p)
>>  		goto out;
>>  
>> -	type = swp_type(entry);
>> -	if (type >= nr_swapfiles)
>> -		goto bad_file;
>> -	p = swap_info[type];
>>  	offset = swp_offset(entry);
>> -	if (unlikely(offset >= p->max))
>> -		goto out;
>> -
>>  	ci = lock_cluster_or_swap_info(p, offset);
>>  
>>  	count = p->swap_map[offset];
>> @@ -3418,10 +3436,6 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage)
>>  	unlock_cluster_or_swap_info(p, ci);
>>  out:
>>  	return err;
>> -
>> -bad_file:
>> -	pr_err("swap_dup: %s%08lx\n", Bad_file, entry.val);
>> -	goto out;
>>  }
>>  
>>  /*
>> -- 
>> 2.14.2
>> 

--
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>

Re: [PATCH -mm] mm, swap: Fix false error message in __swp_swapcount()

[Michal Hocko]

On Tue 24-10-17 23:15:32, Huang, Ying wrote:
> Hi, Michal,
> 
> Michal Hocko <mhocko@kernel.org> writes:
> 
> > On Tue 24-10-17 10:47:00, Huang, Ying wrote:
> >> From: Ying Huang <ying.huang@intel.com>
> >> 
> >> __swp_swapcount() is used in __read_swap_cache_async().  Where the
> >> invalid swap entry (offset > max) may be supplied during swap
> >> readahead.  But __swp_swapcount() will print error message for these
> >> expected invalid swap entry as below, which will make the users
> >> confusing.
> >   ^^
> > confused... And I have to admit this changelog has left me confused as
> > well. What is an invalid swap entry in the readahead? Ohh, let me
> > re-real Fixes: commit. It didn't really help "We can avoid needlessly
> > allocating page for swap slots that are not used by anyone.  No pages
> > have to be read in for these slots."
> >
> > Could you be more specific about when and how this happens please?
> 
> Sorry for confusing.
> 
> When page fault occurs for a swap entry, the original swap readahead
> (not new VMA base swap readahead) may readahead several swap entries
> after the fault swap entry.  The readahead algorithm calculates some of
> the swap entries to readahead via increasing the offset of the fault
> swap entry without checking whether they are beyond the end of the swap
> device and it rely on the __swp_swapcount() and swapcache_prepare() to
> check it.  Although __swp_swapcount() checks for the swap entry passed
> in, it will complain with error message for the expected invalid swap
> entry.  This makes the end user confusing.
> 
> Is this a little clearer.

yes, this makes more sense (modulo the same typo ;)). Can you make this
information into the changelog please? Thanks.
-- 
Michal Hocko
SUSE Labs

--
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>

Re: [PATCH -mm] mm, swap: Fix false error message in __swp_swapcount()

[Huang, Ying]

Michal Hocko <mhocko@kernel.org> writes:

> On Tue 24-10-17 23:15:32, Huang, Ying wrote:
>> Hi, Michal,
>> 
>> Michal Hocko <mhocko@kernel.org> writes:
>> 
>> > On Tue 24-10-17 10:47:00, Huang, Ying wrote:
>> >> From: Ying Huang <ying.huang@intel.com>
>> >> 
>> >> __swp_swapcount() is used in __read_swap_cache_async().  Where the
>> >> invalid swap entry (offset > max) may be supplied during swap
>> >> readahead.  But __swp_swapcount() will print error message for these
>> >> expected invalid swap entry as below, which will make the users
>> >> confusing.
>> >   ^^
>> > confused... And I have to admit this changelog has left me confused as
>> > well. What is an invalid swap entry in the readahead? Ohh, let me
>> > re-real Fixes: commit. It didn't really help "We can avoid needlessly
>> > allocating page for swap slots that are not used by anyone.  No pages
>> > have to be read in for these slots."
>> >
>> > Could you be more specific about when and how this happens please?
>> 
>> Sorry for confusing.
>> 
>> When page fault occurs for a swap entry, the original swap readahead
>> (not new VMA base swap readahead) may readahead several swap entries
>> after the fault swap entry.  The readahead algorithm calculates some of
>> the swap entries to readahead via increasing the offset of the fault
>> swap entry without checking whether they are beyond the end of the swap
>> device and it rely on the __swp_swapcount() and swapcache_prepare() to
>> check it.  Although __swp_swapcount() checks for the swap entry passed
>> in, it will complain with error message for the expected invalid swap
>> entry.  This makes the end user confusing.
>> 
>> Is this a little clearer.
>
> yes, this makes more sense (modulo the same typo ;)). Can you make this
> information into the changelog please? Thanks.

Oh, Yes!  I should fix it.  Sure, I will add this into the changelog.

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>

Re: [PATCH -mm] mm, swap: Fix false error message in __swp_swapcount()

[Minchan Kim]

On Tue, Oct 24, 2017 at 10:47:00AM +0800, Huang, Ying wrote:
> From: Ying Huang <ying.huang@intel.com>
> 
> __swp_swapcount() is used in __read_swap_cache_async().  Where the
> invalid swap entry (offset > max) may be supplied during swap
> readahead.  But __swp_swapcount() will print error message for these
> expected invalid swap entry as below, which will make the users
> confusing.
> 
>   swap_info_get: Bad swap offset entry 0200f8a7
> 
> So the swap entry checking code in __swp_swapcount() is changed to
> avoid printing error message for it.  To avoid to duplicate code with
> __swap_duplicate(), a new helper function named
> __swap_info_get_silence() is added and invoked in both places.

It's the problem caused by readahead, not __swap_info_get which is low-end
primitive function. Instead, please fix high-end swapin_readahead to limit
to last valid block as handling to avoid swap header which is special case,
too.

--
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>

Re: [PATCH -mm] mm, swap: Fix false error message in __swp_swapcount()

[Huang, Ying]

Minchan Kim <minchan@kernel.org> writes:

> On Tue, Oct 24, 2017 at 10:47:00AM +0800, Huang, Ying wrote:
>> From: Ying Huang <ying.huang@intel.com>
>> 
>> __swp_swapcount() is used in __read_swap_cache_async().  Where the
>> invalid swap entry (offset > max) may be supplied during swap
>> readahead.  But __swp_swapcount() will print error message for these
>> expected invalid swap entry as below, which will make the users
>> confusing.
>> 
>>   swap_info_get: Bad swap offset entry 0200f8a7
>> 
>> So the swap entry checking code in __swp_swapcount() is changed to
>> avoid printing error message for it.  To avoid to duplicate code with
>> __swap_duplicate(), a new helper function named
>> __swap_info_get_silence() is added and invoked in both places.
>
> It's the problem caused by readahead, not __swap_info_get which is low-end
> primitive function. Instead, please fix high-end swapin_readahead to limit
> to last valid block as handling to avoid swap header which is special case,
> too.

Yes.  You are right, will send the new version.

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>