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

Harry Yoo
2025-07-02T13:04:11Z

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 <ziy@nvidia.com>
> > > Signed-off-by: David Hildenbrand <david@redhat.com>
> > > ---
> >
> > LGTM,
> > Reviewed-by: Harry Yoo <harry.yoo@oracle.com>
> >
> > 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