Re: [PATCH v2 5/5] psi: introduce psi monitor

[Peter Zijlstra <peterz@infradead.org>]

On Mon, Jan 14, 2019 at 11:30:12AM -0800, Suren Baghdasaryan wrote:
> For memory ordering (which Johannes also pointed out) the critical point is:
> 
> times[cpu] += delta           | if g->polling:
> smp_wmb()                     |   g->polling = polling = 0
> cmpxchg(g->polling, 0, 1)     |   smp_rmb()
>                               |   delta = times[*] (through goto SLOWPATH)
> 
> So that hotpath writes to times[] then g->polling and slowpath reads
> g->polling then times[]. cmpxchg() implies a full barrier, so we can
> drop smp_wmb(). Something like this:
> 
> times[cpu] += delta           | if g->polling:
> cmpxchg(g->polling, 0, 1)     |   g->polling = polling = 0
>                               |   smp_rmb()
>                               |   delta = times[*] (through goto SLOWPATH)
> 
> Would that address your concern about ordering?

cmpxchg() implies smp_mb() before and after, so the smp_wmb() on the
left column is superfluous.

The right hand column is actively wrong; because that reads like it
wants to order a store (g->polling = 0) and a load (d = times[]), and
therefore requires smp_mb().

Also, you probably want to use atomic_t for g->polling, because we
(sadly) have architectures where regular stores and atomic ops don't
work 'right'.