Re: [PATCHv1] arm: reduce power use by contented spin locks with WFE/SEV

[David Vrabel <david.vrabel@citrix.com>]

On 31/07/15 12:41, Stefano Stabellini wrote:
> On Fri, 31 Jul 2015, Julien Grall wrote:
>> Hi David,
>>
>> On 31/07/15 11:52, David Vrabel wrote:
>>> On 31/07/15 11:45, David Vrabel wrote:
>>>> Instead of cpu_relax() while spinning and observing the ticket head,
>>>> introduce spin_relax() which executes a WFE instruction.  After the
>>>> ticket head is changed call spin_signal() to execute an SVE
>>>> instruction to wake any spinners.
>>>>
>>>> This should improve power consumption when locks are contented and
>>>> spinning.
>>>>
>>>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>>>> ---
>>>> I've not tested this but it looks straight-forward...
>>>> ---
>>>>  xen/common/spinlock.c          | 5 +++--
>>>>  xen/include/asm-arm/spinlock.h | 3 ++-
>>>>  xen/include/asm-x86/spinlock.h | 3 +++
>>>>  3 files changed, 8 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
>>>> index 29149d1..fc3e8e7 100644
>>>> --- a/xen/common/spinlock.c
>>>> +++ b/xen/common/spinlock.c
>>>> @@ -141,7 +141,7 @@ void _spin_lock(spinlock_t *lock)
>>>>      while ( tickets.tail != observe_head(&lock->tickets) )
>>>>      {
>>>>          LOCK_PROFILE_BLOCK;
>>>> -        cpu_relax();
>>>> +        spin_relax();
>>>>      }
>>>>      LOCK_PROFILE_GOT;
>>>>      preempt_disable();
>>>> @@ -170,6 +170,7 @@ void _spin_unlock(spinlock_t *lock)
>>>>      preempt_enable();
>>>>      LOCK_PROFILE_REL;
>>>>      add_sized(&lock->tickets.head, 1);
>>>> +    spin_signal();
>>>
>>> It occurs to me that perhaps there should be a barrier between the
>>> add_sized() and the spin_signal() so the update value is visible before
>>> we signal (otherwise the spinner may be woken and observe the old value
>>> and WFE again).
>>
>> sev is usually precede by dsb to ensure that all the instructions before
>> as completed before executing the sev.
> 
> Yes, a dsb() is required. This being common code, we could use wmb().

You should put the barrier required for the SEV in spin_signal() so an
additional barrier is not required on other archs.

David