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)

x86
Linux
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: glandium, Assigned: glandium)

References

()

Details

(Keywords: fixed1.8.1.5)

Attachments

(1 file, 2 obsolete files)

Attached patch Patch against 1.8 branch (obsolete) — 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)
Attached patch Patch against 1.8 branch (obsolete) — Splinter Review
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 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-
Attached patch New patchSplinter Review
(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 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+
not yet for the latest patch
Flags: blocking1.9?
I tested it on a couple of SSL sites. No problem encountered. Could this be landed ?
Whiteboard: [checkin-wanted]
Whiteboard: [checkin-wanted] → [checkin needed]
Patch checked in to trunk.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Do we have a bug open to track this for Branch?
Attachment #264740 - Flags: approval1.8.1.5?
(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+
Whiteboard: [checkin needed 1.8 branch]
Keywords: checkin-needed
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.
Whiteboard: [checkin needed 1.8 branch]
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?
This was added as of Firefox 2.0.0.5 == 1.8.1.5
hey mike, cool tool!  

there is another interesting test case over in 407325 that might be interesting to  figure out a solution to...
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.