Re: [PATCH v3] PM: hibernate: use acquire/release ordering when compress/decompress image
From: Rafael J. Wysocki
Date: Wed Dec 13 2023 - 08:54:16 EST
On Wed, Dec 13, 2023 at 2:11 AM Hongchen Zhang
<zhanghongchen@xxxxxxxxxxx> wrote:
>
> When we test S4(suspend to disk) on LoongArch 3A6000 platform, the
> test case sometimes fails. The dmesg log shows the following error:
> Invalid LZO compressed length
> After we dig into the code, we find out that:
> When compress/decompress the image, the synchronization operation
> between the control thread and the compress/decompress/crc thread
> uses relaxed ordering interface, which is unreliable, and the
> following situation may occur:
> CPU 0 CPU 1
> save_image_lzo lzo_compress_threadfn
> atomic_set(&d->stop, 1);
> atomic_read(&data[thr].stop)
> data[thr].cmp = data[thr].cmp_len;
> WRITE data[thr].cmp_len
> Then CPU0 get a old cmp_len and write to disk. When cpu resume from S4,
> wrong cmp_len is loaded.
>
> To maintain data consistency between two threads, we should use the
> acquire/release ordering interface. So we change atomic_read/atomic_set
> to atomic_read_acquire/atomic_set_release.
>
> Fixes: 081a9d043c98 ("PM / Hibernate: Improve performance of LZO/plain hibernation, checksum image")
> Cc: stable@xxxxxxxxxxxxxxx
> Co-developed-by: Weihao Li <liweihao@xxxxxxxxxxx>
I gather that the tag above is the only difference between this
version of the patch and the previous one.
It is always better to list the changes made between consecutive
versions of a patch.
> Signed-off-by: Weihao Li <liweihao@xxxxxxxxxxx>
> Signed-off-by: Hongchen Zhang <zhanghongchen@xxxxxxxxxxx>
> ---
> kernel/power/swap.c | 38 +++++++++++++++++++-------------------
> 1 file changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
> index a2cb0babb5ec..d44f5937f1e5 100644
> --- a/kernel/power/swap.c
> +++ b/kernel/power/swap.c
> @@ -606,11 +606,11 @@ static int crc32_threadfn(void *data)
> unsigned i;
>
> while (1) {
> - wait_event(d->go, atomic_read(&d->ready) ||
> + wait_event(d->go, atomic_read_acquire(&d->ready) ||
> kthread_should_stop());
> if (kthread_should_stop()) {
> d->thr = NULL;
> - atomic_set(&d->stop, 1);
> + atomic_set_release(&d->stop, 1);
> wake_up(&d->done);
> break;
> }
> @@ -619,7 +619,7 @@ static int crc32_threadfn(void *data)
> for (i = 0; i < d->run_threads; i++)
> *d->crc32 = crc32_le(*d->crc32,
> d->unc[i], *d->unc_len[i]);
> - atomic_set(&d->stop, 1);
> + atomic_set_release(&d->stop, 1);
> wake_up(&d->done);
> }
> return 0;
> @@ -649,12 +649,12 @@ static int lzo_compress_threadfn(void *data)
> struct cmp_data *d = data;
>
> while (1) {
> - wait_event(d->go, atomic_read(&d->ready) ||
> + wait_event(d->go, atomic_read_acquire(&d->ready) ||
> kthread_should_stop());
> if (kthread_should_stop()) {
> d->thr = NULL;
> d->ret = -1;
> - atomic_set(&d->stop, 1);
> + atomic_set_release(&d->stop, 1);
> wake_up(&d->done);
> break;
> }
> @@ -663,7 +663,7 @@ static int lzo_compress_threadfn(void *data)
> d->ret = lzo1x_1_compress(d->unc, d->unc_len,
> d->cmp + LZO_HEADER, &d->cmp_len,
> d->wrk);
> - atomic_set(&d->stop, 1);
> + atomic_set_release(&d->stop, 1);
> wake_up(&d->done);
> }
> return 0;
> @@ -798,7 +798,7 @@ static int save_image_lzo(struct swap_map_handle *handle,
>
> data[thr].unc_len = off;
>
> - atomic_set(&data[thr].ready, 1);
> + atomic_set_release(&data[thr].ready, 1);
> wake_up(&data[thr].go);
> }
>
> @@ -806,12 +806,12 @@ static int save_image_lzo(struct swap_map_handle *handle,
> break;
>
> crc->run_threads = thr;
> - atomic_set(&crc->ready, 1);
> + atomic_set_release(&crc->ready, 1);
> wake_up(&crc->go);
>
> for (run_threads = thr, thr = 0; thr < run_threads; thr++) {
> wait_event(data[thr].done,
> - atomic_read(&data[thr].stop));
> + atomic_read_acquire(&data[thr].stop));
> atomic_set(&data[thr].stop, 0);
>
> ret = data[thr].ret;
> @@ -850,7 +850,7 @@ static int save_image_lzo(struct swap_map_handle *handle,
> }
> }
>
> - wait_event(crc->done, atomic_read(&crc->stop));
> + wait_event(crc->done, atomic_read_acquire(&crc->stop));
> atomic_set(&crc->stop, 0);
> }
>
> @@ -1132,12 +1132,12 @@ static int lzo_decompress_threadfn(void *data)
> struct dec_data *d = data;
>
> while (1) {
> - wait_event(d->go, atomic_read(&d->ready) ||
> + wait_event(d->go, atomic_read_acquire(&d->ready) ||
> kthread_should_stop());
> if (kthread_should_stop()) {
> d->thr = NULL;
> d->ret = -1;
> - atomic_set(&d->stop, 1);
> + atomic_set_release(&d->stop, 1);
> wake_up(&d->done);
> break;
> }
> @@ -1150,7 +1150,7 @@ static int lzo_decompress_threadfn(void *data)
> flush_icache_range((unsigned long)d->unc,
> (unsigned long)d->unc + d->unc_len);
>
> - atomic_set(&d->stop, 1);
> + atomic_set_release(&d->stop, 1);
> wake_up(&d->done);
> }
> return 0;
> @@ -1335,7 +1335,7 @@ static int load_image_lzo(struct swap_map_handle *handle,
> }
>
> if (crc->run_threads) {
> - wait_event(crc->done, atomic_read(&crc->stop));
> + wait_event(crc->done, atomic_read_acquire(&crc->stop));
> atomic_set(&crc->stop, 0);
> crc->run_threads = 0;
> }
> @@ -1371,7 +1371,7 @@ static int load_image_lzo(struct swap_map_handle *handle,
> pg = 0;
> }
>
> - atomic_set(&data[thr].ready, 1);
> + atomic_set_release(&data[thr].ready, 1);
> wake_up(&data[thr].go);
> }
>
> @@ -1390,7 +1390,7 @@ static int load_image_lzo(struct swap_map_handle *handle,
>
> for (run_threads = thr, thr = 0; thr < run_threads; thr++) {
> wait_event(data[thr].done,
> - atomic_read(&data[thr].stop));
> + atomic_read_acquire(&data[thr].stop));
> atomic_set(&data[thr].stop, 0);
>
> ret = data[thr].ret;
> @@ -1421,7 +1421,7 @@ static int load_image_lzo(struct swap_map_handle *handle,
> ret = snapshot_write_next(snapshot);
> if (ret <= 0) {
> crc->run_threads = thr + 1;
> - atomic_set(&crc->ready, 1);
> + atomic_set_release(&crc->ready, 1);
> wake_up(&crc->go);
> goto out_finish;
> }
> @@ -1429,13 +1429,13 @@ static int load_image_lzo(struct swap_map_handle *handle,
> }
>
> crc->run_threads = thr;
> - atomic_set(&crc->ready, 1);
> + atomic_set_release(&crc->ready, 1);
> wake_up(&crc->go);
> }
>
> out_finish:
> if (crc->run_threads) {
> - wait_event(crc->done, atomic_read(&crc->stop));
> + wait_event(crc->done, atomic_read_acquire(&crc->stop));
> atomic_set(&crc->stop, 0);
> }
> stop = ktime_get();
> --
Applied as 6.8 material with some edits in the subject and changelog.
Thanks!