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