[PATCH v2 0/2] Fixes to the hibernate_preallocate_memory()

[PATCH v2 0/2] Fixes to the hibernate_preallocate_memory()

[TGSP]

Changes in v2:
- Optimize code to resolve compilation warnings;
- Add comments, add debug data, easy to view and modify.

v1:
hibernate_preallocate_memory() function is still quite obscure, can
anyone add a little more description?

It seems that the max_size calculation formula is wrong in the comment
part, correct it;

It is also found that when mem preallocate is not enough, it goes
directly down without checking.

xiongxin (2):
  PM: hibernate: fix spelling mistake for annotation
  PM: hibernate: add check of preallocate mem for image size pages

 kernel/power/snapshot.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] PM: hibernate: fix spelling mistake for annotation

[TGSP]

From: xiongxin <xiongxin@kylinos.cn>

The actual calculation formula in the code below is:

max_size = (count - (size + PAGES_FOR_IO)) / 2
	    - 2 * DIV_ROUND_UP(reserved_size, PAGE_SIZE);

But function comments are written differently, the comment is wrong?

By the way, what exactly do the "/ 2" and "2 *" mean?

Cc: stable@vger.kernel.org
Signed-off-by: xiongxin <xiongxin@kylinos.cn>
---
 kernel/power/snapshot.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 2a406753af90..c20ca5fb9adc 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1723,8 +1723,8 @@ static unsigned long minimum_image_size(unsigned long saveable)
  * /sys/power/reserved_size, respectively).  To make this happen, we compute the
  * total number of available page frames and allocate at least
  *
- * ([page frames total] + PAGES_FOR_IO + [metadata pages]) / 2
- *  + 2 * DIV_ROUND_UP(reserved_size, PAGE_SIZE)
+ * ([page frames total] - PAGES_FOR_IO - [metadata pages]) / 2
+ *  - 2 * DIV_ROUND_UP(reserved_size, PAGE_SIZE)
  *
  * of them, which corresponds to the maximum size of a hibernation image.
  *
-- 
2.25.1

Re: [PATCH v2 1/2] PM: hibernate: fix spelling mistake for annotation

[Greg KH]

On Fri, Nov 04, 2022 at 01:41:18PM +0800, TGSP wrote:
> From: xiongxin <xiongxin@kylinos.cn>
> 
> The actual calculation formula in the code below is:
> 
> max_size = (count - (size + PAGES_FOR_IO)) / 2
> 	    - 2 * DIV_ROUND_UP(reserved_size, PAGE_SIZE);
> 
> But function comments are written differently, the comment is wrong?
> 
> By the way, what exactly do the "/ 2" and "2 *" mean?
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: xiongxin <xiongxin@kylinos.cn>

Please do not use an anonymous gmail account for your corporate work
like this.  Work with your company email admins to allow you to send
patches from that address so that they can be verified to actually come
from there.

thanks,

greg k-h

Re: [PATCH v2 1/2] PM: hibernate: fix spelling mistake for annotation

[TGSP]

在 2022/11/4 17:18, Greg KH 写道:
> On Fri, Nov 04, 2022 at 01:41:18PM +0800, TGSP wrote:
>> From: xiongxin <xiongxin@kylinos.cn>
>>
>> The actual calculation formula in the code below is:
>>
>> max_size = (count - (size + PAGES_FOR_IO)) / 2
>> 	    - 2 * DIV_ROUND_UP(reserved_size, PAGE_SIZE);
>>
>> But function comments are written differently, the comment is wrong?
>>
>> By the way, what exactly do the "/ 2" and "2 *" mean?
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: xiongxin <xiongxin@kylinos.cn>
> 
> Please do not use an anonymous gmail account for your corporate work
> like this.  Work with your company email admins to allow you to send
> patches from that address so that they can be verified to actually come
> from there.
> 
> thanks,
> 
> greg k-h

I also wanted to send it directly through the company mailbox, but those 
leaders didn't take it seriously.

Next time I don't use the company email and submit patches as I can as a 
freelancer.

[PATCH v2 2/2] PM: hibernate: add check of preallocate mem for image size pages

[TGSP]

From: xiongxin <xiongxin@kylinos.cn>

Added a check on the return value of preallocate_image_highmem(). If
memory preallocate is insufficient, S4 cannot be done;

I am playing 4K video on a machine with AMD or other graphics card and
only 8GiB memory, and the kernel is not configured with CONFIG_HIGHMEM.
When doing the S4 test, the analysis found that when the pages get from
minimum_image_size() is large enough, The preallocate_image_memory() and
preallocate_image_highmem() calls failed to obtain enough memory. Add
the judgment that memory preallocate is insufficient;

The detailed debugging data is as follows:

image_size: 3225923584, totalram_pages: 1968948 in
hibernate_reserved_size_init();

in hibernate_preallocate_memory():
code pages = minimum_image_size(saveable) = 717992, at this time(line):
count: 2030858
avail_normal: 2053753
highmem: 0
totalreserve_pages: 22895
max_size: 1013336
size: 787579
saveable: 1819905

When the code executes to:
pages = preallocate_image_memory(alloc, avail_normal), at that
time(line):
pages_highmem: 0
avail_normal: 1335761
alloc: 1017522
pages: 1017522

So enter the else branch judged by (pages < alloc), When executed to
size = preallocate_image_memory(alloc, avail_normal):
alloc = max_size - size = 225757;
size = preallocate_image_memory(alloc, avail_normal) = 168671, That is,
preallocate_image_memory() does not apply for all alloc memory pages,
because highmem is not enabled, and size_highmem will return 0 here, so
there is a memory page that has not been preallocated, so I think a
judgment needs to be added here.

But what I can't understand is that although pages are not preallocated
enough, "pages -= free_unnecessary_pages()" in the code below can also
discard some pages that have been preallocated, so I am not sure whether
it is appropriate to add a judgment here.

Cc: stable@vger.kernel.org
Signed-off-by: xiongxin <xiongxin@kylinos.cn>
Signed-off-by: huanglei <huanglei@kylinos.cn>
---
 kernel/power/snapshot.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index c20ca5fb9adc..546d544cf7de 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1854,6 +1854,8 @@ int hibernate_preallocate_memory(void)
 		alloc = (count - pages) - size;
 		pages += preallocate_image_highmem(alloc);
 	} else {
+		unsigned long size_highmem = 0;
+
 		/*
 		 * There are approximately max_size saveable pages at this point
 		 * and we want to reduce this number down to size.
@@ -1863,8 +1865,13 @@ int hibernate_preallocate_memory(void)
 		pages_highmem += size;
 		alloc -= size;
 		size = preallocate_image_memory(alloc, avail_normal);
-		pages_highmem += preallocate_image_highmem(alloc - size);
-		pages += pages_highmem + size;
+		size_highmem = preallocate_image_highmem(alloc - size);
+		if (size_highmem < (alloc - size)) {
+			pr_err("Image allocation is %lu pages short, exit\n",
+				alloc - size - pages_highmem);
+			goto err_out;
+		}
+		pages += pages_highmem + size_highmem + size;
 	}
 
 	/*
-- 
2.25.1

Re: [PATCH v2 2/2] PM: hibernate: add check of preallocate mem for image size pages

[Jiri Slaby]

On 04. 11. 22, 6:41, TGSP wrote:
> From: xiongxin <xiongxin@kylinos.cn>
> 
> Added a check on the return value of preallocate_image_highmem(). If
> memory preallocate is insufficient, S4 cannot be done;
> 
> I am playing 4K video on a machine with AMD or other graphics card and
> only 8GiB memory, and the kernel is not configured with CONFIG_HIGHMEM.
> When doing the S4 test, the analysis found that when the pages get from
> minimum_image_size() is large enough, The preallocate_image_memory() and
> preallocate_image_highmem() calls failed to obtain enough memory. Add
> the judgment that memory preallocate is insufficient;
> 
> The detailed debugging data is as follows:
> 
> image_size: 3225923584, totalram_pages: 1968948 in
> hibernate_reserved_size_init();
> 
> in hibernate_preallocate_memory():
> code pages = minimum_image_size(saveable) = 717992, at this time(line):
> count: 2030858
> avail_normal: 2053753
> highmem: 0
> totalreserve_pages: 22895
> max_size: 1013336
> size: 787579
> saveable: 1819905
> 
> When the code executes to:
> pages = preallocate_image_memory(alloc, avail_normal), at that
> time(line):
> pages_highmem: 0
> avail_normal: 1335761
> alloc: 1017522
> pages: 1017522
> 
> So enter the else branch judged by (pages < alloc), When executed to
> size = preallocate_image_memory(alloc, avail_normal):
> alloc = max_size - size = 225757;
> size = preallocate_image_memory(alloc, avail_normal) = 168671, That is,
> preallocate_image_memory() does not apply for all alloc memory pages,
> because highmem is not enabled, and size_highmem will return 0 here, so
> there is a memory page that has not been preallocated, so I think a
> judgment needs to be added here.
> 
> But what I can't understand is that although pages are not preallocated
> enough, "pages -= free_unnecessary_pages()" in the code below can also
> discard some pages that have been preallocated, so I am not sure whether
> it is appropriate to add a judgment here.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: xiongxin <xiongxin@kylinos.cn>
> Signed-off-by: huanglei <huanglei@kylinos.cn>
> ---
>   kernel/power/snapshot.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index c20ca5fb9adc..546d544cf7de 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -1854,6 +1854,8 @@ int hibernate_preallocate_memory(void)
>   		alloc = (count - pages) - size;
>   		pages += preallocate_image_highmem(alloc);
>   	} else {
> +		unsigned long size_highmem = 0;

This needs not be initialized, right?

> @@ -1863,8 +1865,13 @@ int hibernate_preallocate_memory(void)
>   		pages_highmem += size;
>   		alloc -= size;
>   		size = preallocate_image_memory(alloc, avail_normal);
> -		pages_highmem += preallocate_image_highmem(alloc - size);
> -		pages += pages_highmem + size;
> +		size_highmem = preallocate_image_highmem(alloc - size);
> +		if (size_highmem < (alloc - size)) {
> +			pr_err("Image allocation is %lu pages short, exit\n",
> +				alloc - size - pages_highmem);
> +			goto err_out;
> +		}
> +		pages += pages_highmem + size_highmem + size;
>   	}
>   
>   	/*

-- 
js
suse labs

Re: [PATCH v2 2/2] PM: hibernate: add check of preallocate mem for image size pages

[TGSP]

在 2022/11/4 14:25, Jiri Slaby 写道:
> On 04. 11. 22, 6:41, TGSP wrote:
>> From: xiongxin <xiongxin@kylinos.cn>
>>
>> Added a check on the return value of preallocate_image_highmem(). If
>> memory preallocate is insufficient, S4 cannot be done;
>>
>> I am playing 4K video on a machine with AMD or other graphics card and
>> only 8GiB memory, and the kernel is not configured with CONFIG_HIGHMEM.
>> When doing the S4 test, the analysis found that when the pages get from
>> minimum_image_size() is large enough, The preallocate_image_memory() and
>> preallocate_image_highmem() calls failed to obtain enough memory. Add
>> the judgment that memory preallocate is insufficient;
>>
>> The detailed debugging data is as follows:
>>
>> image_size: 3225923584, totalram_pages: 1968948 in
>> hibernate_reserved_size_init();
>>
>> in hibernate_preallocate_memory():
>> code pages = minimum_image_size(saveable) = 717992, at this time(line):
>> count: 2030858
>> avail_normal: 2053753
>> highmem: 0
>> totalreserve_pages: 22895
>> max_size: 1013336
>> size: 787579
>> saveable: 1819905
>>
>> When the code executes to:
>> pages = preallocate_image_memory(alloc, avail_normal), at that
>> time(line):
>> pages_highmem: 0
>> avail_normal: 1335761
>> alloc: 1017522
>> pages: 1017522
>>
>> So enter the else branch judged by (pages < alloc), When executed to
>> size = preallocate_image_memory(alloc, avail_normal):
>> alloc = max_size - size = 225757;
>> size = preallocate_image_memory(alloc, avail_normal) = 168671, That is,
>> preallocate_image_memory() does not apply for all alloc memory pages,
>> because highmem is not enabled, and size_highmem will return 0 here, so
>> there is a memory page that has not been preallocated, so I think a
>> judgment needs to be added here.
>>
>> But what I can't understand is that although pages are not preallocated
>> enough, "pages -= free_unnecessary_pages()" in the code below can also
>> discard some pages that have been preallocated, so I am not sure whether
>> it is appropriate to add a judgment here.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: xiongxin <xiongxin@kylinos.cn>
>> Signed-off-by: huanglei <huanglei@kylinos.cn>
>> ---
>>   kernel/power/snapshot.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
>> index c20ca5fb9adc..546d544cf7de 100644
>> --- a/kernel/power/snapshot.c
>> +++ b/kernel/power/snapshot.c
>> @@ -1854,6 +1854,8 @@ int hibernate_preallocate_memory(void)
>>           alloc = (count - pages) - size;
>>           pages += preallocate_image_highmem(alloc);
>>       } else {
>> +        unsigned long size_highmem = 0;
> 
> This needs not be initialized, right?

If there is no need to make judgments here, then in the (pages < alloc) 
branch, it is necessary to make judgments on (pages_highmem < alloc)? 
should be the same;

> 
>> @@ -1863,8 +1865,13 @@ int hibernate_preallocate_memory(void)
>>           pages_highmem += size;
>>           alloc -= size;
>>           size = preallocate_image_memory(alloc, avail_normal);
>> -        pages_highmem += preallocate_image_highmem(alloc - size);
>> -        pages += pages_highmem + size;
>> +        size_highmem = preallocate_image_highmem(alloc - size);
>> +        if (size_highmem < (alloc - size)) {
>> +            pr_err("Image allocation is %lu pages short, exit\n",
>> +                alloc - size - pages_highmem);
>> +            goto err_out;
>> +        }
>> +        pages += pages_highmem + size_highmem + size;
>>       }
>>       /*
> 

[PATCH v2 1/2] PM: hibernate: fix spelling mistake for annotation

[xiongxin]

The actual calculation formula in the code below is:

max_size = (count - (size + PAGES_FOR_IO)) / 2
	    - 2 * DIV_ROUND_UP(reserved_size, PAGE_SIZE);

But function comments are written differently, the comment is wrong?

By the way, what exactly do the "/ 2" and "2 *" mean?

Cc: stable@vger.kernel.org
Signed-off-by: xiongxin <xiongxin@kylinos.cn>
---
 kernel/power/snapshot.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 2a406753af90..c20ca5fb9adc 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1723,8 +1723,8 @@ static unsigned long minimum_image_size(unsigned long saveable)
  * /sys/power/reserved_size, respectively).  To make this happen, we compute the
  * total number of available page frames and allocate at least
  *
- * ([page frames total] + PAGES_FOR_IO + [metadata pages]) / 2
- *  + 2 * DIV_ROUND_UP(reserved_size, PAGE_SIZE)
+ * ([page frames total] - PAGES_FOR_IO - [metadata pages]) / 2
+ *  - 2 * DIV_ROUND_UP(reserved_size, PAGE_SIZE)
  *
  * of them, which corresponds to the maximum size of a hibernation image.
  *
-- 
2.25.1