On Jul 21, 2011, at 5:44 PM, Andrew Morton wrote:
> On Tue, 28 Jun 2011 14:54:45 -0500
> Becky Bruce <bec...@kernel.crashing.org> wrote:
>
>> From: Becky Bruce <bec...@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 <bec...@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);
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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
