This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 1/2] Single thread optimization for malloc atomics
- From: Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>
- To: Torvald Riegel <triegel at redhat dot com>
- Cc: "GNU C. Library" <libc-alpha at sourceware dot org>
- Date: Fri, 02 May 2014 14:53:01 -0300
- Subject: Re: [PATCH 1/2] Single thread optimization for malloc atomics
- Authentication-results: sourceware.org; auth=none
- References: <53610133 dot 3070908 at linux dot vnet dot ibm dot com> <1399041298 dot 32485 dot 6373 dot camel at triegel dot csb>
On 02-05-2014 11:34, Torvald Riegel wrote:
> On Wed, 2014-04-30 at 10:57 -0300, Adhemerval Zanella wrote:
>> This patch adds a single-thread optimization for malloc atomic usage to
>> first check if process is single-thread (ST) and if so use normal
>> load/store instead of atomic instructions.
>>
>> It is a respin of my initial attempt to add ST optimization on PowerPC.
>> Now malloc optimization is arch-agnostic and rely in already defined macros
>> in GLIBC (SINGLE_THREAD_P).
> I'm not convinced that this is a good change. First, I'd like to see
> some performance numbers that show whether the change leads to a
> non-negligible increase in performance. That should be a
> microbenchmark, so we can track it over time.
In fact I would prefer to make it more arch-specific and let the the arch-maintainers
evaluate if such change would be preferable. One way to do it is adding a
new macro in the bits/libc-locks, some time __libc_atomic_*; and define it as
catomic_* in ./nptl/sysdeps/pthread/bits/libc-lock.h. Then arch can be replace
its implementation if it suit them.
>
> malloc is currently marked as Async-Signal-Unsafe, but due to locks
> elsewhere in the implementation. The MTSafety docs even talk about what
> would be needed to make it AS-Safe. Your patch would prevent making it
> AS-Safe. The catomic_add that you replace are AS-Safe, the sequential
> code you add is not. Someone added those catomic_* and not sequential
> code, so this is at least indication that there's a disconnect here.
Another point to make this change arch specific. And if we even make malloc
functions AS-Safe in the future, the arch maintainer just need to reevaluate
this change (since it will either make the optimization not required or make
it unsafe).
>
> You also add macros with nondescriptive names, that each have exactly
> one use. There's no documentation for how they differ, and you don't
> update the comments on the MT-Safe docs.
>
>> x86 probably would kike an atomic.h refactor
>> to avoid the double check for '__local_multiple_threads', but that is not
>> the aim of this patch.
> But your patch does introduce this, so you should take care of this as
> well. I guess you wouldn't be happy if someone change x86 without
> caring about Power?
>
I will drop this patch and rework on a arch-specific one.