Message ID | 1545315426-14545-1-git-send-email-vincent.guittot@linaro.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Move pm_runtime accounted time to raw nsec | expand |
On Thu, 20 Dec 2018 at 15:17, Vincent Guittot <vincent.guittot@linaro.org> wrote: > > From: Thara Gopinath <thara.gopinath@linaro.org> > > This patch replaces jiffies based accounting for runtime_active_time > and runtime_suspended_time with ktime base accounting. This makes the > runtime debug counters inline with genpd and other pm subsytems which > uses ktime based accounting. > > Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> > [move from ktime to raw nsec] > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> > --- > drivers/base/power/runtime.c | 17 +++++++++-------- > drivers/base/power/sysfs.c | 11 ++++++++--- > include/linux/pm.h | 6 +++--- > 3 files changed, 20 insertions(+), 14 deletions(-) > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > index e695544..f700524 100644 > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -64,8 +64,8 @@ static int rpm_suspend(struct device *dev, int rpmflags); > */ > void update_pm_runtime_accounting(struct device *dev) > { > - unsigned long now = jiffies; > - unsigned long delta; > + u64 now = ktime_to_ns(ktime_get()); > + u64 delta; > > delta = now - dev->power.accounting_timestamp; > > @@ -75,9 +75,9 @@ void update_pm_runtime_accounting(struct device *dev) > return; > > if (dev->power.runtime_status == RPM_SUSPENDED) > - dev->power.suspended_jiffies += delta; > + dev->power.suspended_time += delta; > else > - dev->power.active_jiffies += delta; > + dev->power.active_time += delta; > } > > static void __update_runtime_status(struct device *dev, enum rpm_status status) > @@ -88,17 +88,18 @@ static void __update_runtime_status(struct device *dev, enum rpm_status status) > > u64 pm_runtime_suspended_time(struct device *dev) > { > - unsigned long flags, time; > + u64 time; > + unsigned long flags; > > spin_lock_irqsave(&dev->power.lock, flags); > > update_pm_runtime_accounting(dev); > > - time = dev->power.suspended_jiffies; > + time = dev->power.suspended_time; > > spin_unlock_irqrestore(&dev->power.lock, flags); > > - return jiffies_to_nsecs(time); > + return time; > } > EXPORT_SYMBOL_GPL(pm_runtime_suspended_time); > > @@ -1503,7 +1504,7 @@ void pm_runtime_init(struct device *dev) > dev->power.request_pending = false; > dev->power.request = RPM_REQ_NONE; > dev->power.deferred_resume = false; > - dev->power.accounting_timestamp = jiffies; > + dev->power.accounting_timestamp = ktime_to_ns(ktime_get()); This change worries me a bit, but it may not be a problem in practice. pm_runtime_init() is called when devices get initialized, via device_initialize(). If timekeeping has not been initialized prior a call to ktime_get() is done, it will fail and causing a NULL pointer deference or something along those lines. In other words, do we know that device_initialize() is always called after timekeeping has been initialized during boot? [...] Kind regards Uffe
On Thu, 20 Dec 2018 at 23:03, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Thu, 20 Dec 2018 at 15:17, Vincent Guittot > <vincent.guittot@linaro.org> wrote: > > > > From: Thara Gopinath <thara.gopinath@linaro.org> > > > > This patch replaces jiffies based accounting for runtime_active_time > > and runtime_suspended_time with ktime base accounting. This makes the > > runtime debug counters inline with genpd and other pm subsytems which > > uses ktime based accounting. > > > > Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> > > [move from ktime to raw nsec] > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> > > --- > > drivers/base/power/runtime.c | 17 +++++++++-------- > > drivers/base/power/sysfs.c | 11 ++++++++--- > > include/linux/pm.h | 6 +++--- > > 3 files changed, 20 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > > index e695544..f700524 100644 > > --- a/drivers/base/power/runtime.c > > +++ b/drivers/base/power/runtime.c > > @@ -64,8 +64,8 @@ static int rpm_suspend(struct device *dev, int rpmflags); > > */ > > void update_pm_runtime_accounting(struct device *dev) > > { > > - unsigned long now = jiffies; > > - unsigned long delta; > > + u64 now = ktime_to_ns(ktime_get()); > > + u64 delta; > > > > delta = now - dev->power.accounting_timestamp; > > > > @@ -75,9 +75,9 @@ void update_pm_runtime_accounting(struct device *dev) > > return; > > > > if (dev->power.runtime_status == RPM_SUSPENDED) > > - dev->power.suspended_jiffies += delta; > > + dev->power.suspended_time += delta; > > else > > - dev->power.active_jiffies += delta; > > + dev->power.active_time += delta; > > } > > > > static void __update_runtime_status(struct device *dev, enum rpm_status status) > > @@ -88,17 +88,18 @@ static void __update_runtime_status(struct device *dev, enum rpm_status status) > > > > u64 pm_runtime_suspended_time(struct device *dev) > > { > > - unsigned long flags, time; > > + u64 time; > > + unsigned long flags; > > > > spin_lock_irqsave(&dev->power.lock, flags); > > > > update_pm_runtime_accounting(dev); > > > > - time = dev->power.suspended_jiffies; > > + time = dev->power.suspended_time; > > > > spin_unlock_irqrestore(&dev->power.lock, flags); > > > > - return jiffies_to_nsecs(time); > > + return time; > > } > > EXPORT_SYMBOL_GPL(pm_runtime_suspended_time); > > > > @@ -1503,7 +1504,7 @@ void pm_runtime_init(struct device *dev) > > dev->power.request_pending = false; > > dev->power.request = RPM_REQ_NONE; > > dev->power.deferred_resume = false; > > - dev->power.accounting_timestamp = jiffies; > > + dev->power.accounting_timestamp = ktime_to_ns(ktime_get()); > > This change worries me a bit, but it may not be a problem in practice. > > pm_runtime_init() is called when devices get initialized, via > device_initialize(). If timekeeping has not been initialized prior a > call to ktime_get() is done, it will fail and causing a NULL pointer > deference or something along those lines. > > In other words, do we know that device_initialize() is always called > after timekeeping has been initialized during boot? That something we discussed at lpc and we should be safe > > [...] > > Kind regards > Uffe
On Thu, Dec 20, 2018 at 11:03 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Thu, 20 Dec 2018 at 15:17, Vincent Guittot > <vincent.guittot@linaro.org> wrote: > > > > From: Thara Gopinath <thara.gopinath@linaro.org> > > > > This patch replaces jiffies based accounting for runtime_active_time > > and runtime_suspended_time with ktime base accounting. This makes the > > runtime debug counters inline with genpd and other pm subsytems which > > uses ktime based accounting. > > > > Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> > > [move from ktime to raw nsec] > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> > > --- > > drivers/base/power/runtime.c | 17 +++++++++-------- > > drivers/base/power/sysfs.c | 11 ++++++++--- > > include/linux/pm.h | 6 +++--- > > 3 files changed, 20 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > > index e695544..f700524 100644 > > --- a/drivers/base/power/runtime.c > > +++ b/drivers/base/power/runtime.c > > @@ -64,8 +64,8 @@ static int rpm_suspend(struct device *dev, int rpmflags); > > */ > > void update_pm_runtime_accounting(struct device *dev) > > { > > - unsigned long now = jiffies; > > - unsigned long delta; > > + u64 now = ktime_to_ns(ktime_get()); > > + u64 delta; > > > > delta = now - dev->power.accounting_timestamp; > > > > @@ -75,9 +75,9 @@ void update_pm_runtime_accounting(struct device *dev) > > return; > > > > if (dev->power.runtime_status == RPM_SUSPENDED) > > - dev->power.suspended_jiffies += delta; > > + dev->power.suspended_time += delta; > > else > > - dev->power.active_jiffies += delta; > > + dev->power.active_time += delta; > > } > > > > static void __update_runtime_status(struct device *dev, enum rpm_status status) > > @@ -88,17 +88,18 @@ static void __update_runtime_status(struct device *dev, enum rpm_status status) > > > > u64 pm_runtime_suspended_time(struct device *dev) > > { > > - unsigned long flags, time; > > + u64 time; > > + unsigned long flags; > > > > spin_lock_irqsave(&dev->power.lock, flags); > > > > update_pm_runtime_accounting(dev); > > > > - time = dev->power.suspended_jiffies; > > + time = dev->power.suspended_time; > > > > spin_unlock_irqrestore(&dev->power.lock, flags); > > > > - return jiffies_to_nsecs(time); > > + return time; > > } > > EXPORT_SYMBOL_GPL(pm_runtime_suspended_time); > > > > @@ -1503,7 +1504,7 @@ void pm_runtime_init(struct device *dev) > > dev->power.request_pending = false; > > dev->power.request = RPM_REQ_NONE; > > dev->power.deferred_resume = false; > > - dev->power.accounting_timestamp = jiffies; > > + dev->power.accounting_timestamp = ktime_to_ns(ktime_get()); > > This change worries me a bit, but it may not be a problem in practice. > > pm_runtime_init() is called when devices get initialized, via > device_initialize(). If timekeeping has not been initialized prior a > call to ktime_get() is done, it will fail and causing a NULL pointer > deference or something along those lines. > > In other words, do we know that device_initialize() is always called > after timekeeping has been initialized during boot? We do. timekeeping_init() is called early in start_kernel() which is way before driver_init() (and that's when devices can start to be initialized) called from rest_init() via kernel_init_freeable() and do_basic_setup().
Hi Rafael, On Fri, 21 Dec 2018 at 09:58, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Dec 20, 2018 at 11:03 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Thu, 20 Dec 2018 at 15:17, Vincent Guittot > > <vincent.guittot@linaro.org> wrote: > > > > > > From: Thara Gopinath <thara.gopinath@linaro.org> > > > > > > This patch replaces jiffies based accounting for runtime_active_time > > > and runtime_suspended_time with ktime base accounting. This makes the > > > runtime debug counters inline with genpd and other pm subsytems which > > > uses ktime based accounting. > > > > > > Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> > > > [move from ktime to raw nsec] > > > Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> > > > --- > > > drivers/base/power/runtime.c | 17 +++++++++-------- > > > drivers/base/power/sysfs.c | 11 ++++++++--- > > > include/linux/pm.h | 6 +++--- > > > 3 files changed, 20 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > > > index e695544..f700524 100644 > > > --- a/drivers/base/power/runtime.c > > > +++ b/drivers/base/power/runtime.c > > > @@ -64,8 +64,8 @@ static int rpm_suspend(struct device *dev, int rpmflags); > > > */ > > > void update_pm_runtime_accounting(struct device *dev) > > > { > > > - unsigned long now = jiffies; > > > - unsigned long delta; > > > + u64 now = ktime_to_ns(ktime_get()); > > > + u64 delta; > > > > > > delta = now - dev->power.accounting_timestamp; > > > > > > @@ -75,9 +75,9 @@ void update_pm_runtime_accounting(struct device *dev) > > > return; > > > > > > if (dev->power.runtime_status == RPM_SUSPENDED) > > > - dev->power.suspended_jiffies += delta; > > > + dev->power.suspended_time += delta; > > > else > > > - dev->power.active_jiffies += delta; > > > + dev->power.active_time += delta; > > > } > > > > > > static void __update_runtime_status(struct device *dev, enum rpm_status status) > > > @@ -88,17 +88,18 @@ static void __update_runtime_status(struct device *dev, enum rpm_status status) > > > > > > u64 pm_runtime_suspended_time(struct device *dev) > > > { > > > - unsigned long flags, time; > > > + u64 time; > > > + unsigned long flags; > > > > > > spin_lock_irqsave(&dev->power.lock, flags); > > > > > > update_pm_runtime_accounting(dev); > > > > > > - time = dev->power.suspended_jiffies; > > > + time = dev->power.suspended_time; > > > > > > spin_unlock_irqrestore(&dev->power.lock, flags); > > > > > > - return jiffies_to_nsecs(time); > > > + return time; > > > } > > > EXPORT_SYMBOL_GPL(pm_runtime_suspended_time); > > > > > > @@ -1503,7 +1504,7 @@ void pm_runtime_init(struct device *dev) > > > dev->power.request_pending = false; > > > dev->power.request = RPM_REQ_NONE; > > > dev->power.deferred_resume = false; > > > - dev->power.accounting_timestamp = jiffies; > > > + dev->power.accounting_timestamp = ktime_to_ns(ktime_get()); > > > > This change worries me a bit, but it may not be a problem in practice. > > > > pm_runtime_init() is called when devices get initialized, via > > device_initialize(). If timekeeping has not been initialized prior a > > call to ktime_get() is done, it will fail and causing a NULL pointer > > deference or something along those lines. > > > > In other words, do we know that device_initialize() is always called > > after timekeeping has been initialized during boot? > > We do. > > timekeeping_init() is called early in start_kernel() which is way > before driver_init() (and that's when devices can start to be > initialized) called from rest_init() via kernel_init_freeable() and > do_basic_setup(). Thanks for the confirmation. I'm going to add your answer in the commit message so we will have the answer next time someone will wonder
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index e695544..f700524 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -64,8 +64,8 @@ static int rpm_suspend(struct device *dev, int rpmflags); */ void update_pm_runtime_accounting(struct device *dev) { - unsigned long now = jiffies; - unsigned long delta; + u64 now = ktime_to_ns(ktime_get()); + u64 delta; delta = now - dev->power.accounting_timestamp; @@ -75,9 +75,9 @@ void update_pm_runtime_accounting(struct device *dev) return; if (dev->power.runtime_status == RPM_SUSPENDED) - dev->power.suspended_jiffies += delta; + dev->power.suspended_time += delta; else - dev->power.active_jiffies += delta; + dev->power.active_time += delta; } static void __update_runtime_status(struct device *dev, enum rpm_status status) @@ -88,17 +88,18 @@ static void __update_runtime_status(struct device *dev, enum rpm_status status) u64 pm_runtime_suspended_time(struct device *dev) { - unsigned long flags, time; + u64 time; + unsigned long flags; spin_lock_irqsave(&dev->power.lock, flags); update_pm_runtime_accounting(dev); - time = dev->power.suspended_jiffies; + time = dev->power.suspended_time; spin_unlock_irqrestore(&dev->power.lock, flags); - return jiffies_to_nsecs(time); + return time; } EXPORT_SYMBOL_GPL(pm_runtime_suspended_time); @@ -1503,7 +1504,7 @@ void pm_runtime_init(struct device *dev) dev->power.request_pending = false; dev->power.request = RPM_REQ_NONE; dev->power.deferred_resume = false; - dev->power.accounting_timestamp = jiffies; + dev->power.accounting_timestamp = ktime_to_ns(ktime_get()); INIT_WORK(&dev->power.work, pm_runtime_work); dev->power.timer_expires = 0; diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c index d713738..96c8a22 100644 --- a/drivers/base/power/sysfs.c +++ b/drivers/base/power/sysfs.c @@ -125,9 +125,12 @@ static ssize_t runtime_active_time_show(struct device *dev, struct device_attribute *attr, char *buf) { int ret; + u64 tmp; spin_lock_irq(&dev->power.lock); update_pm_runtime_accounting(dev); - ret = sprintf(buf, "%i\n", jiffies_to_msecs(dev->power.active_jiffies)); + tmp = dev->power.active_time; + do_div(tmp, NSEC_PER_MSEC); + ret = sprintf(buf, "%llu\n", tmp); spin_unlock_irq(&dev->power.lock); return ret; } @@ -138,10 +141,12 @@ static ssize_t runtime_suspended_time_show(struct device *dev, struct device_attribute *attr, char *buf) { int ret; + u64 tmp; spin_lock_irq(&dev->power.lock); update_pm_runtime_accounting(dev); - ret = sprintf(buf, "%i\n", - jiffies_to_msecs(dev->power.suspended_jiffies)); + tmp = dev->power.suspended_time; + do_div(tmp, NSEC_PER_MSEC); + ret = sprintf(buf, "%llu\n", tmp); spin_unlock_irq(&dev->power.lock); return ret; } diff --git a/include/linux/pm.h b/include/linux/pm.h index e723b78..e5a34e2 100644 --- a/include/linux/pm.h +++ b/include/linux/pm.h @@ -632,9 +632,9 @@ struct dev_pm_info { int runtime_error; int autosuspend_delay; unsigned long last_busy; - unsigned long active_jiffies; - unsigned long suspended_jiffies; - unsigned long accounting_timestamp; + u64 active_time; + u64 suspended_time; + u64 accounting_timestamp; #endif struct pm_subsys_data *subsys_data; /* Owned by the subsystem. */ void (*set_latency_tolerance)(struct device *, s32);