Re: [PATCH] psi: fix aggregation idle shut-off

[Tejun Heo <tj@kernel.org>]

Hello, Andrew.

On Mon, Jan 28, 2019 at 02:03:36PM -0800, Andrew Morton wrote:
> > +#include "../workqueue_internal.h"
> 
> "Only to be included by workqueue and core kernel subsystems"
> 
> I'm not sure that psi qualifies.  Perhaps wq_worker_last_func() should
> be declared in include/linux/workqueue.h.
>
> And perhaps implemented there as well.  It's similar to
> current_wq_worker(), which is inlined and wq_worker_last_func() is
> small enough to justify inlining.

workqueue_internal.h is used mostly to expose some of the internal
details and hooks to scheduler code for concurrency management.  Most
of PSI is tracking state transitions on scheduling events (psi.c is
under kernel/sched/ for this reason), so it's mostly in line with how
the file was being used.

I think keeping it in workqueue_internal.h is better.  Please see
below.

> > +work_func_t wq_worker_last_func(struct task_struct *task)
> > +{
> > +	struct worker *worker = kthread_data(task);
> > +
> > +	return worker->last_func;
> > +}
> 
> The semantics are troublesome.  What guarantees that worker->last_func
> won't change under the caller's feet?  The caller should hold some lock
> (presumably worker->pool->lock) in order to stabilize the
> wq_worker_last_func() return value?
> 
> Also, the comment isn't really true - this is called from PSI, which is
> hardly "the scheduler"?

Hmm... This is the same way wq_worker_waking_up() and
wq_worker_sleeping() are synchronized.  The task being queried is
being scheduled in or out, so it can't change underneath and is in
line with other stuff in kernel_internal.h.

Thanks.

-- 
tejun