linux-kernel - Re: [RFC v3 1/3] PM/runtime: Add a new interface to get accounted time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAPDyKFqwB4qPAjhgSGkZmwyzd38AiFzkFVpqcea6K7Sm7BWcfQ@mail.gmail.com>
Date:   Thu, 20 Dec 2018 10:40:32 +0100
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Vincent Guittot <vincent.guittot@...aro.org>
Cc:     "open list:THERMAL" <linux-pm@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Thara Gopinath <thara.gopinath@...aro.org>,
        jani.nikula@...ux.intel.com,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        rodrigo.vivi@...el.com, David Airlie <airlied@...ux.ie>,
        "Intel graphics driver community testing & development" 
        <intel-gfx@...ts.freedesktop.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>
Subject: Re: [RFC v3 1/3] PM/runtime: Add a new interface to get accounted time

[...]

> > > > Re-thinking this a bit from my earlier comments - and by following the
> > > > above reasoning, it sounds like this better belongs in the
> > > > driver/subsystem, without requiring any data from the core.
> > > >
> > > > The driver/subsystem could just store a timestamp in it's
> > > > ->runtime_suspend() callback and whenever needed, it could compute a
> > > > delta towards it. That should work, right?
> > >
> > > I don't know i915/drm enough to know all that details
> >
> > Okay, so let me re-summarize the main issue I see with your approach
> > in $subject patch.
> >
> > dev->power.accounting_timestamp can't be used to know when last
> > transition was made. If I understand correctly, that is how you use
> > it. No?
>
> Yes. At least that how I have interpreted the current code
>
> >
> > Anyway, as stated, that's because the timestamp becomes updated, if
> > update_pm_runtime_accounting() is called via the sysfs nobs, which
> > means there is no state transition happening, but only accounting data
> > is updated.
>
> Yes I have not realized that the update also happens there which makes
> me think that i have
> may be over interpreted the code and the initialization of
> i915->pmu.suspended_jiffies_last
>
> >
> > So, what I think we can do from the core perspective, if it helps
> > (which I am not sure of):
> > 1. Export a function, which returns the value of dev->power.suspended_jiffies.
> > 2. Export a wrapper function (to deal with locking) which calls
> > update_pm_runtime_accounting(). This wrapper function allows the user
> > the update the total suspended time, also taking into account the time
> > spent in the current state.
>
> Having now in mind that suspended_jiffies can be updated outside state
> transition like via sysfs call,
> we can maybe just implements 2 and return dev->power.suspended_jiffies
>
> something like below
> unsigned long pm_runtime_get_suspended_time(struct device *dev)

"pm_runtime_suspended_time()" should be sufficient I think. The "get"
part would become confusing due to the existing get/put functions that
are part of the runtime PM interface.

> {
> unsigned long time;
> unsigned long flags;
>
> spin_lock_irqsave(&dev->power.lock, flags);
>
> update_pm_runtime_accounting(dev);
>
> time = dev->power.suspended_time;

dev->power.suspended_jiffies

...at least until you converts to ktime :-)

>
> spin_unlock_irqrestore(&dev->power.lock, flags);
>
> return time;
> }

Yes, this looks fine to me!

Kind regards
Uffe