Re: AIX support

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)