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

[Breno Leitao]

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