Date: Wed, 22 May 2019 12:07:29 -0400 From: Alexander Motin <mav@FreeBSD.org> To: lev@FreeBSD.org, Mark Johnston <markj@freebsd.org> Cc: freebsd-fs@freebsd.org, freebsd-hackers@freebsd.org Subject: Re: Commit r345200 (new ARC reclamation threads) looks suspicious to me - second potential problem Message-ID: <2a50e192-e672-7c87-178b-afd509a765df@FreeBSD.org> In-Reply-To: <94d051a3-3427-7a5b-efe7-169cff2265d3@FreeBSD.org> References: <369cb1e9-f36a-a558-6941-23b9b811825a@FreeBSD.org> <20190520164202.GA2130@spy> <28c7430b-fb7c-6472-5c1b-fa3ff63a9e73@FreeBSD.org> <94d051a3-3427-7a5b-efe7-169cff2265d3@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 22.05.2019 11:50, Lev Serebryakov wrote: > On 22.05.2019 18:19, Alexander Motin wrote: > >>>> But looks like `arc_kmem_reap_soon()` is synchronous on FreeBSD! So, >>>> this `delay()` looks very wrong. Am I right? >> >> Why is it wrong? > One second pause after synchronous operation to wait it completion? No. To rate-throttle them. This gives UMA a second to get back into minimally steady state after we ripped all caches from it. As I have told, we do not want to drain caches constantly in a tight loop, we want more or less steady state. > There one more questionable piece of code: > > 6936 static void > 6937 arc_lowmem(void *arg __unused, int howto __unused) > 6938 { > .... > 6947 arc_reduce_target_size(to_free); > 6948 > 6949 mutex_enter(&arc_adjust_lock); > 6950 arc_adjust_needed = B_TRUE; > 6951 zthr_wakeup(arc_adjust_zthr); > > > 4587 static void > 4588 arc_reduce_target_size(int64_t to_free) > 4589 { > ... > 4612 if (asize > arc_c) { > 4613 DTRACE_PROBE2(arc__shrink_adjust, uint64_t, asize, > 4614 uint64_t, arc_c); > 4615 /* See comment in arc_adjust_cb_check() on why > lock+flag */ > 4616 mutex_enter(&arc_adjust_lock); > 4617 arc_adjust_needed = B_TRUE; > 4618 mutex_exit(&arc_adjust_lock); > 4619 zthr_wakeup(arc_adjust_zthr); > 4620 } > 4621 } > > Looks like lock/flag/wakeup sequence (which is now very cheap — mutexes > are not cheap, and this mutex could become contended in low-memory > situation) could be called twice. > > Looks like `arc_reduce_target_size()` should return boolean value and > unconditional signalling in `arc_lowmem()` should become conditional. I don't think this is not a hot path to bother about it. arc_lowmem() calls should be very rare. -- Alexander Motin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?2a50e192-e672-7c87-178b-afd509a765df>