Re: [PATCH] PM / suspend: show workqueues busy name in suspend flow

Re: [PATCH] PM / suspend: show workqueues busy name in suspend flow

[Tejun Heo]

Hello,

On Wed, Jun 22, 2016 at 05:08:29PM +0800, Roger Lu wrote:
> When suspend flow is aborted because of workqueues busy,
> show workqueues busy name for debug purpose.
> 
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> ---
>  include/linux/workqueue.h |  1 +
>  kernel/power/process.c    |  3 +++
>  kernel/workqueue.c        | 35 +++++++++++++++++++++++++++++++++++
>  3 files changed, 39 insertions(+)
> 
> diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> index ca73c50..c9f9b24 100644
> --- a/include/linux/workqueue.h
> +++ b/include/linux/workqueue.h
> @@ -609,6 +609,7 @@ long work_on_cpu(int cpu, long (*fn)(void *), void
>  *arg);
>  #ifdef CONFIG_FREEZER
>  extern void freeze_workqueues_begin(void);
>  extern bool freeze_workqueues_busy(void);
> +extern void show_workqueues_busy(void);
>  extern void thaw_workqueues(void);
>  #endif /* CONFIG_FREEZER */
>  
> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index df058be..b538881 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c
> @@ -89,6 +89,9 @@ static int try_to_freeze_tasks(bool user_only)
>  		       elapsed_msecs / 1000, elapsed_msecs % 1000,
>  		       todo - wq_busy, wq_busy);
>  
> +		if (wq_busy)
> +			show_workqueues_busy();

I think it'd probably be better to call show_workqueue_state() instead
so that we can get a full picture of what's going on.

Thanks.

-- 
tejun

Re: [PATCH] PM / suspend: show workqueues busy name in suspend flow

[Tejun Heo]

Hello, Roger.

On Mon, Jun 27, 2016 at 05:44:05PM +0800, Roger Lu wrote:
> show_workqueue_state() is a better choice to me. However, only freezable
> workqueue is able to affect suspend flow. So, is there other mailing
> list discussing about showing freezable workqueue state only?? Maybe we
> can use that API in this case. Thanks very much.
> 
> freezable workqueue means workqueue is created with flag WQ_FREEZABLE.

It's for debugging anyway and workqueue dumps usually are pretty
short.  I don't think it's fine to use the same function.  We can add
flags in the printouts but I'm not even sure that'd be necessary.

Thanks.

-- 
tejun

Re: [PATCH] PM / suspend: show workqueues busy name in suspend flow

[Roger Lu]

Dear Tejun,

On Tue, 2016-06-28 at 12:56 -0400, Tejun Heo wrote:
> Hello, Roger.
> 
> On Mon, Jun 27, 2016 at 05:44:05PM +0800, Roger Lu wrote:
> > show_workqueue_state() is a better choice to me. However, only freezable
> > workqueue is able to affect suspend flow. So, is there other mailing
> > list discussing about showing freezable workqueue state only?? Maybe we
> > can use that API in this case. Thanks very much.
> > 
> > freezable workqueue means workqueue is created with flag WQ_FREEZABLE.
> 
> It's for debugging anyway and workqueue dumps usually are pretty
> short.  I don't think it's fine to use the same function.  We can add
> flags in the printouts but I'm not even sure that'd be necessary.
> 
> Thanks.
> 

Please allow me to elaborate my previous concern about printing
freezable workqueue info only in this case.

The benefit of it is that debugger can quickly understand which
freezable workqueues block suspend flow and assign this issue to
corresponding owner instead of extracting freezable workqueue info from
show_workqueue_state() first and, then, assigning the issue.

Adding a flag to printout the info we need is great. Perhaps we can do
that. Thanks for the advice.

Sincerely,
Roger Lu.

Re: [PATCH] PM / suspend: show workqueues busy name in suspend flow

[Tejun Heo]

Hello, Roger.

On Wed, Jun 29, 2016 at 11:54:11AM +0800, Roger Lu wrote:
> Please allow me to elaborate my previous concern about printing
> freezable workqueue info only in this case.
> 
> The benefit of it is that debugger can quickly understand which
> freezable workqueues block suspend flow and assign this issue to
> corresponding owner instead of extracting freezable workqueue info from
> show_workqueue_state() first and, then, assigning the issue.

I don't think it matters.  At that point, workqueues are generally
pretty idle anyway and it shouldn't be difficult to tell which work
items are the offending ones.  Besides, freezable and unfreezable
workqueues share the same backend pools, so it isn't easily separable
either.

Thanks.

-- 
tejun