On 7/10/26 22:16, Lorenzo Stoakes wrote:
> There are many instances in which linear_page_index() (as well as
> linear_page_delta()) is open-coded, which is confusing and inconsistent.
> 
> Additionally, vma->vm_pgoff doesn't necessarily make it clear that this is
> the page offset of the start of the VMA range.
> 
> Doing so also aids greppability.
> 
> So use vma_start_pgoff() in favour of directly accessing vma->vm_pgoff, and
> linear_page_index() where we can.
> 
> This also lays the ground for future changes which will add an anonymous
> page offset in order to be able to index MAP_PRIVATE-file backed anon
> folios in terms of their virtual page offset.
> 
> No functional change intended.
> 
> Reviewed-by: Gregory Price <[email protected]>
> Reviewed-by: SJ Park <[email protected]>
> Reviewed-by: Pedro Falcato <[email protected]>
> Signed-off-by: Lorenzo Stoakes <[email protected]>

Reviewed-by: Vlastimil Babka (SUSE) <[email protected]>

Nit:

> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index 2058db9c01d5..d10b8042adb5 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -10,6 +10,7 @@
>  #include <linux/mman.h>
>  #include <linux/mmu_notifier.h>
>  #include <linux/page_idle.h>
> +#include <linux/pagemap.h>
>  #include <linux/pagewalk.h>
>  #include <linux/sched/mm.h>
>  
> @@ -623,8 +624,8 @@ static void damos_va_migrate_dests_add(struct folio 
> *folio,
>       }
>  
>       order = folio_order(folio);
> -     ilx = vma->vm_pgoff >> order;
> -     ilx += (addr - vma->vm_start) >> (PAGE_SHIFT + order);
> +     ilx = vma_start_pgoff(vma) >> order;
> +     ilx += linear_page_delta(vma, addr) >> order;

Could these be combined to linear_page_index(vma, addr) >> order?

>  
>       for (i = 0; i < dests->nr_dests; i++)
>               weight_total += dests->weight_arr[i];

> diff --git a/mm/filemap.c b/mm/filemap.c
> index b39111abdc4b..1dbb4c6f824e 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -3411,8 +3411,8 @@ static struct file *do_sync_mmap_readahead(struct 
> vm_fault *vmf)
>                * of memory.
>                */
>               struct vm_area_struct *vma = vmf->vma;
> -             unsigned long start = vma->vm_pgoff;
> -             unsigned long end = start + vma_pages(vma);
> +             const unsigned long start = vma_start_pgoff(vma);
> +             const unsigned long end = vma_end_pgoff(vma);
>               unsigned long ra_end;
>  
>               ra->order = exec_folio_order();
> @@ -3930,7 +3930,8 @@ vm_fault_t filemap_map_pages(struct vm_fault *vmf,
>               goto out;
>       }
>  
> -     addr = vma->vm_start + ((start_pgoff - vma->vm_pgoff) << PAGE_SHIFT);
> +     addr = vma->vm_start +
> +             ((start_pgoff - vma_start_pgoff(vma)) << PAGE_SHIFT);

This could be linear_page_index(vma, start_pgoff) <<

>       vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, addr, &vmf->ptl);
>       if (!vmf->pte) {
>               folio_unlock(folio);

> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -2049,8 +2049,8 @@ struct mempolicy *get_vma_policy(struct vm_area_struct 
> *vma,
>               pol = get_task_policy(current);
>       if (pol->mode == MPOL_INTERLEAVE ||
>           pol->mode == MPOL_WEIGHTED_INTERLEAVE) {
> -             *ilx += vma->vm_pgoff >> order;
> -             *ilx += (addr - vma->vm_start) >> (PAGE_SHIFT + order);
> +             *ilx += vma_start_pgoff(vma) >> order;
> +             *ilx += linear_page_delta(vma, addr) >> order;

Also?

>       }
>       return pol;
>  }

> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -975,7 +975,7 @@ static int do_mmap_private(struct vm_area_struct *vma,
>               /* read the contents of a file into the copy */
>               loff_t fpos;
>  
> -             fpos = vma->vm_pgoff;
> +             fpos = vma_start_pgoff(vma);
>               fpos <<= PAGE_SHIFT;
>  
>               ret = kernel_read(vma->vm_file, base, len, &fpos);
> @@ -1378,7 +1378,8 @@ static int split_vma(struct vma_iterator *vmi, struct 
> vm_area_struct *vma,
>       delete_nommu_region(vma->vm_region);
>       if (new_below) {
>               vma->vm_region->vm_start = vma->vm_start = addr;
> -             vma->vm_region->vm_pgoff = vma->vm_pgoff += npages;

yuck, good to remove that ugly construct
it's not the only occurence in this function
oh well

> +             vma->vm_pgoff += npages;
> +             vma->vm_region->vm_pgoff = vma_start_pgoff(vma);
>       } else {
>               vma->vm_region->vm_end = vma->vm_end = addr;
>               vma->vm_region->vm_top = addr;

Reply via email to