On Sat, Dec 25, 2004 at 01:48:46AM +0100, Domen Puncer wrote:
> Hi.
>
> Santa brought another present :-)
>
> I'll start mailing new patches these days, and after external trees get
> merged, I'll be bugging you with the old ones.
>
>
> Patchset is at http://coderock.org/kj/2.6.10-kj/
<snip>
> all patches:
> ------------
<snip>
> msleep-drivers_ieee1394_sbp2.patch
Please consider updating to the following patch:
Description: Use ssleep() instead of schedule_timeout() to guarantee the task
delays as expected. The existing code should not really need to run in
TASK_INTERRUPTIBLE, as there is no check for signals (or even an early return
value whatsoever). ssleep() takes care of these issues.
Signed-off-by: Nishanth Aravamudan <[email protected]>
--- 2.6.10-v/drivers/ieee1394/sbp2.c 2004-12-24 13:34:00.000000000 -0800
+++ 2.6.10/drivers/ieee1394/sbp2.c 2005-01-05 14:23:05.000000000 -0800
@@ -902,8 +902,7 @@ alloc_fail:
* connected to the sbp2 device being removed. That host would
* have a certain amount of time to relogin before the sbp2 device
* allows someone else to login instead. One second makes sense. */
- set_current_state(TASK_INTERRUPTIBLE);
- schedule_timeout(HZ);
+ ssleep(1);
/*
* Login to the sbp-2 device
Nishanth Aravamudan wrote:
> Description: Use ssleep() instead of schedule_timeout() to guarantee
> the task
> delays as expected. The existing code should not really need to run in
> TASK_INTERRUPTIBLE, as there is no check for signals (or even an
> early return
> value whatsoever). ssleep() takes care of these issues.
> --- 2.6.10-v/drivers/ieee1394/sbp2.c 2004-12-24 13:34:00.000000000
> -0800
> +++ 2.6.10/drivers/ieee1394/sbp2.c 2005-01-05 14:23:05.000000000 -0800
> @@ -902,8 +902,7 @@ alloc_fail:
> * connected to the sbp2 device being removed. That host would
> * have a certain amount of time to relogin before the sbp2 device
> * allows someone else to login instead. One second makes sense. */
> - set_current_state(TASK_INTERRUPTIBLE);
> - schedule_timeout(HZ);
> + ssleep(1);
Maybe the current code is _deliberately_ accepting interruption by
signals but trying to complete sbp2_probe() anyway. However it seems
more plausible to me to abort the device probe, for example like this:
if (msleep_interruptible(1000)) {
sbp2_remove_device(scsi_id);
return -EINTR;
}
Anyway, signal handling does not appear to be critical there.
--
Stefan Richter
-=====-=-=-= ---= -=--=
http://arcgraph.de/sr/
On Sun, Jan 09, 2005 at 10:01:21AM +0100, Stefan Richter wrote:
> Nishanth Aravamudan wrote:
> >Description: Use ssleep() instead of schedule_timeout() to guarantee
> >the task
> >delays as expected. The existing code should not really need to run in
> >TASK_INTERRUPTIBLE, as there is no check for signals (or even an
> >early return
> >value whatsoever). ssleep() takes care of these issues.
>
> >--- 2.6.10-v/drivers/ieee1394/sbp2.c 2004-12-24 13:34:00.000000000
> >-0800
> >+++ 2.6.10/drivers/ieee1394/sbp2.c 2005-01-05 14:23:05.000000000 -0800
> >@@ -902,8 +902,7 @@ alloc_fail:
> > * connected to the sbp2 device being removed. That host would
> > * have a certain amount of time to relogin before the sbp2 device
> > * allows someone else to login instead. One second makes sense. */
> >- set_current_state(TASK_INTERRUPTIBLE);
> >- schedule_timeout(HZ);
> >+ ssleep(1);
>
> Maybe the current code is _deliberately_ accepting interruption by
> signals but trying to complete sbp2_probe() anyway. However it seems
> more plausible to me to abort the device probe, for example like this:
> if (msleep_interruptible(1000)) {
> sbp2_remove_device(scsi_id);
> return -EINTR;
> }
You might be right, but I'd like to get Ben's input on this, as I honeslty am
unsure. To be fair, I am trying to audit all usage of schedule_timeout() and the
semantic interpretation (to me) of using TASK_INTERRUPTIBLE is that you wish to
sleep a certain amount of time, but also are prepared for an early return on
either signals or wait-queue events. msleep_interruptible() cleanly removes this
second issue, but still requires the caller to respond appropriately if there is
a return value. Hence, I like your change. I think it makes the most sense.
Since I didn't/don't know how the device works, I was not able to make the
change myself. Thanks for your input!
> Anyway, signal handling does not appear to be critical there.
Just out of curiousity, doesn't that run the risk, though, of
signal_pending(current) being true for quite a bit of time following the
timeout?
Thanks,
Nish
On Mon, 2005-01-10 at 09:39 -0800, Nishanth Aravamudan wrote:
> On Sun, Jan 09, 2005 at 10:01:21AM +0100, Stefan Richter wrote:
> > Nishanth Aravamudan wrote:
> > >Description: Use ssleep() instead of schedule_timeout() to guarantee
> > >the task
> > >delays as expected. The existing code should not really need to run in
> > >TASK_INTERRUPTIBLE, as there is no check for signals (or even an
> > >early return
> > >value whatsoever). ssleep() takes care of these issues.
> >
> > >--- 2.6.10-v/drivers/ieee1394/sbp2.c 2004-12-24 13:34:00.000000000
> > >-0800
> > >+++ 2.6.10/drivers/ieee1394/sbp2.c 2005-01-05 14:23:05.000000000 -0800
> > >@@ -902,8 +902,7 @@ alloc_fail:
> > > * connected to the sbp2 device being removed. That host would
> > > * have a certain amount of time to relogin before the sbp2 device
> > > * allows someone else to login instead. One second makes sense. */
> > >- set_current_state(TASK_INTERRUPTIBLE);
> > >- schedule_timeout(HZ);
> > >+ ssleep(1);
> >
> > Maybe the current code is _deliberately_ accepting interruption by
> > signals but trying to complete sbp2_probe() anyway. However it seems
> > more plausible to me to abort the device probe, for example like this:
> > if (msleep_interruptible(1000)) {
> > sbp2_remove_device(scsi_id);
> > return -EINTR;
> > }
>
> You might be right, but I'd like to get Ben's input on this, as I honeslty am
Don't hold your breath waiting for Ben's input. However, I would like to
get one of the two proposed committed and tested by more users as this
is a sore spot. I am not in a position at this time to fully research
and test to make a call.
> unsure. To be fair, I am trying to audit all usage of schedule_timeout() and the
> semantic interpretation (to me) of using TASK_INTERRUPTIBLE is that you wish to
> sleep a certain amount of time, but also are prepared for an early return on
> either signals or wait-queue events. msleep_interruptible() cleanly removes this
> second issue, but still requires the caller to respond appropriately if there is
> a return value. Hence, I like your change. I think it makes the most sense.
> Since I didn't/don't know how the device works, I was not able to make the
> change myself. Thanks for your input!
Sounds like a sign-off. Any other input before I request Stefan to make
the final decision?
> > Anyway, signal handling does not appear to be critical there.
>
> Just out of curiousity, doesn't that run the risk, though, of
> signal_pending(current) being true for quite a bit of time following the
> timeout?
How much of this is "curiosity" vs a real risk?
Dan Dennedy wrote:
> On Mon, 2005-01-10 at 09:39 -0800, Nishanth Aravamudan wrote:
>> On Sun, Jan 09, 2005 at 10:01:21AM +0100, Stefan Richter wrote:
>> > Nishanth Aravamudan wrote:
>> > >@@ -902,8 +902,7 @@ alloc_fail:
>> > > * connected to the sbp2 device being removed. That host would
>> > > * have a certain amount of time to relogin before the sbp2 device
>> > > * allows someone else to login instead. One second makes sense. */
>> > >- set_current_state(TASK_INTERRUPTIBLE);
>> > >- schedule_timeout(HZ);
>> > >+ ssleep(1);
>> >
>> > Maybe the current code is _deliberately_ accepting interruption by
>> > signals but trying to complete sbp2_probe() anyway. However it seems
>> > more plausible to me to abort the device probe, for example like this:
>> > if (msleep_interruptible(1000)) {
>> > sbp2_remove_device(scsi_id);
>> > return -EINTR;
>> > }
[...]
>> I am trying to audit all usage of schedule_timeout() and the
>> semantic interpretation (to me) of using TASK_INTERRUPTIBLE is that you wish to
>> sleep a certain amount of time, but also are prepared for an early return on
>> either signals or wait-queue events. [...]
>
> Sounds like a sign-off. Any other input before I request Stefan to make
> the final decision?
Don't count on me here. I do not even know /which/ situations might
introduce signals or wait queue events at this point. The only one that
occurred to me is when nodemgr is about to be killed. In this situation,
it is hardly beneficial to continue with the login. But there may be
other events I do not know about which should not result in sbp2 giving
up. Sorry, I should have been clear about this in my previous post.
>> > Anyway, signal handling does not appear to be critical there.
Or rather, it is not that important (although desirable) to always wait
for 1000ms. This is just necessary for when another initiator was logged
in into the target but did not reconnect or login again immediately
after a bus reset. (Assuming the other initiator or the local host or
the target require exclusive login, which is more common than concurrent
login.)
>> Just out of curiousity, doesn't that run the risk, though, of
>> signal_pending(current) being true for quite a bit of time following the
>> timeout?
>
> How much of this is "curiosity" vs a real risk?
Well, what might those events be? May we hold them off for one second?
(Or perhaps even longer if we are about to login to several targets.)
Should sbp2 proceeed to login when such events occur? I can't answer
this for sure. Sorry,
--
Stefan Richter
-=====-=-=-= ---= -===-
http://arcgraph.de/sr/
On Thu, 13 Jan 2005 23:52:55 -0500, Dan Dennedy <[email protected]> wrote:
> On Mon, 2005-01-10 at 09:39 -0800, Nishanth Aravamudan wrote:
> > On Sun, Jan 09, 2005 at 10:01:21AM +0100, Stefan Richter wrote:
> > > Nishanth Aravamudan wrote:
> > > >Description: Use ssleep() instead of schedule_timeout() to guarantee
> > > >the task
> > > >delays as expected. The existing code should not really need to run in
> > > >TASK_INTERRUPTIBLE, as there is no check for signals (or even an
> > > >early return
> > > >value whatsoever). ssleep() takes care of these issues.
> > >
> > > >--- 2.6.10-v/drivers/ieee1394/sbp2.c 2004-12-24 13:34:00.000000000
> > > >-0800
> > > >+++ 2.6.10/drivers/ieee1394/sbp2.c 2005-01-05 14:23:05.000000000 -0800
> > > >@@ -902,8 +902,7 @@ alloc_fail:
> > > > * connected to the sbp2 device being removed. That host would
> > > > * have a certain amount of time to relogin before the sbp2 device
> > > > * allows someone else to login instead. One second makes sense. */
> > > >- set_current_state(TASK_INTERRUPTIBLE);
> > > >- schedule_timeout(HZ);
> > > >+ ssleep(1);
> > >
> > > Maybe the current code is _deliberately_ accepting interruption by
> > > signals but trying to complete sbp2_probe() anyway. However it seems
> > > more plausible to me to abort the device probe, for example like this:
> > > if (msleep_interruptible(1000)) {
> > > sbp2_remove_device(scsi_id);
> > > return -EINTR;
> > > }
> >
> > You might be right, but I'd like to get Ben's input on this, as I honeslty am
>
> Don't hold your breath waiting for Ben's input. However, I would like to
> get one of the two proposed committed and tested by more users as this
> is a sore spot. I am not in a position at this time to fully research
> and test to make a call.
>
> > unsure. To be fair, I am trying to audit all usage of schedule_timeout() and the
> > semantic interpretation (to me) of using TASK_INTERRUPTIBLE is that you wish to
> > sleep a certain amount of time, but also are prepared for an early return on
> > either signals or wait-queue events. msleep_interruptible() cleanly removes this
> > second issue, but still requires the caller to respond appropriately if there is
> > a return value. Hence, I like your change. I think it makes the most sense.
> > Since I didn't/don't know how the device works, I was not able to make the
> > change myself. Thanks for your input!
>
> Sounds like a sign-off. Any other input before I request Stefan to make
> the final decision?
Yes, this is an ACK for Stefan's change. Although the exact code he
produced is not quite accurate. It would be most accurate to use
msleep_interruptible(1000);
if (signals_pending(current) {
sbp2_remove_device(scsi_id);
return -EINTR;
}
This accounts for the corner case when the sleep times out and a
signal comes between the second-to-last and last jiffies. Thanks for
both of your input! If you'd prefere me sending a new patch I can do
so from work tomorrow.
> > > Anyway, signal handling does not appear to be critical there.
> >
> > Just out of curiousity, doesn't that run the risk, though, of
> > signal_pending(current) being true for quite a bit of time following the
> > timeout?
>
> How much of this is "curiosity" vs a real risk?
I think it should be ok, actually, the -EINTR should get passed back
to userspace, where it would be handled appropriately. I hope :)
-Nish