Re: [PATCH 1/4] mm/damon: defensively skip zone device folios in damon_get_folio()

On Tue, 28 Jul 2026 22:01:01 -0400 Gregory Price <[email protected]> wrote:

> On Tue, Jul 28, 2026 at 05:50:36PM -0700, SJ Park wrote:
> > Hi Gregory,
> > 
> > 
> > On Tue, 28 Jul 2026 15:47:11 -0400 Gregory Price <[email protected]> wrote:
> > 
> > > All DAMON physical- and virtual-address operations obtain their folios
> > > through damon_get_folio(). That helper already excludes ZONE_DEVICE
> > > memory implicitly via pfn_to_online_page() and folio_test_lru(), but
> > > this is inconsistent with other callers in mm/ which test explicitly.
> > > 
> > > Add an explicit folio_is_zone_device() rejection in damon_get_folio()
> > > so the guarantee lives in one place and covers every caller uniformly,
> > > consistent with other mm walkers that reject zone device folios.
> > 
> > Thank you for checking this and sharing this nice patch!
> > 
> > However, I'm not very sure adding a call that not really required for only
> > making it looks consistent with others is the right choice.  I'm especially
> > concerned if this will confuse future readers including myself.
> > 
> > If this is a hardening purpose, we have CONFIG_DAMON_DEBUG_SANITY for the
> > purpose.  What about using it with a good comment explaining why we doing that?
> > 
> 
> It's actually two fold:
> 
>   1) make zone_device more consistently handled across mm/
>   2) make these checks possible to abstract out later
> 
> In the private node work - every location that we'd need to filter
> folios based on private node membership is a zone_device check.
> 
> Where zone device checks are missing (like here) there's some other
> implicit reason (specific to zone device) that allows it to be avoided.
> 
> I pointed out in the sashiko feedback that DAMON depends on a check
> for folio->pgmap in the hotplug code.  This is all quite fragile, and
> we can see that at least one patch in this series fixes an actual bug.
> 
> It seemed warranted to go audit every service and add the zone_device
> checks to make it clear there's a zone device interaction - even if
> presently unreachable - should some silent, implicit filter change.
> 
> The goal is to come back around and replace these with something like:
> 
>     bool folio_allows_mm_op(struct folio *folio,
>                             enum node_states feature)
>     {
>         if (folio_is_zone_device(folio) ||
>             !node_state(folio_nid(folio), feature));
>     }
> 
>     - if (folio_is_zone_device(folio))
>     + if (folio_allows_mm_op(folio, N_MEMORY_DAMON))
> 
> If we ever get to folios having memdesc flags beyond page flags,
> this also gives us a single mm-wide location for these kinds of
> checks in the future.

Thank you for kindly explaining the motivation.

So, what do you think about adding the check under CONFIG_DAMON_DEBUG_SANITY
with good comments at the moment, and later converting to folio_allows_mm_ops()
together?


Thanks,
SJ

[...]