Re: [lttng-dev] [PATCH 1/2] lttng_ust_init_thread: initialise cached context values

[Mathieu Desnoyers via lttng-dev <lttng-dev@lists.lttng.org>]

----- On Jul 20, 2022, at 4:00 PM, Norbert Lange nolange79@gmail.com wrote:

> Am Di., 19. Juli 2022 um 21:39 Uhr schrieb Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com>:
>>
>> ----- On Jul 18, 2022, at 5:59 PM, Norbert Lange via lttng-dev
>> lttng-dev@lists.lttng.org wrote:
>>
>> > Modify all relevant *_alloc_tls functions so that they take an
>> > argument for 'init'. Setting this to 'true' will read
>> > the actual context value and store it into a thread local
>> > cache.
>> >
>> > The function 'lttng_ust_init_thread' will use this to
>> > precalculate context values. Tracepoints can then avoid
>> > systemcalls.
>>
>> Rather than integrating two unrelated things within "alloc_tls"
>> functions, I would prefer that we split things like this, e.g.:
>>
>> keep lttng_cgroup_ns_alloc_tls() as is.
>>
>> Introduce lttng_ust_cgroup_init_thread() which would
>> call (void)get_cgroup_ns().
>>
>> Likewise for all other contexts.
>>
>> Thoughts ?
> 
> is this just a matter of the function name? The "things" are
> related as they both prepare the same thread local cache.
> 
> ie.
> rename lttng_cgroup_ns_alloc_tls(bool init) to
> lttng_ust_cgroup_init_thread(bool initcache)?
> 
> Don't care much either way,I  just usually try to limit symbols.

So AFAIU when only alloc_tls is needed, you would just change that
for lttng_ust_cgroup_init_thread(false), correct ?

I would then prefer that we introduce flags rather than rely on a
boolean to make it immediately clear what is done from the callsite,
e.g.:

enum lttng_ust_init_thread_flags {
  LTTNG_UST_INIT_THREAD_TLS = (1 << 0),
  LTTNG_UST_INIT_THREAD_CONTEXT_CACHE = (1 << 1),
}

lttng_ust_cgroup_init_thread(int init_thread_flags)

And then call either:

lttng_ust_cgroup_init_thread(LTTNG_UST_INIT_THREAD_TLS)
to initialize the tls.

lttng_ust_cgroup_init_thread(LTTNG_UST_INIT_THREAD_CONTEXT_CACHE)
to initialize the thread context cache.

or
lttng_ust_cgroup_init_thread(LTTNG_UST_INIT_THREAD_TLS | LTTNG_UST_INIT_THREAD_CONTEXT_CACHE)
to initialize both.

Thoughts ?

Thanks,

Mathieu

> 
> Regards,
> Norbert

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev