Subject: Re: [PATCH v1 20/29] mm: convert "movable" flag in page->mapping to a page flag

From: Harry Yoo <[email-masked]>
Date: Wed, 2 Jul 2025 22:01:57 +0900

On Wed, Jul 02, 2025 at 02:01:33PM +0200, David Hildenbrand wrote:
> On 02.07.25 13:54, Harry Yoo wrote:
> > On Mon, Jun 30, 2025 at 03:00:01PM +0200, David Hildenbrand wrote:
> > > Instead, let's use a page flag. As the page flag can result in
> > > false-positives, glue it to the page types for which we
> > > support/implement movable_ops page migration.
> > > 
> > > The flag reused by PageMovableOps() might be sued by other pages, so
> > > warning in case it is set in page_has_movable_ops() might result in
> > > false-positive warnings.
> > > 
> > > Reviewed-by: Zi Yan <[email-masked]>
> > > Signed-off-by: David Hildenbrand <[email-masked]>
> > > ---
> > 
> > LGTM,
> > Reviewed-by: Harry Yoo <[email-masked]>
> > 
> > With a question: is there any reason to change the page flag
> > operations to use atomic bit ops?
> 
> As we have the page lock in there, it's complicated. I thought about this
> when writing that code, and was not able to convince myself that it is safe.
> 
> But that was when I was prototyping and reshuffling patches, and we would
> still have code that would clear the flag.
 
> Given that we only allow setting the flag, it might be okay to use the
> non-atomic variant as long as there can be nobody racing with us when
> modifying flags. Especially trying to lock the folio concurrently is the big
> problem.
>
> In isolate_movable_ops_page(), there is a comment about checking the flag
> before grabbing the page lock, so that should be handled.

Right.

> I'll have to check some other cases in balloon/zsmalloc code.

Okay, it's totally fine to go with atomic version and then
switching back to non atomic ops when we're sure it's safe.

-- 
Cheers,
Harry / Hyeonggon