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@vger.kernel.org Co-developed-by: Weihao Li liweihao@loongson.cn Signed-off-by: Weihao Li liweihao@loongson.cn Signed-off-by: Hongchen Zhang zhanghongchen@loongson.cn --- 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();
On Wed, Dec 13, 2023 at 2:11 AM Hongchen Zhang zhanghongchen@loongson.cn 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@vger.kernel.org Co-developed-by: Weihao Li liweihao@loongson.cn
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@loongson.cn Signed-off-by: Hongchen Zhang zhanghongchen@loongson.cn
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!
Hi Rafael, Thanks for your review. On 2023/12/13 pm 9:52, Rafael J. Wysocki wrote:
On Wed, Dec 13, 2023 at 2:11 AM Hongchen Zhang zhanghongchen@loongson.cn 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@vger.kernel.org Co-developed-by: Weihao Li liweihao@loongson.cn
I gather that the tag above is the only difference between this version of the patch and the previous one.
Yes.
It is always better to list the changes made between consecutive versions of a patch.
Sorry for not updating the changelog, I will remember this next time.
Signed-off-by: Weihao Li liweihao@loongson.cn Signed-off-by: Hongchen Zhang zhanghongchen@loongson.cn
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!
linux-stable-mirror@lists.linaro.org
-
Hongchen Zhang -
Rafael J. Wysocki