Created attachment 140356 [details]
cpuspeed initscript patch for centrino/powernow-k8 cpus

Created attachment 141147 [details]
Updated cpuspeed init script patch for centrino

Updated patch checks if cpufreq_ondemand module is loaded before trying to
unload it.  Prevents error message when running 'cpuspeed stop' and cpu speed
scaling is already disabled.

Created attachment 142708 [details]
Updated cpuspeed init script patch for centrino

Previous patch udpated to cpuspeed-1.2.1-1.42.

Please give rawhide build 1.2.1-1.46 a spin once it hits download mirrors
(building now), I've more or less implemented your patch, plus one other fix...

Jarod, sorry, I've overlooked your reply.  Today I've noticed new version
1.2.1-1.43 in FC6 and also made a quick look at 1.2.1-1.51 in FC7.  Since
initscripts in are almost same, I will not differentiate between them.


First thing I noticed is line 148:

governor=${GOVERNOR:-governor}

which should probably be:

governor=${GOVERNOR:-ondemand}


There's also issue with check right below:

grep -c -w ${governor} ${cpu0freqd}/scaling_available_governors

If governor is ondemand, but cpufreq_ondemand is not loaded, ondemand will not
appear in scaling_available_governors.  Hence following message will be logged:

Invalid governor "ondemand" specified, falling back to ondemand


I'll take closer look into new init script later and report futher issues, if any.

Yep, saw those buglets yesterday, they're already fixed in cvs. I've pushed an
updated rawhide build, but not an fc6 build just yet (on the list for today though).

Created attachment 145530 [details]
cpuspeed-1.2.1-1.53.fc7 initscript minor fixes

Jarod, you're giving me hard time to catch-up with speed of your changes ;).

Attaching small patch with few more tweaks:

- check if INGORE_CHECK was set and has valid value, so script won't choke on
empty variable

- modprobe acpi-cpufreq redirections put in sync with fc6 version, guess you
already have it fixed in cvs

- print [FAILED] if cpuspeed is not running and is requested to stop

- minor typo fix


Some other ideas:

- should error messages only go to syslog?  given that it will mostly be used
on desktop / laptop computers where users do not check logs very often (if
ever), it may be better to (also) print errors on screen

- should script try to stop daemon on computer, where it will only try to
enable scaling using governor?	e.g. stopping stopped cpuspeed on centrino:

# ./cpuspeed stop
Disabling ondemand cpu frequency scaling:		   [  OK  ]
# ./cpuspeed stop
Stopping cpuspeed:					   [FAILED]

i know, that's just a cosmetic issue...

(In reply to comment #7)
> Created an attachment (id=145530) [edit]
> cpuspeed-1.2.1-1.53.fc7 initscript minor fixes
> 
> Jarod, you're giving me hard time to catch-up with speed of your changes ;).

:D

> Attaching small patch with few more tweaks:
> 
> - check if INGORE_CHECK was set and has valid value, so script won't choke on
> empty variable

Whoops, yeah, missed that, will fix...

> - modprobe acpi-cpufreq redirections put in sync with fc6 version, guess you
> already have it fixed in cvs

Yep, already in cvs, build just hadn't been pushed yet.

> - print [FAILED] if cpuspeed is not running and is requested to stop

Hm... Pretty sure we can't put a failure in right there, it could potentially
interfere with the normal cpuspeed daemon shutdown bits. Could be implemented at
the top of the stop() function though -- instead of returning right away when
scaling_driver isn't found, spit out the failure first. However, the lack of
output is intentional. We install cpuspeed chkconfig'd on on all systems, so we
like to silently fail on systems where it isn't supported, rather than alarming
users.

> - minor typo fix

Was also already fixed in cvs. :)

> Some other ideas:
> 
> - should error messages only go to syslog?  given that it will mostly be used
> on desktop / laptop computers where users do not check logs very often (if
> ever), it may be better to (also) print errors on screen

This behavior is for reasons similar to not printing [FAILED]. The errors are
still captured somewhere, should we need to debug why freq scaling isn't
working, but we're hopefully not unnecessarily scaring anyone in the process.

> - should script try to stop daemon on computer, where it will only try to
> enable scaling using governor?	e.g. stopping stopped cpuspeed on centrino:
> 
> # ./cpuspeed stop
> Disabling ondemand cpu frequency scaling:		   [  OK  ]
> # ./cpuspeed stop
> Stopping cpuspeed:					   [FAILED]
> 
> i know, that's just a cosmetic issue...

Ooh, fun, that could get messy in a hurry if we did print failures... Note that
centrino + userspace actually is a valid setup we'd need to stop, so you'd have
to do a fair amount of variable checking...

New build coming later today after I trawl through some more bugs... :)

Whoops, the typo was NOT already fixed, just fixed it...

(In reply to comment #8)

> Hm... Pretty sure we can't put a failure in right there, it could potentially
> interfere with the normal cpuspeed daemon shutdown bits.

Really?  failure is only called when first -n "`pidof cpuspeed`" test fails.  In
that case, cpuspeed is not running at all.  In that case, current version of
init script will not print OK nor FAILED.

# ./cpuspeed start
Starting cpuspeed:                                         [  OK  ]
# ./cpuspeed stop
Stopping cpuspeed:                                         [  OK  ]
# ./cpuspeed stop
Stopping cpuspeed:
#

> However, the lack of output is intentional.

But there is some (incomplete) output.

> This behavior is for reasons similar to not printing [FAILED]. The errors are
> still captured somewhere, should we need to debug why freq scaling isn't
> working, but we're hopefully not unnecessarily scaring anyone in the process.

OK, understand.  Not what I would prefer, but probably better for others...

> Note that centrino + userspace actually is a valid setup we'd need to stop,
> so you'd have to do a fair amount of variable checking...

Now I see, it's possible to start such combination, if user sets GOVERNOR to
userspace.  Yes, lots of checking to do all for everyone correctly.
 
> New build coming later today after I trawl through some more bugs... :)

Good luck with other bugs ;).

(In reply to comment #10)
> > Hm... Pretty sure we can't put a failure in right there, it could
> > potentially interfere with the normal cpuspeed daemon shutdown bits.
> 
> Really?  failure is only called when first -n "`pidof cpuspeed`" test fails.
> In that case, cpuspeed is not running at all.  In that case, current version
> of init script will not print OK nor FAILED.
> 
> # ./cpuspeed start
> Starting cpuspeed:                                         [  OK  ]
> # ./cpuspeed stop
> Stopping cpuspeed:                                         [  OK  ]
> # ./cpuspeed stop
> Stopping cpuspeed:

Ah, I see... We shouldn't be printing that "Stopping cpuspeed: " when its
already stopped...

But I still believe inserting the failure between the killprocs could
potentially cause problems, since the second round of cpuspeed kill attempts
were put in there for a reason (a historical one that I don't know the specifics
of -- that bit is from the cpuspeed author's original init script). The second
round of killing may be needed in some cases to finish shutting cpuspeed down
(maybe on slower high-cpu-count systems? I dunno).

> But there is some (incomplete) output.

I'll get that suppressed shortly, I have an easy fix in mind...

[...]
> Good luck with other bugs ;).

Thank you for your help! :)

(In reply to comment #11)

> But I still believe inserting the failure between the killprocs could
> potentially cause problems, since the second round of cpuspeed kill attempts
> were put in there for a reason (a historical one that I don't know the specifics
> of -- that bit is from the cpuspeed author's original init script). The second
> round of killing may be needed in some cases to finish shutting cpuspeed down
> (maybe on slower high-cpu-count systems? I dunno).

If you manage to nuke that useless "Stopping cpuspeed: ", there's no need to use
failure there.

Just note:  I do not say second killproc round is not needed.  Added failure
call does not interfere with it.  It was added to branch of code, which is only
reached if cpuspeed is *not* running.  In that case, first round of killprocs is
not executed either.

(In reply to comment #12)
> Just note:  I do not say second killproc round is not needed.  Added failure
> call does not interfere with it.  It was added to branch of code, which is only
> reached if cpuspeed is *not* running.  In that case, first round of killprocs is
> not executed either.

Oh, wow, not sure where my head was... You're absolutely correct, that wouldn't
interfere at all. D'oh!

Fedora apologizes that these issues have not been resolved yet. We're
sorry it's taken so long for your bug to be properly triaged and acted
on. We appreciate the time you took to report this issue and want to
make sure no important bugs slip through the cracks.

If you're currently running a version of Fedora Core between 1 and 6,
please note that Fedora no longer maintains these releases. We strongly
encourage you to upgrade to a current Fedora release. In order to
refocus our efforts as a project we are flagging all of the open bugs
for releases which are no longer maintained and closing them.
http://fedoraproject.org/wiki/LifeCycle/EOL

If this bug is still open against Fedora Core 1 through 6, thirty days
from now, it will be closed 'WONTFIX'. If you can reporduce this bug in
the latest Fedora version, please change to the respective version. If
you are unable to do this, please add a comment to this bug requesting
the change.

Thanks for your help, and we apologize again that we haven't handled
these issues to this point.

The process we are following is outlined here:
http://fedoraproject.org/wiki/BugZappers/F9CleanUp

We will be following the process here:
http://fedoraproject.org/wiki/BugZappers/HouseKeeping to ensure this
doesn't happen again.

And if you'd like to join the bug triage team to help make things
better, check out http://fedoraproject.org/wiki/BugZappers

Pretty sure we got this fixed a while ago, closing bug...