* Re: [PATCH v2 1/2] PM: hibernate: dynamically allocate crc->unc_len/unc for configurable threads
2025-09-11 8:09 ` [PATCH v2 1/2] PM: hibernate: dynamically allocate crc->unc_len/unc for configurable threads Xueqin Luo
@ 2025-09-11 13:10 ` Rafael J. Wysocki
2025-09-11 16:56 ` Christophe JAILLET
1 sibling, 0 replies; 4+ messages in thread
From: Rafael J. Wysocki @ 2025-09-11 13:10 UTC (permalink / raw)
To: Xueqin Luo; +Cc: rafael, pavel, lenb, linux-pm, linux-kernel
On Thu, Sep 11, 2025 at 10:10 AM Xueqin Luo <luoxueqin@kylinos.cn> wrote:
>
> The current implementation uses fixed-size arrays for crc->unc_len and
> crc->unc, which limits the number of compression threads to a compile-time
> constant (CMP_THREADS). This patch converts them to dynamically allocated
> arrays, sized according to the actual number of threads selected at runtime.
Please don't say "this patch" (or similar) in patch changelogs. It's
better to use imperative sentences like "Convert them to dynamically
allocated arrays, ...".
>
> Signed-off-by: Xueqin Luo <luoxueqin@kylinos.cn>
> ---
> kernel/power/swap.c | 44 ++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 40 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
> index 0beff7eeaaba..bd149a54c081 100644
> --- a/kernel/power/swap.c
> +++ b/kernel/power/swap.c
> @@ -585,8 +585,8 @@ struct crc_data {
> wait_queue_head_t go; /* start crc update */
> wait_queue_head_t done; /* crc update done */
> u32 *crc32; /* points to handle's crc32 */
> - size_t *unc_len[CMP_THREADS]; /* uncompressed lengths */
> - unsigned char *unc[CMP_THREADS]; /* uncompressed data */
> + size_t **unc_len; /* uncompressed lengths */
> + unsigned char **unc; /* uncompressed data */
> };
>
> /*
> @@ -721,7 +721,21 @@ static int save_compressed_image(struct swap_map_handle *handle,
>
> crc = kzalloc(sizeof(*crc), GFP_KERNEL);
> if (!crc) {
> - pr_err("Failed to allocate crc\n");
> + pr_err("Failed to allocate crc structure\n");
> + ret = -ENOMEM;
> + goto out_clean;
> + }
> +
> + crc->unc_len = kcalloc(nr_threads, sizeof(size_t *), GFP_KERNEL);
> + if (!crc->unc_len) {
> + pr_err("Failed to allocate crc->unc_len for %d threads\n", nr_threads);
> + ret = -ENOMEM;
> + goto out_clean;
> + }
> +
> + crc->unc = kcalloc(nr_threads, sizeof(unsigned char *), GFP_KERNEL);
> + if (!crc->unc) {
> + pr_err("Failed to allocate crc->unc for %d threads\n", nr_threads);
> ret = -ENOMEM;
> goto out_clean;
> }
Can you avoid code duplication by defining helpers for allocating and
freeing them both and using those helpers where applicable (image
creation and uncompression)?
> @@ -886,6 +900,10 @@ static int save_compressed_image(struct swap_map_handle *handle,
> out_clean:
> hib_finish_batch(&hb);
> if (crc) {
> + if (crc->unc)
> + kfree(crc->unc);
> + if (crc->unc_len)
> + kfree(crc->unc_len);
> if (crc->thr)
> kthread_stop(crc->thr);
> kfree(crc);
> @@ -1241,7 +1259,21 @@ static int load_compressed_image(struct swap_map_handle *handle,
>
> crc = kzalloc(sizeof(*crc), GFP_KERNEL);
> if (!crc) {
> - pr_err("Failed to allocate crc\n");
> + pr_err("Failed to allocate crc structure\n");
> + ret = -ENOMEM;
> + goto out_clean;
> + }
> +
> + crc->unc_len = kcalloc(nr_threads, sizeof(size_t *), GFP_KERNEL);
> + if (!crc->unc_len) {
> + pr_err("Failed to allocate crc->unc_len for %d threads\n", nr_threads);
> + ret = -ENOMEM;
> + goto out_clean;
> + }
> +
> + crc->unc = kcalloc(nr_threads, sizeof(unsigned char *), GFP_KERNEL);
> + if (!crc->unc) {
> + pr_err("Failed to allocate crc->unc for %d threads\n", nr_threads);
> ret = -ENOMEM;
> goto out_clean;
> }
> @@ -1507,6 +1539,10 @@ static int load_compressed_image(struct swap_map_handle *handle,
> for (i = 0; i < ring_size; i++)
> free_page((unsigned long)page[i]);
> if (crc) {
> + if (crc->unc)
> + kfree(crc->unc);
> + if (crc->unc_len)
> + kfree(crc->unc_len);
> if (crc->thr)
> kthread_stop(crc->thr);
> kfree(crc);
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2 1/2] PM: hibernate: dynamically allocate crc->unc_len/unc for configurable threads
2025-09-11 8:09 ` [PATCH v2 1/2] PM: hibernate: dynamically allocate crc->unc_len/unc for configurable threads Xueqin Luo
2025-09-11 13:10 ` Rafael J. Wysocki
@ 2025-09-11 16:56 ` Christophe JAILLET
1 sibling, 0 replies; 4+ messages in thread
From: Christophe JAILLET @ 2025-09-11 16:56 UTC (permalink / raw)
To: Xueqin Luo, rafael, pavel, lenb, linux-pm, linux-kernel
Le 11/09/2025 à 10:09, Xueqin Luo a écrit :
> The current implementation uses fixed-size arrays for crc->unc_len and
> crc->unc, which limits the number of compression threads to a compile-time
> constant (CMP_THREADS). This patch converts them to dynamically allocated
> arrays, sized according to the actual number of threads selected at runtime.
>
> Signed-off-by: Xueqin Luo <luoxueqin@kylinos.cn>
> ---
> kernel/power/swap.c | 44 ++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 40 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
> index 0beff7eeaaba..bd149a54c081 100644
> --- a/kernel/power/swap.c
> +++ b/kernel/power/swap.c
> @@ -585,8 +585,8 @@ struct crc_data {
> wait_queue_head_t go; /* start crc update */
> wait_queue_head_t done; /* crc update done */
> u32 *crc32; /* points to handle's crc32 */
> - size_t *unc_len[CMP_THREADS]; /* uncompressed lengths */
> - unsigned char *unc[CMP_THREADS]; /* uncompressed data */
> + size_t **unc_len; /* uncompressed lengths */
> + unsigned char **unc; /* uncompressed data */
spaces and tabulations now make things look unaligned.
> };
>
> /*
> @@ -721,7 +721,21 @@ static int save_compressed_image(struct swap_map_handle *handle,
>
> crc = kzalloc(sizeof(*crc), GFP_KERNEL);
> if (!crc) {
> - pr_err("Failed to allocate crc\n");
> + pr_err("Failed to allocate crc structure\n");
Usually, it is not needed to log messages for memory allocation
failures, because such failures are already verbose.
checkpatch.pl should complain about it.
CJ
> + ret = -ENOMEM;
> + goto out_clean;
> + }
> +
> + crc->unc_len = kcalloc(nr_threads, sizeof(size_t *), GFP_KERNEL);
> + if (!crc->unc_len) {
> + pr_err("Failed to allocate crc->unc_len for %d threads\n", nr_threads);
> + ret = -ENOMEM;
> + goto out_clean;
> + }
> +
> + crc->unc = kcalloc(nr_threads, sizeof(unsigned char *), GFP_KERNEL);
> + if (!crc->unc) {
> + pr_err("Failed to allocate crc->unc for %d threads\n", nr_threads);
> ret = -ENOMEM;
> goto out_clean;
> }
> @@ -886,6 +900,10 @@ static int save_compressed_image(struct swap_map_handle *handle,
> out_clean:
> hib_finish_batch(&hb);
> if (crc) {
> + if (crc->unc)
> + kfree(crc->unc);
> + if (crc->unc_len)
> + kfree(crc->unc_len);
> if (crc->thr)
> kthread_stop(crc->thr);
> kfree(crc);
> @@ -1241,7 +1259,21 @@ static int load_compressed_image(struct swap_map_handle *handle,
>
> crc = kzalloc(sizeof(*crc), GFP_KERNEL);
> if (!crc) {
> - pr_err("Failed to allocate crc\n");
> + pr_err("Failed to allocate crc structure\n");
> + ret = -ENOMEM;
> + goto out_clean;
> + }
> +
> + crc->unc_len = kcalloc(nr_threads, sizeof(size_t *), GFP_KERNEL);
> + if (!crc->unc_len) {
> + pr_err("Failed to allocate crc->unc_len for %d threads\n", nr_threads);
> + ret = -ENOMEM;
> + goto out_clean;
> + }
> +
> + crc->unc = kcalloc(nr_threads, sizeof(unsigned char *), GFP_KERNEL);
> + if (!crc->unc) {
> + pr_err("Failed to allocate crc->unc for %d threads\n", nr_threads);
> ret = -ENOMEM;
> goto out_clean;
> }
> @@ -1507,6 +1539,10 @@ static int load_compressed_image(struct swap_map_handle *handle,
> for (i = 0; i < ring_size; i++)
> free_page((unsigned long)page[i]);
> if (crc) {
> + if (crc->unc)
> + kfree(crc->unc);
> + if (crc->unc_len)
> + kfree(crc->unc_len);
> if (crc->thr)
> kthread_stop(crc->thr);
> kfree(crc);
^ permalink raw reply [flat|nested] 4+ messages in thread