Message-ID: <95a44be0-b2f8-464a-8984-771d892b1cac@iki.fi>
Date: Wed, 14 Aug 2024 10:46:38 +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>
 <CY5PR11MB6392F178646E31D9BF417118FDEB2@CY5PR11MB6392.namprd11.prod.outlook.com>
 <dc89bc84-c9a9-4792-8857-7eafc0234cfb@eisentraut.org>
 <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>
Content-Language: en-US
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <CY8PR15MB5602C6D2B148615EBD7F1AF5DB872@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/95a44be0-b2f8-464a-8984-771d892b1cac%40iki.fi>
Precedence: bulk

On 14/08/2024 06:31, Srirama Kucherlapati wrote:
> Hi Heikki & Team,
> 
> I tried to look at the assembly code changes with our team, in the below 
> file.
> 
> diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
> index 29ac6cdcd9..69582f4ae7 100644
> --- a/src/include/storage/s_lock.h
> +++ b/src/include/storage/s_lock.h
> static __inline__ int
> tas(volatile slock_t *lock)
> @@ -424,17 +430,15 @@ tas(volatile slock_t *lock)
> __asm__ __volatile__(
> "        lwarx   %0,0,%3,1        \n"
> "        cmpwi   %0,0                \n"
> "        bne     $+16                \n"                /* branch to li 
> %1,1 */
> "        addi    %0,%0,1                \n"
> "        stwcx.  %0,0,%3                \n"
> "        beq     $+12                \n"                /* branch to 
> lwsync */
> "        li      %1,1                \n"
> "        b       $+12                \n"                /* branch to end 
> of asm sequence */
> "        lwsync                                \n"
> "        li      %1,0                \n"
> :        "=&b"(_t), "=r"(_res), "+m"(*lock)
> :        "r"(lock)
> :        "memory", "cc");
> 
> For the changes in the above file,  this code is very specific to power 
> architecture we need to use the IBM Power specific asm code only, rather 
> than using the GNU assembler. Also, all these asm specific code is under 
> the macro __ppc__, which should not impact any other platforms. I see 
> there is a GCC specific implementation (under this macro #if 
> defined(HAVE_GCC__SYNC_INT32_TAS)) in the same file as well.

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?

If any changes are required, please include them in the patch. That'll 
make it clear what exactly you're proposing.

> +#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 was trying to understand here wrt to both the assemble changes if you 
> are looking for anything specific to the architecture.

I don't know. You tell me what makes most sense on AIX / powerpc.

> Attached is the patch for the previous comments, kindly please let me 
> know your comments.

Is this all that's needed to resurrect AIX support?

-- 
Heikki Linnakangas
Neon (https://neon.tech)