On 14/08/2024 18:22, Srirama Kucherlapati wrote:
Hi Heikki,
I have attached the merged patch with all the changes, the earlier patch was
just only the changes specific to older review comments.
> I'm sorry, I don't understand what you're saying here. Do you
mean that
> we don't need to do anything here, and the code we have in
s_lock.h in
> 'master' now will work fine on AIX? Or do we need to (re-)do some
> changes to support AIX again? If we only support GCC, can we use the
> __sync_lock_test_and_set() builtin instead?
Here we need these changes for ppc. These changes are not for
enabling the AIX support, but this is implementing “Enhanced PowerPC
Architecture”. This routine is more of compare_and_increment, which
is different from GCC __sync_lock_test_and_set(). Also I tried to
write a sample function to check the assemble generated by
__sync_lock_test_and_set(), which turned out to be different set of
assemble code.
I still don't understand. We have Linux powerpc systems running happily
in the buildfarm. They are happy with the current spinlock
implementation. Why is this needed? What happens without it?
How is this different from __sync_lock_test_and_set()? Is the
__sync_lock_test_and_set() on that platform broken, less efficient, or
just different but equally good?
> > +#define TAS(lock) _check_lock((slock_t *)
(lock),
> > 0, 1)
> >
>> +#define S_UNLOCK(lock) _clear_lock((slock_t *) (lock), 0)
>>
> > The above changes are specific to AIX kernel and it operates on
fixed
> > kernel memory. This is more like a compare_and_swap
functionality with
> > sync capability. For all the assemble code I think it would be
better to
> > use the IBM Power specific asm code to gain additional performance.
> You mean we don't need the above? Ok, good.
I mean this part of the code is needed as this is specific to AIX
kernel memory operation which is different from
__sync_lock_test_and_set().
How is it different from __sync_lock_test_and_set()? Why is it needed?
What is AIX kernel memory operation?
I would like to mention that the changes made in
src/include/storage/s_lock.h
are pretty much required and need to be operated in assemble specific to IBM
Power architecture.
Note that your patch both modifies the existing powerpc implementation,
and introduces a new AIX-specific one. They cannot *both* be required,
because only one of them will ever be compiled on a given platform.
Which is it? Or are you trying to make this work on multiple different
CPUs on AIX, so that different implementation gets chosen on different CPUs?
Is the mkldexport stuff still needed on modern AIX? Or was it specific
to XLC and never needed on GCC? How do other products do that?
On a general note: it's your responsibility to explain all the changes
in a way that others will understand and can verify. It is especially
important for something critical and platform-specific like spinlocks,
because others don't have easy access to the hardware to test these
things independently. I also want to remind you that from the Postgres
community point of view, you are introducing support for a new platform,
AIX, not merely resurrecting the old stuff. Every change needs to be
justified anew.
--
Heikki Linnakangas
Neon (https://neon.tech)
