[PATCH] Update tunable min/max values

[Carlos O'Donell]

On 7/2/20 3:08 PM, H.J. Lu wrote:
> On Thu, Jul 02, 2020 at 02:00:54PM -0400, Carlos O'Donell wrote:
>> On 6/6/20 5:51 PM, H.J. Lu wrote:
>>> On Fri, Jun 5, 2020 at 3:45 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>
>>>> On Thu, Jun 04, 2020 at 02:00:35PM -0700, H.J. Lu wrote:
>>>>> On Mon, Jun 1, 2020 at 7:08 PM Carlos O'Donell <carlos@redhat.com> wrote:
>>>>>>
>>>>>> On Mon, Jun 1, 2020 at 6:44 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>> Tunables are designed to pass info from user to glibc, not the other
>>>>>>> way around.  When __libc_main is called, init_cacheinfo is never
>>>>>>> called.  I can call init_cacheinfo from __libc_main.  But there is no
>>>>>>> interface to update min and max values from init_cacheinfo.  I don't
>>>>>>> think --list-tunables will work here without changes to tunables.
>>>>>>
>>>>>> You have a dynamic threshold.
>>>>>>
>>>>>> You have to tell the user what that minimum is, otherwise they can't
>>>>>> use the tunable reliably.
>>>>>>
>>>>>> This is the first instance of a min/max that is dynamically determined.
>>>>>>
>>>>>> You must fetch the cache info ahead of the tunable initialization, that
>>>>>> is you must call init_cacheinfo before __init_tunables.
>>>>>>
>>>>>> You can initialize the tunable data dynamically like this:
>>>>>>
>>>>>> /* Dynamically set the min and max of glibc.foo.bar.  */
>>>>>> tunable_id_t id = TUNABLE_ENUM_NAME (glibc, foo, bar);
>>>>>> tunable_list[id].type.min = lowval;
>>>>>> tunable_list[id].type.max = highval;
>>>>>>
>>>>>> We do something similar for maybe_enable_malloc_check.
>>>>>>
>>>>>> Then once the tunables are parsed, and the cpu features are loaded
>>>>>> you can print the tunables, and the printed tunables will have meaningful
>>>>>> min and max values.
>>>>>>
>>>>>> If you have circular dependency, then you must process the cpu features
>>>>>> first without reading from the tunables, then allow the tunables to be
>>>>>> initialized from the system, *then* process the tunables to alter the existing
>>>>>> cpu feature settings.
>>>>>>
>>>>>
>>>>> How about this?  I got
>>>>>
>>>>
>>>> Here is the updated patch, which depends on
>>>>
>>>> https://sourceware.org/pipermail/libc-alpha/2020-June/114820.html
>>>>
>>>> to add "%d" support to _dl_debug_vdprintf.  I got
>>>>
>>>> $ ./elf/ld.so ./libc.so --list-tunables
>>>> glibc.elision.skip_lock_after_retries: 3 (min: -2147483648, max: 2147483647)
>>>> glibc.malloc.trim_threshold: 0x0 (min: 0x0, max: 0xffffffff)
>>>> glibc.malloc.perturb: 0 (min: 0, max: 255)
>>>> glibc.cpu.x86_shared_cache_size: 0x100000 (min: 0x0, max: 0xffffffff)
>>>> glibc.elision.tries: 3 (min: -2147483648, max: 2147483647)
>>>> glibc.elision.enable: 0 (min: 0, max: 1)
>>>> glibc.malloc.mxfast: 0x0 (min: 0x0, max: 0xffffffff)
>>>> glibc.elision.skip_lock_busy: 3 (min: -2147483648, max: 2147483647)
>>>> glibc.malloc.top_pad: 0x0 (min: 0x0, max: 0xffffffff)
>>>> glibc.cpu.x86_non_temporal_threshold: 0x600000 (min: 0x0, max: 0xffffffff)
>>>> glibc.cpu.x86_shstk:
>>>> glibc.cpu.hwcap_mask: 0x6 (min: 0x0, max: 0xffffffff)
>>>> glibc.malloc.mmap_max: 0 (min: -2147483648, max: 2147483647)
>>>> glibc.elision.skip_trylock_internal_abort: 3 (min: -2147483648, max: 2147483647)
>>>> glibc.malloc.tcache_unsorted_limit: 0x0 (min: 0x0, max: 0xffffffff)
>>>> glibc.cpu.x86_ibt:
>>>> glibc.cpu.hwcaps:
>>>> glibc.elision.skip_lock_internal_abort: 3 (min: -2147483648, max: 2147483647)
>>>> glibc.malloc.arena_max: 0x0 (min: 0x1, max: 0xffffffff)
>>>> glibc.malloc.mmap_threshold: 0x0 (min: 0x0, max: 0xffffffff)
>>>> glibc.cpu.x86_data_cache_size: 0x8000 (min: 0x0, max: 0xffffffff)
>>>> glibc.malloc.tcache_count: 0x0 (min: 0x0, max: 0xffffffff)
>>>> glibc.malloc.arena_test: 0x0 (min: 0x1, max: 0xffffffff)
>>>> glibc.pthread.mutex_spin_count: 100 (min: 0, max: 32767)
>>>> glibc.malloc.tcache_max: 0x0 (min: 0x0, max: 0xffffffff)
>>>> glibc.malloc.check: 0 (min: 0, max: 3)
>>>> $
>>>>
>>>> Ok for master?
>>>>
>>>
>>> Here is the updated patch.  To support --list-tunables, a target should add
>>>
>>> CPPFLAGS-version.c = -DLIBC_MAIN=__libc_main_body
>>> CPPFLAGS-libc-main.S = -DLIBC_MAIN=__libc_main_body
>>>
>>> and start.S should be updated to define __libc_main and call
>>> __libc_main_body:
>>>
>>> extern void __libc_main_body (int argc, char **argv)
>>>   __attribute__ ((noreturn, visibility ("hidden")));
>>>
>>> when LIBC_MAIN is defined.
>>
>> I like where this patch is going, but the __libc_main wiring up means
>> we'll have to delay this until glibc 2.33 opens for development and
>> give the architectures time to fill in the required pieces of assembly.
>>
>> Can we split this into:
>>
>> (a) Minimum required to implement the feature e.g. just the tunable without
>>     my requested changes.
>>
>> (b) A second patch which implements the --list-tunables that users can
>>     then use to know what the values they can choose are.
>>
>> That way we can commit (a) right now, and then commit (b) when we
>> reopen for development?
>>
> 
> Like this?

Almost.

Why do we still use a constructor?

Why don't we accurately set the min and max?

+#if HAVE_TUNABLES
+  TUNABLE_UPDATE (x86_non_temporal_threshold, long int,
+		  __x86_shared_non_temporal_threshold, 0,
+		  (long int) -1);
+  TUNABLE_UPDATE (x86_rep_movsb_threshold, long int,
+		  __x86_rep_movsb_threshold,
+		  minimum_rep_movsb_threshold, (long int) -1);
+  TUNABLE_UPDATE (x86_rep_stosb_threshold, long int,
+		  __x86_rep_stosb_threshold, 0, (long int) -1);

A min and max of 0 and -1 respectively could have been set in the tunables
list file and are not dynamic?

I'd expect your patch would do everything except actually implement
--list-tunables.

We need a manual page, and I accept that showing a "lower value" will
have to wait for --list-tunables.

Otherwise the patch is looking ready.

-- 
Cheers,
Carlos.