Re: spinlock_irqsave() && flags (Was: pm80xx: Spinlock fix)
From: Dan Williams
Date: Fri Jan 03 2014 - 15:02:15 EST
On Thu, Jan 2, 2014 at 2:31 AM, Suresh Thiagarajan
> On Fri, Dec 27, 2013 at 9:48 PM, Oleg Nesterov <oleg@xxxxxxxxxx> 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
>> 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;
See commit 312d3e56119a "[SCSI] libsas: remove ata_port.lock
management duties from lldds"
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/