Re: [PATCH v2 18/33] mm/vma: use vma_start_pgoff(), linear_page_index() in mm code

[Vlastimil Babka (SUSE)]

On 7/16/26 11:27, Lorenzo Stoakes (ARM) wrote:
> On Wed, Jul 15, 2026 at 05:23:09PM +0200, Vlastimil Babka (SUSE) wrote:
>> 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 <gourry@xxxxxxxxxx>
>> > Reviewed-by: SJ Park <sj@xxxxxxxxxx>
>> > Reviewed-by: Pedro Falcato <pfalcato@xxxxxxx>
>> > Signed-off-by: Lorenzo Stoakes <ljs@xxxxxxxxxx>
>>
>> Reviewed-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>
> 
> Thanks!
> 
>>
>> 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?
> 
> So this is:
> 
> 	(x >> order) + (y >> order)
> 
> And linear_page_index(vma, addr) >> order is:
> 
> 	(x + y) >> order
> 
> Consider:
> 
> 	order = 1, x = 0b11, y = 0b01
> 
> 	(x    >> order) + (y    >> order) =
> 	(0b11 >> 1    ) + (0b01 >> 1    ) =
> 	0b1             + 0               =
> 	1
> 
> 	(x    + y   ) >> order =
> 	(0b11 + 0b01) >> 1     =
> 	(0b100      ) >> 1     =
> 	2
> 
> They are not equivalent - the carry bit changes the result.

Right, thanks. Didn't consider it would be actually working with unaligned
addresses but seems like it can.

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

Thanks for pretending you didn't notice this one, which was even more
obviously incorrect :)

>> >  	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?
> 
> See above.
> 
>>
>> >  	}
>> >  	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
> 
> Yup at least improved it here :)

And a later patch removes the other occurence.

>>
>> > +		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;
> 
> Cheers, Lorenzo