New issue
Advanced search Search tips

Issue 797372 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Feature



Sign in to add a comment

Disabling pm_print_times requires insider knowledge, remount / RW and manually hacking two scripts

Issue description

UserAgent: Mozilla/5.0 (X11; CrOS x86_64 10176.7.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.14 Safari/537.36
Platform: Any

Steps to reproduce the problem:
[Discussion (re-)started in https://chromium-review.googlesource.com/c/chromiumos/platform/power_manager/+/169934, see more context there]

A typical suspend/resume cycle generates about 800 lines of kernel logs, 700 caused by pm_print_times alone. Switching to a more traditional level of logging fitting on one screen requires an inordinate effort:

- Find about /sys/power/pm_print_times. Try a naive echo 0 > /sys/power/pm_print_times, fail.
- Find that there is relevant code in suspend_stress_test. Comment it out, fail.
- Find that there relevant code in powerd_suspend. Comment it out, success at last?

While the value of having logs that verbose turned on by default could be debated, this request is ONLY about providing *developers* using suspend_stress_test and other tests  more transparency and control, for instance an option like:

  suspend_stress_test --nopm_times

What is the expected behavior?

What went wrong?
Verbose logs that can't be disabled

Did this work before? Yes 2012

Chrome version: 64.0.3282.14  Channel: dev
OS Version: 10176.7.0
Flash Version: N/A
 

Comment 1 by vsu...@google.com, Dec 22 2017

Components: OS>Kernel>Power

Comment 2 by derat@chromium.org, Dec 23 2017

Cc: jwer...@chromium.org dbasehore@chromium.org tbroch@chromium.org snanda@chromium.org diand...@chromium.org
Status: Untriaged (was: Unconfirmed)
Cc: coconutruben@chromium.org
Seems like if we remove this,

  if [ -e /sys/power/pm_print_times ]; then
    printf '1' > /sys/power/pm_print_times
  fi

from powerd_suspend then suspend_stress_test will preserve the setting accordingly,

  echo "${preserved_pm_print_times}" > /sys/power/pm_print_times

and then developers can just manage it via the sysfs param.

Remaining question ... should pm_print_times be default of '1' or '0' after boot by default?  Currently its '0'.  If folks feel strongly it should be '1' then whats the best mechanism to change (CHROMIUM-only kernel change, udev, powerd.conf upstart).




Owner: tbroch@chromium.org
Status: Assigned (was: Untriaged)
> Seems like if we remove this,
> 
>  if [ -e /sys/power/pm_print_times ]; then
>    printf '1' > /sys/power/pm_print_times
>  fi

> ... from powerd_suspend then suspend_stress_test will preserve the setting accordingly,

+1 Verified


> CHROMIUM-only kernel change, udev, powerd.conf upstart ?

https://chromium-review.googlesource.com/c/chromiumos/platform2/+/209963/10/power_manager/init/powerd.conf

echo 5 > /sys/power/pm_test_delay

As mentioned in the CL, I believe there's a decent amount of value for "pm_print_times" being turned on for end users so that they show up in feedback reports.

I'm OK with it somehow being defaulted to off for developers or if developers simply have the ability to turn it off.
Thx Todd for this working code (under review)
https://chromium-review.googlesource.com/c/chromiumos/platform2/+/855644
Project Member

Comment 9 by bugdroid1@chromium.org, Jan 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/0cf581b8f8034eb3bb4c09fca9eb47558d35978d

commit 0cf581b8f8034eb3bb4c09fca9eb47558d35978d
Author: Todd Broch <tbroch@chromium.org>
Date: Fri Jan 19 01:40:20 2018

power: suspend_stress_test: add flag for pm_print_times.

pm_print_times is enabled always during suspend stress test.
This results in many prints to the console which might not be
required sometimes. So add a flag 'pm_print_times' which is by
default enabled. Now pm_print_times can be disabled by adding
--nopm_print_times whenever required.

BUG=b:35531852,  chromium:797372 
TEST=Run suspend stress test with the following option and check
the console.
suspend_stress_test --nopm_print_times

Change-Id: I9fe7c17dc219b8218157ba5837156e5952051bee
Reviewed-on: https://chromium-review.googlesource.com/857596
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Todd Broch <tbroch@chromium.org>
Reviewed-by: Marc Herbert <marc.herbert@intel.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/0cf581b8f8034eb3bb4c09fca9eb47558d35978d/power_manager/tools/suspend_stress_test

Project Member

Comment 10 by bugdroid1@chromium.org, Jan 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/2ba62e3df289ac3264d31d1e4a51eb3dd1c73922

commit 2ba62e3df289ac3264d31d1e4a51eb3dd1c73922
Author: Todd Broch <tbroch@chromium.org>
Date: Fri Jan 19 01:40:20 2018

power: Enable pm_print_times once at boot.

Enabling pm_print_times should be managed outside powerd_suspend such
that stress testing can avoid the excess log lines if desired.

BUG= chromium:797372 
TEST=manual,
1. boot system
2. cat /sys/power/pm_print_times see '1'
3. echo 0 > /sys/power/pm_print_times
4. powerd_dbus_suspend
5. wake device
6. cat /sys/power/pm_print_times see '0'

no longer see verbose resume timings like
  [ 3222.957070] call kbl_r5514_5663_max+ returned 0 after 8 usecs

Change-Id: Iada5d52b93346803ee710caf44a3d645d00745fb
Reviewed-on: https://chromium-review.googlesource.com/865933
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Todd Broch <tbroch@chromium.org>
Reviewed-by: Marc Herbert <marc.herbert@intel.com>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/2ba62e3df289ac3264d31d1e4a51eb3dd1c73922/power_manager/init/shared/powerd-pre-start.sh
[modify] https://crrev.com/2ba62e3df289ac3264d31d1e4a51eb3dd1c73922/power_manager/powerd/powerd_suspend

Status: Fixed (was: Assigned)

Sign in to add a comment