From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCHv1] arm: reduce power use by contented spin locks with WFE/SEV Date: Fri, 31 Jul 2015 12:01:33 +0100 Message-ID: <55BB558D.10808@citrix.com> References: <1438339545-22400-1-git-send-email-david.vrabel@citrix.com> <55BB537E.9050407@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZL857-0002M5-Ql for xen-devel@lists.xenproject.org; Fri, 31 Jul 2015 11:02:29 +0000 In-Reply-To: <55BB537E.9050407@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: David Vrabel , Ian Campbell Cc: xen-devel@lists.xenproject.org, Stefano Stabellini , Tim Deegan , Jan Beulich , Andrew Cooper List-Id: xen-devel@lists.xenproject.org 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 >> --- >> 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. > spin_relax() and spin_signal() might be better named arch_lock_relax() > and arch_lock_signal() to match the naming of existing arch_lock_*() hooks. > > I think someone with more arm experience (and the means to test) should > take this patch on. I will give a look next week. Regards, -- Julien Grall