Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753589AbaACUCK (ORCPT ); Fri, 3 Jan 2014 15:02:10 -0500 Received: from mail-qa0-f51.google.com ([209.85.216.51]:47270 "EHLO mail-qa0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752849AbaACUCH (ORCPT ); Fri, 3 Jan 2014 15:02:07 -0500 MIME-Version: 1.0 In-Reply-To: References: <52B8518B.4060204@gmail.com> <52B8569D.4050101@redhat.com> <20131223163410.GA28220@redhat.com> <20131223172744.GA2069@redhat.com> <20131223182323.GA8656@gmail.com> <20131223183341.GA6082@redhat.com> <20131224082931.GA20471@gmail.com> <20131227161802.GA21077@redhat.com> Date: Fri, 3 Jan 2014 12:02:06 -0800 Message-ID: Subject: Re: spinlock_irqsave() && flags (Was: pm80xx: Spinlock fix) From: Dan Williams To: Suresh Thiagarajan Cc: Oleg Nesterov , Ingo Molnar , Jason Seba , Peter Zijlstra , Ingo Molnar , Linus Torvalds , Tomas Henzl , Jack Wang , Viswas G , "linux-scsi@vger.kernel.org" , "JBottomley@parallels.com" , Vasanthalakshmi Tharmarajan , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2597 Lines: 76 On Thu, Jan 2, 2014 at 2:31 AM, Suresh Thiagarajan wrote: > > > On Fri, Dec 27, 2013 at 9:48 PM, Oleg Nesterov wrote: >> On 12/24, Suresh Thiagarajan wrote: >>> >>> Below is a small pseudo code on protecting/serializing the flag for global access. >>> struct temp >>> { >>> ... >>> spinlock_t lock; >>> unsigned long lock_flags; >>> }; >>> void my_lock(struct temp *t) >>> { >>> unsigned long flag; // thread-private variable as suggested >>> spin_lock_irqsave(&t->lock, flag); >>> t->lock_flags = flag; //updating inside critical section now to serialize the access to flag >>> } >>> >>> void my_unlock(struct temp *t) >>> { >>> unsigned long flag = t->lock_flags; >>> t->lock_flags = 0; //clearing it before getting out of critical section >>> spin_unlock_irqrestore(&t->lock, flag); >>> } >> >> Yes, this should work as a quick fix. And you do not need to clear ->lock_flags >> in my_unlock(). >> >> But when I look at original patch again, I no longer understand why do >> you need pm8001_ha->lock_flags at all. Of course I do not understand this >> code, I am sure I missed something, but at first glance it seems that only >> this sequence >> >> spin_unlock_irq(&pm8001_ha->lock); >> t->task_done(t); >> spin_lock_irq(&pm8001_ha->lock); >> >> should be fixed? >> >> If yes, why you can't simply do spin_unlock() + spin_lock() around >> t->task_done() ? This won't enable irqs, but spin_unlock_irqrestore() >> doesn't necessarily enables irqs too, so ->task_done() can run with >> irqs disabled? >> >> And note that the pattern above has a lot of users, perhaps it makes >> sense to start with something like the patch below? > > Thanks James, Oleg and all for your inputs. > Will start with review and testing this patch and then work/investigate to keep shortest and clearest critical > section so that we can have the lock and unlock within the same routine. > Fwiw we solved this in libsas a while back with a similar pattern proposed by Oleg: unsigned long flags; local_irq_save(flags); spin_unlock(lock); ... spin_lock_lock(lock); local_irq_restore(flags); See commit 312d3e56119a "[SCSI] libsas: remove ata_port.lock management duties from lldds" -- Dan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/