Bug 213999 - cpuspeed: initscript stop action incorrect for centrino/powernow-k8
cpuspeed: initscript stop action incorrect for centrino/powernow-k8
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: cpuspeed (Show other bugs)
6
i386 Linux
medium Severity medium
: ---
: ---
Assigned To: Jarod Wilson
bzcl34nup
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2006-11-04 08:14 EST by Tomas Hoger
Modified: 2008-04-04 09:17 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-04-04 09:17:56 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
cpuspeed initscript patch for centrino/powernow-k8 cpus (742 bytes, patch)
2006-11-04 08:14 EST, Tomas Hoger
no flags Details | Diff
Updated cpuspeed init script patch for centrino (801 bytes, patch)
2006-11-14 08:03 EST, Tomas Hoger
no flags Details | Diff
Updated cpuspeed init script patch for centrino (744 bytes, patch)
2006-12-04 03:26 EST, Tomas Hoger
no flags Details | Diff
cpuspeed-1.2.1-1.53.fc7 initscript minor fixes (1.46 KB, patch)
2007-01-13 12:31 EST, Tomas Hoger
no flags Details | Diff

  None (edit)
Description Tomas Hoger 2006-11-04 08:14:00 EST
Description of problem:

Running '/etc/init.d/cpuspeed stop' on system with centrino scaling driver
produces error message:

ERROR: Module cpufreq_ondemand is in use

CPU speed is still dynamically scaled.


Version-Release number of selected component (if applicable):

cpuspeed-1.2.1-1.40.fc6


Expected results:

After "stopping" cpuspeed, CPU should run at full speed and it's frequency
should not be scaled.


Additional info:

cpufreq_ondemand can not be unloaded because ondemand is still configured as
scaling governor.  Attached patch sets governor to userspace and sets maximum
CPU frequency before unloading cpufreq_ondemand.  Works fine for me ;).
Comment 1 Tomas Hoger 2006-11-04 08:14:01 EST
Created attachment 140356 [details]
cpuspeed initscript patch for centrino/powernow-k8 cpus
Comment 2 Tomas Hoger 2006-11-14 08:03:24 EST
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.
Comment 3 Tomas Hoger 2006-12-04 03:26:34 EST
Created attachment 142708 [details]
Updated cpuspeed init script patch for centrino

Previous patch udpated to cpuspeed-1.2.1-1.42.
Comment 4 Jarod Wilson 2006-12-14 17:41:39 EST
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...
Comment 5 Tomas Hoger 2007-01-11 08:30:08 EST
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.
Comment 6 Jarod Wilson 2007-01-11 10:34:30 EST
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).
Comment 7 Tomas Hoger 2007-01-13 12:31:00 EST
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...
Comment 8 Jarod Wilson 2007-01-15 11:53:02 EST
(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... :)
Comment 9 Jarod Wilson 2007-01-15 11:57:47 EST
Whoops, the typo was NOT already fixed, just fixed it...
Comment 10 Tomas Hoger 2007-01-16 07:43:59 EST
(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 ;).
Comment 11 Jarod Wilson 2007-01-16 10:23:30 EST
(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! :)
Comment 12 Tomas Hoger 2007-01-16 15:02:11 EST
(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.
Comment 13 Jarod Wilson 2007-01-16 15:16:15 EST
(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!
Comment 14 Bug Zapper 2008-04-04 00:25:31 EDT
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
Comment 15 Jarod Wilson 2008-04-04 09:17:56 EDT
Pretty sure we got this fixed a while ago, closing bug...

Note You need to log in before you can comment on or make changes to this bug.