Re: [PATCH v6 3/4] mm: huge_memory: refactor enabled_store() with change_enabled()

Breno Leitao
2026-03-16T10:13:31Z

Hello Sohil,

On Fri, Mar 13, 2026 at 03:31:25PM -0700, Sohil Mehta wrote:
> Hello,
>
> On 3/11/2026 3:17 AM, Breno Leitao wrote:
>
> > +static bool set_global_enabled_mode(enum global_enabled_mode mode)
> > +{
> > + static const unsigned long thp_flags[] = {
> > + TRANSPARENT_HUGEPAGE_FLAG,
> > + TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG,
> > + };
> > + enum global_enabled_mode m;
> > + bool changed = false;
> > +
> > + for (m = 0; m < ARRAY_SIZE(thp_flags); m++) {
> > + if (m == mode)
> > + changed |= !__test_and_set_bit(thp_flags[m],
> > + &transparent_hugepage_flags);
> > + else
> > + changed |= __test_and_clear_bit(thp_flags[m],
> > + &transparent_hugepage_flags);
>
> I am not a mm expert and typically do not follow the mm list. Is there
> an issue with the usage of non-atomic variants here? The commit message
> says this uses the same pattern as set_anon_enabled_mode().
>
> However, set_anon_enabled_mode() has a spinlock=>huge_anon_orders_lock
> protecting the access. But, transparent_hugepage_flags seems to be
> unprotected in that regard.

I don't think that the atomic vs non-atomic will not help much, given
this is a compoud operation. Independently if this is atomic or not, it
is racy with anyone changing these fields (transparent_hugepage_flags).
In other words, Atomic ops make each individual bit flip safe, but
set_global_enabled_mode() and defrag_store() need to flip multiple bits
as a group. With atomic ops, two concurrent writers can still interleave
and leave the flags in an invalid state.

That said, Although I don't think this patch is making it worse, I think
the is a racy issue here that we can make better.

My suggestion is to move the rest of the helpers (defrag_store()) to use
sysfs_match_string(), and then create a thp_flags_lock spinlock to
protect operations against transparent_hugepage_flags. Any concern about
this approach?

Thanks for reviewing it,
--breno