[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH V2] x86 spinlock: Fix memory corruption on completing completions
- To: Oleg Nesterov <oleg@xxxxxxxxxx>
- From: Raghavendra K T <raghavendra.kt@xxxxxxxxxxxxxxxxxx>
- Date: Mon, 09 Feb 2015 22:33:50 +0530
- Cc: jeremy@xxxxxxxx, kvm@xxxxxxxxxxxxxxx, peterz@xxxxxxxxxxxxx, jasowang@xxxxxxxxxx, virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx, paul.gortmaker@xxxxxxxxxxxxx, hpa@xxxxxxxxx, ak@xxxxxxxxxxxxxxx, a.ryabinin@xxxxxxxxxxx, x86@xxxxxxxxxx, borntraeger@xxxxxxxxxx, mingo@xxxxxxxxxx, xen-devel@xxxxxxxxxxxxxxxxxxxx, paulmck@xxxxxxxxxxxxxxxxxx, Davidlohr Bueso <dave@xxxxxxxxxxxx>, sasha.levin@xxxxxxxxxx, davej@xxxxxxxxxx, tglx@xxxxxxxxxxxxx, waiman.long@xxxxxx, linux-kernel@xxxxxxxxxxxxxxx, pbonzini@xxxxxxxxxx, akpm@xxxxxxxxxxxxxxxxxxxx, torvalds@xxxxxxxxxxxxxxxxxxxx
- Delivery-date: Mon, 09 Feb 2015 17:02:17 +0000
- List-id: Xen developer discussion <xen-devel.lists.xen.org>
Ccing Davidlohr, (sorry that I got confused with similar address in cc
list).
On 02/09/2015 08:44 PM, Oleg Nesterov wrote:
On 02/09, Raghavendra K T wrote:
+static inline void __ticket_check_and_clear_slowpath(arch_spinlock_t *lock)
+{
+ arch_spinlock_t old, new;
+ __ticket_t diff;
+
+ old.tickets = READ_ONCE(lock->tickets);
+ diff = (old.tickets.tail & ~TICKET_SLOWPATH_FLAG) - old.tickets.head;
+
+ /* try to clear slowpath flag when there are no contenders */
+ if ((old.tickets.tail & TICKET_SLOWPATH_FLAG) &&
+ (diff == TICKET_LOCK_INC)) {
+ new = old;
+ new.tickets.tail &= ~TICKET_SLOWPATH_FLAG;
+ cmpxchg(&lock->head_tail, old.head_tail, new.head_tail);
+ }
+}
Can't we simplify it? We own .head, and we already know it. We only need
to clear TICKET_SLOWPATH_FLAG in .tail atomically?
IOW,
static inline void __ticket_check_and_clear_slowpath(arch_spinlock_t
*lock, __ticket_t head)
{
__ticket_t old_tail, new_tail;
new_tail = head + TICKET_LOCK_INC;
old_tail = new_tail | TICKET_SLOWPATH_FLAG;
if (READ_ONCE(lock->tickets.tail) == old_tail)
cmpxchg(&lock->tickets.tail, old_tail, new_tail);
}
Plus
- __ticket_check_and_clear_slowpath(lock);
+ __ticket_check_and_clear_slowpath(lock, inc.tail);
Or I missed something?
Thanks.. Perfect, 'll update with this change. (Jeremy had hinted
similar).
And I think it would be better to avoid ifdef(CONFIG_PARAVIRT_SPINLOCKS),
ww can just do
if (TICKET_SLOWPATH_FLAG)
__ticket_check_and_clear_slowpath();
Okay.
While at it, I think current arch_spin_unlock() has similar structure
and wanted to clean it up. considering we define
TICKET_SLOWPATH_FLAG 0 or 1, I think compiler would be smart enough
to generate appropriate code and we could avoid #ifdef.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
