Hi
I'm trying to write a block device driver which does some network stuff to satisfy the requests. The problem is, that the network stuff wants to grab the io_request_lock which does not work because this lock is already locked when I come into the request_fn of my device.
I looked at the implementation of the nbd which just calls
spin_unlock_irq(&io_request_lock);
... do network io ...
spin_lock_irq(&io_request_lock);
This seems to work but it looks very dangerous to me (and ugly, too). Isn't there a better way to do this?
Thanks very much
Reto
> I looked at the implementation of the nbd which just calls
>
> spin_unlock_irq(&io_request_lock);
> ... do network io ...
> spin_lock_irq(&io_request_lock);
>
> This seems to work but it looks very dangerous to me (and ugly, too). Isn't there a better way to do this?
It is only dangerous if you unlock it in the wrong place, unlocking it as much
as possible is good behaviour. You need it locked until you get the actual
request off the queue, you need it locked when you complete the request. The
rest of the time you can be polite
>
> > I looked at the implementation of the nbd which just calls
> >
> > spin_unlock_irq(&io_request_lock);
> > ... do network io ...
> > spin_lock_irq(&io_request_lock);
> >
> > This seems to work but it looks very dangerous to me (and ugly, too). Isn't there a better way to do this?
>
> It is only dangerous if you unlock it in the wrong place, unlocking it as much
> as possible is good behaviour. You need it locked until you get the actual
> request off the queue, you need it locked when you complete the request. The
> rest of the time you can be polite
>
>
I'm sorry but I still have some doubts:
The add_request function calls
spin_lock_irqsave(&io_request_lock,flags);
and then calls our request_fn which does
spin_unlock_irq(&io_request_lock);
...do network I/O ...
spin_lock_irq(&io_request_lock);
we finish the request and return to the add_request function which calls
spin_unlock_irqrestore(&io_request_lock,flags);
and restores the flags.
Isn't it possible now that the flags which we restore are out of date now?
Is this idiom the right one to use for 2.2?
Thanks,
Reto
> spin_lock_irq(&io_request_lock);
> we finish the request and return to the add_request function which calls
> spin_unlock_irqrestore(&io_request_lock,flags);
> and restores the flags.
>
> Isn't it possible now that the flags which we restore are out of date now?
> Is this idiom the right one to use for 2.2?
It is fine for 2.2 as well.
The flags you restore are ok. It restores the interrupt state to the state it
was in when you were called. Think of save_flags/restore_flags as bracketing
regions of code (and being nestable in pairs). The only real bizarre rule is
that you cannot save_flags in one function and restore_flags in another without
upsetting DaveM - as it breaks on the sparc if you do that
> > spin_lock_irq(&io_request_lock);
> > we finish the request and return to the add_request function which calls
> > spin_unlock_irqrestore(&io_request_lock,flags);
> > and restores the flags.
> >
> > Isn't it possible now that the flags which we restore are out of date now?
> > Is this idiom the right one to use for 2.2?
>
> It is fine for 2.2 as well.
>
> The flags you restore are ok. It restores the interrupt state to the state it
> was in when you were called. Think of save_flags/restore_flags as bracketing
> regions of code (and being nestable in pairs). The only real bizarre rule is
> that you cannot save_flags in one function and restore_flags in another without
> upsetting DaveM - as it breaks on the sparc if you do that
Yes, and I believe that this is what's broken about the SCSI midlayer. The the
io_request_lock cannot be completely released in a SCSI HBA because the flags
for that save are way up the stack somewhere. This means that if you need to,
for example, sleep on something like loop coming up, you can spin_unlock_irq,
but you can't restore flags, so for the UP case, you just hang. To avoid this,
I'm having to build a kernel thread to handle all of this kind of stuff in a
separate context. What a PITA.
-matt
> Yes, and I believe that this is what's broken about the SCSI midlayer. The the
> io_request_lock cannot be completely released in a SCSI HBA because the flags
You can drop it with spin_unlock_irq and that is fine. I do that with no problems
in the I2O scsi driver for example
On Fri, 8 Dec 2000, Alan Cox wrote:
> > Yes, and I believe that this is what's broken about the SCSI midlayer. The the
> > io_request_lock cannot be completely released in a SCSI HBA because the flags
>
> You can drop it with spin_unlock_irq and that is fine. I do that with no
> problems in the I2O scsi driver for example
I am (like, I think I *finally* got locking sorta right in my QLogic driver),
but doesn't this still leave ints blocked for this CPU at least?
-matt
>
>
>
> On Fri, 8 Dec 2000, Alan Cox wrote:
>
> > > Yes, and I believe that this is what's broken about the SCSI midlayer. The the
> > > io_request_lock cannot be completely released in a SCSI HBA because the flags
> >
> > You can drop it with spin_unlock_irq and that is fine. I do that with no
> > problems in the I2O scsi driver for example
>
> I am (like, I think I *finally* got locking sorta right in my QLogic driver),
> but doesn't this still leave ints blocked for this CPU at least?
>
> -matt
>
>
>
I am actually concerned about the following case:
The add_request ON CPU_1 function calls
spin_lock_irqsave(&io_request_lock,flags);
Our I/O Function unlocks the spinlock and goes to sleep.
Finally, the add_request function, NOW ON CPU_2 calls
spin_unlock_irqrestore(&io_request_lock,flags);
and restores the flags of CPU_1 on CPU_2.
What am I missing? Are the flags which we restore valid for all the CPU's the same?
On Fri, Dec 08, 2000 at 04:03:58PM -0800, Matthew Jacob wrote:
>
>
> On Fri, 8 Dec 2000, Alan Cox wrote:
>
> > > Yes, and I believe that this is what's broken about the SCSI midlayer. The the
> > > io_request_lock cannot be completely released in a SCSI HBA because the flags
> >
> > You can drop it with spin_unlock_irq and that is fine. I do that with no
> > problems in the I2O scsi driver for example
>
> I am (like, I think I *finally* got locking sorta right in my QLogic driver),
> but doesn't this still leave ints blocked for this CPU at least?
spin_unlock_irq() does a __sti()
spin_unlock() doesn't.
-Andi
>
> I am actually concerned about the following case:
>
> The add_request ON CPU_1 function calls
> spin_lock_irqsave(&io_request_lock,flags);
>
> Our I/O Function unlocks the spinlock and goes to sleep.
>
> Finally, the add_request function, NOW ON CPU_2 calls
> spin_unlock_irqrestore(&io_request_lock,flags);
> and restores the flags of CPU_1 on CPU_2.
>
> What am I missing? Are the flags which we restore valid for all the CPU's
> the same?
Absolutely not. They're state *for that CPU*.
Before I gave up doing my own locking in my HBA, I was using flags in my
softc, so I could do things like this in transitioning from an int svc routine
to another module (in this case, a SCSI target mode handler):
<interrupt starts>
spinlock_irq_save(&isp->isp_lk, isp->isp_lkflags);
<get stuff from response queue>
spinlock_irq_restore(&isp->isp_lk, isp->isp_lkflags);
<call scsi_target_mode_handler>
spinlock_irq_save(&isp->isp_lk, isp->isp_lkflags);
<reinstruct card>
spinlock_irq_restore(&isp->isp_lk, isp->isp_lkflags);
(return from interrupt)
So, if you acquire the lock, the flags go with *that* CPU's acquisition of the
lock. But that's dangerous in that you're passing the flags around to be
possibly 'restored' (incorrectly) in some other context. That's why flags
seems to be usually an auto variable construct.
But what I still don't get is why it's okay to do a spin_unlock_irq on
io_request_lock (because you don't have access to the flags that were locked
with it (if any)) and not end up with a system that can deadlock. Well,
enlightenment will come some day I'm sure.
-matt
> >
> > On Fri, 8 Dec 2000, Alan Cox wrote:
> >
> > > > Yes, and I believe that this is what's broken about the SCSI midlayer. The the
> > > > io_request_lock cannot be completely released in a SCSI HBA because the flags
> > >
> > > You can drop it with spin_unlock_irq and that is fine. I do that with no
> > > problems in the I2O scsi driver for example
> >
> > I am (like, I think I *finally* got locking sorta right in my QLogic driver),
> > but doesn't this still leave ints blocked for this CPU at least?
>
> spin_unlock_irq() does a __sti()
> spin_unlock() doesn't.
Umm. Okay, but you haven't changed your processor priority though, right?
(I just don't *get* i386 stuff... I'll go off and ponder SParc code - &that& I
understand).
-matt
> > You can drop it with spin_unlock_irq and that is fine. I do that with no
> > problems in the I2O scsi driver for example
>
> I am (like, I think I *finally* got locking sorta right in my QLogic driver),
> but doesn't this still leave ints blocked for this CPU at least?
spin_unlock_irq unconditionally enables
On Fri, Dec 08, 2000 at 04:19:45PM -0800, Matthew Jacob wrote:
>
> > >
> > > On Fri, 8 Dec 2000, Alan Cox wrote:
> > >
> > > > > Yes, and I believe that this is what's broken about the SCSI midlayer. The the
> > > > > io_request_lock cannot be completely released in a SCSI HBA because the flags
> > > >
> > > > You can drop it with spin_unlock_irq and that is fine. I do that with no
> > > > problems in the I2O scsi driver for example
> > >
> > > I am (like, I think I *finally* got locking sorta right in my QLogic driver),
> > > but doesn't this still leave ints blocked for this CPU at least?
> >
> > spin_unlock_irq() does a __sti()
> > spin_unlock() doesn't.
>
> Umm. Okay, but you haven't changed your processor priority though, right?
There is no concept of spl levels in Linux ;) It only knows about
interrupts on or off.
> (I just don't *get* i386 stuff... I'll go off and ponder SParc code - &that& I
> understand).
It is very simple[1] : interrupts can be on or off.
__cli() disables them, __sti() enables them again.
When you want nesting use the _flags versions.
-Andi
[1] in linux, there are some architecture extensions for interrupt priorities, but linux
doesn't use them.