Re: + hpsa-use-msleep-instead-of-schedule_timeout.patch added to -mm tree

Re: + hpsa-use-msleep-instead-of-schedule_timeout.patch added to -mm tree

[Jiri Slaby]

On 12/08/2009 11:04 PM, akpm@linux-foundation.org wrote:
> Subject: hpsa: use msleep() instead of schedule_timeout
> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> 
> Use msleep() instead of schedule_timeout

The patch does more than that and moreover in a wrong manner, see below.

> @@ -3262,8 +3262,8 @@ static int hpsa_pci_init(struct ctlr_inf
>  		if (!(readl(h->vaddr + SA5_DOORBELL) & CFGTBL_ChangeReq))
>  			break;
>  		/* delay and try again */
> -		set_current_state(TASK_INTERRUPTIBLE);
> -		schedule_timeout(10);
> +		set_current_state(TASK_UNINTERRUPTIBLE);
> +		msleep(10);

Why do you change interruptible sleep to uninterruptible? And you
intermix jiffies with msecs. Use schedule_timeout_interruptible(10).

> @@ -3302,7 +3302,8 @@ static int __devinit hpsa_init_one(struc
>  
>  		/* Some devices (notably the HP Smart Array 5i Controller)
>  		   need a little pause here */
> -		schedule_timeout_uninterruptible(HPSA_POST_RESET_PAUSE);
> +		set_current_state(TASK_UNINTERRUPTIBLE);
> +		msleep(HPSA_POST_RESET_PAUSE_MSECS);

Hmm, setting the state is superfluous, as msleep does the job itself.

> diff -puN drivers/scsi/hpsa.h~hpsa-use-msleep-instead-of-schedule_timeout drivers/scsi/hpsa.h
> --- a/drivers/scsi/hpsa.h~hpsa-use-msleep-instead-of-schedule_timeout
> +++ a/drivers/scsi/hpsa.h
...
> @@ -139,7 +139,7 @@ struct ctlr_info {
>  #define HPSA_BOARD_READY_ITERATIONS \
>  	((HPSA_BOARD_READY_WAIT_SECS * 1000) / \
>  		HPSA_BOARD_READY_POLL_INTERVAL_MSECS)
> -#define HPSA_POST_RESET_PAUSE (30 * HZ)
> +#define HPSA_POST_RESET_PAUSE_MSECS (3000)

Ehm?

-- 
js

Re: + hpsa-use-msleep-instead-of-schedule_timeout.patch added to -mm tree

[Jiri Slaby]

On 12/08/2009 11:25 PM, Jiri Slaby wrote:
>> @@ -3302,7 +3302,8 @@ static int __devinit hpsa_init_one(struc
>>  
>>  		/* Some devices (notably the HP Smart Array 5i Controller)
>>  		   need a little pause here */
>> -		schedule_timeout_uninterruptible(HPSA_POST_RESET_PAUSE);
>> +		set_current_state(TASK_UNINTERRUPTIBLE);
>> +		msleep(HPSA_POST_RESET_PAUSE_MSECS);
> 
> Hmm, setting the state is superfluous, as msleep does the job itself.

(FWIW addressed by Andrew already.)

-- 
js

Re: + hpsa-use-msleep-instead-of-schedule_timeout.patch added to -mm tree

[scameron]

On Tue, Dec 08, 2009 at 11:25:05PM +0100, Jiri Slaby wrote:
> On 12/08/2009 11:04 PM, akpm@linux-foundation.org wrote:
> > Subject: hpsa: use msleep() instead of schedule_timeout
> > From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> > 
> > Use msleep() instead of schedule_timeout
> 
> The patch does more than that and moreover in a wrong manner, see below.
> 
> > @@ -3262,8 +3262,8 @@ static int hpsa_pci_init(struct ctlr_inf
> >  		if (!(readl(h->vaddr + SA5_DOORBELL) & CFGTBL_ChangeReq))
> >  			break;
> >  		/* delay and try again */
> > -		set_current_state(TASK_INTERRUPTIBLE);
> > -		schedule_timeout(10);
> > +		set_current_state(TASK_UNINTERRUPTIBLE);
> > +		msleep(10);
> 
> Why do you change interruptible sleep to uninterruptible? And you

Because the I think the interruptible was wrong, we aren't interested
in signals at this point.

> intermix jiffies with msecs. Use schedule_timeout_interruptible(10).

Probably it should have been msecs all along, not jiffies.  Looking at that particular
part of the code, it's very old, and might even be superflous given the controllers
this driver supports (and those it does not support.)  I'm thinking that
schedule_timeout(10) was written back when HZ was commonly 100, so that would
have been 100 msecs, not 10.  Considering the comment above this code describing
the circumstances that this is for, I doubt that code has ever run, actually.
You have to replace a failed drive just as driver is loading.

I will talk to the firmware guys and see if I can figure out what the original
case that lead to this code being put in is still relevant.  We might be
able to get rid of this section altogether.

> 
> > @@ -3302,7 +3302,8 @@ static int __devinit hpsa_init_one(struc
> >  
> >  		/* Some devices (notably the HP Smart Array 5i Controller)
> >  		   need a little pause here */
> > -		schedule_timeout_uninterruptible(HPSA_POST_RESET_PAUSE);
> > +		set_current_state(TASK_UNINTERRUPTIBLE);
> > +		msleep(HPSA_POST_RESET_PAUSE_MSECS);
> 
> Hmm, setting the state is superfluous, as msleep does the job itself.

Yeah, Andrew pointed that out and sent me a patch.  Thanks.

> 
> > diff -puN drivers/scsi/hpsa.h~hpsa-use-msleep-instead-of-schedule_timeout drivers/scsi/hpsa.h
> > --- a/drivers/scsi/hpsa.h~hpsa-use-msleep-instead-of-schedule_timeout
> > +++ a/drivers/scsi/hpsa.h
> ...
> > @@ -139,7 +139,7 @@ struct ctlr_info {
> >  #define HPSA_BOARD_READY_ITERATIONS \
> >  	((HPSA_BOARD_READY_WAIT_SECS * 1000) / \
> >  		HPSA_BOARD_READY_POLL_INTERVAL_MSECS)
> > -#define HPSA_POST_RESET_PAUSE (30 * HZ)
> > +#define HPSA_POST_RESET_PAUSE_MSECS (3000)
> 
> Ehm?

Jeez, you'd think I could multiply by 1000 without screwing up.  Guess not.

> 
> -- 
> js

Re: + hpsa-use-msleep-instead-of-schedule_timeout.patch added to -mm tree

[Andrew Morton]

On Tue, 08 Dec 2009 23:25:05 +0100
Jiri Slaby <jirislaby@gmail.com> wrote:

> On 12/08/2009 11:04 PM, akpm@linux-foundation.org wrote:
> > Subject: hpsa: use msleep() instead of schedule_timeout
> > From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> > 
> > Use msleep() instead of schedule_timeout
> 
> The patch does more than that and moreover in a wrong manner, see below.
> 
> > @@ -3262,8 +3262,8 @@ static int hpsa_pci_init(struct ctlr_inf
> >  		if (!(readl(h->vaddr + SA5_DOORBELL) & CFGTBL_ChangeReq))
> >  			break;
> >  		/* delay and try again */
> > -		set_current_state(TASK_INTERRUPTIBLE);
> > -		schedule_timeout(10);
> > +		set_current_state(TASK_UNINTERRUPTIBLE);
> > +		msleep(10);
> 
> Why do you change interruptible sleep to uninterruptible?

Stealth bugfix ;)

If the calling process (called "modprobe") has signal_pending()
(someone ^C'ed it) then the TASK_INTERRUPTIBLE sleep will be a no-op
and the driver will probably go and screw things up.

> And you
> intermix jiffies with msecs. Use schedule_timeout_interruptible(10).
> 
> > @@ -3302,7 +3302,8 @@ static int __devinit hpsa_init_one(struc
> >  
> >  		/* Some devices (notably the HP Smart Array 5i Controller)
> >  		   need a little pause here */
> > -		schedule_timeout_uninterruptible(HPSA_POST_RESET_PAUSE);
> > +		set_current_state(TASK_UNINTERRUPTIBLE);
> > +		msleep(HPSA_POST_RESET_PAUSE_MSECS);
> 
> Hmm, setting the state is superfluous, as msleep does the job itself.

yup, I fixed those.

> > diff -puN drivers/scsi/hpsa.h~hpsa-use-msleep-instead-of-schedule_timeout drivers/scsi/hpsa.h
> > --- a/drivers/scsi/hpsa.h~hpsa-use-msleep-instead-of-schedule_timeout
> > +++ a/drivers/scsi/hpsa.h
> ...
> > @@ -139,7 +139,7 @@ struct ctlr_info {
> >  #define HPSA_BOARD_READY_ITERATIONS \
> >  	((HPSA_BOARD_READY_WAIT_SECS * 1000) / \
> >  		HPSA_BOARD_READY_POLL_INTERVAL_MSECS)
> > -#define HPSA_POST_RESET_PAUSE (30 * HZ)
> > +#define HPSA_POST_RESET_PAUSE_MSECS (3000)
> 
> Ehm?

30 seconds would have sucked anyway ;)

Re: + hpsa-use-msleep-instead-of-schedule_timeout.patch added to -mm tree

[Jiri Slaby]

On 12/08/2009 11:44 PM, Andrew Morton wrote:
> On Tue, 08 Dec 2009 23:25:05 +0100
> Jiri Slaby <jirislaby@gmail.com> wrote:
> 
>> On 12/08/2009 11:04 PM, akpm@linux-foundation.org wrote:
>>> Subject: hpsa: use msleep() instead of schedule_timeout
>>> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
>>>
>>> Use msleep() instead of schedule_timeout
>>
>> The patch does more than that and moreover in a wrong manner, see below.
>>
>>> @@ -3262,8 +3262,8 @@ static int hpsa_pci_init(struct ctlr_inf
>>>  		if (!(readl(h->vaddr + SA5_DOORBELL) & CFGTBL_ChangeReq))
>>>  			break;
>>>  		/* delay and try again */
>>> -		set_current_state(TASK_INTERRUPTIBLE);
>>> -		schedule_timeout(10);
>>> +		set_current_state(TASK_UNINTERRUPTIBLE);
>>> +		msleep(10);
>>
>> Why do you change interruptible sleep to uninterruptible?
> 
> Stealth bugfix ;)
> 
> If the calling process (called "modprobe") has signal_pending()
> (someone ^C'ed it) then the TASK_INTERRUPTIBLE sleep will be a no-op
> and the driver will probably go and screw things up.

Ok, then it should have been in the changelog. The changelog was just
"Use msleep() instead of schedule_timeout". Apart it doesn't mention why
it is changed (it might be obvious for some that it's a cleanup), it
doesn't do merely that.

>>> diff -puN drivers/scsi/hpsa.h~hpsa-use-msleep-instead-of-schedule_timeout drivers/scsi/hpsa.h
>>> --- a/drivers/scsi/hpsa.h~hpsa-use-msleep-instead-of-schedule_timeout
>>> +++ a/drivers/scsi/hpsa.h
>> ...
>>> @@ -139,7 +139,7 @@ struct ctlr_info {
>>>  #define HPSA_BOARD_READY_ITERATIONS \
>>>  	((HPSA_BOARD_READY_WAIT_SECS * 1000) / \
>>>  		HPSA_BOARD_READY_POLL_INTERVAL_MSECS)
>>> -#define HPSA_POST_RESET_PAUSE (30 * HZ)
>>> +#define HPSA_POST_RESET_PAUSE_MSECS (3000)
>>
>> Ehm?
> 
> 30 seconds would have sucked anyway ;)

Might be. But again, then it should be explained in the changelog. And
as this is totally separate thing, also in a separate patch.

At least I though this is the politics.

thanks,
-- 
js

Re: + hpsa-use-msleep-instead-of-schedule_timeout.patch added to -mm tree

[scameron]

On Wed, Dec 09, 2009 at 10:51:32AM +0100, Jiri Slaby wrote:
> On 12/08/2009 11:44 PM, Andrew Morton wrote:
> > On Tue, 08 Dec 2009 23:25:05 +0100
> > Jiri Slaby <jirislaby@gmail.com> wrote:
> > 
> >> On 12/08/2009 11:04 PM, akpm@linux-foundation.org wrote:
> >>> Subject: hpsa: use msleep() instead of schedule_timeout
> >>> From: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> >>>
> >>> Use msleep() instead of schedule_timeout
> >>
> >> The patch does more than that and moreover in a wrong manner, see below.
> >>
> >>> @@ -3262,8 +3262,8 @@ static int hpsa_pci_init(struct ctlr_inf
> >>>  		if (!(readl(h->vaddr + SA5_DOORBELL) & CFGTBL_ChangeReq))
> >>>  			break;
> >>>  		/* delay and try again */
> >>> -		set_current_state(TASK_INTERRUPTIBLE);
> >>> -		schedule_timeout(10);
> >>> +		set_current_state(TASK_UNINTERRUPTIBLE);
> >>> +		msleep(10);
> >>
> >> Why do you change interruptible sleep to uninterruptible?
> > 
> > Stealth bugfix ;)
> > 
> > If the calling process (called "modprobe") has signal_pending()
> > (someone ^C'ed it) then the TASK_INTERRUPTIBLE sleep will be a no-op
> > and the driver will probably go and screw things up.
> 
> Ok, then it should have been in the changelog. The changelog was just
> "Use msleep() instead of schedule_timeout". Apart it doesn't mention why
> it is changed (it might be obvious for some that it's a cleanup), it
> doesn't do merely that.

Sorry.  I'll try to do better.  FWIW, these all get rolled up into one
big patch on account of this being a new driver not yet in Linus's
tree, so those change log entries wouldn't go into git anyway, so
far as I know.

> 
> >>> diff -puN drivers/scsi/hpsa.h~hpsa-use-msleep-instead-of-schedule_timeout drivers/scsi/hpsa.h
> >>> --- a/drivers/scsi/hpsa.h~hpsa-use-msleep-instead-of-schedule_timeout
> >>> +++ a/drivers/scsi/hpsa.h
> >> ...
> >>> @@ -139,7 +139,7 @@ struct ctlr_info {
> >>>  #define HPSA_BOARD_READY_ITERATIONS \
> >>>  	((HPSA_BOARD_READY_WAIT_SECS * 1000) / \
> >>>  		HPSA_BOARD_READY_POLL_INTERVAL_MSECS)
> >>> -#define HPSA_POST_RESET_PAUSE (30 * HZ)
> >>> +#define HPSA_POST_RESET_PAUSE_MSECS (3000)
> >>
> >> Ehm?
> > 
> > 30 seconds would have sucked anyway ;)
> 

BTW, I talked to a firmware guy familiar with the original problem
and he looked through the firmware code for the the controllers this
driver supports and said he couldn't see any reason it should take more
than a couple seconds, so, three seconds in the driver seems reasonable.
If it turns out that people start complaining that the boards aren't
getting initialized in time, we can revisit it.  It's also my own
experience that this section takes next to no time at all.

> Might be. But again, then it should be explained in the changelog. And
> as this is totally separate thing, also in a separate patch.
> 
> At least I though this is the politics.
> 
> thanks,
> -- 
> js