On Fri, Nov 29, 2013 at 02:11:14AM +0800, Jianyu Zhan wrote:
>
> Hi, Vladimir,
>
> On Fri, Nov 29, 2013 at 1:41 AM, Vladimir Murzin <murzi...@gmail.com> wrote:
> >
> > Any numbers for efficiency?
> >
>
> For the original implementation, vmalloc_to_pfn() wraps the vmalloc_to_page(),
> which means
>
> pfn ------> struct page ------> pfn
> | |
> vmalloc_to_page() vmalloc_to_pfn()
>
> So this patch interchange the implementation, do the dirty page table
> walking code in vmalloc_to_pfn(), and then vmalloc_to_page() uses it, the
> graph
> now becomes
>
> pfn ------> struct page
> | |
> vmalloc_to_pfn() vmalloc_to_page()
>
>
> >> /*
> >> - * Walk a vmap address to the struct page it maps.
> >> + * Walk a vmap address to the physical pfn it maps to.
> >> */
> >> -struct page *vmalloc_to_page(const void *vmalloc_addr)
> >> +unsigned long vmalloc_to_pfn(const void *vmalloc_addr)
> >> {
> >> unsigned long addr = (unsigned long) vmalloc_addr;
> >> - struct page *page = NULL;
> >> + unsigned long pfn;
> >
> > uninitialized pfn will lead to a bug.
> >
>
> Why? The coding pratice has mandates we use it after we initialize it,
> And if we initialize it , to what value will it promise no bug?
Unless you initialize it conditionally. I bet gcc warned you about this ;)
> It is unlikely a rubbish initial value will creep in.
>
>
> >> /*
> >> @@ -244,23 +244,23 @@ struct page *vmalloc_to_page(const void
> >> *vmalloc_addr)
> >> ptep = pte_offset_map(pmd, addr);
> >> pte = *ptep;
> >> if (pte_present(pte))
> >> - page = pte_page(pte);
> >> + pfn = pte_page(pte);
> >
> > page_to_pfn is missed here.
> >
> > Have you ever tested there is no functional changes?
>
> Oh, gods. My fault. It did has no functional changes.
>
> I just sent the incorrect patch...
>
> it should be
> - page = pte_page(pte);
> + pfn = pte_pfn(pte);;
>
> Here is the resent patch:
>
I think it is incorrect too. Originally, vmalloc_to_page might return NULL
under some conditions. With your implementation it will return pfn_to_page(0)
which is not the same as NULL.
Vladimir
>
> ---
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 0fdf968..e4f0db2 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -220,12 +220,12 @@ int is_vmalloc_or_module_addr(const void *x)
> }
>
> /*
> - * Walk a vmap address to the struct page it maps.
> + * Walk a vmap address to the physical pfn it maps to.
> */
> -struct page *vmalloc_to_page(const void *vmalloc_addr)
> +unsigned long vmalloc_to_pfn(const void *vmalloc_addr)
> {
> unsigned long addr = (unsigned long) vmalloc_addr;
> - struct page *page = NULL;
> + unsigned long pfn = 0;
> pgd_t *pgd = pgd_offset_k(addr);
>
> /*
> @@ -244,23 +244,23 @@ struct page *vmalloc_to_page(const void *vmalloc_addr)
> ptep = pte_offset_map(pmd, addr);
> pte = *ptep;
> if (pte_present(pte))
> - page = pte_page(pte);
> + pfn = pte_pfn(pte);
> pte_unmap(ptep);
> }
> }
> }
> - return page;
> + return pfn;
> }
> -EXPORT_SYMBOL(vmalloc_to_page);
> +EXPORT_SYMBOL(vmalloc_to_pfn);
>
> /*
> - * Map a vmalloc()-space virtual address to the physical page frame number.
> + * Map a vmalloc()-space virtual address to the struct page.
> */
> -unsigned long vmalloc_to_pfn(const void *vmalloc_addr)
> +struct page *vmalloc_to_page(const void *vmalloc_addr)
> {
> - return page_to_pfn(vmalloc_to_page(vmalloc_addr));
> + return pfn_to_page(vmalloc_to_pfn(vmalloc_addr));
> }
> -EXPORT_SYMBOL(vmalloc_to_pfn);
> +EXPORT_SYMBOL(vmalloc_to_page);
>
>
> /*** Global kva allocator ***/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
