Some event waiting in PSM make the CPU wake from idle with no reason

RESOLVED FIXED

Status

()

Core
Security: PSM
--
enhancement
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

({fixed1.8.1.5})

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

10 years ago
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.
Attachment #264666 - Flags: review?(darin.moz)
(Assignee)

Comment 1

10 years ago
Created attachment 264719 [details] [diff] [review]
Patch against 1.8 branch

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

10 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

10 years ago
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.
Attachment #264719 - Attachment is obsolete: true
Attachment #264740 - Flags: review?(kengert)

Comment 4

10 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

10 years ago
not yet for the latest patch

Updated

10 years ago
Flags: blocking1.9?
(Assignee)

Comment 6

10 years ago
I tested it on a couple of SSL sites. No problem encountered. Could this be landed ?
Whiteboard: [checkin-wanted]
Whiteboard: [checkin-wanted] → [checkin needed]

Comment 7

10 years ago
Patch checked in to trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]

Comment 8

10 years ago
Do we have a bug open to track this for Branch?

Updated

10 years ago
Attachment #264740 - Flags: approval1.8.1.5?

Comment 9

10 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 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

10 years ago
Whiteboard: [checkin needed 1.8 branch]

Updated

10 years ago
Keywords: checkin-needed

Comment 11

10 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

10 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

10 years ago
This was added as of Firefox 2.0.0.5 == 1.8.1.5

Comment 14

10 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

10 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.