Re: [Xen-devel] [PATCH 2/2] xen/spinlocks: fix placement of preempt_[dis|en]able()
Hi,
On 13/03/2020 09:00, Jürgen Groß wrote:
On 13.03.20 09:55, Jan Beulich wrote:
On 13.03.2020 09:05, Juergen Gross wrote:
@@ -199,10 +199,10 @@ unsigned long _spin_lock_irqsave(spinlock_t *lock)
void _spin_unlock(spinlock_t *lock)
{
arch_lock_release_barrier();
- preempt_enable();
LOCK_PROFILE_REL;
rel_lock(>debug);
add_sized(>tickets.head, 1);
+ preempt_enable();
arch_lock_signal();
}
arch_lock_signal() is a barrier on Arm, hence just like for patch 1
I wonder whether the insertion wouldn't better come after it.
The important barrier in spin_unlock() is arch_lock_release_barrier().
The one in arch_lock_signal() is just to ensure that waking up the other
CPUs will not happen before the unlock is seen. The barrier would not
have been necessary if the we didn't use 'sev'.
Either way is fine for me. It should be noted that preemption is only
relevant on the local cpu. So this is about trading lock state
visibility against preemption disabled time, and I agree the visible
time of the lock held should be minimized at higher priority than the
preemption disabled time.
I don't think the rationale is about "performance" here. The rationale
is you don't know the implementation of arch_lock_signal(). If you get
preempted by a thread trying to acquire the same lock, then it may not
do the right thing.
Linux will also re-enable preemption only after the unlock has been
completed. So it would be best to follow the same pattern.
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/2] xen/spinlocks: fix placement of preempt_[dis|en]able()
On 13.03.20 09:55, Jan Beulich wrote:
On 13.03.2020 09:05, Juergen Gross wrote:
@@ -199,10 +199,10 @@ unsigned long _spin_lock_irqsave(spinlock_t *lock)
void _spin_unlock(spinlock_t *lock)
{
arch_lock_release_barrier();
-preempt_enable();
LOCK_PROFILE_REL;
rel_lock(>debug);
add_sized(>tickets.head, 1);
+preempt_enable();
arch_lock_signal();
}
arch_lock_signal() is a barrier on Arm, hence just like for patch 1
I wonder whether the insertion wouldn't better come after it.
Either way is fine for me. It should be noted that preemption is only
relevant on the local cpu. So this is about trading lock state
visibility against preemption disabled time, and I agree the visible
time of the lock held should be minimized at higher priority than the
preemption disabled time.
I'll modify my patches accordingly, adding a note in this regard to
the commit message.
Juergen
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/2] xen/spinlocks: fix placement of preempt_[dis|en]able()
On 13.03.2020 09:05, Juergen Gross wrote:
> @@ -199,10 +199,10 @@ unsigned long _spin_lock_irqsave(spinlock_t *lock)
> void _spin_unlock(spinlock_t *lock)
> {
> arch_lock_release_barrier();
> -preempt_enable();
> LOCK_PROFILE_REL;
> rel_lock(>debug);
> add_sized(>tickets.head, 1);
> +preempt_enable();
> arch_lock_signal();
> }
arch_lock_signal() is a barrier on Arm, hence just like for patch 1
I wonder whether the insertion wouldn't better come after it.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 2/2] xen/spinlocks: fix placement of preempt_[dis|en]able()
In case Xen ever gains preemption support the spinlock coding's
placement of preempt_disable() and preempt_enable() should be outside
of the locked section.
Signed-off-by: Juergen Gross
---
xen/common/spinlock.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 344981c54a..f05fb068cd 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -160,6 +160,7 @@ void inline _spin_lock_cb(spinlock_t *lock, void (*cb)(void
*), void *data)
LOCK_PROFILE_VAR;
check_lock(>debug);
+preempt_disable();
tickets.head_tail = arch_fetch_and_add(>tickets.head_tail,
tickets.head_tail);
while ( tickets.tail != observe_head(>tickets) )
@@ -171,7 +172,6 @@ void inline _spin_lock_cb(spinlock_t *lock, void (*cb)(void
*), void *data)
}
got_lock(>debug);
LOCK_PROFILE_GOT;
-preempt_disable();
arch_lock_acquire_barrier();
}
@@ -199,10 +199,10 @@ unsigned long _spin_lock_irqsave(spinlock_t *lock)
void _spin_unlock(spinlock_t *lock)
{
arch_lock_release_barrier();
-preempt_enable();
LOCK_PROFILE_REL;
rel_lock(>debug);
add_sized(>tickets.head, 1);
+preempt_enable();
arch_lock_signal();
}
@@ -242,15 +242,18 @@ int _spin_trylock(spinlock_t *lock)
return 0;
new = old;
new.tail++;
+preempt_disable();
if ( cmpxchg(>tickets.head_tail,
old.head_tail, new.head_tail) != old.head_tail )
+{
+preempt_enable();
return 0;
+}
got_lock(>debug);
#ifdef CONFIG_DEBUG_LOCK_PROFILE
if (lock->profile)
lock->profile->time_locked = NOW();
#endif
-preempt_disable();
/*
* cmpxchg() is a full barrier so no need for an
* arch_lock_acquire_barrier().
--
2.16.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
