Re: [PATCH v4 2/4] mm: huge_memory: refactor anon_enabled_store() with change_anon_orders()

[Breno Leitao <leitao@debian.org>]

On Mon, Mar 09, 2026 at 02:43:11PM +0100, David Hildenbrand (Arm) wrote:
> On 3/9/26 12:07, Breno Leitao wrote:
> > +static bool change_anon_orders(int order, enum anon_enabled_mode mode)
> 
> I would suggest something a bit longer but clearer
> 
> "set_anon_enabled_mode_for_order()"
> 
> Or shorter
> 
> "set_anon_enabled_mode"
set_anon_enabled_mode() seems to be better. Then I have:

set_global_enabled_mode() and set_anon_enabled_mode().

> 1) set vs. change. the function returns whether actually something
>    changed.
> 
> 2) We're not really changing "anon_orders". Yeah, we're updating
>    variables that are named "huge_anon_orders_XXX", but that's more an
>    implementation detail when setting the anon_enabled mode for a
>    specific order.
> 
> > +{
> > +	static unsigned long *orders[] = {
> > +		&huge_anon_orders_always,
> > +		&huge_anon_orders_madvise,
> > +		&huge_anon_orders_inherit,
> > +	};
> 
> Having a "order" and "orders" variable that have different semantics is
> a bit confusing. Don't really have a better suggestion. "enabled_orders"
> ? hm.

Ack. renaming to enabled_orders.

> > +	enum anon_enabled_mode m;
> > +	bool changed = false;
> > +
> > +	spin_lock(&huge_anon_orders_lock);
> > +	for (m = 0; m < ARRAY_SIZE(orders); m++) {
> > +		if (m == mode)
> > +			changed |= !test_and_set_bit(order, orders[m]);
> > +		else
> > +			changed |= test_and_clear_bit(order, orders[m]);
> > +	}
> 
> Can we use the non-atomic variant here? __test_and_set_bit(). Just
> wondering as the lock protects concurrent modifications.

Ack!

I will respin a new version,
--breno