From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ej1-f42.google.com (mail-ej1-f42.google.com [209.85.218.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C91024A0C for ; Sat, 28 Dec 2024 16:45:19 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.218.42 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735404323; cv=none; b=BhWihchPsUrLDjkvyM4ktvfWk6xQbiWUuqT6OuZMFSsa5eeV/tewoPafAgM3CLZ9ZqoF/q77Vgsq8oJrO9mtPIV+9qgglpoeaGD9SNjnIJtryN5+oBlSEMg0CCzwbXrs3TWTI+QcxDI+fz4TgEp2yP+Cg6G5+fKrdSBKZai0aLM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735404323; c=relaxed/simple; bh=XPRidIZVC18CQUlEKufW8F52jxa4BJHyPOo2xrs11V0=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=Grl0CK82O1CJDsffpuTJS/jmbt7CS69Yo37DVQqWYZGmTjVRjjXdGR0iK+tn5eSXdYLRL6kqKeNQFO6/fZX657jSvXeK4ZHw2BNJ3RZ/fmsQ4EqJT4QsgaZyoj7av8ANUdG+CH+X7tN3FxTTlGQxw5bXPj0J3poxVeuwZPRzxyM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=colorfullife.com; spf=pass smtp.mailfrom=colorfullife.com; dkim=pass (2048-bit key) header.d=colorfullife-com.20230601.gappssmtp.com header.i=@colorfullife-com.20230601.gappssmtp.com header.b=DVlWArlU; arc=none smtp.client-ip=209.85.218.42 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=colorfullife.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=colorfullife.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=colorfullife-com.20230601.gappssmtp.com header.i=@colorfullife-com.20230601.gappssmtp.com header.b="DVlWArlU" Received: by mail-ej1-f42.google.com with SMTP id a640c23a62f3a-aa67f31a858so1441663066b.2 for ; Sat, 28 Dec 2024 08:45:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=colorfullife-com.20230601.gappssmtp.com; s=20230601; t=1735404318; x=1736009118; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=Cz1VqmSfgwulj4TtgG0EFMyBOjhXN78eW0/1No1Ds5s=; b=DVlWArlUBAMQJbrOEpknvPgE6MCbg52xa47A0eVowXnoLhmSldmT7SuMX/kiU+xaAQ mh0uKIddoNfiAWZb/l8R7QjcTQnlPedjrIFDF+9tWkBDtTmDjFYdQ5RI8FFGysyNdJqI RH6EdaKdRuhKL1Q6rOpa6wGZuGm27lODmIlA366H7JehGyB3xdDGUtyOLLrtvF8OCTkv BD4kYkXhKtQvFTlZA1y3JgICJU5Flqu92i3iS4bqnj0gfvE0L3t2QgA8nReSqXfII+pc PUf05uwqM2FDgVqFEAMns9jnWQ3bkHgbIjzclc0ek9X0QlCcyVlb8Bvc/l9+nBcB4HOd Clvg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1735404318; x=1736009118; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Cz1VqmSfgwulj4TtgG0EFMyBOjhXN78eW0/1No1Ds5s=; b=pnpoGPWlHZcRH3PCzMZNXQx2xIKiNeewukbRLMicWDwomJlZkjcbZ2B5o3KLoF9Zha 5yGpOcu7ZuPg2bJ+sZWECk2pSg0IxxM0+kHqSgxx2wNYZpjWy6DQE1KNVIV7ZtKCFPjM 1b+SrNiJWHnb95UmqzhjvUxfCxeesg80I0eIdnz/nGSyfcNxQ4efl/KpBHgqQ3HCvGVc 8rsSHqlAfrLmBuy+e4guEH337tmzYOWmsqz6MUY8dCcHljMqpGd1M5MsSmS1uMMBCbmM RYBCTy/o35J5aoNGAWTggL4F6iIyGxgOZ1tQG+2I1QY+VbFCqgYQniZEgmBYT1YJ//Zh OMYA== X-Forwarded-Encrypted: i=1; AJvYcCV08ZVpChN1rpulg9q1CqFQQ3qALEK5SKwlP8NT/HCDdbG03xDYnNyJ4tgu9RRedS5faQlTZBE2S1pQEMM=@vger.kernel.org X-Gm-Message-State: AOJu0YxIrG0sGKPT7xvdNm+/ozpUyzWir5RWEoDW9qx+YmDJAtIAe6Zk ieffNuHqndXX/o4IRjixb6Mkxz7PqZHzi14MzBBq9GLiKiwSqFGUBJ3vnajQEA== X-Gm-Gg: ASbGncu+LK5eIHv1kx7ygwsvr7fGUguwwajMJk+EEO8+FX55iVVO7bDagMw59e+0DQp zAPh71Y8Q3/+0zjRiYZtbgRZOf7quOpojyuUhiJFizdXy2tt8N81c5XH1hetM4TVjQJqCFsOV17 XUmiuYrYgoCTBtpmANqQwUE0L6eA4m7snL7BgIHst7eiNj8ICplEq7dDuXs4GCOidyMrRaAFEzb bUUhKbYPeUH+nYzuMbPxq4TbReUNVRJ2MBzIlZkI3Vg3npWoJZP+RtRbpKs125eC+1SNLU7rcKT nWjTOP4PN+AM6+Qd7/YraZolge2XVTpuUwU3eTQOgC2VNxEtzEmGTcHk7cdPKgoq18bvWApqr1J hznMIZWAvLVFqnwVB+Zg= X-Google-Smtp-Source: AGHT+IEwCOkVYEmRIyllgGUbBkB/nkRg+m8awrnv/+Y37IbH6AsgZsVAegd6S/68CdBE0Kokug+VPA== X-Received: by 2002:a17:907:7f8e:b0:aa6:9198:75a2 with SMTP id a640c23a62f3a-aac334e51afmr2680214366b.44.1735404317930; Sat, 28 Dec 2024 08:45:17 -0800 (PST) Received: from ?IPV6:2003:d9:974e:9900:6aac:89d9:5e45:a0e6? (p200300d9974e99006aac89d95e45a0e6.dip0.t-ipconnect.de. [2003:d9:974e:9900:6aac:89d9:5e45:a0e6]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-aac0e895366sm1258629566b.73.2024.12.28.08.45.16 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 28 Dec 2024 08:45:16 -0800 (PST) Message-ID: Date: Sat, 28 Dec 2024 17:45:15 +0100 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: [RESEND PATCH] fs/pipe: Introduce a check to skip sleeping processes during pipe read/write To: Oleg Nesterov Cc: Linus Torvalds , WangYuli , linux-fsdevel , Linux Kernel Mailing List , Christian Brauner References: <75B06EE0B67747ED+20241225094202.597305-1-wangyuli@uniontech.com> <20241226201158.GB11118@redhat.com> <1df49d97-df0e-4471-9e40-a850b758d981@colorfullife.com> <20241228143248.GB5302@redhat.com> <20241228152229.GC5302@redhat.com> Content-Language: en-US From: Manfred Spraul In-Reply-To: <20241228152229.GC5302@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi Oleg, On 12/28/24 4:22 PM, Oleg Nesterov wrote: > On 12/28, Oleg Nesterov wrote: >>> int __wake_up(struct wait_queue_head *wq_head, unsigned int mode, >>> int nr_exclusive, void *key) >>> { >>> + if (list_empty(&wq_head->head)) { >>> + struct list_head *pn; >>> + >>> + /* >>> + * pairs with spin_unlock_irqrestore(&wq_head->lock); >>> + * We actually do not need to acquire wq_head->lock, we just >>> + * need to be sure that there is no prepare_to_wait() that >>> + * completed on any CPU before __wake_up was called. >>> + * Thus instead of load_acquiring the spinlock and dropping >>> + * it again, we load_acquire the next list entry and check >>> + * that the list is not empty. >>> + */ >>> + pn = smp_load_acquire(&wq_head->head.next); >>> + >>> + if(pn == &wq_head->head) >>> + return 0; >>> + } >> Too subtle for me ;) >> >> I have some concerns, but I need to think a bit more to (try to) actually >> understand this change. > If nothing else, consider > > int CONDITION; > wait_queue_head_t WQ; > > void wake(void) > { > CONDITION = 1; > wake_up(WQ); > } > > void wait(void) > { > DEFINE_WAIT_FUNC(entry, woken_wake_function); > > add_wait_queue(WQ, entry); > if (!CONDITION) > wait_woken(entry, ...); > remove_wait_queue(WQ, entry); > } > > this code is correct even if LOAD(CONDITION) can leak into the critical > section in add_wait_queue(), so CPU running wait() can actually do > > // add_wait_queue > spin_lock(WQ->lock); > LOAD(CONDITION); // false! > list_add(entry, head); > spin_unlock(WQ->lock); > > if (!false) // result of the LOAD above > wait_woken(entry, ...); > > Now suppose that another CPU executes wake() between LOAD(CONDITION) > and list_add(entry, head). With your patch wait() will miss the event. > The same for __pollwait(), I think... > > No? Yes, you are right. CONDITION =1 is worst case written to memory from the store_release() in spin_unlock(). this pairs with the load_acquire for spin_lock(), thus LOAD(CONDITION) is safe. It could still work for prepare_to_wait and thus fs/pipe, since then the smb_mb() in set_current_state prevents earlier execution. --     Manfred