Last Comment Bug 380558 - Some event waiting in PSM make the CPU wake from idle with no reason
: Some event waiting in PSM make the CPU wake from idle with no reason
Status: RESOLVED FIXED
: fixed1.8.1.5
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: unspecified
: x86 Linux
: -- enhancement with 12 votes (vote)
: ---
Assigned To: Mike Hommey [:glandium]
:
: David Keeler [:keeler] (use needinfo?)
Mentors:
http://www.linuxpowertop.org/known.ph...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-05-13 05:52 PDT by Mike Hommey [:glandium]
Modified: 2008-01-17 06:40 PST (History)
21 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch against 1.8 branch (1.10 KB, patch)
2007-05-13 05:52 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
Patch against 1.8 branch (1.14 KB, patch)
2007-05-14 00:02 PDT, Mike Hommey [:glandium]
kaie: review-
Details | Diff | Splinter Review
New patch (2.77 KB, patch)
2007-05-14 06:41 PDT, Mike Hommey [:glandium]
kaie: review+
dveditz: approval1.8.1.5+
Details | Diff | Splinter Review

Description Mike Hommey [:glandium] 2007-05-13 05:52:31 PDT
Created attachment 264666 [details] [diff] [review]
Patch against 1.8 branch

This is the result of the work from Intel to get better battery life on laptops under linux. Here is a copy of what the original patch taken from linuxpowertop.com says:

Firefox had 2 condition variables that were waited on with a timeout of 250 miliseconds in a loop.
It appears that the author of the code uses 250 milliseconds as "inifite", however, the PR_WaitCondVar() API has an actual value for inifite, PR_INTERVAL_NO_TIMEOUT. Use that instead.

I'm attaching here a slightly modified patch and adding its original author to the Cc list (Edit: I would have, if Bugzilla would allow to Cc to adresses not associated to a bugzilla account).
The difference with the original patch (http://www.linuxpowertop.org/patches/firefox-2.0.patch ) is that I removed the wait_time variables that are useless by the usage of PR_INTERVAL_NO_TIMEOUT.
Comment 1 Mike Hommey [:glandium] 2007-05-14 00:02:22 PDT
Created attachment 264719 [details] [diff] [review]
Patch against 1.8 branch

Same patch, with correct paths.
Comment 2 Kai Engert (:kaie) 2007-05-14 05:52:08 PDT
Comment on attachment 264719 [details] [diff] [review]
Patch against 1.8 branch

I'm the original author of this code. Thanks for working on this and trying to optimize it.


Regarding nsCertVerificationThread, I tend to agree this patch is safe.

The condition variable gets set in both cases where we care: When exit is requested and when a new job gets pushed into the queue.


Regarding nsSSLThread, life is more complicated. In addition to mExitRequested we have 2 additional exit conditions that we must watch out for.


Regarding "pending work", we must ensure mCond will get signaled each time work is passed to us by assigning a socket to mBusySocket. It appears this is done already.


And when "mSocketScheduledToBeDestroyed" gets set, the condition variable will currently not get signaled. Can you please fix that?


Finally, we must land this patch on the "trunk" first, in the latest development version. This might be a risky change and it should bake there for a while before we dare to land this on 1.8 branch. It should get sufficient testing with all sorts of SSL access.
Comment 3 Mike Hommey [:glandium] 2007-05-14 06:41:54 PDT
Created attachment 264740 [details] [diff] [review]
New patch

(Note the previous patch applied as-is perfectly fine on trunk, only with a hunk beeing offsetted by 62 lines)

Here is a modified patch following what I understood from your hints. I only saw one place where mSocketScheduledToBeDestroyed should be notified.

This diff has been done against the trunk with cvs diff -u8.
Comment 4 Kai Engert (:kaie) 2007-05-14 09:31:33 PDT
Comment on attachment 264740 [details] [diff] [review]
New patch

I think this should be sufficient. r=kengert for trunk

Have you tested this patch yourself and tried to connect to some SSL sites?
Comment 5 Mike Hommey [:glandium] 2007-05-14 11:37:06 PDT
not yet for the latest patch
Comment 6 Mike Hommey [:glandium] 2007-05-26 11:32:40 PDT
I tested it on a couple of SSL sites. No problem encountered. Could this be landed ?
Comment 7 Kai Engert (:kaie) 2007-05-29 18:16:09 PDT
Patch checked in to trunk.
Comment 8 alanjstr 2007-06-25 09:41:42 PDT
Do we have a bug open to track this for Branch?
Comment 9 Kai Engert (:kaie) 2007-06-27 06:24:36 PDT
(In reply to comment #8)
> Do we have a bug open to track this for Branch?

No separate bug needed.
I requested approval for the patch for the stable branch.


(Removing request to block 1.9, as 1.9 already has it)
Comment 10 Daniel Veditz [:dveditz] 2007-06-28 12:09:01 PDT
Comment on attachment 264740 [details] [diff] [review]
New patch

approved for 1.8.1.5, a=dveditz for release-drivers
Comment 11 Kai Engert (:kaie) 2007-07-06 18:50:20 PDT
Fixed for 1.8.1.5

I'd like to encourage people on the cc list to test tomorrow's nightly 1.8 branch build and confirm that SSL is still working as expected, just to have this tested some more. Thanks.
Comment 12 pbrobinson 2007-09-21 02:56:43 PDT
With the last 3 releases of Firefox being Fire Drill releases was this actually fixed in 1.8.1.5 or has it been pushed back?
Comment 13 Kai Engert (:kaie) 2007-09-21 03:11:51 PDT
This was added as of Firefox 2.0.0.5 == 1.8.1.5
Comment 14 chris hofmann 2007-12-07 04:07:42 PST
hey mike, cool tool!  

there is another interesting test case over in 407325 that might be interesting to  figure out a solution to...
Comment 15 Dhaval Giani 2008-01-17 06:40:27 PST
i've been following the case in bug 407325, which is still happening even though firefox is minimized, or a non ajax page is in the selected tab. the wakeups jump to over 150 with firefox being responsible for 1/3 of the wakeups on the whole system.

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