Re: [PATCH 2/5] hugetlb: add phys addr to struct huge_bootmem_page

[Andrew Morton <akpm@linux-foundation.org>]

On Tue, 28 Jun 2011 14:54:45 -0500
Becky Bruce <beckyb@kernel.crashing.org> wrote:

> From: Becky Bruce <beckyb@kernel.crashing.org>
> 
> This is needed on HIGHMEM systems - we don't always have a virtual
> address so store the physical address and map it in as needed.
> 
> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
> ---
>  include/linux/hugetlb.h |    3 +++
>  mm/hugetlb.c            |    8 +++++++-
>  2 files changed, 10 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 59225ef..19644e0 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -231,6 +231,9 @@ struct hstate {
>  struct huge_bootmem_page {
>  	struct list_head list;
>  	struct hstate *hstate;
> +#ifdef CONFIG_HIGHMEM
> +	phys_addr_t phys;
> +#endif
>  };
>  
>  struct page *alloc_huge_page_node(struct hstate *h, int nid);
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 6402458..2db81ea 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1105,8 +1105,14 @@ static void __init gather_bootmem_prealloc(void)
>  	struct huge_bootmem_page *m;
>  
>  	list_for_each_entry(m, &huge_boot_pages, list) {
> -		struct page *page = virt_to_page(m);
>  		struct hstate *h = m->hstate;
> +#ifdef CONFIG_HIGHMEM
> +		struct page *page = pfn_to_page(m->phys >> PAGE_SHIFT);
> +		free_bootmem_late((unsigned long)m,
> +				  sizeof(struct huge_bootmem_page));
> +#else
> +		struct page *page = virt_to_page(m);
> +#endif
>  		__ClearPageReserved(page);
>  		WARN_ON(page_count(page) != 1);
>  		prep_compound_huge_page(page, h->order);

nit: wrapping both definitions and statements in an ifdef like this is
a bit nasty from a readability and maintainability point of view - it's
inviting people to later make changes which fail to compile when the
config option is flipped.

This is better:

--- a/mm/hugetlb.c~hugetlb-add-phys-addr-to-struct-huge_bootmem_page-fix
+++ a/mm/hugetlb.c
@@ -1106,12 +1106,14 @@ static void __init gather_bootmem_preall
 
 	list_for_each_entry(m, &huge_boot_pages, list) {
 		struct hstate *h = m->hstate;
+		struct page *page;
+
 #ifdef CONFIG_HIGHMEM
-		struct page *page = pfn_to_page(m->phys >> PAGE_SHIFT);
+		page = pfn_to_page(m->phys >> PAGE_SHIFT);
 		free_bootmem_late((unsigned long)m,
 				  sizeof(struct huge_bootmem_page));
 #else
-		struct page *page = virt_to_page(m);
+		page = virt_to_page(m);
 #endif
 		__ClearPageReserved(page);
 		WARN_ON(page_count(page) != 1);
_