Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: kHbHi/ij+rm0+8UiKEMWdIXa6mdGl8N/qYYNbiFjKOIbWyKl76qKwQIYBfNoa6c9qeSTqm438f
 5a+J5gtkdMjg==
IronPort-SDR: D4dNIC/fQGk/CW5xPPs6mtaEpw7hfoyc4R0kcaR1TF0bej7gaiSSB/rAERDvfOrFriMuk4a+Pu
 bSJtDUyApLtg==
Date:   Sat, 6 Jun 2020 15:14:59 +0800
From:   Chen Yu <yu.c.chen@intel.com>
To:     Michal Miroslaw <mirq-linux@rere.qmqm.pl>
Cc:     "Rafael J . Wysocki" <rafael@kernel.org>,
        Len Brown <len.brown@intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2][RFC] PM-runtime: add tracepoints to cover all
 usage_count changes
Message-ID: <20200606071459.GA1298@chenyu-office.sh.intel.com>
References: <cover.1591380524.git.yu.c.chen@intel.com>
 <6ce5c2d21758363b7c9a31187eda1787bc4a6160.1591380524.git.yu.c.chen@intel.com>
 <20200605193311.GB9553@qmqm.qmqm.pl>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200605193311.GB9553@qmqm.qmqm.pl>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,
On Fri, Jun 05, 2020 at 09:33:11PM +0200, Michal Miroslaw wrote:
> On Sat, Jun 06, 2020 at 03:05:52AM +0800, Chen Yu wrote:
> > Commit d229290689ae ("PM-runtime: add tracepoints for usage_count changes")
> > has added some tracepoints to monitor the change of runtime usage, and
> > there is something to improve:
> > 1. There are some places that adjust the usage count have not
> >    been traced yet. For example, pm_runtime_get_noresume() and
> >    pm_runtime_put_noidle()
> > 2. The change of the usage count will not be tracked if decreased
> >    from 1 to 0.
> [...]
> > @@ -1448,16 +1453,17 @@ EXPORT_SYMBOL_GPL(pm_runtime_forbid);
> >   */
> >  void pm_runtime_allow(struct device *dev)
> >  {
> > +	bool is_zero;
> > +
> >  	spin_lock_irq(&dev->power.lock);
> >  	if (dev->power.runtime_auto)
> >  		goto out;
> >  
> >  	dev->power.runtime_auto = true;
> > -	if (atomic_dec_and_test(&dev->power.usage_count))
> > +	is_zero = atomic_dec_and_test(&dev->power.usage_count);
> > +	trace_rpm_usage_rcuidle(dev, RPM_AUTO | RPM_ASYNC);
> > +	if (is_zero)
> >  		rpm_idle(dev, RPM_AUTO | RPM_ASYNC);
> > -	else
> > -		trace_rpm_usage_rcuidle(dev, RPM_AUTO | RPM_ASYNC);
> > -
> [...]
> 
> IIRC, rpm_idle() has a tracepoint already.
> 
Yes, this is what I concerned previously. If someone
want to track the change of usage_count, then he might
have to enable both trace rpm_usage and rpm_idle so
as to track the moment when the counter drops from 1 to
0. It might be more consistent if we only enable
trace rpm_usage to track the whole process.
> > @@ -1523,9 +1529,8 @@ static void update_autosuspend(struct device *dev, int old_delay, int old_use)
> >  		/* If it used to be allowed then prevent it. */
> >  		if (!old_use || old_delay >= 0) {
> >  			atomic_inc(&dev->power.usage_count);
> > -			rpm_resume(dev, 0);
> > -		} else {
> >  			trace_rpm_usage_rcuidle(dev, 0);
> > +			rpm_resume(dev, 0);
> >  		}
> >  	}
> [...]
> 
> This actually changes logic, so it doesn't match the patch description.
> 
This patch intends to adjust the logic to be consistent with
the change of usage_counter, that is to say, only after the
counter has been possibly modified, we record it. In current
logic above, it tracks the usage count where the latter does
not change.

Thanks,
Chenyu
> Best Regards
> Michał Mirosław