[Question] about patch: don't use [delayed_]work_pending()

[Question] about patch: don't use [delayed_]work_pending()

[qiaozhou]

Hi Tejun,

I have a question related with below patch, and need your suggestion.

In our system, we do cpu clock init in of_clk_init path, and use pm qos 
to maintain cpu/cci clock. Firstly we init a CCI_CLK_QOS and set a 
default value, then update CCI_CLK_QOS to limit CCI min frequency 
according to current cpu frequency. Before calling 
pm_qos_update_request, irq is disabled, but after the calling, irq is 
enabled in cancel_delayed_work_sync, which causes some inconvenience 
before Before this patch is applied, it checks pending work and won't do 
cancel_delayed_work_sync in this boot up phase.

The simple calling sequence is like this:

start_kernel -> of_clk_init -> cpu_clk_init -> pm_qos_add_request(xx, 
default_value),

then pm_qos_update_request.

I don't know whether it's meaningful to still check pending work here, 
or it's not suggested to use pm_qos_update_request in this early boot up 
phase. Could you help to share some opinions? (I can fix this issue by 
adding the current qos value directly instead of default value, though.)

Thanks a lot.

commit ed1ac6e91a3ff7c561008ba57747cd6cbc49385e
Author: Tejun Heo <tj@kernel.org>
Date:   Fri Jan 11 13:37:33 2013 +0100

     PM: don't use [delayed_]work_pending()

     There's no need to test whether a (delayed) work item is pending
     before queueing, flushing or cancelling it, so remove work_pending()
     tests used in those cases.

     Signed-off-by: Tejun Heo <tj@kernel.org>
     Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

...

@@ -359,8 +359,7 @@ void pm_qos_update_request(struct pm_qos_request *req,
                 return;
         }

-       if (delayed_work_pending(&req->work))
-               cancel_delayed_work_sync(&req->work);
+       cancel_delayed_work_sync(&req->work);
...

Best Regards

Qiao

Re: [Question] about patch: don't use [delayed_]work_pending()

[Tejun Heo]

Hello,

On Thu, Sep 01, 2016 at 05:09:36PM +0800, qiaozhou wrote:
> In our system, we do cpu clock init in of_clk_init path, and use pm qos to
> maintain cpu/cci clock. Firstly we init a CCI_CLK_QOS and set a default
> value, then update CCI_CLK_QOS to limit CCI min frequency according to
> current cpu frequency. Before calling pm_qos_update_request, irq is
> disabled, but after the calling, irq is enabled in cancel_delayed_work_sync,
> which causes some inconvenience before Before this patch is applied, it
> checks pending work and won't do cancel_delayed_work_sync in this boot up
> phase.

So, cancel_delayed_work_sync() usually shouldn't be called with irq
disabled as it's a possibly blocking call.

> The simple calling sequence is like this:
> 
> start_kernel -> of_clk_init -> cpu_clk_init -> pm_qos_add_request(xx,
> default_value),
> 
> then pm_qos_update_request.
> 
> I don't know whether it's meaningful to still check pending work here, or
> it's not suggested to use pm_qos_update_request in this early boot up phase.
> Could you help to share some opinions? (I can fix this issue by adding the
> current qos value directly instead of default value, though.)

Hmmm... but I suppose this is super-early in the boot.  Would it make
sense to have a static variable (e.g. bool clk_fully_initailized) to
gate the cancel_delayed_sync() call?

Thanks.

-- 
tejun

Re: [Question] about patch: don't use [delayed_]work_pending()

[qiaozhou]

On 2016年09月02日 02:45, Tejun Heo wrote:
> Hello,
>
> On Thu, Sep 01, 2016 at 05:09:36PM +0800, qiaozhou wrote:
>> In our system, we do cpu clock init in of_clk_init path, and use pm qos to
>> maintain cpu/cci clock. Firstly we init a CCI_CLK_QOS and set a default
>> value, then update CCI_CLK_QOS to limit CCI min frequency according to
>> current cpu frequency. Before calling pm_qos_update_request, irq is
>> disabled, but after the calling, irq is enabled in cancel_delayed_work_sync,
>> which causes some inconvenience before Before this patch is applied, it
>> checks pending work and won't do cancel_delayed_work_sync in this boot up
>> phase.
> So, cancel_delayed_work_sync() usually shouldn't be called with irq
> disabled as it's a possibly blocking call.
Agree.
>
>> The simple calling sequence is like this:
>>
>> start_kernel -> of_clk_init -> cpu_clk_init -> pm_qos_add_request(xx,
>> default_value),
>>
>> then pm_qos_update_request.
>>
>> I don't know whether it's meaningful to still check pending work here, or
>> it's not suggested to use pm_qos_update_request in this early boot up phase.
>> Could you help to share some opinions? (I can fix this issue by adding the
>> current qos value directly instead of default value, though.)
> Hmmm... but I suppose this is super-early in the boot.  Would it make
> sense to have a static variable (e.g. bool clk_fully_initailized) to
> gate the cancel_delayed_sync() call?
You're right that it's indeed super-early stage. But currently we can't 
control the gate of can_delayed_work_sync, since it's inside 
pm_qos_update_request. Out of our control. We can choose to not call 
pm_qos_update_request to avoid this issue, and use pm_qos_add_request 
alternatively. Good to have it.
Thanks a lot.

Re: [Question] about patch: don't use [delayed_]work_pending()

[Tejun Heo]

Hello,

On Fri, Sep 02, 2016 at 09:17:04AM +0800, qiaozhou wrote:
> > > I don't know whether it's meaningful to still check pending work here, or
> > > it's not suggested to use pm_qos_update_request in this early boot up phase.
> > > Could you help to share some opinions? (I can fix this issue by adding the
> > > current qos value directly instead of default value, though.)
> > Hmmm... but I suppose this is super-early in the boot.  Would it make
> > sense to have a static variable (e.g. bool clk_fully_initailized) to
> > gate the cancel_delayed_sync() call?
>
> You're right that it's indeed super-early stage. But currently we can't
> control the gate of can_delayed_work_sync, since it's inside
> pm_qos_update_request. Out of our control. We can choose to not call
> pm_qos_update_request to avoid this issue, and use pm_qos_add_request
> alternatively. Good to have it.

Ah sorry, didn't understand that the offending cancel_sync call is in
the generic part.  Hmm... but yeah, we should still be able to take
the same approach.  I'll see what's the right thing to gate the
operation there.

Thanks.

-- 
tejun

Re: [Question] about patch: don't use [delayed_]work_pending()

[Tejun Heo]

On Fri, Sep 02, 2016 at 09:50:07AM -0400, Tejun Heo wrote:
> Hello,
> 
> On Fri, Sep 02, 2016 at 09:17:04AM +0800, qiaozhou wrote:
> > > > I don't know whether it's meaningful to still check pending work here, or
> > > > it's not suggested to use pm_qos_update_request in this early boot up phase.
> > > > Could you help to share some opinions? (I can fix this issue by adding the
> > > > current qos value directly instead of default value, though.)
> > > Hmmm... but I suppose this is super-early in the boot.  Would it make
> > > sense to have a static variable (e.g. bool clk_fully_initailized) to
> > > gate the cancel_delayed_sync() call?
> >
> > You're right that it's indeed super-early stage. But currently we can't
> > control the gate of can_delayed_work_sync, since it's inside
> > pm_qos_update_request. Out of our control. We can choose to not call
> > pm_qos_update_request to avoid this issue, and use pm_qos_add_request
> > alternatively. Good to have it.
> 
> Ah sorry, didn't understand that the offending cancel_sync call is in
> the generic part.  Hmm... but yeah, we should still be able to take
> the same approach.  I'll see what's the right thing to gate the
> operation there.

Does the following patch work?

Subject: power: avoid calling cancel_delayed_work_sync() during early boot

of_clk_init() ends up calling into pm_qos_update_request() very early
during boot where irq is expected to stay disabled.
pm_qos_update_request() uses cancel_delayed_work_sync() which
correctly assumes that irq is enabled on invocation and
unconditionally disables and re-enables it.

Gate cancel_delayed_work_sync() invocation with kevented_up() to avoid
enabling irq unexpectedly during early boot.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Qiao Zhou <qiaozhou@asrmicro.com>
Link: http://lkml.kernel.org/r/d2501c4c-8e7b-bea3-1b01-000b36b5dfe9@asrmicro.com
---
 kernel/power/qos.c |   11 ++++++++++-
  1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/kernel/power/qos.c b/kernel/power/qos.c
index 97b0df7..168ff44 100644
--- a/kernel/power/qos.c
+++ b/kernel/power/qos.c
@@ -482,7 +482,16 @@ void pm_qos_update_request(struct pm_qos_request *req,
 		return;
 	}
 
-	cancel_delayed_work_sync(&req->work);
+	/*
+	 * This function may be called very early during boot, for example,
+	 * from of_clk_init(), where irq needs to stay disabled.
+	 * cancel_delayed_work_sync() assumes that irq is enabled on
+	 * invocation and re-enables it on return.  Avoid calling it until
+	 * workqueue is initialized.
+	 */
+	if (keventd_up())
+		cancel_delayed_work_sync(&req->work);
+
 	__pm_qos_update_request(req, new_value);
 }
 EXPORT_SYMBOL_GPL(pm_qos_update_request);

Re: [Question] about patch: don't use [delayed_]work_pending()

[qiaozhou]

On 2016年09月02日 22:21, Tejun Heo wrote:
> On Fri, Sep 02, 2016 at 09:50:07AM -0400, Tejun Heo wrote:
>> Hello,
>>
>> On Fri, Sep 02, 2016 at 09:17:04AM +0800, qiaozhou wrote:
>>>>> I don't know whether it's meaningful to still check pending work here, or
>>>>> it's not suggested to use pm_qos_update_request in this early boot up phase.
>>>>> Could you help to share some opinions? (I can fix this issue by adding the
>>>>> current qos value directly instead of default value, though.)
>>>> Hmmm... but I suppose this is super-early in the boot.  Would it make
>>>> sense to have a static variable (e.g. bool clk_fully_initailized) to
>>>> gate the cancel_delayed_sync() call?
>>> You're right that it's indeed super-early stage. But currently we can't
>>> control the gate of can_delayed_work_sync, since it's inside
>>> pm_qos_update_request. Out of our control. We can choose to not call
>>> pm_qos_update_request to avoid this issue, and use pm_qos_add_request
>>> alternatively. Good to have it.
>> Ah sorry, didn't understand that the offending cancel_sync call is in
>> the generic part.  Hmm... but yeah, we should still be able to take
>> the same approach.  I'll see what's the right thing to gate the
>> operation there.
> Does the following patch work?
I'll have a try next Monday and let you know ASAP. Thanks for the patch.
>
> Subject: power: avoid calling cancel_delayed_work_sync() during early boot
>
> of_clk_init() ends up calling into pm_qos_update_request() very early
> during boot where irq is expected to stay disabled.
> pm_qos_update_request() uses cancel_delayed_work_sync() which
> correctly assumes that irq is enabled on invocation and
> unconditionally disables and re-enables it.
>
> Gate cancel_delayed_work_sync() invocation with kevented_up() to avoid
> enabling irq unexpectedly during early boot.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Qiao Zhou <qiaozhou@asrmicro.com>
> Link: http://lkml.kernel.org/r/d2501c4c-8e7b-bea3-1b01-000b36b5dfe9@asrmicro.com
> ---
>   kernel/power/qos.c |   11 ++++++++++-
>    1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/power/qos.c b/kernel/power/qos.c
> index 97b0df7..168ff44 100644
> --- a/kernel/power/qos.c
> +++ b/kernel/power/qos.c
> @@ -482,7 +482,16 @@ void pm_qos_update_request(struct pm_qos_request *req,
>   		return;
>   	}
>   
> -	cancel_delayed_work_sync(&req->work);
> +	/*
> +	 * This function may be called very early during boot, for example,
> +	 * from of_clk_init(), where irq needs to stay disabled.
> +	 * cancel_delayed_work_sync() assumes that irq is enabled on
> +	 * invocation and re-enables it on return.  Avoid calling it until
> +	 * workqueue is initialized.
> +	 */
> +	if (keventd_up())
> +		cancel_delayed_work_sync(&req->work);
> +
>   	__pm_qos_update_request(req, new_value);
>   }
>   EXPORT_SYMBOL_GPL(pm_qos_update_request);

Re: [Question] about patch: don't use [delayed_]work_pending()

[qiaozhou]

On 2016年09月02日 22:21, Tejun Heo wrote:
> On Fri, Sep 02, 2016 at 09:50:07AM -0400, Tejun Heo wrote:
>> Hello,
>>
>> On Fri, Sep 02, 2016 at 09:17:04AM +0800, qiaozhou wrote:
>>>>> I don't know whether it's meaningful to still check pending work here, or
>>>>> it's not suggested to use pm_qos_update_request in this early boot up phase.
>>>>> Could you help to share some opinions? (I can fix this issue by adding the
>>>>> current qos value directly instead of default value, though.)
>>>> Hmmm... but I suppose this is super-early in the boot.  Would it make
>>>> sense to have a static variable (e.g. bool clk_fully_initailized) to
>>>> gate the cancel_delayed_sync() call?
>>> You're right that it's indeed super-early stage. But currently we can't
>>> control the gate of can_delayed_work_sync, since it's inside
>>> pm_qos_update_request. Out of our control. We can choose to not call
>>> pm_qos_update_request to avoid this issue, and use pm_qos_add_request
>>> alternatively. Good to have it.
>> Ah sorry, didn't understand that the offending cancel_sync call is in
>> the generic part.  Hmm... but yeah, we should still be able to take
>> the same approach.  I'll see what's the right thing to gate the
>> operation there.
> Does the following patch work?
The patch can fix this issue. Thanks a lot.
>
> Subject: power: avoid calling cancel_delayed_work_sync() during early boot
>
> of_clk_init() ends up calling into pm_qos_update_request() very early
> during boot where irq is expected to stay disabled.
> pm_qos_update_request() uses cancel_delayed_work_sync() which
> correctly assumes that irq is enabled on invocation and
> unconditionally disables and re-enables it.
>
> Gate cancel_delayed_work_sync() invocation with kevented_up() to avoid
> enabling irq unexpectedly during early boot.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Qiao Zhou <qiaozhou@asrmicro.com>
> Link: http://lkml.kernel.org/r/d2501c4c-8e7b-bea3-1b01-000b36b5dfe9@asrmicro.com
> ---
>   kernel/power/qos.c |   11 ++++++++++-
>    1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/power/qos.c b/kernel/power/qos.c
> index 97b0df7..168ff44 100644
> --- a/kernel/power/qos.c
> +++ b/kernel/power/qos.c
> @@ -482,7 +482,16 @@ void pm_qos_update_request(struct pm_qos_request *req,
>   		return;
>   	}
>   
> -	cancel_delayed_work_sync(&req->work);
> +	/*
> +	 * This function may be called very early during boot, for example,
> +	 * from of_clk_init(), where irq needs to stay disabled.
> +	 * cancel_delayed_work_sync() assumes that irq is enabled on
> +	 * invocation and re-enables it on return.  Avoid calling it until
> +	 * workqueue is initialized.
> +	 */
> +	if (keventd_up())
> +		cancel_delayed_work_sync(&req->work);
> +
>   	__pm_qos_update_request(req, new_value);
>   }
>   EXPORT_SYMBOL_GPL(pm_qos_update_request);

[PATCH] power: avoid calling cancel_delayed_work_sync() during early boot

[Tejun Heo]

of_clk_init() ends up calling into pm_qos_update_request() very early
during boot where irq is expected to stay disabled.
pm_qos_update_request() uses cancel_delayed_work_sync() which
correctly assumes that irq is enabled on invocation and
unconditionally disables and re-enables it.

Gate cancel_delayed_work_sync() invocation with kevented_up() to avoid
enabling irq unexpectedly during early boot.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-and-tested-by: Qiao Zhou <qiaozhou@asrmicro.com>
Link: http://lkml.kernel.org/r/d2501c4c-8e7b-bea3-1b01-000b36b5dfe9@asrmicro.com
---

Rafael, can you please route this patch?

Thanks.

 kernel/power/qos.c |   11 ++++++++++-
  1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/kernel/power/qos.c b/kernel/power/qos.c
index 97b0df7..168ff44 100644
--- a/kernel/power/qos.c
+++ b/kernel/power/qos.c
@@ -482,7 +482,16 @@ void pm_qos_update_request(struct pm_qos_request *req,
 		return;
 	}
 
-	cancel_delayed_work_sync(&req->work);
+	/*
+	 * This function may be called very early during boot, for example,
+	 * from of_clk_init(), where irq needs to stay disabled.
+	 * cancel_delayed_work_sync() assumes that irq is enabled on
+	 * invocation and re-enables it on return.  Avoid calling it until
+	 * workqueue is initialized.
+	 */
+	if (keventd_up())
+		cancel_delayed_work_sync(&req->work);
+
 	__pm_qos_update_request(req, new_value);
 }
 EXPORT_SYMBOL_GPL(pm_qos_update_request);

Re: [PATCH] power: avoid calling cancel_delayed_work_sync() during early boot

[Rafael J. Wysocki]

On Mon, Sep 5, 2016 at 2:38 PM, Tejun Heo <tj@kernel.org> wrote:
> of_clk_init() ends up calling into pm_qos_update_request() very early
> during boot where irq is expected to stay disabled.
> pm_qos_update_request() uses cancel_delayed_work_sync() which
> correctly assumes that irq is enabled on invocation and
> unconditionally disables and re-enables it.
>
> Gate cancel_delayed_work_sync() invocation with kevented_up() to avoid
> enabling irq unexpectedly during early boot.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-and-tested-by: Qiao Zhou <qiaozhou@asrmicro.com>
> Link: http://lkml.kernel.org/r/d2501c4c-8e7b-bea3-1b01-000b36b5dfe9@asrmicro.com
> ---
>
> Rafael, can you please route this patch?

I will.

Thanks,
Rafael

Re: [Question] about patch: don't use [delayed_]work_pending()

[Andreas Mohr]

Hi,

[no properly binding reference via In-Reply-To: available thus manually re-creating, sorry]

https://lkml.org/lkml/2016/9/2/335

I came up with the following somewhat random thoughts:


*** this treatment is exclusive to a single use case, i.e.
not covering things consistently (API-wide)

> +++ b/kernel/power/qos.c
> @@ -482,7 +482,16 @@ void pm_qos_update_request(struct pm_qos_request
> *req,
>  		return;
>  	}
>  
> -	cancel_delayed_work_sync(&req->work);
> +	/*
> +	 * This function may be called very early during boot, for
> example,
> +	 * from of_clk_init(), where irq needs to stay disabled.
> +	 * cancel_delayed_work_sync() assumes that irq is enabled on
> +	 * invocation and re-enables it on return.  Avoid calling it
> until
> +	 * workqueue is initialized.
> +	 */
> +	if (keventd_up())
> +		cancel_delayed_work_sync(&req->work);
> +
>  	__pm_qos_update_request(req, new_value);
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_update_request);


Reason: any other [early-boot] invoker of cancel_delayed_work_sync()
would hit the same issue,
without any fix then available locally each.

This may or may not be intentional.
Just wanted to point it out.


The more global question here obviously is
how this annoying
irq handling side effect
ought to be handled cleanly / symmetrically
(and ideally with minimal effect to hotpaths,
which probably is the usual inherent outcome of
an elegant/clean solution),
especially given that it is early handling vs. normal.
However unfortunately I do not have any final ideas here ATM.



*** try_to_grab_pending() layer violation (asymmetry)
It *internally* does a local_irq_save(*flags)
and then passes these flags to the *external* user,
where the user
after having called try_to_grab_pending()
then has to invoke these same
*internal layer implementation details*
(local_irq_restore(flags))
That's just fugly asymmetric handling.
If an API layer happens to have
some certain
*internal*
"context"/"resource" requirements
[either by having an explicit API function
to establish these requirements,
or by "dirtily"/"implicitly" passing these out
via an out parameter],
then this API layer
*always* should *itself* provide
correspondingly required
resource handling functions
*at this very layer implementation*
Since the API currently is named try_to_grab_pending(),
this suggests that the signature of the
corresponding resource cleanup function
could be something like
    work_grab_context_release(context_reference);

Not properly providing such a public API
will result in e.g. the following major disadvantages:
- a flood of changes necessary
  *at all API users*
  in case use protocol of these
  implementation details
  (local_irq_restore())
  happen to get changed
- all users of this API layer
  having to #include the specific header
  of the inner implementation details layer
  (local_irq_restore())
  rather than the cleanly plain API layer itself only
  (either
   within the API layer header in case of an inline helper, or
   within the API layer implemention internally only in case of non-inline)
  IOW, by getting this wrong,
  #include handling of this header of the internal implementation layer requirements
  is not properly under the control of the API layer implementer
  any more


Also,
comment
"try_to_grab_pending - steal work item from worklist and disable irq"
seems to have wrong order
since I would think that
"disable irq"
is the *pre-requisite* for being able to
reliably (atomically) steal work items from work list.
[[hmm, and let's hope that NMIs are (known to be?) not involved]]

HTH,

Andreas Mohr

Re: [Question] about patch: don't use [delayed_]work_pending()

[Tejun Heo]

On Sun, Sep 04, 2016 at 03:29:39AM +0200, Andreas Mohr wrote:
> Reason: any other [early-boot] invoker of cancel_delayed_work_sync()
> would hit the same issue,
> without any fix then available locally each.
> 
> This may or may not be intentional.
> Just wanted to point it out.

idk, invoking a blocking API from early boot is pretty special (as
with everything during early boot), so I think it's fine to handle
them as execeptions.

Thanks.

-- 
tejun

Re: [Question] about patch: don't use [delayed_]work_pending()

[Andreas Mohr]

On Mon, Sep 05, 2016 at 08:41:39AM -0400, Tejun Heo wrote:
> On Sun, Sep 04, 2016 at 03:29:39AM +0200, Andreas Mohr wrote:
> > Reason: any other [early-boot] invoker of cancel_delayed_work_sync()
> > would hit the same issue,
> > without any fix then available locally each.
> > 
> > This may or may not be intentional.
> > Just wanted to point it out.
> 
> idk, invoking a blocking API from early boot is pretty special (as
> with everything during early boot), so I think it's fine to handle
> them as execeptions.

Yup, this sounds like the rationale that I would have expected.

Andreas