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

[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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> >  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
> >