Re: [PATCH v2] parisc: Rewrite light-weight syscall and futex code

[John David Anglin <dave.anglin@bell.net>]

On 2021-12-23 1:47 p.m., Helge Deller wrote:
> ...
>> In order to do this, we need a mechanism to trigger COW breaks outside the
>> critical region. Fortunately, parisc has the "stbys,e" instruction. When
>> the leftmost byte of a word is addressed, this instruction triggers all
>> the exceptions of a normal store but it does not write to memory. Thus,
>> we can use it to trigger COW breaks outside the critical region without
>> modifying the data that is to be updated atomically.
> ...
>> diff --git a/arch/parisc/include/asm/futex.h b/arch/parisc/include/asm/futex.h
>> index 9cd4dd6e63ad..8f97db995b04 100644
>> --- a/arch/parisc/include/asm/futex.h
>> +++ b/arch/parisc/include/asm/futex.h
> ...
>> +static inline bool _futex_force_interruptions(unsigned long ua)
>> +{
>> +	bool result;
>> +
>> +	__asm__ __volatile__(
>> +		"1:\tldw 0(%1), %0\n"
>> +		"2:\tstbys,e %%r0, 0(%1)\n"
>> +		"\tcomclr,= %%r0, %%r0, %0\n"
>> +		"3:\tldi 1, %0\n"
>> +		ASM_EXCEPTIONTABLE_ENTRY(1b, 3b)
>> +		ASM_EXCEPTIONTABLE_ENTRY(2b, 3b)
>> +		: "=&r" (result) : "r" (ua) : "memory"
>> +	);
>> +	return result;
> I wonder if we can get rid of the comclr,= instruction by using
> ASM_EXCEPTIONTABLE_ENTRY_EFAULT instead of ASM_EXCEPTIONTABLE_ENTRY,
> e.g.:
>
> diff --git a/arch/parisc/include/asm/futex.h b/arch/parisc/include/asm/futex.h
> index 8f97db995b04..ea052f013865 100644
> --- a/arch/parisc/include/asm/futex.h
> +++ b/arch/parisc/include/asm/futex.h
> @@ -21,20 +21,21 @@ static inline unsigned long _futex_hash_index(unsigned long ua)
>    * if load and store fault.
>    */
>
> -static inline bool _futex_force_interruptions(unsigned long ua)
> +static inline unsigned long _futex_force_interruptions(unsigned long ua)
>   {
> -	bool result;
> +	register unsigned long error __asm__ ("r8") = 0;
> +	register unsigned long temp;
>
>   	__asm__ __volatile__(
> -		"1:\tldw 0(%1), %0\n"
> -		"2:\tstbys,e %%r0, 0(%1)\n"
> -		"\tcomclr,= %%r0, %%r0, %0\n"
> -		"3:\tldi 1, %0\n"
> -		ASM_EXCEPTIONTABLE_ENTRY(1b, 3b)
> -		ASM_EXCEPTIONTABLE_ENTRY(2b, 3b)
> -		: "=&r" (result) : "r" (ua) : "memory"
> +		"1:\tldw 0(%2), %0\n"
> +		"2:\tstbys,e %%r0, 0(%2)\n"
> +		"3:\n"
> +		ASM_EXCEPTIONTABLE_ENTRY_EFAULT(1b, 3b)
> +		ASM_EXCEPTIONTABLE_ENTRY_EFAULT(2b, 3b)
> +		: "=r" (temp), "=r" (error)
> +		: "r" (ua), "1" (error) : "memory"
>   	);
> -	return result;
> +	return error;
>   }
I don't think this is a win.

1) Register %r8 needs to get loaded with 0. So, that's one instruction.
2) Register %r8 is a caller saves register. Using it will cause gcc to save and restore it from stack. This may
cause a stack frame to be created when one isn't needed. The save and restore instructions are more
expensive, particularly if they cause a TLB miss.

Note that the comclr both clears result and nullifies the following ldi instruction, so it is not executed in the fast path.

Dave

-- 
John David Anglin  dave.anglin@bell.net