From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757589Ab3LWSY4 (ORCPT ); Mon, 23 Dec 2013 13:24:56 -0500 Received: from mx1.redhat.com ([209.132.183.28]:22717 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753085Ab3LWSYy (ORCPT ); Mon, 23 Dec 2013 13:24:54 -0500 Date: Mon, 23 Dec 2013 19:24:55 +0100 From: Oleg Nesterov To: Linus Torvalds Cc: Jason Seba , Peter Zijlstra , Ingo Molnar , Tomas Henzl , Jack Wang , Suresh Thiagarajan , Viswas G , "linux-scsi@vger.kernel.org" , "JBottomley@parallels.com" , Vasanthalakshmi Tharmarajan , Linux Kernel Mailing List Subject: Re: spinlock_irqsave() && flags (Was: pm80xx: Spinlock fix) Message-ID: <20131223182455.GA5656@redhat.com> References: <52B8357D.60202@redhat.com> <52B83B89.9040700@gmail.com> <52B8518B.4060204@gmail.com> <52B8569D.4050101@redhat.com> <20131223163410.GA28220@redhat.com> <20131223172744.GA2069@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/23, Linus Torvalds wrote: > > On Mon, Dec 23, 2013 at 9:27 AM, Oleg Nesterov wrote: > > > > In short, is this code > > > > spinlock_t LOCK; > > unsigned long FLAGS; > > > > void my_lock(void) > > { > > spin_lock_irqsave(&LOCK, FLAGS); > > } > > > > void my_unlock(void) > > { > > spin_unlock_irqrestore(&LOCK, FLAGS); > > } > > > > correct or not? > > Hell no. "flags" needs to be a thread-private variable, or at least > protected some way (ie the above could work if everything is inside a > bigger lock, to serialize access to FLAGS). This was my understanding (although, once again, it seems to me this can suprisingly work with the current implementation). However, the code above already has the users. Do you think it makes sense to add something like void spinlock_irqsave_careful(spinlock_t *lock, unsigned long *flags) { unsigned long _flags; spinlock_irqsave(lock, _flags); *flags = flags; } void spinlock_irqrestore_careful(spinlock_t *lock, unsigned long *flags) { unsigned long _flags = *flags; spinlock_irqrestore(lock, _flags); } into include/linux/spinlock.h ? Oleg. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Oleg Nesterov Subject: Re: spinlock_irqsave() && flags (Was: pm80xx: Spinlock fix) Date: Mon, 23 Dec 2013 19:24:55 +0100 Message-ID: <20131223182455.GA5656@redhat.com> References: <52B8357D.60202@redhat.com> <52B83B89.9040700@gmail.com> <52B8518B.4060204@gmail.com> <52B8569D.4050101@redhat.com> <20131223163410.GA28220@redhat.com> <20131223172744.GA2069@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Linus Torvalds Cc: Jason Seba , Peter Zijlstra , Ingo Molnar , Tomas Henzl , Jack Wang , Suresh Thiagarajan , Viswas G , "linux-scsi@vger.kernel.org" , "JBottomley@parallels.com" , Vasanthalakshmi Tharmarajan , Linux Kernel Mailing List List-Id: linux-scsi@vger.kernel.org On 12/23, Linus Torvalds wrote: > > On Mon, Dec 23, 2013 at 9:27 AM, Oleg Nesterov wrote: > > > > In short, is this code > > > > spinlock_t LOCK; > > unsigned long FLAGS; > > > > void my_lock(void) > > { > > spin_lock_irqsave(&LOCK, FLAGS); > > } > > > > void my_unlock(void) > > { > > spin_unlock_irqrestore(&LOCK, FLAGS); > > } > > > > correct or not? > > Hell no. "flags" needs to be a thread-private variable, or at least > protected some way (ie the above could work if everything is inside a > bigger lock, to serialize access to FLAGS). This was my understanding (although, once again, it seems to me this can suprisingly work with the current implementation). However, the code above already has the users. Do you think it makes sense to add something like void spinlock_irqsave_careful(spinlock_t *lock, unsigned long *flags) { unsigned long _flags; spinlock_irqsave(lock, _flags); *flags = flags; } void spinlock_irqrestore_careful(spinlock_t *lock, unsigned long *flags) { unsigned long _flags = *flags; spinlock_irqrestore(lock, _flags); } into include/linux/spinlock.h ? Oleg.