From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6417854764 for ; Tue, 21 Jan 2025 23:10:13 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737501015; cv=none; b=L8w/Tzc8Tac0AP+si1KXzq9r3gwzH4fGCLyf37qxUayDuQo/X/SDCoQdqwbtQy1MGX4vdA6zlTEecumeLIIZXQjjLyqWdJ4q43sGw52iAK1ALoezGOWGUyqnR5ye16VydjwYXj/+DgpecmZq93FnOEFfW+8ivu0j/041+oQRgyY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737501015; c=relaxed/simple; bh=/7yB3Gl5KzINZWNv2/SNwnVmCQ8oZEClAsP6YfY88XE=; h=From:Message-ID:Date:MIME-Version:Subject:To:Cc:References: In-Reply-To:Content-Type; b=JXFGADlWiHMmh8s+u2aluzpE6fcC/gYh7lpPX2RlhMP6A1+NWAgYTf0nCiyHSvuEUSwOZLLcJ17XZwE4X8vgQWv3IJXfyJqCD2QJtYT13LukvfZCXSTK/ROfXBW104Qc/lavFIelrEYeYE7lERNWwhtBlkX6EaIa1RrjE5MgoJM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=fqPElNsA; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="fqPElNsA" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1737501012; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=je9rAF3qbzg5dx0xBzcNHf2j/oJUucJyBPDZdw4CPN4=; b=fqPElNsAGQjeoH0rahjqrFe18XBJZvQuTpETWtZpWLgAj3xnULKlskIXYy5JEQ2EyUPf3p 1vjqS7R9ScxlfsJJNM5BXLvtTVS8SMYk1V7oAInpQW/j4k9K5QLmjUOnK3opGFt9pU6KcR t8nuj+lu3vRSLB+d7uWpYoHko8JJvis= Received: from mail-qt1-f199.google.com (mail-qt1-f199.google.com [209.85.160.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-536-oVwCalykPzC07McaArjBrA-1; Tue, 21 Jan 2025 18:10:10 -0500 X-MC-Unique: oVwCalykPzC07McaArjBrA-1 X-Mimecast-MFC-AGG-ID: oVwCalykPzC07McaArjBrA Received: by mail-qt1-f199.google.com with SMTP id d75a77b69052e-4679d6ef2f9so170356651cf.1 for ; Tue, 21 Jan 2025 15:10:10 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1737501010; x=1738105810; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:user-agent:mime-version:date:message-id:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=je9rAF3qbzg5dx0xBzcNHf2j/oJUucJyBPDZdw4CPN4=; b=IRzGFFJc/qTUjbqHluB7GwzpkLEj3KxO0k1N0atpu6cKXHt3zNpLtI28AFW7M2WDdX lWrK3USAiyVAuERYxDJJwoBXt969Z5iZMmbX7b5v3mB1lGJhofUSowuuX2z2hXdTvCkq tkAx2I14cmv5gYmq2g3bxsa9SsGwjTmuvSWjTTGlI1ZuHAMo2hDNtC43pLEogIqvJa5U yDhkUBA1xlZ/RhU3uCYaW+MsnWkh5rYObVedlCu2GXBOmcWmDpql5v4dnGt17T9p52mM cLKToFboySsU39U4WWSALDGir7zUHuLjcjrunCRSiIPXzWe/tT0o5yZSinAbN3ZYA3ns aFTg== X-Forwarded-Encrypted: i=1; AJvYcCWfKv+giaCo74dAFhDfmgSu1iJqC2BaYyXPOx5aYUdg7WvKI9LRgX/FKM/G0Blxazbzs/MCwwAqvV0+YRE=@vger.kernel.org X-Gm-Message-State: AOJu0Ywkk6WS50pU/dE6oAP5uWqxbz/GtIMtvOiD1gM+1QU3QHqEme1Y ufOiE5cFGivZUmHYSM95P4Cs69KPxaPnOc2u04LzOfzqthfl8TvYlLPPUrU6XFwGkBTuqJ3O7SE sRGohP7cZu71sIhTwcoD097B6fbvcmB470oS6p+vCRSTjUS1BSw9S4AKqBwq2JQ== X-Gm-Gg: ASbGncvzG9HkwwPw//44UtqtwR2W69EOHcznNprg8uP5AdvATzYnp3WEy4hW6oMV2An mW1iiPn4bqfVUBFdJ58qqgTtM8cWDpj5+evzAQKBExnHIy5uoD3FXRmlxU01pZv2O23Tz1Y/7fE D9Sm8nNtFLv7z3HRUGD0cM0E12BurR8xoTLtNlZx0JUZ7FTHENHNdMybsa2uQECl7z0VsCXSzCV yJAMUhNBcr0WSPDNfStO75X19KwIXTn2cnSindifYMFubF6w+X9ospkenGzkaJ4jAw1hLJI4wIl T+BLHD6T2pAINCoOi6SkDyRG+9HhHMdODnwN/rui X-Received: by 2002:ac8:57c1:0:b0:467:6e25:3f30 with SMTP id d75a77b69052e-46e12a609cdmr287488151cf.12.1737501010428; Tue, 21 Jan 2025 15:10:10 -0800 (PST) X-Google-Smtp-Source: AGHT+IHc32tWDI6zLY3uHqam6La/Xkyn5/jb3lfa39ZSUjZYz53kS3ZO6v9kXrYEB8vCqTkiZISKAg== X-Received: by 2002:ac8:57c1:0:b0:467:6e25:3f30 with SMTP id d75a77b69052e-46e12a609cdmr287487831cf.12.1737501009967; Tue, 21 Jan 2025 15:10:09 -0800 (PST) Received: from ?IPV6:2601:188:c100:5710:315f:57b3:b997:5fca? ([2601:188:c100:5710:315f:57b3:b997:5fca]) by smtp.gmail.com with ESMTPSA id d75a77b69052e-46e258c82a6sm38267861cf.59.2025.01.21.15.10.08 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 21 Jan 2025 15:10:09 -0800 (PST) From: Waiman Long X-Google-Original-From: Waiman Long Message-ID: <87d76c28-990c-47a8-9ed7-3cf6aec79d27@redhat.com> Date: Tue, 21 Jan 2025 18:10:08 -0500 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] locking/semaphore: Use raw_spin_trylock_irqsave() in down_trylock() To: Peter Zijlstra Cc: Ingo Molnar , Will Deacon , Boqun Feng , linux-kernel@vger.kernel.org References: <20250120193608.2312690-1-longman@redhat.com> <20250121080850.GC8603@noisy.programming.kicks-ass.net> Content-Language: en-US In-Reply-To: <20250121080850.GC8603@noisy.programming.kicks-ass.net> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 1/21/25 3:08 AM, Peter Zijlstra wrote: > On Mon, Jan 20, 2025 at 02:36:08PM -0500, Waiman Long wrote: >> A circular lock dependency splat has been seen with down_trylock(). >> >> [ 4011.795602] ====================================================== >> [ 4011.795603] WARNING: possible circular locking dependency detected >> [ 4011.795607] 6.12.0-41.el10.s390x+debug >> [ 4011.795612] ------------------------------------------------------ >> [ 4011.795613] dd/32479 is trying to acquire lock: >> [ 4011.795617] 0015a20accd0d4f8 ((console_sem).lock){-.-.}-{2:2}, at: down_trylock+0x26/0x90 >> [ 4011.795636] >> [ 4011.795636] but task is already holding lock: >> [ 4011.795637] 000000017e461698 (&zone->lock){-.-.}-{2:2}, at: rmqueue_bulk+0xac/0x8f0 >> [ 4011.795644] >> [ 4011.795644] which lock already depends on the new lock. >> : >> [ 4011.796025] (console_sem).lock --> hrtimer_bases.lock --> &zone->lock >> [ 4011.796025] >> [ 4011.796029] Possible unsafe locking scenario: >> [ 4011.796029] >> [ 4011.796030] CPU0 >> [ 4011.796031] ---- >> [ 4011.796032] lock(&zone->lock); >> [ 4011.796034] lock(hrtimer_bases.lock); >> [ 4011.796036] lock(&zone->lock); >> [ 4011.796038] lock((console_sem).lock); >> [ 4011.796039] >> [ 4011.796039] *** DEADLOCK *** > Urgh, I hate this ^ bit of the lockdep output, it pretends to be > something useful, while it is the least useful part. Doubly so for > anything with more than 2 locks involved. Sorry for not including other relevant information.                the existing dependency chain (in reverse order) is:                -> #4 (&zone->lock){-.-.}-{2:2}:                -> #3 (hrtimer_bases.lock){-.-.}-{2:2}:                -> #2 (&rq->__lock){-.-.}-{2:2}:                -> #1 (&p->pi_lock){-.-.}-{2:2}:                -> #0 ((console_sem).lock){-.-.}-{2:2}: The last one is actually due to calling try_to_wake_up() while holding the console.sem raw_spinlock. Another way to break the circular locking dependency is to use the wake_q in the semaphore. I had proposed that in the past, but you said that the problem might be gone with console rewrite. Now the console rewrite should have been done with the v6.12 kernel, the problem is still there. >> The calling sequence was >> rmqueue_pcplist() >> => __rmqueue_pcplist() >> => rmqueue_bulk() >> => expand() >> => __add_to_free_list() >> => __warn_printk() >> => ... >> => console_trylock() >> => __down_trylock_console_sem() >> => down_trylock() >> => _raw_spin_lock_irqsave() >> >> Normally, a trylock call should avoid this kind of circular lock >> dependency splat, but down_trylock() is an exception. Fix this problem >> by using raw_spin_trylock_irqsave() in down_trylock() to make it behave >> like the other trylock calls. >> >> Signed-off-by: Waiman Long >> --- >> kernel/locking/semaphore.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/kernel/locking/semaphore.c b/kernel/locking/semaphore.c >> index 34bfae72f295..cb27cbf5162f 100644 >> --- a/kernel/locking/semaphore.c >> +++ b/kernel/locking/semaphore.c >> @@ -127,6 +127,7 @@ EXPORT_SYMBOL(down_killable); >> * >> * NOTE: This return value is inverted from both spin_trylock and >> * mutex_trylock! Be careful about this when converting code. >> + * I.e. 0 on success, 1 on failure. >> * >> * Unlike mutex_trylock, this function can be used from interrupt context, >> * and the semaphore can be released by any task or interrupt. >> @@ -136,7 +137,8 @@ int __sched down_trylock(struct semaphore *sem) >> unsigned long flags; >> int count; >> >> - raw_spin_lock_irqsave(&sem->lock, flags); >> + if (!raw_spin_trylock_irqsave(&sem->lock, flags)) >> + return 1; >> count = sem->count - 1; >> if (likely(count >= 0)) >> sem->count = count; > Urgh, this is terrible *again*. Didn't you try and do something > similarly daft with the rt_mutex_trylock ? And you didn't learn from > that? I also a bit of concern as it is a change in behavior which  may have unintended consequence. I think using the wake_q in semaphore will be a less risky choice. What do you think? Cheers, Longman