Re: [PATCH 4/8] elf: Let environment aliases override overridable system-wide tunables

On 08/07/26 23:17, DJ Delorie wrote:
> 
> LGTM
> Reviewed-by: DJ Delorie <[email protected]>
> 
> Adhemerval Zanella <[email protected]> writes:
>> +/* Records tunables that were set from GLIBC_TUNABLES during this call, so
> 
> I wonder if it would make sense to have one variable that had the
> "origin priority" of the set value, from TUNABLE_UNSET through
> TUNABLE_LOCKED, so a single comparison tells us if a given source can
> change the value and bump the priority, replacing initialized, locked,
> and the new set_by_env?
> 
> TUNABLE_UNSET
> TUNABLE_GLOBAL
> TUNABLE_LEGACY
> TUNABLE_ENVIRON
> TUNABLE_LOCKED
> 
> (something for after the release, as it's purely internal)

It should be feasible to move the tunable_set_by_env to 'struct _tunable'
itself and replace it with this definition. It would require add a bit of more
logic to set the correct value, but I think we might also remove the 
'struct _tunable::initialized' and use this new field instead. 

> 
>> +   that a legacy environment-variable alias does not override themi (the
>> +   canonical GLIBC_TUNABLES form takes precedence over the aliases).
>> +   A tunable that was set only from the system-wide cache is deliberately not
>> +   recorded here, so an alias may still override an overridable cache default;
>> +   a nonoverridable one remains protected by tunable_t::locked.  */
>> +static bool tunable_set_by_env[tunables_list_size];
> 
> Ok.
> 
>>        if (!tunable_initialize (tunables[i].t, tunables[i].value,
>>  			       tunables[i].len))
>>  	parse_tunable_print_error (&tunables[i]);
>> +      else
>> +	/* GLIBC_TUNABLES set this tunable; a legacy alias must not
>> +	   override it.  */
>> +	tunable_set_by_env[i] = true;
>>      }
>>  }
> 
> Ok.
> 
>> @@ -498,9 +510,13 @@ __tunables_init (char **envp, char **argv)
>>  
>>    for (int i = 0; i < tunable_num_env_alias; i++)
>>      {
>> -      /* Skip over tunables that have either been set or already initialized.  */
>> +      /* Skip aliases whose tunable was already set through GLIBC_TUNABLES,
>> +	 which takes precedence over the alias.  A value coming only from the
>> +	 system-wide cache does not block the alias here: an overridable cache
>> +	 default may still be overridden, while a nonoverridable one is
>> +	 protected by tunable_t::locked.  */
>>        if (tunables_env_alias[i].t == NULL
>> -	  || tunables_env_alias[i].t->initialized)
>> +	  || tunable_set_by_env[tunable_env_alias_list[i]])
>>  	continue;
>>  
> 
> Ok.
> 
>> diff --git a/elf/tst-tunconf1.c b/elf/tst-tunconf1.c
> 
>> +  /* Interaction with legacy environment-variable aliases (MALLOC_*).  */
>> +  int32_t mmap_max = TUNABLE_GET_FULL (glibc, malloc, mmap_max, int32_t, NULL);
>> +  size_t top_pad = TUNABLE_GET_FULL (glibc, malloc, top_pad, size_t, NULL);
>> +  size_t arena_max = TUNABLE_GET_FULL (glibc, malloc, arena_max, size_t, NULL);
>> +
>> +  /* Overridable cache default (100); the MALLOC_MMAP_MAX_ alias overrides
>> +     it, just like GLIBC_TUNABLES would.  */
>> +  printf("mmap_max is %d (should be 200, from MALLOC_MMAP_MAX_ alias)\n",
>> +	 mmap_max);
>> +  TEST_COMPARE (mmap_max, 200);
>> +
>> +  /* Nonoverridable cache default (100); the MALLOC_TOP_PAD_ alias must not
>> +     override it.  */
>> +  printf("top_pad is %ld (should be 100, from /etc nonoverridable)\n",
>> +	 (long)top_pad);
>> +  TEST_COMPARE ((long)top_pad, 100);
>> +
>> +  /* Set both by GLIBC_TUNABLES (300) and by the MALLOC_ARENA_MAX alias
>> +     (400); the canonical GLIBC_TUNABLES form wins.  */
>> +  printf("arena_max is %ld (should be 300, from GLIBC_TUNABLES)\n",
>> +	 (long)arena_max);
>> +  TEST_COMPARE ((long)arena_max, 300);
> 
> Ok.
> 
>> diff --git a/elf/tst-tunconf1.root/etc/tunables.conf b/elf/tst-tunconf1.root/etc/tunables.conf
> 
>> +# Interaction with legacy environment-variable aliases (MALLOC_*), checked
>> +# in the test case:
>> +#  - mmap_max: overridable cache default, must be overridable by the
>> +#    MALLOC_MMAP_MAX_ alias just like by GLIBC_TUNABLES.
>> +#  - top_pad: nonoverridable cache default, must NOT be overridable by the
>> +#    MALLOC_TOP_PAD_ alias.
>> +glibc.malloc.mmap_max=100
>> +-glibc.malloc.top_pad=100
> 
> Ok.
>