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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190128223843.GA2593@cmpxchg.org>
Date:   Mon, 28 Jan 2019 17:38:43 -0500
From:   Johannes Weiner <hannes@...xchg.org>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Peter Zijlstra <peterz@...radead.org>, Tejun Heo <tj@...nel.org>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] psi: fix aggregation idle shut-off

On Mon, Jan 28, 2019 at 02:03:36PM -0800, Andrew Morton wrote:
> On Wed, 16 Jan 2019 14:35:01 -0500 Johannes Weiner <hannes@...xchg.org> wrote:
> > +/**
> > + * wq_worker_last_func - retrieve worker's last work function
> > + *
> > + * Determine the last function a worker executed. This is called from
> > + * the scheduler to get a worker's last known identity.
> > + *
> > + * CONTEXT:
> > + * spin_lock_irq(rq->lock)
> > + *
> > + * Return:
> > + * The last work function %current executed as a worker, NULL if it
> > + * hasn't executed any work yet.
> > + */
> > +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"?

psi isn't scheduler core, but it only works from a scheduler context.

The psi task change hook already requires being called under the
rq->lock while the task in question cannot change its scheduling
state, to record it and start the clock on the new state.

That same context ensures wq_worker_last_func() is safe. We're using
it from where the worker is inside the scheduler and going to *sleep*
(not just preemption), so couldn't possibly be changing last_func.

So IMO it makes sense to keep this last_func thing a part of the
private API between scheduler context and the workqueue code.