Message-ID: <40da8288-05fd-4028-adab-81db2092ba8c@iki.fi>
Date: Wed, 14 Aug 2024 23:09:06 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: AIX support
To: Srirama Kucherlapati <sriram.rk@in.ibm.com>,
 Heikki Linnakangas <hlinnaka@gmail.com>,
 Laurenz Albe <laurenz.albe@cybertec.at>, Bruce Momjian <bruce@momjian.us>
Cc: Peter Eisentraut <peter@eisentraut.org>,
 Alvaro Herrera <alvherre@alvh.no-ip.org>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>,
 Noah Misch <noah@leadboat.com>, Michael Paquier <michael@paquier.xyz>,
 Andres Freund <andres@anarazel.de>, Tom Lane <tgl@sss.pgh.pa.us>,
 Thomas Munro <thomas.munro@gmail.com>, "tvk1271@gmail.com"
 <tvk1271@gmail.com>,
 "postgres-ibm-aix@wwpdl.vnet.ibm.com" <postgres-ibm-aix@wwpdl.vnet.ibm.com>
References: <CY5PR11MB639218D04CFAED0FC09ED9DFFDED2@CY5PR11MB6392.namprd11.prod.outlook.com>
 <DS0PR15MB5623A624B285AF799705C50ADBF42@DS0PR15MB5623.namprd15.prod.outlook.com>
 <a7afc612-67c1-49a4-8767-546f6ab368c4@iki.fi> <ZlKz4bNLaNl8PBmM@momjian.us>
 <CY8PR15MB5602AA47C13A33DBF68BEA73DBFB2@CY8PR15MB5602.namprd15.prod.outlook.com>
 <698dd3efec644b5c0dd6220d73033f8153ae2fde.camel@cybertec.at>
 <CY8PR15MB56029161FE37A1DE9DE36B70DBC72@CY8PR15MB5602.namprd15.prod.outlook.com>
 <CY8PR15MB5602CAFFE00371049D68CD25DBCD2@CY8PR15MB5602.namprd15.prod.outlook.com>
 <d2c57c97-cb4d-4fd7-83d0-5f92b2aa03ca@gmail.com>
 <CY8PR15MB560282A99569E110AEED3257DBCF2@CY8PR15MB5602.namprd15.prod.outlook.com>
 <CY8PR15MB56029A55A848E9089D29FBE6DBC92@CY8PR15MB5602.namprd15.prod.outlook.com>
 <CY8PR15MB5602C6D2B148615EBD7F1AF5DB872@CY8PR15MB5602.namprd15.prod.outlook.com>
 <95a44be0-b2f8-464a-8984-771d892b1cac@iki.fi>
 <CY8PR15MB5602A5D9064A401347684A06DB872@CY8PR15MB5602.namprd15.prod.outlook.com>
Content-Language: en-US
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <CY8PR15MB5602A5D9064A401347684A06DB872@CY8PR15MB5602.namprd15.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/40da8288-05fd-4028-adab-81db2092ba8c%40iki.fi>
Precedence: bulk

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)