public inbox for [email protected]
help / color / mirror / Atom feedFrom: Heikki Linnakangas <[email protected]>
To: Srirama Kucherlapati <[email protected]>
To: Heikki Linnakangas <[email protected]>
To: Laurenz Albe <[email protected]>
To: Bruce Momjian <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Noah Misch <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Thomas Munro <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: AIX support
Date: Wed, 14 Aug 2024 23:09:06 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <CY8PR15MB5602A5D9064A401347684A06DB872@CY8PR15MB5602.namprd15.prod.outlook.com>
References: <CY5PR11MB639218D04CFAED0FC09ED9DFFDED2@CY5PR11MB6392.namprd11.prod.outlook.com>
<DS0PR15MB5623A624B285AF799705C50ADBF42@DS0PR15MB5623.namprd15.prod.outlook.com>
<[email protected]>
<[email protected]>
<CY8PR15MB5602AA47C13A33DBF68BEA73DBFB2@CY8PR15MB5602.namprd15.prod.outlook.com>
<[email protected]>
<CY8PR15MB56029161FE37A1DE9DE36B70DBC72@CY8PR15MB5602.namprd15.prod.outlook.com>
<CY8PR15MB5602CAFFE00371049D68CD25DBCD2@CY8PR15MB5602.namprd15.prod.outlook.com>
<[email protected]>
<CY8PR15MB560282A99569E110AEED3257DBCF2@CY8PR15MB5602.namprd15.prod.outlook.com>
<CY8PR15MB56029A55A848E9089D29FBE6DBC92@CY8PR15MB5602.namprd15.prod.outlook.com>
<CY8PR15MB5602C6D2B148615EBD7F1AF5DB872@CY8PR15MB5602.namprd15.prod.outlook.com>
<[email protected]>
<CY8PR15MB5602A5D9064A401347684A06DB872@CY8PR15MB5602.namprd15.prod.outlook.com>
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)
view thread (20+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: AIX support
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox