On Thu, Jan 2, 2014 at 2:31 AM, Suresh Thiagarajan
<Suresh.Thiagarajan@xxxxxxxx> wrote:
>
>
> 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
>>
>> 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-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
