Re: [RFC PATCH 3/3] RISC-V: Implement CPU yielding for busy loops with Zihintpause/Zawrs

[Vineet Gupta <vineetg@rivosinc.com>]

Hi Christoph,

My 2 cents.

On 4/18/24 10:17, Palmer Dabbelt wrote:
> On Thu, 18 Apr 2024 02:46:35 PDT (-0700), christoph.muellner@vrull.eu wrote:
>> The macro atomic_spin_nop can be used to implement arch-specific
>> CPU yielding that is used in busy loops (e.g. in pthread_spin_lock).
>> This patch introduces an ifunc-based implementation for RISC-V,
>> that uses Zihintpause's PAUSE instruction for that matter (as PAUSE
>> is a HINT instruction there is not dependency to Zihintpause at
>> runtime).  Further, we test for Zawrs via hwprobe() and if found
>> we use WRS.STO instead of PAUSE.

[snip]

>> +ENTRY (__cpu_relax_generic)
>> +	/* While we can use the `pause` instruction without
>> +	   the need of Zihintpause (because it is a HINT instruction),
>> +	   we still have to enable Zihintpause for the assembler.  */
>> +	pause
>> +	ret
>> +END (__cpu_relax_generic)

[snip]

>> +.option push
>> +.option arch, +zawrs
>> +ENTRY (__cpu_relax_zawrs)
>> +	wrs.sto
> This has the same forward progress/eventual success violation as the 
> code you sent for GCC and Linux does.  It doesn't really matter if the 
> user of the reservation is in a builtin, an asm block, or a function.  
> The compiler just doesn't know about those reservation rules and isn't 
> going to generate code that follows them.

So, this routine maps to atomic_spin_nop () called in generic code in a
bunch of places.

The easiest case is nptl/pthread_spin_lock.c which looks something like this

__pthread_spin_lock (pthread_spinlock_t *lock)
...
   do
    {
      atomic_spin_nop ();
      val = atomic_load_relaxed (lock);
    }
  while (val != 0);

This works fine for a PAUSE based implementation which doesn't need a
prior reservation, but WRS does and even if both the load and spin are
inlined, reservation happens after WRS which is wrong.

So I think before we can implement this optimization for riscv, we need
a generic glibc change to replace of atomic_spin_nop () with a variant
which also takes the lock/memory under consideration.
The fallback implementation of atomic_load_and_spin_if_cond() will be
what the above loop does. For arches that do implement the API, based on
ISA semantics, they can choose to ignore the mem arg completely (RISC-V
PAUSE based implementation) or implement a little loop which explicitly
takes reservation on the mem/lock and calls WRS.

Does that make sense ?

We just need to sort out the 2nd use of atomic_spin_nop in
nptl/pthread_mutex_lock.c where this conversion might not be straight fwd.
The recent back off back spin looping is getting in the way of obvious
solution, however for discussion sake if we ignore it, the code looks
like this:

PTHREAD_MUTEX_LOCK (pthread_mutex_t *mutex)
...
       int cnt=0, max_cnt = something;
       do
        {
            if (cnt ++>= max_cnt)
            {
                 LLL_MUTEX_LOCK (mutex);
                 break;
            }
            atomic_spin_nop ();
       }
       while (LLL_MUTEX_READ_LOCK (mutex) != 0 || LLL_MUTEX_TRYLOCK
(mutex) != 0);

And this seems like it can be converted to a
atomic_load_and_spin_if_cond(mutex, !=0 ) kind of API.

Let me know if you want me to spin this - pun intended ;-)

-Vineet


> That said, as far as I can tell the same grey area in the WRS spec that 
> I pointed out during one of the Linux reviews still exists.   So if 
> there's some hardware that has a more generous set of LR->WRS rules than 
> the ISA-required LR->SC rules that's fine, we just need to know what the 
> hardware actually is so we can write down the rules and then stick to 
> them -- it's just a performance thing for WRS, so having weaker rules 
> seems entirely plausible.
>
> I don't know of any hardware with WRS, but sorry if I missed one.  Do 
> you have something publicly availiable that behaves this way?