From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [PATCH] PM / runtime: Use synchronous runtime PM call in rpm_put_suppliers() To: Ulf Hansson , "Rafael J. Wysocki" Cc: linux-clk , "linux-pm@vger.kernel.org" , "Rafael J. Wysocki" , Bartlomiej Zolnierkiewicz From: Marek Szyprowski Message-id: <55b66219-e210-fe27-441c-177125afc3e0@samsung.com> Date: Mon, 27 Mar 2017 12:14:12 +0200 MIME-version: 1.0 In-reply-to: Content-type: text/plain; charset=utf-8; format=flowed References: <1490174570-18345-1-git-send-email-m.szyprowski@samsung.com> Sender: linux-pm-owner@vger.kernel.org List-ID: Hi Ulf, On 2017-03-23 13:47, Ulf Hansson wrote: > On 22 March 2017 at 21:56, Rafael J. Wysocki wrote: >> On Wed, Mar 22, 2017 at 12:34 PM, Ulf Hansson wrote: >>> On 22 March 2017 at 10:22, Marek Szyprowski wrote: >>>> rpm_put_suppliers() might be called as a result of pm_runtime_put_sync() >>>> call, where the called explicitly wants to perform the operation in >>>> synchronous way to avoid some deadlocks related to concurrent code >>>> execution in the PM workers. However the current code for handling device >>>> links always use asynchronous calls. This patch changes it to always use >>>> the synchronous calls. >>> This is a good idea! It is better to leave the decision to the "leaf" >>> node of whether to use asynchronous mode or not. >> One immediate concern that I have is that this happens in an SRCU >> read-side critical section and sync runtime suspend can take arbitrary >> amount of time, especially if those devices have suppliers etc. You >> may very well wait for an entire subtree to suspend synchronously >> here. > Yes that's a problem. Can we address that somehow? Just a stupid question: why this is issue in case of rpm_put_suppliers() and call to pm_runtime_put_sync(), but it is okay in rpm_get_suppliers() and pm_runtime_get_sync(), which can also take a long time to complete? >> Moreover, if the suppliers have parents, those parents won't be >> suspended synchronously anyway, so this only addresses the first level >> of hierarchy in that case. >> >> Also the behavior for suppliers follows the behavior for parents and I >> don't quite see the reason for these two cases to behave differently >> in principle. > Yes, you are right! > > We shouldn't change this for suppliers unless we also change this for parents. Right, this can also lead to the deadlock similar to the one observed with links. Once again, it looks that if caller wants a synchronous operation, it should be propagated to parents also. >>>> This patch fixes the potential deadlock, which appears during adding >>>> runtime PM support to clock framework, which uses global mutex, which is >>>> reentrant only for the current process, so trying to execute runtime PM >>>> callback from the worker results in deadlock (the mentioned mutex is >>>> already taken by the current process, which waits for execution of PM >>>> worker). >>> This just tells that the locking mechanism for clocks are fragile. >> Right. >> >> We seem to be working around problems in there with this patch. >> >>> Whether it is a good motivation for this change, I am not sure. >>> >>> In either case, I like the change! >> Why exactly do you like it? > Okay, let me try to elaborate. > > For the async runtime PM suspend/idle case (including runtime PM autosuspend): > When a driver for a children/consumer decides to use the async runtime > PM APIs, I assume it's because the driver expects the device to be > used in some kind of a "bursty" manner. Meaning it doesn't make sense > to save power in-between every request, but instead leave the device > powered for little while after a request has been served. The purpose > is often to avoid the wakeup-latency for *each* request, when these > comes in bursts. > > For these cases the driver has already traded some power savings for > improved performance of requests in bursts. When this trade is done > for the children/consumer, I think it seems reasonable to not further > defer the parent/supplier to be be runtime suspended/idled, as that > would further trade power for performance, which is what happens when > rpm_idle|suspend() decides to punt this to a work. > > Moreover, scheduling a work for each parent/supplier seems suboptimal > to me, as in the async case we are already executing from within a > work. In cases of a higher level of device hierarchy, several works > becomes scheduled. > > For the sync runtime PM suspend/idle case: > When a driver for a children/consumers decides to use the sync runtime > PM APIs, it's most likely because it wants the device to enter its low > power state as soon as possible. In other words, there is no need to > trade power for performance, either because power is more important > than performance or because the wakeup latency is negligible. To me, I > think it seems reasonable to respect this policy for parents/suppliers > as well instead of always punting to a work. Thanks for the detailed explanation. It looks that is should be safe to use sync for both links and parents in all cases... Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland