Re: [PATCH] bootmem: Stop using page->index

[Yosry Ahmed <yosryahmed@google.com>]

On Thu, Sep 12, 2024 at 11:56 AM Matthew Wilcox (Oracle)
<willy@infradead.org> wrote:
>
> Encode the type into the bottom four bits of page->private and the
> info into the remaining bits.  Also turn the bootmem type into a
> named enum.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  arch/x86/mm/init_64.c        |  9 ++++-----
>  include/linux/bootmem_info.h | 25 +++++++++++++++++--------
>  mm/bootmem_info.c            | 11 ++++++-----
>  mm/sparse.c                  |  8 ++++----
>  4 files changed, 31 insertions(+), 22 deletions(-)
>
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index ff253648706f..4d5fde324136 100644
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -987,13 +987,12 @@ int arch_add_memory(int nid, u64 start, u64 size,
>
>  static void __meminit free_pagetable(struct page *page, int order)
>  {
> -       unsigned long magic;
> -       unsigned int nr_pages = 1 << order;
> -
>         /* bootmem page has reserved flag */
>         if (PageReserved(page)) {
> -               magic = page->index;
> -               if (magic == SECTION_INFO || magic == MIX_SECTION_INFO) {
> +               enum bootmem_type type = bootmem_type(page);
> +               unsigned long nr_pages = 1 << order;
> +
> +               if (type == SECTION_INFO || type == MIX_SECTION_INFO) {
>                         while (nr_pages--)
>                                 put_page_bootmem(page++);
>                 } else
> diff --git a/include/linux/bootmem_info.h b/include/linux/bootmem_info.h
> index cffa38a73618..e2fe5de93dcc 100644
> --- a/include/linux/bootmem_info.h
> +++ b/include/linux/bootmem_info.h
> @@ -6,11 +6,10 @@
>  #include <linux/kmemleak.h>
>
>  /*
> - * Types for free bootmem stored in page->lru.next. These have to be in
> - * some random range in unsigned long space for debugging purposes.
> + * Types for free bootmem stored in the low bits of page->private.
>   */
> -enum {
> -       MEMORY_HOTPLUG_MIN_BOOTMEM_TYPE = 12,
> +enum bootmem_type {
> +       MEMORY_HOTPLUG_MIN_BOOTMEM_TYPE = 1,
>         SECTION_INFO = MEMORY_HOTPLUG_MIN_BOOTMEM_TYPE,
>         MIX_SECTION_INFO,
>         NODE_INFO,
> @@ -21,9 +20,19 @@ enum {
>  void __init register_page_bootmem_info_node(struct pglist_data *pgdat);
>
>  void get_page_bootmem(unsigned long info, struct page *page,
> -                     unsigned long type);
> +               enum bootmem_type type);
>  void put_page_bootmem(struct page *page);
>
> +static inline enum bootmem_type bootmem_type(const struct page *page)
> +{
> +       return (unsigned long)page->private & 0xf;
> +}
> +
> +static inline unsigned long bootmem_info(const struct page *page)
> +{
> +       return (unsigned long)page->private >> 4;
> +}

Would it be better to define the number of bits used for the type as a
macro and use it throughout (bootmem_type(), bootmem_info(),
get_page_bootmem())?.

We can also add a static assertion or build bug in case 4 bits is not
enough anymore (e.g. if someone changes the enum values).

> +
>  /*
>   * Any memory allocated via the memblock allocator and not via the
>   * buddy will be marked reserved already in the memmap. For those
> @@ -31,7 +40,7 @@ void put_page_bootmem(struct page *page);
>   */
>  static inline void free_bootmem_page(struct page *page)
>  {
> -       unsigned long magic = page->index;
> +       enum bootmem_type type = bootmem_type(page);
>
>         /*
>          * The reserve_bootmem_region sets the reserved flag on bootmem
> @@ -39,7 +48,7 @@ static inline void free_bootmem_page(struct page *page)
>          */
>         VM_BUG_ON_PAGE(page_ref_count(page) != 2, page);
>
> -       if (magic == SECTION_INFO || magic == MIX_SECTION_INFO)
> +       if (type == SECTION_INFO || type == MIX_SECTION_INFO)
>                 put_page_bootmem(page);
>         else
>                 VM_BUG_ON_PAGE(1, page);
> @@ -54,7 +63,7 @@ static inline void put_page_bootmem(struct page *page)
>  }
>
>  static inline void get_page_bootmem(unsigned long info, struct page *page,
> -                                   unsigned long type)
> +                                   enum bootmem_type type)
>  {
>  }
>
> diff --git a/mm/bootmem_info.c b/mm/bootmem_info.c
> index fa7cb0c87c03..95f288169a38 100644
> --- a/mm/bootmem_info.c
> +++ b/mm/bootmem_info.c
> @@ -14,23 +14,24 @@
>  #include <linux/memory_hotplug.h>
>  #include <linux/kmemleak.h>
>
> -void get_page_bootmem(unsigned long info, struct page *page, unsigned long type)
> +void get_page_bootmem(unsigned long info, struct page *page,
> +               enum bootmem_type type)
>  {
> -       page->index = type;
> +       BUG_ON(type > 0xf);
> +       BUG_ON(info > (ULONG_MAX >> 4));
>         SetPagePrivate(page);
> -       set_page_private(page, info);
> +       set_page_private(page, info << 4 | type);
>         page_ref_inc(page);
>  }
>
>  void put_page_bootmem(struct page *page)
>  {
> -       unsigned long type = page->index;
> +       enum bootmem_type type = bootmem_type(page);
>
>         BUG_ON(type < MEMORY_HOTPLUG_MIN_BOOTMEM_TYPE ||
>                type > MEMORY_HOTPLUG_MAX_BOOTMEM_TYPE);
>
>         if (page_ref_dec_return(page) == 1) {
> -               page->index = 0;
>                 ClearPagePrivate(page);
>                 set_page_private(page, 0);
>                 INIT_LIST_HEAD(&page->lru);
> diff --git a/mm/sparse.c b/mm/sparse.c
> index dc38539f8560..6ba5354cf2e1 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -720,19 +720,19 @@ static void depopulate_section_memmap(unsigned long pfn, unsigned long nr_pages,
>  static void free_map_bootmem(struct page *memmap)
>  {
>         unsigned long maps_section_nr, removing_section_nr, i;
> -       unsigned long magic, nr_pages;
> +       unsigned long type, nr_pages;
>         struct page *page = virt_to_page(memmap);
>
>         nr_pages = PAGE_ALIGN(PAGES_PER_SECTION * sizeof(struct page))
>                 >> PAGE_SHIFT;
>
>         for (i = 0; i < nr_pages; i++, page++) {
> -               magic = page->index;
> +               type = bootmem_type(page);
>
> -               BUG_ON(magic == NODE_INFO);
> +               BUG_ON(type == NODE_INFO);
>
>                 maps_section_nr = pfn_to_section_nr(page_to_pfn(page));
> -               removing_section_nr = page_private(page);
> +               removing_section_nr = bootmem_info(page);
>
>                 /*
>                  * When this function is called, the removing section is
> --
> 2.43.0
>
>