Date: Mon, 20 May 2019 10:10:16 -0600 From: Ian Lepore <ian@freebsd.org> To: lev@FreeBSD.org, freebsd-fs@freebsd.org, freebsd-hackers@FreeBSD.org, Alexander Motin <mav@FreeBSD.org> Subject: Re: Commit r345200 (new ARC reclamation threads) looks suspicious to me. Message-ID: <174f71126ca39907370a8904c07546b712ad91b9.camel@freebsd.org> In-Reply-To: <55989579-a228-498e-2842-453cad6f315f@FreeBSD.org> References: <55989579-a228-498e-2842-453cad6f315f@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?174f71126ca39907370a8904c07546b712ad91b9.camel>