Commit r345200 (new ARC reclamation threads) looks suspicious to me - second potential problem

[Alexander Motin]

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