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

[Slawa Olhovchenkov]

On Mon, May 20, 2019 at 10:10:16AM -0600, Ian Lepore wrote:

> 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.

What about arc_no_grow, for example?
arc_no_grow set in arc_reap_cb_check(), called from arc_reap_zthr
thread and in arc_lowmem().
arc_no_grow test in arc_adapt(), called from
arc_read()/arc_get_data_impl() called from many unsynced thread.

How synced visibility of this varibale?