Re: [PATCH v2] PM: hibernate: flush icache for restored pages from task context

[Rafael J. Wysocki (Intel)]

On Sat, Sep 26, 2026 at 5:24 AM Xiong Xin <xiongxin@xxxxxxxxxx> wrote:
>
> With `hibernate=nocompress`, load_image() submits read bios asynchronously
> and hib_end_io() completes them from hardirq/softirq context.  Commit
> f6cf0545ec69 ("PM / Hibernate: Call flush_icache_range() on pages restored
> in-place") calls flush_icache_range() there, which on ARM64 ends with
> kick_all_cpus_sync() and so triggers
>
>     WARN_ON_ONCE(!in_task());
>
> in smp_call_function(), as in_task() is always false in interrupt context.
>
> Only the nocompress path is affected: load_compressed_image() flushes from
> the decompression kthread, in task context.
>
> Defer the flush to task context.  hib_end_io() now only records the
> restored page on a per-batch list (page->lru is free to use, as safe
> in-place pages are on no LRU).  Once hib_wait_io() returns to load_image(),
> hib_flush_icache_pages() walks the list and flushes each page, preserving
> the asynchronous batched I/O throughput.
>
> Fixes: f6cf0545ec69 ("PM / Hibernate: Call flush_icache_range() on pages restored in-place")
> Assisted-by: GLM:5.3
> Acked-by: Riwen Lu <luriwen@xxxxxxxxxx>
> Signed-off-by: Xiong Xin <xiongxin@xxxxxxxxxx>

It would be good to say what changed between v1 and v2, for example
like in this patch:

https://lore.kernel.org/all/20260930-acpi_spcr-v4-1-ab649aa1f09c@xxxxxxxxx/

Also Sashiko still complains about this version:

https://sashiko.dev/#/patchset/20260926032416.467937-1-xiongxin%40kylinos.cn

so please let me know what you think about its findings.

Thanks!

> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
> index c78f1593600b..6d1019a2780f 100644
> --- a/kernel/power/swap.c
> +++ b/kernel/power/swap.c
> @@ -223,6 +223,8 @@ struct hib_bio_batch {
>         wait_queue_head_t       wait;
>         blk_status_t            error;
>         struct blk_plug         plug;
> +       struct list_head        icache_pages;
> +       spinlock_t              icache_lock;
>  };
>
>  static void hib_init_batch(struct hib_bio_batch *hb)
> @@ -230,6 +232,8 @@ static void hib_init_batch(struct hib_bio_batch *hb)
>         atomic_set(&hb->count, 0);
>         init_waitqueue_head(&hb->wait);
>         hb->error = BLK_STS_OK;
> +       INIT_LIST_HEAD(&hb->icache_pages);
> +       spin_lock_init(&hb->icache_lock);
>         blk_start_plug(&hb->plug);
>  }
>
> @@ -249,11 +253,19 @@ static void hib_end_io(struct bio *bio)
>                          (unsigned long long)bio->bi_iter.bi_sector);
>         }
>
> -       if (bio_data_dir(bio) == WRITE)
> +       if (bio_data_dir(bio) == WRITE) {
>                 put_page(page);
> -       else if (clean_pages_on_read)
> -               flush_icache_range((unsigned long)page_address(page),
> -                                  (unsigned long)page_address(page) + PAGE_SIZE);
> +       } else if (clean_pages_on_read) {
> +               /*
> +                * Stash the page on the batch list, load_image()
> +                * will flush it once hib_wait_io() has returned to task
> +                * context. The page is neither on an LRU nor in the page
> +                * cache, so its lru member is free to use as link node.
> +                */
> +               spin_lock(&hb->icache_lock);
> +               list_add(&page->lru, &hb->icache_pages);
> +               spin_unlock(&hb->icache_lock);
> +       }
>
>         if (bio->bi_status && !hb->error)
>                 hb->error = bio->bi_status;
> @@ -295,6 +307,23 @@ static int hib_wait_io(struct hib_bio_batch *hb)
>         return blk_status_to_errno(hb->error);
>  }
>
> +/*
> + * Flush the icache for pages restored since the last hib_wait_io().
> + * Must be called from task context (after hib_wait_io() returned): all
> + * bios of the batch have completed, so the icache_pages list is no longer
> + * touched by hib_end_io() and can be walked without the lock.
> + */
> +static void hib_flush_icache_pages(struct hib_bio_batch *hb)
> +{
> +       struct page *page, *tmp;
> +
> +       list_for_each_entry_safe(page, tmp, &hb->icache_pages, lru) {
> +               list_del_init(&page->lru);
> +               flush_icache_range((unsigned long)page_address(page),
> +                                  (unsigned long)page_address(page) + PAGE_SIZE);
> +       }
> +}
> +
>  /*
>   * Saving part
>   */
> @@ -1116,8 +1145,12 @@ static int load_image(struct swap_map_handle *handle,
>                 ret = swap_read_page(handle, data_of(*snapshot), &hb);
>                 if (ret)
>                         break;
> -               if (snapshot->sync_read)
> +               if (snapshot->sync_read) {
>                         ret = hib_wait_io(&hb);
> +                       if (ret)
> +                               break;
> +                       hib_flush_icache_pages(&hb);
> +               }
>                 if (ret)
>                         break;
>                 if (!(nr_pages % m))
> @@ -1131,10 +1164,17 @@ static int load_image(struct swap_map_handle *handle,
>         if (!ret)
>                 ret = err2;
>         if (!ret) {
> +               hib_flush_icache_pages(&hb);
>                 pr_info("Image loading done\n");
>                 ret = snapshot_write_finalize(snapshot);
>                 if (!ret && !snapshot_image_loaded(snapshot))
>                         ret = -ENODATA;
> +       } else {
> +               /*
> +                * Reinit the list instead of walking it to avoid
> +                * use-after-free.
> +                */
> +               INIT_LIST_HEAD(&hb.icache_pages);
>         }
>         swsusp_show_speed(start, stop, nr_to_read, "Read");
>         return ret;
> --
> 2.25.1
>