Re: [RFC PATCH v1 14/57] pm/hibernate: Remove PAGE_SIZE compile-time constant assumption

Ryan Roberts
2024-10-16T14:42:55Z

+ Rafael J. Wysocki, Len Brown, Pavel Machek

This was a rather tricky series to get the recipients correct for and my script
did not realize that "supporter" was a pseudonym for "maintainer" so you were
missed off the original post. Appologies!

More context in cover letter:
https://lore.kernel.org/all/20241014105514.3206191-1-ryan.roberts@arm.com/

On 14/10/2024 11:58, Ryan Roberts wrote:
> To prepare for supporting boot-time page size selection, refactor code
> to remove assumptions about PAGE_SIZE being compile-time constant. Code
> intended to be equivalent when compile-time page size is active.
>
> "struct linked_page", "struct swap_map_page" and "struct swsusp_header"
> were all previously sized to be exactly PAGE_SIZE. Refactor those
> structures to remove the padding, then superimpose them on a page at
> runtime.
>
> "struct cmp_data" and "struct dec_data" previously contained embedded
> "unc" and "cmp" arrays, who's sizes were derived from PAGE_SIZE. We
> can't use flexible array approach here since there are 2 arrays in the
> structure, so convert to pointers and define an allocator and
> deallocator for each struct.
>
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
>
> ***NOTE***
> Any confused maintainers may want to read the cover note here for context:
> https://lore.kernel.org/all/20241014105514.3206191-1-ryan.roberts@arm.com/
>
> kernel/power/power.h | 2 +-
> kernel/power/snapshot.c | 2 +-
> kernel/power/swap.c | 129 +++++++++++++++++++++++++++++++++-------
> 3 files changed, 108 insertions(+), 25 deletions(-)
>
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index de0e6b1077f23..74af2eb8d48a4 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -16,7 +16,7 @@ struct swsusp_info {
> unsigned long image_pages;
> unsigned long pages;
> unsigned long size;
> -} __aligned(PAGE_SIZE);
> +} __aligned(PAGE_SIZE_MAX);
>
> #ifdef CONFIG_HIBERNATION
> /* kernel/power/snapshot.c */
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index 405eddbda4fc5..144e92f786e35 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -155,7 +155,7 @@ struct pbe *restore_pblist;
>
> struct linked_page {
> struct linked_page *next;
> - char data[LINKED_PAGE_DATA_SIZE];
> + char data[];
> } __packed;
>
> /*
> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
> index 82b884b67152f..ffd4c864acfa2 100644
> --- a/kernel/power/swap.c
> +++ b/kernel/power/swap.c
> @@ -59,6 +59,7 @@ static bool clean_pages_on_decompress;
> */
>
> #define MAP_PAGE_ENTRIES (PAGE_SIZE / sizeof(sector_t) - 1)
> +#define NEXT_SWAP_INDEX MAP_PAGE_ENTRIES
>
> /*
> * Number of free pages that are not high.
> @@ -78,8 +79,11 @@ static inline unsigned long reqd_free_pages(void)
> }
>
> struct swap_map_page {
> - sector_t entries[MAP_PAGE_ENTRIES];
> - sector_t next_swap;
> + /*
> + * A PAGE_SIZE structure with (PAGE_SIZE / sizeof(sector_t)) entries.
> + * The last entry, [NEXT_SWAP_INDEX], is `.next_swap`.
> + */
> + sector_t entries[1];
> };
>
> struct swap_map_page_list {
> @@ -103,8 +107,6 @@ struct swap_map_handle {
> };
>
> struct swsusp_header {
> - char reserved[PAGE_SIZE - 20 - sizeof(sector_t) - sizeof(int) -
> - sizeof(u32) - sizeof(u32)];
> u32 hw_sig;
> u32 crc32;
> sector_t image;
> @@ -113,6 +115,7 @@ struct swsusp_header {
> char sig[10];
> } __packed;
>
> +static char *swsusp_header_pg;
> static struct swsusp_header *swsusp_header;
>
> /*
> @@ -315,7 +318,7 @@ static int mark_swapfiles(struct swap_map_handle *handle, unsigned int flags)
> {
> int error;
>
> - hib_submit_io(REQ_OP_READ, swsusp_resume_block, swsusp_header, NULL);
> + hib_submit_io(REQ_OP_READ, swsusp_resume_block, swsusp_header_pg, NULL);
> if (!memcmp("SWAP-SPACE",swsusp_header->sig, 10) ||
> !memcmp("SWAPSPACE2",swsusp_header->sig, 10)) {
> memcpy(swsusp_header->orig_sig,swsusp_header->sig, 10);
> @@ -329,7 +332,7 @@ static int mark_swapfiles(struct swap_map_handle *handle, unsigned int flags)
> if (flags & SF_CRC32_MODE)
> swsusp_header->crc32 = handle->crc32;
> error = hib_submit_io(REQ_OP_WRITE | REQ_SYNC,
> - swsusp_resume_block, swsusp_header, NULL);
> + swsusp_resume_block, swsusp_header_pg, NULL);
> } else {
> pr_err("Swap header not found!\n");
> error = -ENODEV;
> @@ -466,7 +469,7 @@ static int swap_write_page(struct swap_map_handle *handle, void *buf,
> offset = alloc_swapdev_block(root_swap);
> if (!offset)
> return -ENOSPC;
> - handle->cur->next_swap = offset;
> + handle->cur->entries[NEXT_SWAP_INDEX] = offset;
> error = write_page(handle->cur, handle->cur_swap, hb);
> if (error)
> goto out;
> @@ -643,8 +646,8 @@ struct cmp_data {
> wait_queue_head_t done; /* compression done */
> size_t unc_len; /* uncompressed length */
> size_t cmp_len; /* compressed length */
> - unsigned char unc[UNC_SIZE]; /* uncompressed buffer */
> - unsigned char cmp[CMP_SIZE]; /* compressed buffer */
> + unsigned char *unc; /* uncompressed buffer */
> + unsigned char *cmp; /* compressed buffer */
> };
>
> /* Indicates the image size after compression */
> @@ -683,6 +686,45 @@ static int compress_threadfn(void *data)
> return 0;
> }
>
> +static void free_cmp_data(struct cmp_data *data, unsigned nr_threads)
> +{
> + int i;
> +
> + if (!data)
> + return;
> +
> + for (i = 0; i < nr_threads; i++) {
> + vfree(data[i].unc);
> + vfree(data[i].cmp);
> + }
> +
> + vfree(data);
> +}
> +
> +static struct cmp_data *alloc_cmp_data(unsigned nr_threads)
> +{
> + struct cmp_data *data = NULL;
> + int i = -1;
> +
> + data = vzalloc(array_size(nr_threads, sizeof(*data)));
> + if (!data)
> + goto fail;
> +
> + for (i = 0; i < nr_threads; i++) {
> + data[i].unc = vzalloc(UNC_SIZE);
> + if (!data[i].unc)
> + goto fail;
> + data[i].cmp = vzalloc(CMP_SIZE);
> + if (!data[i].cmp)
> + goto fail;
> + }
> +
> + return data;
> +fail:
> + free_cmp_data(data, nr_threads);
> + return NULL;
> +}
> +
> /**
> * save_compressed_image - Save the suspend image data after compression.
> * @handle: Swap map handle to use for saving the image.
> @@ -724,7 +766,7 @@ static int save_compressed_image(struct swap_map_handle *handle,
> goto out_clean;
> }
>
> - data = vzalloc(array_size(nr_threads, sizeof(*data)));
> + data = alloc_cmp_data(nr_threads);
> if (!data) {
> pr_err("Failed to allocate %s data\n", hib_comp_algo);
> ret = -ENOMEM;
> @@ -902,7 +944,7 @@ static int save_compressed_image(struct swap_map_handle *handle,
> if (data[thr].cc)
> crypto_free_comp(data[thr].cc);
> }
> - vfree(data);
> + free_cmp_data(data, nr_threads);
> }
> if (page) free_page((unsigned long)page);
>
> @@ -1036,7 +1078,7 @@ static int get_swap_reader(struct swap_map_handle *handle,
> release_swap_reader(handle);
> return error;
> }
> - offset = tmp->map->next_swap;
> + offset = tmp->map->entries[NEXT_SWAP_INDEX];
> }
> handle->k = 0;
> handle->cur = handle->maps->map;
> @@ -1150,8 +1192,8 @@ struct dec_data {
> wait_queue_head_t done; /* decompression done */
> size_t unc_len; /* uncompressed length */
> size_t cmp_len; /* compressed length */
> - unsigned char unc[UNC_SIZE]; /* uncompressed buffer */
> - unsigned char cmp[CMP_SIZE]; /* compressed buffer */
> + unsigned char *unc; /* uncompressed buffer */
> + unsigned char *cmp; /* compressed buffer */
> };
>
> /*
> @@ -1189,6 +1231,45 @@ static int decompress_threadfn(void *data)
> return 0;
> }
>
> +static void free_dec_data(struct dec_data *data, unsigned nr_threads)
> +{
> + int i;
> +
> + if (!data)
> + return;
> +
> + for (i = 0; i < nr_threads; i++) {
> + vfree(data[i].unc);
> + vfree(data[i].cmp);
> + }
> +
> + vfree(data);
> +}
> +
> +static struct dec_data *alloc_dec_data(unsigned nr_threads)
> +{
> + struct dec_data *data = NULL;
> + int i = -1;
> +
> + data = vzalloc(array_size(nr_threads, sizeof(*data)));
> + if (!data)
> + goto fail;
> +
> + for (i = 0; i < nr_threads; i++) {
> + data[i].unc = vzalloc(UNC_SIZE);
> + if (!data[i].unc)
> + goto fail;
> + data[i].cmp = vzalloc(CMP_SIZE);
> + if (!data[i].cmp)
> + goto fail;
> + }
> +
> + return data;
> +fail:
> + free_dec_data(data, nr_threads);
> + return NULL;
> +}
> +
> /**
> * load_compressed_image - Load compressed image data and decompress it.
> * @handle: Swap map handle to use for loading data.
> @@ -1231,7 +1312,7 @@ static int load_compressed_image(struct swap_map_handle *handle,
> goto out_clean;
> }
>
> - data = vzalloc(array_size(nr_threads, sizeof(*data)));
> + data = alloc_dec_data(nr_threads);
> if (!data) {
> pr_err("Failed to allocate %s data\n", hib_comp_algo);
> ret = -ENOMEM;
> @@ -1510,7 +1591,7 @@ static int load_compressed_image(struct swap_map_handle *handle,
> if (data[thr].cc)
> crypto_free_comp(data[thr].cc);
> }
> - vfree(data);
> + free_dec_data(data, nr_threads);
> }
> vfree(page);
>
> @@ -1569,9 +1650,9 @@ int swsusp_check(bool exclusive)
> hib_resume_bdev_file = bdev_file_open_by_dev(swsusp_resume_device,
> BLK_OPEN_READ, holder, NULL);
> if (!IS_ERR(hib_resume_bdev_file)) {
> - clear_page(swsusp_header);
> + clear_page(swsusp_header_pg);
> error = hib_submit_io(REQ_OP_READ, swsusp_resume_block,
> - swsusp_header, NULL);
> + swsusp_header_pg, NULL);
> if (error)
> goto put;
>
> @@ -1581,7 +1662,7 @@ int swsusp_check(bool exclusive)
> /* Reset swap signature now */
> error = hib_submit_io(REQ_OP_WRITE | REQ_SYNC,
> swsusp_resume_block,
> - swsusp_header, NULL);
> + swsusp_header_pg, NULL);
> } else {
> error = -EINVAL;
> }
> @@ -1631,12 +1712,12 @@ int swsusp_unmark(void)
> int error;
>
> hib_submit_io(REQ_OP_READ, swsusp_resume_block,
> - swsusp_header, NULL);
> + swsusp_header_pg, NULL);
> if (!memcmp(HIBERNATE_SIG,swsusp_header->sig, 10)) {
> memcpy(swsusp_header->sig,swsusp_header->orig_sig, 10);
> error = hib_submit_io(REQ_OP_WRITE | REQ_SYNC,
> swsusp_resume_block,
> - swsusp_header, NULL);
> + swsusp_header_pg, NULL);
> } else {
> pr_err("Cannot find swsusp signature!\n");
> error = -ENODEV;
> @@ -1653,9 +1734,11 @@ int swsusp_unmark(void)
>
> static int __init swsusp_header_init(void)
> {
> - swsusp_header = (struct swsusp_header*) __get_free_page(GFP_KERNEL);
> - if (!swsusp_header)
> + swsusp_header_pg = (char *)__get_free_page(GFP_KERNEL);
> + if (!swsusp_header_pg)
> panic("Could not allocate memory for swsusp_header\n");
> + swsusp_header = (struct swsusp_header *)(swsusp_header_pg +
> + PAGE_SIZE - sizeof(struct swsusp_header));
> return 0;
> }
>