[PATCH 0/3] Shrink time spent in copy_data_pages()

[PATCH 0/3] Shrink time spent in copy_data_pages()

[Nícolas F. R. A. Prado]

This series shrinks the time spent in copy_data_pages(). Patches 1 and 2
are to ease debugging the time spent in that function, while patch 3
does the actual work to allow it to be shorter - based on a module
parameter since it has drawbacks for uncompressed hibernation images.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---
Nícolas F. R. A. Prado (3):
      PM: hibernate: Print speed statistics of copy_data_pages()
      PM: hibernate: Add platform_enter hibernation test level
      PM: hibernate: Allow disabling zero page check in copy_data_pages()

 kernel/power/hibernate.c | 13 +++++++++++--
 kernel/power/main.c      |  1 +
 kernel/power/power.h     |  1 +
 kernel/power/snapshot.c  | 42 +++++++++++++++++++++++++++++++-----------
 4 files changed, 44 insertions(+), 13 deletions(-)
---
base-commit: ed46b8c4c78fc332340a084eceeb10d762a9cb6d
change-id: 20260518-hibernation-decrease-time-in-copy-data-pages-f9435814666b

Best regards,
-- 
Nícolas F. R. A. Prado <nfraprado@collabora.com>

[PATCH 1/3] PM: hibernate: Print speed statistics of copy_data_pages()

[Nícolas F. R. A. Prado]

copy_data_pages() can take a long (multi-second) time to finish, and
currently the only indication of that is the timestamp difference
between print messages right before and right after. The timestamp is
also immediately reset afterwards to the time before image creation,
making it even harder to spot this delay. And this function runs in a
critical section with a single CPU online and syscore suspended, so it
should be kept as quick as possible to keep the system responsive.

Call into swsusp_show_speed() to report the amount of data, time taken,
and copy speed of copy_data_pages() to make it easier to spot delays and
verify performance improvements. The current time is obtained through
sched_clock() instead of ktime_get() since timekeeping is suspended in
this region.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---
 kernel/power/snapshot.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index e249e5786fbc..4f452baf31dc 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -13,6 +13,7 @@
 #include <linux/version.h>
 #include <linux/module.h>
 #include <linux/mm.h>
+#include <linux/sched/clock.h>
 #include <linux/suspend.h>
 #include <linux/delay.h>
 #include <linux/bitops.h>
@@ -2109,6 +2110,7 @@ static int swsusp_alloc(struct memory_bitmap *copy_bm,
 asmlinkage __visible int swsusp_save(void)
 {
 	unsigned int nr_pages, nr_highmem;
+	ktime_t start, stop;
 
 	pr_info("Creating image:\n");
 
@@ -2132,7 +2134,10 @@ asmlinkage __visible int swsusp_save(void)
 	 * Kill them.
 	 */
 	drain_local_pages(NULL);
+	start = ns_to_ktime(sched_clock());
 	nr_copy_pages = copy_data_pages(&copy_bm, &orig_bm, &zero_bm);
+	stop = ns_to_ktime(sched_clock());
+	swsusp_show_speed(start, stop, nr_copy_pages, "Copied");
 
 	/*
 	 * End of critical section. From now on, we can write to memory,

-- 
2.53.0

Re: [PATCH 1/3] PM: hibernate: Print speed statistics of copy_data_pages()

[Rafael J. Wysocki]

On Mon, May 18, 2026 at 11:34 PM Nícolas F. R. A. Prado
<nfraprado@collabora.com> wrote:
>
> copy_data_pages() can take a long (multi-second) time to finish, and
> currently the only indication of that is the timestamp difference
> between print messages right before and right after. The timestamp is
> also immediately reset afterwards to the time before image creation,
> making it even harder to spot this delay. And this function runs in a
> critical section with a single CPU online and syscore suspended, so it
> should be kept as quick as possible to keep the system responsive.
>
> Call into swsusp_show_speed() to report the amount of data, time taken,
> and copy speed of copy_data_pages() to make it easier to spot delays and
> verify performance improvements. The current time is obtained through
> sched_clock() instead of ktime_get() since timekeeping is suspended in
> this region.

Why not use local_clock() for this?

> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> ---
>  kernel/power/snapshot.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index e249e5786fbc..4f452baf31dc 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -13,6 +13,7 @@
>  #include <linux/version.h>
>  #include <linux/module.h>
>  #include <linux/mm.h>
> +#include <linux/sched/clock.h>
>  #include <linux/suspend.h>
>  #include <linux/delay.h>
>  #include <linux/bitops.h>
> @@ -2109,6 +2110,7 @@ static int swsusp_alloc(struct memory_bitmap *copy_bm,
>  asmlinkage __visible int swsusp_save(void)
>  {
>         unsigned int nr_pages, nr_highmem;
> +       ktime_t start, stop;
>
>         pr_info("Creating image:\n");
>
> @@ -2132,7 +2134,10 @@ asmlinkage __visible int swsusp_save(void)
>          * Kill them.
>          */
>         drain_local_pages(NULL);
> +       start = ns_to_ktime(sched_clock());
>         nr_copy_pages = copy_data_pages(&copy_bm, &orig_bm, &zero_bm);
> +       stop = ns_to_ktime(sched_clock());
> +       swsusp_show_speed(start, stop, nr_copy_pages, "Copied");
>
>         /*
>          * End of critical section. From now on, we can write to memory,
>
> --
> 2.53.0
>

Re: [PATCH 1/3] PM: hibernate: Print speed statistics of copy_data_pages()

[Nícolas F. R. A. Prado]

On Tue, 2026-05-26 at 13:25 +0200, Rafael J. Wysocki wrote:
> On Mon, May 18, 2026 at 11:34 PM Nícolas F. R. A. Prado
> <nfraprado@collabora.com> wrote:
> > 
> > copy_data_pages() can take a long (multi-second) time to finish,
> > and
> > currently the only indication of that is the timestamp difference
> > between print messages right before and right after. The timestamp
> > is
> > also immediately reset afterwards to the time before image
> > creation,
> > making it even harder to spot this delay. And this function runs in
> > a
> > critical section with a single CPU online and syscore suspended, so
> > it
> > should be kept as quick as possible to keep the system responsive.
> > 
> > Call into swsusp_show_speed() to report the amount of data, time
> > taken,
> > and copy speed of copy_data_pages() to make it easier to spot
> > delays and
> > verify performance improvements. The current time is obtained
> > through
> > sched_clock() instead of ktime_get() since timekeeping is suspended
> > in
> > this region.
> 
> Why not use local_clock() for this?

No particular reason. I'll switch to it for the next version.

Thanks,
Nícolas

[PATCH 2/3] PM: hibernate: Add platform_enter hibernation test level

[Nícolas F. R. A. Prado]

Add a 'platform_enter' hibernation test level to allow testing the full
hibernation path up until the last point of return in
hibernation_platform_enter().

In particular this allows seeing print messages from the code run after
the hibernation image is created on platforms that don't have an easily
accessible serial console, since these messages are not present upon
hibernation resume.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---
 kernel/power/hibernate.c | 13 +++++++++++--
 kernel/power/main.c      |  1 +
 kernel/power/power.h     |  1 +
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 001e4556b137..15cb7a090770 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -75,6 +75,7 @@ enum {
 static int hibernation_mode = HIBERNATION_SHUTDOWN;
 
 bool freezer_test_done;
+bool platform_enter_test_done;
 
 static const struct platform_hibernation_ops *hibernation_ops;
 
@@ -636,6 +637,11 @@ int hibernation_platform_enter(void)
 		goto Power_up;
 	}
 
+	if (hibernation_test(TEST_PLATFORM_ENTER)) {
+		platform_enter_test_done = true;
+		goto Power_up;
+	}
+
 	hibernation_ops->enter();
 	/* We should never get here */
 	while (1);
@@ -700,6 +706,8 @@ static int power_down(void)
 		break;
 	case HIBERNATION_PLATFORM:
 		error = hibernation_platform_enter();
+		if (platform_enter_test_done)
+			goto exit;
 		if (error == -EAGAIN || error == -EBUSY) {
 			events_check_enabled = false;
 			pr_info("Wakeup event detected during hibernation, rolling back.\n");
@@ -848,7 +856,7 @@ int hibernate(void)
 			else
 				error = power_down();
 		}
-		if (error) {
+		if (error || platform_enter_test_done) {
 			/* recover any devices that refused to thaw */
 			dpm_resume_suspended_devices(PMSG_RECOVER);
 		}
@@ -870,8 +878,9 @@ int hibernate(void)
 	}
 	thaw_processes();
 
-	/* Don't bother checking whether freezer_test_done is true */
+	/* Don't bother checking whether any of the test_done flags are true */
 	freezer_test_done = false;
+	platform_enter_test_done = false;
  Exit:
 	filesystems_thaw();
 	pm_notifier_call_chain(PM_POST_HIBERNATION);
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 7f53ef65b789..04f0c429519c 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -273,6 +273,7 @@ static const char * const pm_tests[__TEST_AFTER_LAST] = {
 	[TEST_PLATFORM] = "platform",
 	[TEST_DEVICES] = "devices",
 	[TEST_FREEZER] = "freezer",
+	[TEST_PLATFORM_ENTER] = "platform_enter",
 };
 
 static ssize_t pm_test_show(struct kobject *kobj, struct kobj_attribute *attr,
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 7ccd709af93f..0d33d0e46ed0 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -259,6 +259,7 @@ enum {
 	TEST_PLATFORM,
 	TEST_DEVICES,
 	TEST_FREEZER,
+	TEST_PLATFORM_ENTER,
 	/* keep last */
 	__TEST_AFTER_LAST
 };

-- 
2.53.0

Re: [PATCH 2/3] PM: hibernate: Add platform_enter hibernation test level

[Rafael J. Wysocki]

On Mon, May 18, 2026 at 11:34 PM Nícolas F. R. A. Prado
<nfraprado@collabora.com> wrote:
>
> Add a 'platform_enter' hibernation test level to allow testing the full
> hibernation path up until the last point of return in
> hibernation_platform_enter().
>
> In particular this allows seeing print messages from the code run after
> the hibernation image is created on platforms that don't have an easily
> accessible serial console, since these messages are not present upon
> hibernation resume.
>
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> ---
>  kernel/power/hibernate.c | 13 +++++++++++--
>  kernel/power/main.c      |  1 +
>  kernel/power/power.h     |  1 +
>  3 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 001e4556b137..15cb7a090770 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -75,6 +75,7 @@ enum {
>  static int hibernation_mode = HIBERNATION_SHUTDOWN;
>
>  bool freezer_test_done;
> +bool platform_enter_test_done;
>
>  static const struct platform_hibernation_ops *hibernation_ops;
>
> @@ -636,6 +637,11 @@ int hibernation_platform_enter(void)
>                 goto Power_up;
>         }
>
> +       if (hibernation_test(TEST_PLATFORM_ENTER)) {
> +               platform_enter_test_done = true;
> +               goto Power_up;
> +       }
> +
>         hibernation_ops->enter();
>         /* We should never get here */
>         while (1);
> @@ -700,6 +706,8 @@ static int power_down(void)
>                 break;
>         case HIBERNATION_PLATFORM:
>                 error = hibernation_platform_enter();
> +               if (platform_enter_test_done)
> +                       goto exit;
>                 if (error == -EAGAIN || error == -EBUSY) {
>                         events_check_enabled = false;
>                         pr_info("Wakeup event detected during hibernation, rolling back.\n");
> @@ -848,7 +856,7 @@ int hibernate(void)
>                         else
>                                 error = power_down();
>                 }
> -               if (error) {
> +               if (error || platform_enter_test_done) {
>                         /* recover any devices that refused to thaw */
>                         dpm_resume_suspended_devices(PMSG_RECOVER);
>                 }
> @@ -870,8 +878,9 @@ int hibernate(void)
>         }
>         thaw_processes();
>
> -       /* Don't bother checking whether freezer_test_done is true */
> +       /* Don't bother checking whether any of the test_done flags are true */
>         freezer_test_done = false;
> +       platform_enter_test_done = false;
>   Exit:
>         filesystems_thaw();
>         pm_notifier_call_chain(PM_POST_HIBERNATION);
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 7f53ef65b789..04f0c429519c 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -273,6 +273,7 @@ static const char * const pm_tests[__TEST_AFTER_LAST] = {
>         [TEST_PLATFORM] = "platform",
>         [TEST_DEVICES] = "devices",
>         [TEST_FREEZER] = "freezer",
> +       [TEST_PLATFORM_ENTER] = "platform_enter",
>  };
>
>  static ssize_t pm_test_show(struct kobject *kobj, struct kobj_attribute *attr,
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index 7ccd709af93f..0d33d0e46ed0 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -259,6 +259,7 @@ enum {
>         TEST_PLATFORM,
>         TEST_DEVICES,
>         TEST_FREEZER,
> +       TEST_PLATFORM_ENTER,
>         /* keep last */
>         __TEST_AFTER_LAST
>  };
>

In the first place, test levels are designed to be applicable to both
system suspend and hibernation and I don't see how this test can be
applied to system suspend.

Second, the last device power off transition during hibernation may
not be reversible and adding a test that goes past a point of no
return is not particularly useful.

Re: [PATCH 2/3] PM: hibernate: Add platform_enter hibernation test level

[Nícolas F. R. A. Prado]

On Tue, 2026-05-26 at 13:28 +0200, Rafael J. Wysocki wrote:
> On Mon, May 18, 2026 at 11:34 PM Nícolas F. R. A. Prado
> <nfraprado@collabora.com> wrote:
> > 
> > Add a 'platform_enter' hibernation test level to allow testing the
> > full
> > hibernation path up until the last point of return in
> > hibernation_platform_enter().
> > 
> > In particular this allows seeing print messages from the code run
> > after
> > the hibernation image is created on platforms that don't have an
> > easily
> > accessible serial console, since these messages are not present
> > upon
> > hibernation resume.
> > 
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > ---
> >  kernel/power/hibernate.c | 13 +++++++++++--
> >  kernel/power/main.c      |  1 +
> >  kernel/power/power.h     |  1 +
> >  3 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> > index 001e4556b137..15cb7a090770 100644
> > --- a/kernel/power/hibernate.c
> > +++ b/kernel/power/hibernate.c
> > @@ -75,6 +75,7 @@ enum {
> >  static int hibernation_mode = HIBERNATION_SHUTDOWN;
> > 
> >  bool freezer_test_done;
> > +bool platform_enter_test_done;
> > 
> >  static const struct platform_hibernation_ops *hibernation_ops;
> > 
> > @@ -636,6 +637,11 @@ int hibernation_platform_enter(void)
> >                 goto Power_up;
> >         }
> > 
> > +       if (hibernation_test(TEST_PLATFORM_ENTER)) {
> > +               platform_enter_test_done = true;
> > +               goto Power_up;
> > +       }
> > +
> >         hibernation_ops->enter();
> >         /* We should never get here */
> >         while (1);
> > @@ -700,6 +706,8 @@ static int power_down(void)
> >                 break;
> >         case HIBERNATION_PLATFORM:
> >                 error = hibernation_platform_enter();
> > +               if (platform_enter_test_done)
> > +                       goto exit;
> >                 if (error == -EAGAIN || error == -EBUSY) {
> >                         events_check_enabled = false;
> >                         pr_info("Wakeup event detected during
> > hibernation, rolling back.\n");
> > @@ -848,7 +856,7 @@ int hibernate(void)
> >                         else
> >                                 error = power_down();
> >                 }
> > -               if (error) {
> > +               if (error || platform_enter_test_done) {
> >                         /* recover any devices that refused to thaw
> > */
> >                         dpm_resume_suspended_devices(PMSG_RECOVER);
> >                 }
> > @@ -870,8 +878,9 @@ int hibernate(void)
> >         }
> >         thaw_processes();
> > 
> > -       /* Don't bother checking whether freezer_test_done is true
> > */
> > +       /* Don't bother checking whether any of the test_done flags
> > are true */
> >         freezer_test_done = false;
> > +       platform_enter_test_done = false;
> >   Exit:
> >         filesystems_thaw();
> >         pm_notifier_call_chain(PM_POST_HIBERNATION);
> > diff --git a/kernel/power/main.c b/kernel/power/main.c
> > index 7f53ef65b789..04f0c429519c 100644
> > --- a/kernel/power/main.c
> > +++ b/kernel/power/main.c
> > @@ -273,6 +273,7 @@ static const char * const
> > pm_tests[__TEST_AFTER_LAST] = {
> >         [TEST_PLATFORM] = "platform",
> >         [TEST_DEVICES] = "devices",
> >         [TEST_FREEZER] = "freezer",
> > +       [TEST_PLATFORM_ENTER] = "platform_enter",
> >  };
> > 
> >  static ssize_t pm_test_show(struct kobject *kobj, struct
> > kobj_attribute *attr,
> > diff --git a/kernel/power/power.h b/kernel/power/power.h
> > index 7ccd709af93f..0d33d0e46ed0 100644
> > --- a/kernel/power/power.h
> > +++ b/kernel/power/power.h
> > @@ -259,6 +259,7 @@ enum {
> >         TEST_PLATFORM,
> >         TEST_DEVICES,
> >         TEST_FREEZER,
> > +       TEST_PLATFORM_ENTER,
> >         /* keep last */
> >         __TEST_AFTER_LAST
> >  };
> > 
> 
> In the first place, test levels are designed to be applicable to both
> system suspend and hibernation and I don't see how this test can be
> applied to system suspend.

Indeed this is specific to system hibernation, in fact, to the
"platform" hibernation method. I hadn't noticed the test levels were
designed to be generic.

That said I still think hibernation-specific (and even hibernation-
method-specific) test levels would be a useful addition since they
would allow validating the steps between the end of hibernation image
creation and system power down are working correctly.

As far as I can tell the reason against that is the current uAPI?

Would having test levels listed in pm_test that are only applicable for
certain sleep states (and method combinations) not be acceptable even
if they are clearly named? eg "disk-platform-platform_enter"?

Another option could be having a separate file to list the test levels
that are particular to each sleep state and method.

Or perhaps we could add another test entry in /sys/power/disk like
test_resume. However that would only work for tests that don't depend
on a specific hibernation method, so not useful for this specific
patch, but could still be an option.

> 
> Second, the last device power off transition during hibernation may
> not be reversible and adding a test that goes past a point of no
> return is not particularly useful.

On my platform, this stage is definitely reversible, and is definitely
useful, since it allowed me to benchmark the hibernation image creation
process which would otherwise be impossible.

This is a test that the user chooses to run. Even if it doesn't work on
some platforms, it can simply not be run on those, while being useful
on the platforms that do support it.

Thanks,
Nícolas

[PATCH 3/3] PM: hibernate: Allow disabling zero page check in copy_data_pages()

[Nícolas F. R. A. Prado]

The current implementation of copy_data_pages() has at its center
do_copy_page(), which copies data of all saveable pages in a loop one
long at a time, while checking whether the page is fully zero to allow
it to not be saved in the hibernation image.

As the comment above do_copy_page() mentions, this loop was used instead
of the more optimized copy_page() and memcpy() due to them depending on
fpu_begin()/fpu_end() (see [1] and commit 95018f7c94cb ("[PATCH] swsusp:
do not use memcpy for snapshotting memory")). However, since commit
c6dbd3e5e69c ("x86/mmx_32: Remove X86_USE_3DNOW"), this limitation no
longer holds, and it only affected x86-32 in the first place.

From testing it is clear that removing the zero page check and using
copy_page() makes it almost 3 times quicker:
- Current:
    PM: hibernation: Copied 4940240 kbytes in 11.33 seconds (436.03 MB/s)
- With just the zero check removed:
    PM: hibernation: Copied 4974664 kbytes in 9.03 seconds (550.90 MB/s)
- With the zero check removed and using copy_page() instead:
    PM: hibernation: Copied 6275216 kbytes in 3.96 seconds (1584.65 MB/s)

Given that copy_data_pages() runs inside a critical section where only
a single CPU is online and syscore is suspended, it should be kept as
short as possible to keep the system responsive.

While switching from the loop to copy_page() brings big speed
improvements, it also makes the zero page check much costlier since it
can no longer be done in the middle of the copy. In fact upon testing
adding the zero check alongside copy_page() made it slower than the
current code.

The following shows a rough comparison of a few more metrics between the
current code and when both copy_page() is used and the zero page check
is disabled:
- Total time to hibernate:
  - before: 13.77s
  - after: 14.13s
- Total time to resume:
  - before: 5.85s
  - after: 5.85s
- Total time in syscore_suspend():
  - before: 11.3s
  - after: 4.08s
- Total image size written to disk:
  - before: 4956296kB => 2606402kB compressed = 2.60MB
  - after: 6274608 => 2616624kB compressed = 2.61MB

As can be seen the total time to hibernate is roughly the same,
suggesting that the time saved with the more efficient copy_page() is
payed by having to compress more data (the zero pages). The time to
resume remains the same. And the hibernation image size is basically the
same, as the zero pages compress well (the case would be quite different
if compression is disabled). The big win here is that the time between
syscore_suspend() and syscore_resume() is much lower, meaning the system
will be more responsive throughout hibernation.

Expose a 'nozerocheck' module parameter to allow the zero page check to
be disabled and the faster copy_page() to be used, giving the option for
userspace to choose between a more responsive system and reducing the
disk usage particularly when compression is disabled.

Testing was done on the SteamDeck OLED.

[1] https://lore.kernel.org/all/1096877559.9064.45.camel@desktop.cunninghams/

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---
 kernel/power/snapshot.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 4f452baf31dc..00feac18faae 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -40,6 +40,9 @@
 
 #include "power.h"
 
+static bool nozerocheck;
+module_param(nozerocheck, bool, 0644);
+
 #if defined(CONFIG_STRICT_KERNEL_RWX) && defined(CONFIG_ARCH_HAS_SET_MEMORY)
 static bool hibernate_restore_protection;
 static bool hibernate_restore_protection_active;
@@ -1425,11 +1428,9 @@ static unsigned int count_data_pages(void)
 }
 
 /*
- * This is needed, because copy_page and memcpy are not usable for copying
- * task structs. Returns true if the page was filled with only zeros,
- * otherwise false.
+ * Returns true if the page was filled with only zeros, otherwise false.
  */
-static inline bool do_copy_page(long *dst, long *src)
+static inline bool do_copy_page_zerocheck(long *dst, long *src)
 {
 	long z = 0;
 	int n;
@@ -1441,6 +1442,12 @@ static inline bool do_copy_page(long *dst, long *src)
 	return !z;
 }
 
+static inline bool do_copy_page_nozerocheck(long *dst, long *src)
+{
+	copy_page(dst, src);
+	return false;
+}
+
 /**
  * safe_copy_page - Copy a page in a safe way.
  *
@@ -1450,7 +1457,8 @@ static inline bool do_copy_page(long *dst, long *src)
  * always returns 'true'. Returns true if the page was entirely composed of
  * zeros, otherwise it will return false.
  */
-static bool safe_copy_page(void *dst, struct page *s_page)
+static bool safe_copy_page(void *dst, struct page *s_page,
+			   bool (*do_copy_page)(long *dst, long *src))
 {
 	bool zeros_only;
 
@@ -1471,7 +1479,8 @@ static inline struct page *page_is_saveable(struct zone *zone, unsigned long pfn
 		saveable_highmem_page(zone, pfn) : saveable_page(zone, pfn);
 }
 
-static bool copy_data_page(unsigned long dst_pfn, unsigned long src_pfn)
+static bool copy_data_page(unsigned long dst_pfn, unsigned long src_pfn,
+			   bool (*do_copy_page)(long *dst, long *src))
 {
 	struct page *s_page, *d_page;
 	void *src, *dst;
@@ -1491,12 +1500,13 @@ static bool copy_data_page(unsigned long dst_pfn, unsigned long src_pfn)
 			 * The page pointed to by src may contain some kernel
 			 * data modified by kmap_atomic()
 			 */
-			zeros_only = safe_copy_page(buffer, s_page);
+			zeros_only = safe_copy_page(buffer, s_page, do_copy_page);
 			dst = kmap_local_page(d_page);
 			copy_page(dst, buffer);
 			kunmap_local(dst);
 		} else {
-			zeros_only = safe_copy_page(page_address(d_page), s_page);
+			zeros_only = safe_copy_page(page_address(d_page),
+						    s_page, do_copy_page);
 		}
 	}
 	return zeros_only;
@@ -1504,10 +1514,12 @@ static bool copy_data_page(unsigned long dst_pfn, unsigned long src_pfn)
 #else
 #define page_is_saveable(zone, pfn)	saveable_page(zone, pfn)
 
-static inline int copy_data_page(unsigned long dst_pfn, unsigned long src_pfn)
+static inline int
+copy_data_page(unsigned long dst_pfn, unsigned long src_pfn,
+	       bool (*do_copy_page)(long *dst, long *src))
 {
 	return safe_copy_page(page_address(pfn_to_page(dst_pfn)),
-				pfn_to_page(src_pfn));
+				pfn_to_page(src_pfn), do_copy_page);
 }
 #endif /* CONFIG_HIGHMEM */
 
@@ -1524,6 +1536,9 @@ static unsigned long copy_data_pages(struct memory_bitmap *copy_bm,
 	unsigned long copied_pages = 0;
 	struct zone *zone;
 	unsigned long pfn, copy_pfn;
+	bool (*do_copy_page)(long *dst, long *src);
+
+	do_copy_page = nozerocheck ? do_copy_page_nozerocheck : do_copy_page_zerocheck;
 
 	for_each_populated_zone(zone) {
 		unsigned long max_zone_pfn;
@@ -1541,7 +1556,7 @@ static unsigned long copy_data_pages(struct memory_bitmap *copy_bm,
 		pfn = memory_bm_next_pfn(orig_bm);
 		if (unlikely(pfn == BM_END_OF_MAP))
 			break;
-		if (copy_data_page(copy_pfn, pfn)) {
+		if (copy_data_page(copy_pfn, pfn, do_copy_page)) {
 			memory_bm_set_bit(zero_bm, pfn);
 			/* Use this copy_pfn for a page that is not full of zeros */
 			continue;

-- 
2.53.0

Re: [PATCH 3/3] PM: hibernate: Allow disabling zero page check in copy_data_pages()

[Rafael J. Wysocki]

On Mon, May 18, 2026 at 11:34 PM Nícolas F. R. A. Prado
<nfraprado@collabora.com> wrote:
>
> The current implementation of copy_data_pages() has at its center
> do_copy_page(), which copies data of all saveable pages in a loop one
> long at a time, while checking whether the page is fully zero to allow
> it to not be saved in the hibernation image.
>
> As the comment above do_copy_page() mentions, this loop was used instead
> of the more optimized copy_page() and memcpy() due to them depending on
> fpu_begin()/fpu_end() (see [1] and commit 95018f7c94cb ("[PATCH] swsusp:
> do not use memcpy for snapshotting memory")). However, since commit
> c6dbd3e5e69c ("x86/mmx_32: Remove X86_USE_3DNOW"), this limitation no
> longer holds, and it only affected x86-32 in the first place.
>
> From testing it is clear that removing the zero page check and using
> copy_page() makes it almost 3 times quicker:
> - Current:
>     PM: hibernation: Copied 4940240 kbytes in 11.33 seconds (436.03 MB/s)
> - With just the zero check removed:
>     PM: hibernation: Copied 4974664 kbytes in 9.03 seconds (550.90 MB/s)
> - With the zero check removed and using copy_page() instead:
>     PM: hibernation: Copied 6275216 kbytes in 3.96 seconds (1584.65 MB/s)
>
> Given that copy_data_pages() runs inside a critical section where only
> a single CPU is online and syscore is suspended, it should be kept as
> short as possible to keep the system responsive.

I'm not sure if being responsive during hibernation is all that important.

Also, the change is about using copy_page() to reduce the time spent
in syscore_suspend(), so the subject is a bit confusing.

> While switching from the loop to copy_page() brings big speed
> improvements, it also makes the zero page check much costlier since it
> can no longer be done in the middle of the copy. In fact upon testing
> adding the zero check alongside copy_page() made it slower than the
> current code.

Guess what, CPUs have caches.

> The following shows a rough comparison of a few more metrics between the
> current code and when both copy_page() is used and the zero page check
> is disabled:
> - Total time to hibernate:
>   - before: 13.77s
>   - after: 14.13s

Score for the current code.

> - Total time to resume:
>   - before: 5.85s
>   - after: 5.85s
> - Total time in syscore_suspend():
>   - before: 11.3s
>   - after: 4.08s

Why does this matter?

> - Total image size written to disk:
>   - before: 4956296kB => 2606402kB compressed = 2.60MB
>   - after: 6274608 => 2616624kB compressed = 2.61MB

"kB" seems to be missing in the bottom row.

Which means that this is a score for the current code again because it
needs to copy and compress fewer pages which means that less energy
needs to be used (which may be kind of relevant in low-battery
situations).

> As can be seen the total time to hibernate is roughly the same,
> suggesting that the time saved with the more efficient copy_page() is
> payed by having to compress more data (the zero pages). The time to
> resume remains the same. And the hibernation image size is basically the
> same, as the zero pages compress well (the case would be quite different
> if compression is disabled). The big win here is that the time between
> syscore_suspend() and syscore_resume() is much lower, meaning the system
> will be more responsive throughout hibernation.
>
> Expose a 'nozerocheck' module parameter

I'd rather not.

Either the switch-over is compelling enough to do it unconditionally,
or it is not, in which case the existing code can be used just fine.

As it stands, I'm not convinced.

> to allow the zero page check to
> be disabled and the faster copy_page() to be used, giving the option for
> userspace to choose between a more responsive system and reducing the
> disk usage particularly when compression is disabled.
>
> Testing was done on the SteamDeck OLED.
>
> [1] https://lore.kernel.org/all/1096877559.9064.45.camel@desktop.cunninghams/
>
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> ---
>  kernel/power/snapshot.c | 37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index 4f452baf31dc..00feac18faae 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -40,6 +40,9 @@
>
>  #include "power.h"
>
> +static bool nozerocheck;
> +module_param(nozerocheck, bool, 0644);
> +
>  #if defined(CONFIG_STRICT_KERNEL_RWX) && defined(CONFIG_ARCH_HAS_SET_MEMORY)
>  static bool hibernate_restore_protection;
>  static bool hibernate_restore_protection_active;
> @@ -1425,11 +1428,9 @@ static unsigned int count_data_pages(void)
>  }
>
>  /*
> - * This is needed, because copy_page and memcpy are not usable for copying
> - * task structs. Returns true if the page was filled with only zeros,
> - * otherwise false.
> + * Returns true if the page was filled with only zeros, otherwise false.
>   */
> -static inline bool do_copy_page(long *dst, long *src)
> +static inline bool do_copy_page_zerocheck(long *dst, long *src)
>  {
>         long z = 0;
>         int n;
> @@ -1441,6 +1442,12 @@ static inline bool do_copy_page(long *dst, long *src)
>         return !z;
>  }
>
> +static inline bool do_copy_page_nozerocheck(long *dst, long *src)
> +{
> +       copy_page(dst, src);
> +       return false;
> +}
> +
>  /**
>   * safe_copy_page - Copy a page in a safe way.
>   *
> @@ -1450,7 +1457,8 @@ static inline bool do_copy_page(long *dst, long *src)
>   * always returns 'true'. Returns true if the page was entirely composed of
>   * zeros, otherwise it will return false.
>   */
> -static bool safe_copy_page(void *dst, struct page *s_page)
> +static bool safe_copy_page(void *dst, struct page *s_page,
> +                          bool (*do_copy_page)(long *dst, long *src))
>  {
>         bool zeros_only;
>
> @@ -1471,7 +1479,8 @@ static inline struct page *page_is_saveable(struct zone *zone, unsigned long pfn
>                 saveable_highmem_page(zone, pfn) : saveable_page(zone, pfn);
>  }
>
> -static bool copy_data_page(unsigned long dst_pfn, unsigned long src_pfn)
> +static bool copy_data_page(unsigned long dst_pfn, unsigned long src_pfn,
> +                          bool (*do_copy_page)(long *dst, long *src))
>  {
>         struct page *s_page, *d_page;
>         void *src, *dst;
> @@ -1491,12 +1500,13 @@ static bool copy_data_page(unsigned long dst_pfn, unsigned long src_pfn)
>                          * The page pointed to by src may contain some kernel
>                          * data modified by kmap_atomic()
>                          */
> -                       zeros_only = safe_copy_page(buffer, s_page);
> +                       zeros_only = safe_copy_page(buffer, s_page, do_copy_page);
>                         dst = kmap_local_page(d_page);
>                         copy_page(dst, buffer);
>                         kunmap_local(dst);
>                 } else {
> -                       zeros_only = safe_copy_page(page_address(d_page), s_page);
> +                       zeros_only = safe_copy_page(page_address(d_page),
> +                                                   s_page, do_copy_page);
>                 }
>         }
>         return zeros_only;
> @@ -1504,10 +1514,12 @@ static bool copy_data_page(unsigned long dst_pfn, unsigned long src_pfn)
>  #else
>  #define page_is_saveable(zone, pfn)    saveable_page(zone, pfn)
>
> -static inline int copy_data_page(unsigned long dst_pfn, unsigned long src_pfn)
> +static inline int
> +copy_data_page(unsigned long dst_pfn, unsigned long src_pfn,
> +              bool (*do_copy_page)(long *dst, long *src))
>  {
>         return safe_copy_page(page_address(pfn_to_page(dst_pfn)),
> -                               pfn_to_page(src_pfn));
> +                               pfn_to_page(src_pfn), do_copy_page);
>  }
>  #endif /* CONFIG_HIGHMEM */
>
> @@ -1524,6 +1536,9 @@ static unsigned long copy_data_pages(struct memory_bitmap *copy_bm,
>         unsigned long copied_pages = 0;
>         struct zone *zone;
>         unsigned long pfn, copy_pfn;
> +       bool (*do_copy_page)(long *dst, long *src);
> +
> +       do_copy_page = nozerocheck ? do_copy_page_nozerocheck : do_copy_page_zerocheck;
>
>         for_each_populated_zone(zone) {
>                 unsigned long max_zone_pfn;
> @@ -1541,7 +1556,7 @@ static unsigned long copy_data_pages(struct memory_bitmap *copy_bm,
>                 pfn = memory_bm_next_pfn(orig_bm);
>                 if (unlikely(pfn == BM_END_OF_MAP))
>                         break;
> -               if (copy_data_page(copy_pfn, pfn)) {
> +               if (copy_data_page(copy_pfn, pfn, do_copy_page)) {
>                         memory_bm_set_bit(zero_bm, pfn);
>                         /* Use this copy_pfn for a page that is not full of zeros */
>                         continue;
>
> --
> 2.53.0
>

Re: [PATCH 3/3] PM: hibernate: Allow disabling zero page check in copy_data_pages()

[Nícolas F. R. A. Prado]

On Tue, 2026-05-26 at 13:53 +0200, Rafael J. Wysocki wrote:
> On Mon, May 18, 2026 at 11:34 PM Nícolas F. R. A. Prado
> <nfraprado@collabora.com> wrote:
> > 
> > The current implementation of copy_data_pages() has at its center
> > do_copy_page(), which copies data of all saveable pages in a loop
> > one
> > long at a time, while checking whether the page is fully zero to
> > allow
> > it to not be saved in the hibernation image.
> > 
> > As the comment above do_copy_page() mentions, this loop was used
> > instead
> > of the more optimized copy_page() and memcpy() due to them
> > depending on
> > fpu_begin()/fpu_end() (see [1] and commit 95018f7c94cb ("[PATCH]
> > swsusp:
> > do not use memcpy for snapshotting memory")). However, since commit
> > c6dbd3e5e69c ("x86/mmx_32: Remove X86_USE_3DNOW"), this limitation
> > no
> > longer holds, and it only affected x86-32 in the first place.
> > 
> > From testing it is clear that removing the zero page check and
> > using
> > copy_page() makes it almost 3 times quicker:
> > - Current:
> >     PM: hibernation: Copied 4940240 kbytes in 11.33 seconds (436.03
> > MB/s)
> > - With just the zero check removed:
> >     PM: hibernation: Copied 4974664 kbytes in 9.03 seconds (550.90
> > MB/s)
> > - With the zero check removed and using copy_page() instead:
> >     PM: hibernation: Copied 6275216 kbytes in 3.96 seconds (1584.65
> > MB/s)
> > 
> > Given that copy_data_pages() runs inside a critical section where
> > only
> > a single CPU is online and syscore is suspended, it should be kept
> > as
> > short as possible to keep the system responsive.
> 
> I'm not sure if being responsive during hibernation is all that
> important.
> 
> Also, the change is about using copy_page() to reduce the time spent
> in syscore_suspend(), so the subject is a bit confusing.
> 
> > While switching from the loop to copy_page() brings big speed
> > improvements, it also makes the zero page check much costlier since
> > it
> > can no longer be done in the middle of the copy. In fact upon
> > testing
> > adding the zero check alongside copy_page() made it slower than the
> > current code.
> 
> Guess what, CPUs have caches.
> 
> > The following shows a rough comparison of a few more metrics
> > between the
> > current code and when both copy_page() is used and the zero page
> > check
> > is disabled:
> > - Total time to hibernate:
> >   - before: 13.77s
> >   - after: 14.13s
> 
> Score for the current code.
> 
> > - Total time to resume:
> >   - before: 5.85s
> >   - after: 5.85s
> > - Total time in syscore_suspend():
> >   - before: 11.3s
> >   - after: 4.08s
> 
> Why does this matter?

Reducing the time spent here means reduced time between
pm_wakeup_pending() checks, which ultimately means the kernel is
quicker to abort hibernation in reaction to a pending wakeup.

Thanks,
Nícolas

> 
> > - Total image size written to disk:
> >   - before: 4956296kB => 2606402kB compressed = 2.60MB
> >   - after: 6274608 => 2616624kB compressed = 2.61MB
> 
> "kB" seems to be missing in the bottom row.
> 
> Which means that this is a score for the current code again because
> it
> needs to copy and compress fewer pages which means that less energy
> needs to be used (which may be kind of relevant in low-battery
> situations).
> 
> > As can be seen the total time to hibernate is roughly the same,
> > suggesting that the time saved with the more efficient copy_page()
> > is
> > payed by having to compress more data (the zero pages). The time to
> > resume remains the same. And the hibernation image size is
> > basically the
> > same, as the zero pages compress well (the case would be quite
> > different
> > if compression is disabled). The big win here is that the time
> > between
> > syscore_suspend() and syscore_resume() is much lower, meaning the
> > system
> > will be more responsive throughout hibernation.
> > 
> > Expose a 'nozerocheck' module parameter
> 
> I'd rather not.
> 
> Either the switch-over is compelling enough to do it unconditionally,
> or it is not, in which case the existing code can be used just fine.
> 
> As it stands, I'm not convinced.
> 
> > to allow the zero page check to
> > be disabled and the faster copy_page() to be used, giving the
> > option for
> > userspace to choose between a more responsive system and reducing
> > the
> > disk usage particularly when compression is disabled.
> > 
> > Testing was done on the SteamDeck OLED.
> > 
> > [1]
> > https://lore.kernel.org/all/1096877559.9064.45.camel@desktop.cunninghams/
> > 
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > ---
> >  kernel/power/snapshot.c | 37 ++++++++++++++++++++++++++-----------
> >  1 file changed, 26 insertions(+), 11 deletions(-)
> > 
> > diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> > index 4f452baf31dc..00feac18faae 100644
> > --- a/kernel/power/snapshot.c
> > +++ b/kernel/power/snapshot.c
> > @@ -40,6 +40,9 @@
> > 
> >  #include "power.h"
> > 
> > +static bool nozerocheck;
> > +module_param(nozerocheck, bool, 0644);
> > +
> >  #if defined(CONFIG_STRICT_KERNEL_RWX) &&
> > defined(CONFIG_ARCH_HAS_SET_MEMORY)
> >  static bool hibernate_restore_protection;
> >  static bool hibernate_restore_protection_active;
> > @@ -1425,11 +1428,9 @@ static unsigned int count_data_pages(void)
> >  }
> > 
> >  /*
> > - * This is needed, because copy_page and memcpy are not usable for
> > copying
> > - * task structs. Returns true if the page was filled with only
> > zeros,
> > - * otherwise false.
> > + * Returns true if the page was filled with only zeros, otherwise
> > false.
> >   */
> > -static inline bool do_copy_page(long *dst, long *src)
> > +static inline bool do_copy_page_zerocheck(long *dst, long *src)
> >  {
> >         long z = 0;
> >         int n;
> > @@ -1441,6 +1442,12 @@ static inline bool do_copy_page(long *dst,
> > long *src)
> >         return !z;
> >  }
> > 
> > +static inline bool do_copy_page_nozerocheck(long *dst, long *src)
> > +{
> > +       copy_page(dst, src);
> > +       return false;
> > +}
> > +
> >  /**
> >   * safe_copy_page - Copy a page in a safe way.
> >   *
> > @@ -1450,7 +1457,8 @@ static inline bool do_copy_page(long *dst,
> > long *src)
> >   * always returns 'true'. Returns true if the page was entirely
> > composed of
> >   * zeros, otherwise it will return false.
> >   */
> > -static bool safe_copy_page(void *dst, struct page *s_page)
> > +static bool safe_copy_page(void *dst, struct page *s_page,
> > +                          bool (*do_copy_page)(long *dst, long
> > *src))
> >  {
> >         bool zeros_only;
> > 
> > @@ -1471,7 +1479,8 @@ static inline struct page
> > *page_is_saveable(struct zone *zone, unsigned long pfn
> >                 saveable_highmem_page(zone, pfn) :
> > saveable_page(zone, pfn);
> >  }
> > 
> > -static bool copy_data_page(unsigned long dst_pfn, unsigned long
> > src_pfn)
> > +static bool copy_data_page(unsigned long dst_pfn, unsigned long
> > src_pfn,
> > +                          bool (*do_copy_page)(long *dst, long
> > *src))
> >  {
> >         struct page *s_page, *d_page;
> >         void *src, *dst;
> > @@ -1491,12 +1500,13 @@ static bool copy_data_page(unsigned long
> > dst_pfn, unsigned long src_pfn)
> >                          * The page pointed to by src may contain
> > some kernel
> >                          * data modified by kmap_atomic()
> >                          */
> > -                       zeros_only = safe_copy_page(buffer,
> > s_page);
> > +                       zeros_only = safe_copy_page(buffer, s_page,
> > do_copy_page);
> >                         dst = kmap_local_page(d_page);
> >                         copy_page(dst, buffer);
> >                         kunmap_local(dst);
> >                 } else {
> > -                       zeros_only =
> > safe_copy_page(page_address(d_page), s_page);
> > +                       zeros_only =
> > safe_copy_page(page_address(d_page),
> > +                                                   s_page,
> > do_copy_page);
> >                 }
> >         }
> >         return zeros_only;
> > @@ -1504,10 +1514,12 @@ static bool copy_data_page(unsigned long
> > dst_pfn, unsigned long src_pfn)
> >  #else
> >  #define page_is_saveable(zone, pfn)    saveable_page(zone, pfn)
> > 
> > -static inline int copy_data_page(unsigned long dst_pfn, unsigned
> > long src_pfn)
> > +static inline int
> > +copy_data_page(unsigned long dst_pfn, unsigned long src_pfn,
> > +              bool (*do_copy_page)(long *dst, long *src))
> >  {
> >         return safe_copy_page(page_address(pfn_to_page(dst_pfn)),
> > -                               pfn_to_page(src_pfn));
> > +                               pfn_to_page(src_pfn),
> > do_copy_page);
> >  }
> >  #endif /* CONFIG_HIGHMEM */
> > 
> > @@ -1524,6 +1536,9 @@ static unsigned long copy_data_pages(struct
> > memory_bitmap *copy_bm,
> >         unsigned long copied_pages = 0;
> >         struct zone *zone;
> >         unsigned long pfn, copy_pfn;
> > +       bool (*do_copy_page)(long *dst, long *src);
> > +
> > +       do_copy_page = nozerocheck ? do_copy_page_nozerocheck :
> > do_copy_page_zerocheck;
> > 
> >         for_each_populated_zone(zone) {
> >                 unsigned long max_zone_pfn;
> > @@ -1541,7 +1556,7 @@ static unsigned long copy_data_pages(struct
> > memory_bitmap *copy_bm,
> >                 pfn = memory_bm_next_pfn(orig_bm);
> >                 if (unlikely(pfn == BM_END_OF_MAP))
> >                         break;
> > -               if (copy_data_page(copy_pfn, pfn)) {
> > +               if (copy_data_page(copy_pfn, pfn, do_copy_page)) {
> >                         memory_bm_set_bit(zero_bm, pfn);
> >                         /* Use this copy_pfn for a page that is not
> > full of zeros */
> >                         continue;
> > 
> > --
> > 2.53.0
> > 

Re: [PATCH 3/3] PM: hibernate: Allow disabling zero page check in copy_data_pages()

[Rafael J. Wysocki]

On Tue, Jun 2, 2026 at 12:52 AM Nícolas F. R. A. Prado
<nfraprado@collabora.com> wrote:
>
> On Tue, 2026-05-26 at 13:53 +0200, Rafael J. Wysocki wrote:
> > On Mon, May 18, 2026 at 11:34 PM Nícolas F. R. A. Prado
> > <nfraprado@collabora.com> wrote:
> > >
> > > The current implementation of copy_data_pages() has at its center
> > > do_copy_page(), which copies data of all saveable pages in a loop
> > > one
> > > long at a time, while checking whether the page is fully zero to
> > > allow
> > > it to not be saved in the hibernation image.
> > >
> > > As the comment above do_copy_page() mentions, this loop was used
> > > instead
> > > of the more optimized copy_page() and memcpy() due to them
> > > depending on
> > > fpu_begin()/fpu_end() (see [1] and commit 95018f7c94cb ("[PATCH]
> > > swsusp:
> > > do not use memcpy for snapshotting memory")). However, since commit
> > > c6dbd3e5e69c ("x86/mmx_32: Remove X86_USE_3DNOW"), this limitation
> > > no
> > > longer holds, and it only affected x86-32 in the first place.
> > >
> > > From testing it is clear that removing the zero page check and
> > > using
> > > copy_page() makes it almost 3 times quicker:
> > > - Current:
> > >     PM: hibernation: Copied 4940240 kbytes in 11.33 seconds (436.03
> > > MB/s)
> > > - With just the zero check removed:
> > >     PM: hibernation: Copied 4974664 kbytes in 9.03 seconds (550.90
> > > MB/s)
> > > - With the zero check removed and using copy_page() instead:
> > >     PM: hibernation: Copied 6275216 kbytes in 3.96 seconds (1584.65
> > > MB/s)
> > >
> > > Given that copy_data_pages() runs inside a critical section where
> > > only
> > > a single CPU is online and syscore is suspended, it should be kept
> > > as
> > > short as possible to keep the system responsive.
> >
> > I'm not sure if being responsive during hibernation is all that
> > important.
> >
> > Also, the change is about using copy_page() to reduce the time spent
> > in syscore_suspend(), so the subject is a bit confusing.
> >
> > > While switching from the loop to copy_page() brings big speed
> > > improvements, it also makes the zero page check much costlier since
> > > it
> > > can no longer be done in the middle of the copy. In fact upon
> > > testing
> > > adding the zero check alongside copy_page() made it slower than the
> > > current code.
> >
> > Guess what, CPUs have caches.
> >
> > > The following shows a rough comparison of a few more metrics
> > > between the
> > > current code and when both copy_page() is used and the zero page
> > > check
> > > is disabled:
> > > - Total time to hibernate:
> > >   - before: 13.77s
> > >   - after: 14.13s
> >
> > Score for the current code.
> >
> > > - Total time to resume:
> > >   - before: 5.85s
> > >   - after: 5.85s
> > > - Total time in syscore_suspend():
> > >   - before: 11.3s
> > >   - after: 4.08s
> >
> > Why does this matter?
>
> Reducing the time spent here means reduced time between
> pm_wakeup_pending() checks, which ultimately means the kernel is
> quicker to abort hibernation in reaction to a pending wakeup.

That could be spelled out more directly in the patch changelog.

I'm still unsure why it matters though.  Hibernation is slow pretty
much by definition, so why would anyone expect a fast response to a
wakeup event during hibernation?  If they have latency constraints,
shouldn't they use suspend instead?

Re: [PATCH 3/3] PM: hibernate: Allow disabling zero page check in copy_data_pages()

[Nícolas F. R. A. Prado]

On Tue, 2026-06-02 at 10:04 +0200, Rafael J. Wysocki wrote:
> On Tue, Jun 2, 2026 at 12:52 AM Nícolas F. R. A. Prado
> <nfraprado@collabora.com> wrote:
> > 
> > On Tue, 2026-05-26 at 13:53 +0200, Rafael J. Wysocki wrote:
> > > On Mon, May 18, 2026 at 11:34 PM Nícolas F. R. A. Prado
> > > <nfraprado@collabora.com> wrote:
> > > > 
> > > > The current implementation of copy_data_pages() has at its
> > > > center
> > > > do_copy_page(), which copies data of all saveable pages in a
> > > > loop
> > > > one
> > > > long at a time, while checking whether the page is fully zero
> > > > to
> > > > allow
> > > > it to not be saved in the hibernation image.
> > > > 
> > > > As the comment above do_copy_page() mentions, this loop was
> > > > used
> > > > instead
> > > > of the more optimized copy_page() and memcpy() due to them
> > > > depending on
> > > > fpu_begin()/fpu_end() (see [1] and commit 95018f7c94cb
> > > > ("[PATCH]
> > > > swsusp:
> > > > do not use memcpy for snapshotting memory")). However, since
> > > > commit
> > > > c6dbd3e5e69c ("x86/mmx_32: Remove X86_USE_3DNOW"), this
> > > > limitation
> > > > no
> > > > longer holds, and it only affected x86-32 in the first place.
> > > > 
> > > > From testing it is clear that removing the zero page check and
> > > > using
> > > > copy_page() makes it almost 3 times quicker:
> > > > - Current:
> > > >     PM: hibernation: Copied 4940240 kbytes in 11.33 seconds
> > > > (436.03
> > > > MB/s)
> > > > - With just the zero check removed:
> > > >     PM: hibernation: Copied 4974664 kbytes in 9.03 seconds
> > > > (550.90
> > > > MB/s)
> > > > - With the zero check removed and using copy_page() instead:
> > > >     PM: hibernation: Copied 6275216 kbytes in 3.96 seconds
> > > > (1584.65
> > > > MB/s)
> > > > 
> > > > Given that copy_data_pages() runs inside a critical section
> > > > where
> > > > only
> > > > a single CPU is online and syscore is suspended, it should be
> > > > kept
> > > > as
> > > > short as possible to keep the system responsive.
> > > 
> > > I'm not sure if being responsive during hibernation is all that
> > > important.
> > > 
> > > Also, the change is about using copy_page() to reduce the time
> > > spent
> > > in syscore_suspend(), so the subject is a bit confusing.
> > > 
> > > > While switching from the loop to copy_page() brings big speed
> > > > improvements, it also makes the zero page check much costlier
> > > > since
> > > > it
> > > > can no longer be done in the middle of the copy. In fact upon
> > > > testing
> > > > adding the zero check alongside copy_page() made it slower than
> > > > the
> > > > current code.
> > > 
> > > Guess what, CPUs have caches.
> > > 
> > > > The following shows a rough comparison of a few more metrics
> > > > between the
> > > > current code and when both copy_page() is used and the zero
> > > > page
> > > > check
> > > > is disabled:
> > > > - Total time to hibernate:
> > > >   - before: 13.77s
> > > >   - after: 14.13s
> > > 
> > > Score for the current code.
> > > 
> > > > - Total time to resume:
> > > >   - before: 5.85s
> > > >   - after: 5.85s
> > > > - Total time in syscore_suspend():
> > > >   - before: 11.3s
> > > >   - after: 4.08s
> > > 
> > > Why does this matter?
> > 
> > Reducing the time spent here means reduced time between
> > pm_wakeup_pending() checks, which ultimately means the kernel is
> > quicker to abort hibernation in reaction to a pending wakeup.
> 
> That could be spelled out more directly in the patch changelog.
> 
> I'm still unsure why it matters though.  Hibernation is slow pretty
> much by definition, so why would anyone expect a fast response to a
> wakeup event during hibernation?  If they have latency constraints,
> shouldn't they use suspend instead?

We can't just rely on suspend because battery life also matters.

Indeed hibernation is slow, but if it can be made faster, or at least
quicker to respond to wakeup events, then why not?

-- 
Thanks,

Nícolas