Re: [PATCH] mm: introduce __linear_page_index()

[Andrew Morton]

On Tue, 5 Feb 2013 15:03:39 +0800
Huang Shijie <b32955@xxxxxxxxxxxxx> wrote:

> There are many places we should get the offset(in PAGE_SIZE unit) of
> an address within a non-hugetlb vma.
> 
> In order to simplify the code, add a new helper __linear_page_index()
> to do the work.
> 

Seems nice.

> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -310,15 +310,23 @@ static inline loff_t page_file_offset(struct page *page)
>  extern pgoff_t linear_hugepage_index(struct vm_area_struct *vma,
>  				     unsigned long address);
>  
> -static inline pgoff_t linear_page_index(struct vm_area_struct *vma,
> +/* The offset for an address within a non-hugetlb vma, in PAGE_SIZE unit. */

"The offset into the mapped file for ..."

> +static inline pgoff_t __linear_page_index(struct vm_area_struct *vma,
>  					unsigned long address)
>  {
>  	pgoff_t pgoff;
> +
> +	pgoff = (address - vma->vm_start) >> PAGE_SHIFT;
> +	return pgoff + vma->vm_pgoff;
> +}
> +
> +static inline pgoff_t linear_page_index(struct vm_area_struct *vma,
> +					unsigned long address)
> +{
>  	if (unlikely(is_vm_hugetlb_page(vma)))
>  		return linear_hugepage_index(vma, address);
> -	pgoff = (address - vma->vm_start) >> PAGE_SHIFT;
> -	pgoff += vma->vm_pgoff;
> -	return pgoff >> (PAGE_CACHE_SHIFT - PAGE_SHIFT);
> +	return __linear_page_index(vma, address) >>
> +				(PAGE_CACHE_SHIFT - PAGE_SHIFT);
>  }

I don't think we need bother creating both linear_page_index() and
__linear_page_index().  Realistically, we won't be supporting
PAGE_SHIFT!=PAGE_CACHE_SHIFT.  And most (or all?) of the sites which
you changed should have been using PAGE_CACHE_SHIFT anyway!

> @@ -1201,8 +1199,7 @@ SYSCALL_DEFINE1(shmdt, char __user *, shmaddr)
>  
>  		/* finding a matching vma now does not alter retval */
>  		if ((vma->vm_ops == &shm_vm_ops) &&
> -			(vma->vm_start - addr)/PAGE_SIZE == vma->vm_pgoff)
> -
> +			0 == __linear_page_index(vma, addr))

erk, please don't do this - it makes kernel developers fall over in
shock.  Let's do

			__linear_page_index(vma, addr) == 0

(This won't compile if someone forgets a `=', so the usual reason for
the backward comparison isn't valid).


--
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/