RE: mm/DAMON: Profiling enhancements for DAMON

[Prasad, Aravinda]

> -----Original Message-----
> From: Hansen, Dave <dave.hansen@xxxxxxxxx>
> Sent: Friday, December 15, 2023 11:40 PM
> To: Prasad, Aravinda <aravinda.prasad@xxxxxxxxx>; damon@xxxxxxxxxxxxxxx; linux-
> mm@xxxxxxxxx; sj@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Cc: s2322819@xxxxxxxx; Kumar, Sandeep4 <sandeep4.kumar@xxxxxxxxx>;
> Huang, Ying <ying.huang@xxxxxxxxx>; Williams, Dan J
> <dan.j.williams@xxxxxxxxx>; Subramoney, Sreenivas
> <sreenivas.subramoney@xxxxxxxxx>; Kervinen, Antti <antti.kervinen@xxxxxxxxx>;
> Kanevskiy, Alexander <alexander.kanevskiy@xxxxxxxxx>; Alan Nair
> <alan.nair@xxxxxxxxx>
> Subject: Re: mm/DAMON: Profiling enhancements for DAMON
> 
> On 12/14/23 23:46, Aravinda Prasad wrote:
> > +static int damon_young_pmd(pmd_t *pmd, unsigned long addr,
> > +		unsigned long next, struct mm_walk *walk) {
> > +	spinlock_t *ptl;
> > +	struct damon_young_walk_private *priv = walk->private;
> > +
> > +	if (!pmd_present(*pmd) || pmd_none(*pmd))
> > +		goto out;
> > +
> > +	ptl = pmd_lock(walk->mm, pmd);
> > +	if (pmd_young(*pmd) || mmu_notifier_test_young(walk->mm, addr))
> > +		priv->young = true;
> > +
> > +	*priv->folio_sz = (1UL << PMD_SHIFT);
> > +	spin_unlock(ptl);
> > +out:
> > +	return 0;
> > +}
> 
> There are a number of paired p*_present() and p_*none() checks in this patch.
> What is their function (especially pmd_none())?  For instance,
> damon_young_pmd() gets called via the pagewalk infrastructure:

Hmm.. I think, they can be removed. Looks like pagewalk infrastructure should take care.
But will double check.

> 
> > static int walk_pmd_range(pud_t *pud, unsigned long addr, unsigned long end,
> >                           struct mm_walk *walk) {
> ...
> >                 next = pmd_addr_end(addr, end);
> >                 if (pmd_none(*pmd)) {
> ...
> >                         continue;
> >                 }
> ...
> >                 if (ops->pmd_entry)
> >                         err = ops->pmd_entry(pmd, addr, next, walk);
> 
> I'd suggest taking a closer look at the code you are replacing and other mm_walk
> users to make sure the handlers are doing appropriate checks.

Sure.