Re: [PATCH v1 6/7] mm/ksm: convert break_ksm() to use walk_page_range_vma()

[David Hildenbrand]

> > +int break_ksm_pud_entry(pud_t *pud, unsigned long addr, unsigned long next,
> > +			struct mm_walk *walk)
> > +{
> > +	/* We only care about page tables to walk to a single base page. */
> > +	if (pud_leaf(*pud) || !pud_present(*pud))
> > +		return 1;
> > +	return 0;
> > +}
>
> Is this needed?  I thought the pgtable walker handlers this already.
>
> [...]

Most probably yes. I was trying to avoid about PUD splits, but I guess 
we simply should not care in VMAs that are considered by KSM (MERGABLE). 
Most probably never ever happens.

> >   static int break_ksm(struct vm_area_struct *vma, unsigned long addr)
> >   {
> > -	struct page *page;
> >   	vm_fault_t ret = 0;
> >   
> > +	if (WARN_ON_ONCE(!IS_ALIGNED(addr, PAGE_SIZE)))
> > +		return -EINVAL;
> > +
> >   	do {
> >   		bool ksm_page = false;
> >   
> >   		cond_resched();
> > -		page = follow_page(vma, addr,
> > -				FOLL_GET | FOLL_MIGRATION | FOLL_REMOTE);
> > -		if (IS_ERR_OR_NULL(page))
> > -			break;
> > -		if (PageKsm(page))
> > -			ksm_page = true;
> > -		put_page(page);
> > +		ret = walk_page_range_vma(vma, addr, addr + PAGE_SIZE,
> > +					  &break_ksm_ops, &ksm_page);
> > +		if (WARN_ON_ONCE(ret < 0))
> > +			return ret;
>
> I'm not sure this would be worth it, especially with a 4% degrade.  The
> next patch will be able to bring 50- LOC, but this patch does 60+ anyway,
> based on another new helper just introduced...
>
> I just don't see whether there's strong enough reason to do so to drop
> FOLL_MIGRATE.  It's different to the previous VM_FAULT_WRITE refactor
> because of the unshare approach was much of a good reasoning to me.
>
> Perhaps I missed something?

My main motivation is to remove most of that GUP hackery here, which is
1) Getting a reference on a page and waiting for migration to finish
   even though both is unnecessary.
2) As we don't have sufficient control, we added FOLL_MIGRATION hacks to
   MM core to work around limitations in the GUP-based approacj.
3) We rely on legacy follow_page() interface that we should really get
   rid of in the long term.

All we want to do is walk the page tables and make a decision if 
something we care about is mapped. Instead of leaking these details via 
hacks into GUP code and making that code harder to grasp/maintain, this 
patch moves that logic to the actual user, while reusing generic page 
walking code.

Yes, we have to extend page walking code, but it's just the natural, 
non-hacky way of doing it.

Regarding the 4% performance degradation (if I wouldn't have added the 
benchmarks, nobody would know and probably care ;) ), I am not quite 
sure why that is the case. We're just walking page tables after all in 
both cases. Maybe the callback-based implementation of pagewalk code is 
less efficient, but we might be able to improve that implementation if 
we really care about performance here. Maybe removing 
break_ksm_pud_entry() already improves the numbers slightly.

Thanks!

--
Thanks,

David / dhildenb