Commit r345200 (new ARC reclamation threads) looks suspicious to me.

[Ian Lepore]

On Mon, 2019-05-20 at 18:38 +0300, Lev Serebryakov wrote:
>  I'm looking at last commit to
> 'sys/cddl/contrib/opensolaris/uts/common/fs/zfs/arc.c' (r345200) and
> have one question.
> 
>  Is it Ok for two threads to communicate via simple global variable?
> Now
> new code has (line 315):
> 
> static boolean_t        arc_adjust_needed = B_FALSE;
> 
>  And after that some threads run code like this:
> 
> mutex_enter(&arc_adjust_lock);
> arc_adjust_needed = B_TRUE;
> mutex_exit(&arc_adjust_lock);
> zthr_wakeup(arc_adjust_zthr);
> 
>  And thread `arc_adjust_zthr` has code like this (line 4874):
> 
> return (arc_adjust_needed);
> 
>  This variable is not atomic. It is not updated and/or read in atomic
> way. What code gives guarantees that `arc_adjust_zthr` will detect
> this
> change? I don't see any. Am I wrong?

The arc_adjust_needed variable is the gating condition associated with
a condition variable and lock.  It is only read or changed while
holding a lock, and the acquiring and releasing of that lock provides
the needed memory barriers.  In this case, the association with the
condition variable and lock is somewhat obscured by the way the zthread
timer stuff works.  The arc_adjust_cb_check() function is called from
line 193 of contrib/opensolaris/uts/common/fs/zfs/zthr.c, and that's
where you'll find the code that makes it clear this is an idiomatic
condition variable pattern.

-- Ian