Re: LWLock optimization for multicore Power machines

From: Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Bernd Helmle <mailings(at)oopsware(dot)de>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: LWLock optimization for multicore Power machines
Date: 2017-03-25 20:11:06
Message-ID: CAPpHfdt5KUU24dzfFXdL-b-zFXuf24uh+FVO599RcaO-35i7uA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On Sat, Mar 25, 2017 at 8:44 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> writes:
> > [ lwlock-power-3.patch ]
>
> I experimented with this patch a bit. I can't offer help on testing it
> on large PPC machines, but I can report that it breaks the build on
> Apple PPC machines, apparently because of nonstandard assembly syntax.
> I get "Parameter syntax error" on these four lines of assembly:
>
> and 3,r0,r4
> cmpwi 3,0
> add 3,r0,r5
> stwcx. 3,0,r2
>
> I am not sure what the "3"s are meant to be --- if that's a hard-coded
> register number, then it's unacceptable code to begin with, IMO.
> You should be able to get gcc to give you a scratch register of its
> choosing via appropriate use of the register assignment part of the
> asm syntax. I think there are examples in s_lock.h.
>

Right. Now I use variable bind to register instead of hard-coded register
for that purpose.

I'm also unhappy that this patch changes the generic implementation of
> LWLockAttemptLock. That means that we'd need to do benchmarking on *every*
> machine type to see what we're paying elsewhere for the benefit of PPC.
> It seems unlikely that putting an extra level of function call into that
> hot-spot is zero cost.
>
> Lastly, putting machine-specific code into atomics.c is a really bad idea.
> We have a convention for where to put such code, and that is not it.
>
> You could address both the second and third concerns, I think, by putting
> the PPC asm function into port/atomics/arch-ppc.h (which is where it
> belongs anyway) and making the generic implementation be a "static inline"
> function in port/atomics/fallback.h. In that way, at least with compilers
> that do inlines sanely, we could expect that this patch doesn't change the
> generated code for LWLockAttemptLock at all on machines other than PPC.
>

I moved PPC implementation of pg_atomic_fetch_mask_add_u32() into
port/atomics/arch-ppc.h. I also had to declare pg_atomic_uint32 there to
satisfy usage of this type as argument
of pg_atomic_fetch_mask_add_u32_impl().

I moved generic implementation of pg_atomic_fetch_mask_add_u32() into
port/atomics/generic.h. That seems to be more appropriate place for that
than fallback.h. Because fallback.h has definitions for cases when we
don't have atomic for this platform at all. generic.h has definitions of
lacking atomics using defined atomics.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attachment Content-Type Size
lwlock-power-4.patch application/octet-stream 7.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2017-03-25 20:15:39 Re: standardized backwards incompatibility tag for commits
Previous Message Andres Freund 2017-03-25 19:32:23 standardized backwards incompatibility tag for commits