* Oleg Nesterov <oleg@xxxxxxxxxx> wrote: > On 12/23, Ingo Molnar wrote: > > > > * Oleg Nesterov <oleg@xxxxxxxxxx> wrote: > > > > > Initially I thought that this is obviously wrong, irqsave/irqrestore > > > assume that "flags" is owned by the caller, not by the lock. And > > > iirc this was certainly wrong in the past. > > > > > > But when I look at spinlock.c it seems that this code can actually > > > work. _irqsave() writes to FLAGS after it takes the lock, and > > > _irqrestore() has a copy of FLAGS before it drops this lock. > > > > I don't think that's true: if it was then the lock would not be > > irqsave, a hardware-irq could come in after the lock has been taken > > and before flags are saved+disabled. > > I do agree that this pattern is not safe, that is why I decided to ask. > > But, unless I missed something, with the current implementation > spin_lock_irqsave(lock, global_flags) does: > > unsigned long local_flags; > > local_irq_save(local_flags); > spin_lock(lock); > > global_flags = local_flags; > > so the access to global_flags is actually serialized by lock. You are right, today that's true technically because IIRC due to Sparc quirks we happen to return 'flags' as a return value - still it's very ugly and it could break anytime if we decide to do more aggressive optimizations and actually directly save into 'flags'. Note that even today there's a narrow exception: on UP we happen to build it the other way around, so that we do: local_irq_save(global_flags); __acquire(lock); This does not matter for any real code because on UP there is no physical lock and __acquire() is empty code-wise, but any compiler driven locking analysis tool using __attribute__ __context__(), if built on UP, would see the unsafe locking pattern. Thanks, Ingo -- 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
