diff mbox series

[-next,2/2] PM: hibernate: add check of preallocate mem for image size pages

Message ID 20221101022840.1351163-3-tgsp002@gmail.com
State New
Headers
Series Fixes to the hibernate_preallocate_memory() |

Commit Message

TGSP Nov. 1, 2022, 2:28 a.m. UTC 
  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;

"pages -= free_unnecessary_pages()" below will let pages to drop a lot,
so I wonder if it makes sense 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 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

kernel test robot Nov. 1, 2022, 9:23 p.m. UTC | #1 
Hi TGSP,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20221031]

url:    https://github.com/intel-lab-lkp/linux/commits/TGSP/Fixes-to-the-hibernate_preallocate_memory/20221101-105416
patch link:    https://lore.kernel.org/r/20221101022840.1351163-3-tgsp002%40gmail.com
patch subject: [PATCH -next 2/2] PM: hibernate: add check of preallocate mem for image size pages
config: i386-randconfig-a011
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/1c277961f43154d671520086d9626e081e072b90
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review TGSP/Fixes-to-the-hibernate_preallocate_memory/20221101-105416
        git checkout 1c277961f43154d671520086d9626e081e072b90
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash kernel/power/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> kernel/power/snapshot.c:1867:3: warning: variable 'size_highmem' is uninitialized when used here [-Wuninitialized]
                   size_highmem += preallocate_image_highmem(alloc - size);
                   ^~~~~~~~~~~~
   kernel/power/snapshot.c:1741:28: note: initialize the variable 'size_highmem' to silence this warning
           unsigned long size_highmem;
                                     ^
                                      = 0
   1 warning generated.


vim +/size_highmem +1867 kernel/power/snapshot.c

  1713	
  1714	/**
  1715	 * hibernate_preallocate_memory - Preallocate memory for hibernation image.
  1716	 *
  1717	 * To create a hibernation image it is necessary to make a copy of every page
  1718	 * frame in use.  We also need a number of page frames to be free during
  1719	 * hibernation for allocations made while saving the image and for device
  1720	 * drivers, in case they need to allocate memory from their hibernation
  1721	 * callbacks (these two numbers are given by PAGES_FOR_IO (which is a rough
  1722	 * estimate) and reserved_size divided by PAGE_SIZE (which is tunable through
  1723	 * /sys/power/reserved_size, respectively).  To make this happen, we compute the
  1724	 * total number of available page frames and allocate at least
  1725	 *
  1726	 * ([page frames total] - PAGES_FOR_IO - [metadata pages]) / 2
  1727	 *  - 2 * DIV_ROUND_UP(reserved_size, PAGE_SIZE)
  1728	 *
  1729	 * of them, which corresponds to the maximum size of a hibernation image.
  1730	 *
  1731	 * If image_size is set below the number following from the above formula,
  1732	 * the preallocation of memory is continued until the total number of saveable
  1733	 * pages in the system is below the requested image size or the minimum
  1734	 * acceptable image size returned by minimum_image_size(), whichever is greater.
  1735	 */
  1736	int hibernate_preallocate_memory(void)
  1737	{
  1738		struct zone *zone;
  1739		unsigned long saveable, size, max_size, count, highmem, pages = 0;
  1740		unsigned long alloc, save_highmem, pages_highmem, avail_normal;
  1741		unsigned long size_highmem;
  1742		ktime_t start, stop;
  1743		int error;
  1744	
  1745		pr_info("Preallocating image memory\n");
  1746		start = ktime_get();
  1747	
  1748		error = memory_bm_create(&orig_bm, GFP_IMAGE, PG_ANY);
  1749		if (error) {
  1750			pr_err("Cannot allocate original bitmap\n");
  1751			goto err_out;
  1752		}
  1753	
  1754		error = memory_bm_create(&copy_bm, GFP_IMAGE, PG_ANY);
  1755		if (error) {
  1756			pr_err("Cannot allocate copy bitmap\n");
  1757			goto err_out;
  1758		}
  1759	
  1760		alloc_normal = 0;
  1761		alloc_highmem = 0;
  1762	
  1763		/* Count the number of saveable data pages. */
  1764		save_highmem = count_highmem_pages();
  1765		saveable = count_data_pages();
  1766	
  1767		/*
  1768		 * Compute the total number of page frames we can use (count) and the
  1769		 * number of pages needed for image metadata (size).
  1770		 */
  1771		count = saveable;
  1772		saveable += save_highmem;
  1773		highmem = save_highmem;
  1774		size = 0;
  1775		for_each_populated_zone(zone) {
  1776			size += snapshot_additional_pages(zone);
  1777			if (is_highmem(zone))
  1778				highmem += zone_page_state(zone, NR_FREE_PAGES);
  1779			else
  1780				count += zone_page_state(zone, NR_FREE_PAGES);
  1781		}
  1782		avail_normal = count;
  1783		count += highmem;
  1784		count -= totalreserve_pages;
  1785	
  1786		/* Compute the maximum number of saveable pages to leave in memory. */
  1787		max_size = (count - (size + PAGES_FOR_IO)) / 2
  1788				- 2 * DIV_ROUND_UP(reserved_size, PAGE_SIZE);
  1789		/* Compute the desired number of image pages specified by image_size. */
  1790		size = DIV_ROUND_UP(image_size, PAGE_SIZE);
  1791		if (size > max_size)
  1792			size = max_size;
  1793		/*
  1794		 * If the desired number of image pages is at least as large as the
  1795		 * current number of saveable pages in memory, allocate page frames for
  1796		 * the image and we're done.
  1797		 */
  1798		if (size >= saveable) {
  1799			pages = preallocate_image_highmem(save_highmem);
  1800			pages += preallocate_image_memory(saveable - pages, avail_normal);
  1801			goto out;
  1802		}
  1803	
  1804		/* Estimate the minimum size of the image. */
  1805		pages = minimum_image_size(saveable);
  1806		/*
  1807		 * To avoid excessive pressure on the normal zone, leave room in it to
  1808		 * accommodate an image of the minimum size (unless it's already too
  1809		 * small, in which case don't preallocate pages from it at all).
  1810		 */
  1811		if (avail_normal > pages)
  1812			avail_normal -= pages;
  1813		else
  1814			avail_normal = 0;
  1815		if (size < pages)
  1816			size = min_t(unsigned long, pages, max_size);
  1817	
  1818		/*
  1819		 * Let the memory management subsystem know that we're going to need a
  1820		 * large number of page frames to allocate and make it free some memory.
  1821		 * NOTE: If this is not done, performance will be hurt badly in some
  1822		 * test cases.
  1823		 */
  1824		shrink_all_memory(saveable - size);
  1825	
  1826		/*
  1827		 * The number of saveable pages in memory was too high, so apply some
  1828		 * pressure to decrease it.  First, make room for the largest possible
  1829		 * image and fail if that doesn't work.  Next, try to decrease the size
  1830		 * of the image as much as indicated by 'size' using allocations from
  1831		 * highmem and non-highmem zones separately.
  1832		 */
  1833		pages_highmem = preallocate_image_highmem(highmem / 2);
  1834		alloc = count - max_size;
  1835		if (alloc > pages_highmem)
  1836			alloc -= pages_highmem;
  1837		else
  1838			alloc = 0;
  1839		pages = preallocate_image_memory(alloc, avail_normal);
  1840		if (pages < alloc) {
  1841			/* We have exhausted non-highmem pages, try highmem. */
  1842			alloc -= pages;
  1843			pages += pages_highmem;
  1844			pages_highmem = preallocate_image_highmem(alloc);
  1845			if (pages_highmem < alloc) {
  1846				pr_err("Image allocation is %lu pages short\n",
  1847					alloc - pages_highmem);
  1848				goto err_out;
  1849			}
  1850			pages += pages_highmem;
  1851			/*
  1852			 * size is the desired number of saveable pages to leave in
  1853			 * memory, so try to preallocate (all memory - size) pages.
  1854			 */
  1855			alloc = (count - pages) - size;
  1856			pages += preallocate_image_highmem(alloc);
  1857		} else {
  1858			/*
  1859			 * There are approximately max_size saveable pages at this point
  1860			 * and we want to reduce this number down to size.
  1861			 */
  1862			alloc = max_size - size;
  1863			size = preallocate_highmem_fraction(alloc, highmem, count);
  1864			pages_highmem += size;
  1865			alloc -= size;
  1866			size = preallocate_image_memory(alloc, avail_normal);
> 1867			size_highmem += preallocate_image_highmem(alloc - size);
  1868			if (size_highmem < (alloc - size)) {
  1869				pr_err("Image allocation is %lu pages short, exit\n",
  1870					alloc - size - pages_highmem);
  1871				goto err_out;
  1872			}
  1873			pages_highmem += size_highmem;
  1874			pages += pages_highmem + size;
  1875		}
  1876	
  1877		/*
  1878		 * We only need as many page frames for the image as there are saveable
  1879		 * pages in memory, but we have allocated more.  Release the excessive
  1880		 * ones now.
  1881		 */
  1882		pages -= free_unnecessary_pages();
  1883	
  1884	 out:
  1885		stop = ktime_get();
  1886		pr_info("Allocated %lu pages for snapshot\n", pages);
  1887		swsusp_show_speed(start, stop, pages, "Allocated");
  1888	
  1889		return 0;
  1890	
  1891	 err_out:
  1892		swsusp_free();
  1893		return -ENOMEM;
  1894	}
  1895
Rafael J. Wysocki Nov. 3, 2022, 4:47 p.m. UTC | #2 
On Tue, Nov 1, 2022 at 3:28 AM TGSP <tgsp002@gmail.com> 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;
>
> "pages -= free_unnecessary_pages()" below will let pages to drop a lot,
> so I wonder if it makes sense 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 | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index c20ca5fb9adc..670abf89cf31 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -1738,6 +1738,7 @@ int hibernate_preallocate_memory(void)
>         struct zone *zone;
>         unsigned long saveable, size, max_size, count, highmem, pages = 0;
>         unsigned long alloc, save_highmem, pages_highmem, avail_normal;
> +       unsigned long size_highmem;

Please define this in the block where it will be used.

>         ktime_t start, stop;
>         int error;
>
> @@ -1863,7 +1864,13 @@ int hibernate_preallocate_memory(void)
>                 pages_highmem += size;

The line above can be dropped.

>                 alloc -= size;
>                 size = preallocate_image_memory(alloc, avail_normal);
> -               pages_highmem += preallocate_image_highmem(alloc - size);
> +               size_highmem += preallocate_image_highmem(alloc - size);

Did you mean "="?

Assuming you did, this could be

        size_highmem = size + preallocate_image_highmem(alloc - size);
        if (size_highmem < alloc) {

> +               if (size_highmem < (alloc - size)) {

The inner parens were not necessary.

> +                       pr_err("Image allocation is %lu pages short, exit\n",
> +                               alloc - size - pages_highmem);
> +                       goto err_out;
> +               }
> +               pages_highmem += size_highmem;
>                 pages += pages_highmem + size;
>         }
>
> --

But overall it would be better to avoid bailing out.
Rafael J. Wysocki Nov. 3, 2022, 4:57 p.m. UTC | #3 
On Thu, Nov 3, 2022 at 5:47 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Nov 1, 2022 at 3:28 AM TGSP <tgsp002@gmail.com> 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;
> >
> > "pages -= free_unnecessary_pages()" below will let pages to drop a lot,
> > so I wonder if it makes sense 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 | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> > index c20ca5fb9adc..670abf89cf31 100644
> > --- a/kernel/power/snapshot.c
> > +++ b/kernel/power/snapshot.c
> > @@ -1738,6 +1738,7 @@ int hibernate_preallocate_memory(void)
> >         struct zone *zone;
> >         unsigned long saveable, size, max_size, count, highmem, pages = 0;
> >         unsigned long alloc, save_highmem, pages_highmem, avail_normal;
> > +       unsigned long size_highmem;
>
> Please define this in the block where it will be used.
>
> >         ktime_t start, stop;
> >         int error;
> >
> > @@ -1863,7 +1864,13 @@ int hibernate_preallocate_memory(void)
> >                 pages_highmem += size;
>
> The line above can be dropped.

Not really, it needs to be replaced with

        size_highmem = size ;

> >                 alloc -= size;
> >                 size = preallocate_image_memory(alloc, avail_normal);
> > -               pages_highmem += preallocate_image_highmem(alloc - size);
> > +               size_highmem += preallocate_image_highmem(alloc - size);
>
> Did you mean "="?
>
> Assuming you did, this could be
>
>         size_highmem = size + preallocate_image_highmem(alloc - size);

And here

                  size_highmem += preallocate_image_highmem(alloc - size);

which is what you had in the original patch.

>         if (size_highmem < alloc) {
>
> > +               if (size_highmem < (alloc - size)) {
>
> The inner parens were not necessary.
>
> > +                       pr_err("Image allocation is %lu pages short, exit\n",
> > +                               alloc - size - pages_highmem);
> > +                       goto err_out;
> > +               }
> > +               pages_highmem += size_highmem;
> >                 pages += pages_highmem + size;
> >         }
> >
> > --
>
> But overall it would be better to avoid bailing out.
Rafael J. Wysocki Nov. 3, 2022, 5:11 p.m. UTC | #4 
On Tue, Nov 1, 2022 at 3:28 AM TGSP <tgsp002@gmail.com> 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;

So I'm not sure what the problem is.  Can you please explain it in more detail?

The if (pages < alloc) appears to be false in your case, so there
should be enough free pages to create an image.

Maybe reserved_size is too low?
diff mbox series

Patch

diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index c20ca5fb9adc..670abf89cf31 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1738,6 +1738,7 @@  int hibernate_preallocate_memory(void)
 	struct zone *zone;
 	unsigned long saveable, size, max_size, count, highmem, pages = 0;
 	unsigned long alloc, save_highmem, pages_highmem, avail_normal;
+	unsigned long size_highmem;
 	ktime_t start, stop;
 	int error;
 
@@ -1863,7 +1864,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);
+		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_highmem += size_highmem;
 		pages += pages_highmem + size;
 	}