From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752432AbbBEWha (ORCPT ); Thu, 5 Feb 2015 17:37:30 -0500 Received: from smtp2.provo.novell.com ([137.65.250.81]:37568 "EHLO smtp2.provo.novell.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751270AbbBEWh2 (ORCPT ); Thu, 5 Feb 2015 17:37:28 -0500 Message-ID: <1423175834.6835.27.camel@stgolabs.net> Subject: Re: sched: memory corruption on completing completions From: Davidlohr Bueso To: Linus Torvalds Cc: Sasha Levin , Waiman Long , Peter Zijlstra , Ingo Molnar , Andrew Morton , Andrey Ryabinin , Dave Jones , LKML , Raghavendra K T Date: Thu, 05 Feb 2015 14:37:14 -0800 In-Reply-To: References: <54D2AA16.6030706@oracle.com> <1423169986.6835.24.camel@stgolabs.net> <54D3DA75.70402@oracle.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.12.7 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2015-02-05 at 13:34 -0800, Linus Torvalds wrote: > On Thu, Feb 5, 2015 at 1:02 PM, Sasha Levin wrote: > > > > Interestingly enough, according to that article this behaviour seems to be > > "by design": > > Oh, it's definitely by design, it's just that the design looked at > spinlocks without the admittedly very subtle issue of lifetime vs > unlocking. > > Spinlocks (and completions) are special - for other locks we have > basically allowed lifetimes to be separate from the lock state, and if > you have a data structure with a mutex in it, you'll have to have some > separate lifetime rule outside of the lock itself. But spinlocks and > completions have their locking state tied into their lifetime. For spinlocks I find this very much a virtue. Tight lifetimes allow the overall locking logic to be *simple* - keeping people from being "smart" and bloating up spinlocks. Similarly, I hate how the paravirt alternative blends in with regular (sane) bare metal code. What was preventing this instead?? #ifdef CONFIG_PARAVIRT_SPINLOCKS static __always_inline void arch_spin_unlock(arch_spinlock_t *lock) { if (!static_key_false(¶virt_ticketlocks_enabled)) return; add_smp(&lock->tickets.head, TICKET_LOCK_INC); /* Do slowpath tail stuff... */ } #else static __always_inline void arch_spin_unlock(arch_spinlock_t *lock) { __add(&lock->tickets.head, TICKET_LOCK_INC, UNLOCK_LOCK_PREFIX); } #endif I just don't see the point to all this TICKET_SLOWPATH_FLAG: #ifdef CONFIG_PARAVIRT_SPINLOCKS #define __TICKET_LOCK_INC 2 #define TICKET_SLOWPATH_FLAG ((__ticket_t)1) #else #define __TICKET_LOCK_INC 1 #define TICKET_SLOWPATH_FLAG ((__ticket_t)0) #endif when it is only for paravirt -- and the word slowpath implies the general steps as part of the generic algorithm. Lets keep code for simple locks simple. > Completions are so very much by design (because dynamic completions on > the stack is one of the core use cases), and spinlocks do it because > in some cases you cannot sanely avoid it (and one of those cases is > the implementation of completions - they aren't actually first-class > locking primitives of their own, although they actually *used* to be, > originally). > > It is possible that the paravirt spinlocks could be saved by: > > - moving the clearing of TICKET_SLOWPATH_FLAG into the fastpath locking code. Ouch, to avoid deadlocks they explicitly need the unlock to occur before the slowpath tail flag is read. > - making sure that the "unlock" path never does a *write* to the > possibly stale lock. KASan would still complain about the read, but we > could just say that it's a speculative read - bad form, but not > disastrous. Yeah, you just cannot have a slowpath without reads or writes :D Thanks, Davidlohr