On Wed, 19 Jan 2022, Matthew Wilcox wrote:
> Hi Hugh,
>
> What do you think to this simplification? Dave dislikes the ?: usage
> in this context, and I thought we could usefully put the condition
> inside the (inline) function.
Yes, that's an improvement. I think when I created vma_address_end()
originally (long before posting), there was only one caller who needed
to worry about PageKsm, so I avoided inflicting that awkwardness on the
others; but now it looks like most (of its few) callers do have to worry.
But I'll suggest one nit, which you can ignore if you feel strongly
otherwise: I'd prefer vma_address_end(page, vma, address) as a better
match to vma_address(page, vma).
>
> I could also go for:
>
> if (!PageCompound(page))
> return address + PAGE_SIZE;
> VM_BUG_ON_PAGE(PageKsm(page), page); /* KSM page->index unusable */
Another way to do it, yes: I don't mind either way.
Thanks,
Hugh
>
> in case anyone starts to create compound KSM pages.
>
> diff --git a/mm/internal.h b/mm/internal.h
> index c774075b3893..7cd33ee4df32 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -462,13 +462,13 @@ vma_address(struct page *page, struct vm_area_struct *vma)
> * Assumes that vma_address() already returned a good starting address.
> * If page is a compound head, the entire compound page is considered.
> */
> -static inline unsigned long
> -vma_address_end(struct page *page, struct vm_area_struct *vma)
> +static inline unsigned long vma_address_end(struct page *page,
> + unsigned long address, struct vm_area_struct *vma)
> {
> pgoff_t pgoff;
> - unsigned long address;
>
> - VM_BUG_ON_PAGE(PageKsm(page), page); /* KSM page->index unusable */
> + if (PageKsm(page) || !PageCompound(page))
> + return address + PAGE_SIZE;
> pgoff = page_to_pgoff(page) + compound_nr(page);
> address = vma->vm_start + ((pgoff - vma->vm_pgoff) << PAGE_SHIFT);
> /* Check for address beyond vma (or wrapped through 0?) */
> diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
> index f7b331081791..fcd7b9ccfb1e 100644
> --- a/mm/page_vma_mapped.c
> +++ b/mm/page_vma_mapped.c
> @@ -181,15 +181,7 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)
> return true;
> }
>
> - /*
> - * Seek to next pte only makes sense for THP.
> - * But more important than that optimization, is to filter out
> - * any PageKsm page: whose page->index misleads vma_address()
> - * and vma_address_end() to disaster.
> - */
> - end = PageTransCompound(page) ?
> - vma_address_end(page, pvmw->vma) :
> - pvmw->address + PAGE_SIZE;
> + end = vma_address_end(page, pvmw->address, pvmw->vma);
> if (pvmw->pte)
> goto next_pte;
> restart:
> diff --git a/mm/rmap.c b/mm/rmap.c
> index a531b64d53fa..5d5dc2a60a26 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -946,7 +946,7 @@ static bool page_mkclean_one(struct page *page, struct vm_area_struct *vma,
> */
> mmu_notifier_range_init(&range, MMU_NOTIFY_PROTECTION_PAGE,
> 0, vma, vma->vm_mm, address,
> - vma_address_end(page, vma));
> + vma_address_end(page, address, vma));
> mmu_notifier_invalidate_range_start(&range);
>
> while (page_vma_mapped_walk(&pvmw)) {
> @@ -1453,8 +1453,7 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
> * Note that the page can not be free in this function as call of
> * try_to_unmap() must hold a reference on the page.
> */
> - range.end = PageKsm(page) ?
> - address + PAGE_SIZE : vma_address_end(page, vma);
> + range.end = vma_address_end(page, address, vma);
> mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, vma->vm_mm,
> address, range.end);
> if (PageHuge(page)) {
> @@ -1757,8 +1756,7 @@ static bool try_to_migrate_one(struct page *page, struct vm_area_struct *vma,
> * Note that the page can not be free in this function as call of
> * try_to_unmap() must hold a reference on the page.
> */
> - range.end = PageKsm(page) ?
> - address + PAGE_SIZE : vma_address_end(page, vma);
> + range.end = vma_address_end(page, address, vma);
> mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, vma->vm_mm,
> address, range.end);
> if (PageHuge(page)) {
