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

Breno Leitao
2026-03-10T16:32:19Z

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