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

[Becky Bruce]

On Jul 21, 2011, at 5:44 PM, Andrew Morton wrote:

> On Tue, 28 Jun 2011 14:54:45 -0500
> Becky Bruce <beckyb@xxxxxxxxxxxxxxxxxxx> wrote:
> 
>> From: Becky Bruce <beckyb@xxxxxxxxxxxxxxxxxxx>
>> 
>> 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@xxxxxxxxxxxxxxxxxxx>
>> ---
>> 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);

Andrew,

I totally agree, this is better, thanks.   I see you've put my original patch and your fix into -mm;  I'd like it to percolate there for a bit before it goes to Linus to be sure I haven't broken anything.  It should be safe, as I don't think this particular function was ever expected to work on highmem platforms, but it's possible I'm overlooking something.

Cheers,
-Becky

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/