Closed
Bug 380558
Opened 17 years ago
Closed 17 years ago
Some event waiting in PSM make the CPU wake from idle with no reason
Categories
(Core :: Security: PSM, enhancement)
Tracking
()
RESOLVED
FIXED
People
(Reporter: glandium, Assigned: glandium)
References
()
Details
(Keywords: fixed1.8.1.5)
Attachments
(1 file, 2 obsolete files)
2.77 KB,
patch
|
KaiE
:
review+
dveditz
:
approval1.8.1.5+
|
Details | Diff | Splinter Review |
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.
Attachment #264666 -
Flags: review?(darin.moz)
Assignee | ||
Comment 1•17 years ago
|
||
Same patch, with correct paths.
Assignee: kengert → mh+mozilla
Attachment #264666 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #264719 -
Flags: review?(darin.moz)
Attachment #264666 -
Flags: review?(darin.moz)
Comment 2•17 years ago
|
||
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.
Attachment #264719 -
Flags: review?(darin.moz) → review-
Assignee | ||
Comment 3•17 years ago
|
||
(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.
Attachment #264719 -
Attachment is obsolete: true
Attachment #264740 -
Flags: review?(kengert)
Comment 4•17 years ago
|
||
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?
Attachment #264740 -
Flags: review?(kengert) → review+
Assignee | ||
Comment 5•17 years ago
|
||
not yet for the latest patch
Updated•17 years ago
|
Flags: blocking1.9?
Assignee | ||
Comment 6•17 years ago
|
||
I tested it on a couple of SSL sites. No problem encountered. Could this be landed ?
Whiteboard: [checkin-wanted]
Updated•17 years ago
|
Whiteboard: [checkin-wanted] → [checkin needed]
Comment 7•17 years ago
|
||
Patch checked in to trunk.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Updated•17 years ago
|
Whiteboard: [checkin needed]
Updated•17 years ago
|
Attachment #264740 -
Flags: approval1.8.1.5?
Comment 9•17 years ago
|
||
(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)
Flags: blocking1.9?
Comment 10•17 years ago
|
||
Comment on attachment 264740 [details] [diff] [review] New patch approved for 1.8.1.5, a=dveditz for release-drivers
Attachment #264740 -
Flags: approval1.8.1.5? → approval1.8.1.5+
Updated•17 years ago
|
Whiteboard: [checkin needed 1.8 branch]
Updated•17 years ago
|
Keywords: checkin-needed
Comment 11•17 years ago
|
||
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.
Keywords: checkin-needed → fixed1.8.1.5
Whiteboard: [checkin needed 1.8 branch]
Comment 12•17 years ago
|
||
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•17 years ago
|
||
This was added as of Firefox 2.0.0.5 == 1.8.1.5
Comment 14•17 years ago
|
||
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•16 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•