From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ej1-f54.google.com (mail-ej1-f54.google.com [209.85.218.54]) (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 AA34618C910 for ; Tue, 31 Dec 2024 11:14:44 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.218.54 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735643687; cv=none; b=nCBog4BtLg2hPXA4wTm3i7qQdq/PQ4ZIZZvd2xe2wJEGhMUsTzEiIvW82MEWbNnx7r0TNjb8BVqB360OPnWli1tacXFel55WiryGjSc/8CKbEspqg6xOS/YTsinsXCJbUytkulmv1UTBlcU4/SOJRBgZ3CAG8lCsmZcLY3Kz55g= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735643687; c=relaxed/simple; bh=rq/DxHG9rMHRWsWAy2l+UZxbUbez/GjIShBVfWWjWyU=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version; b=QykQHNQ119cCn++LFHTFD+V32pW/N3egpLp2D6HvpgO4gd4iSf89x1hsGGI0rYZCBA4QU25H1a2wOipB6R9bVoCxrG6sMjfAjhvmzl+vykO819Cupu56UCOYdVgQAgNlEyXRgX2IdJoCvEvQPeB1eNwnRau6UaIOxE2zdBnQMAM= 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=JXcjPrnn; arc=none smtp.client-ip=209.85.218.54 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="JXcjPrnn" Received: by mail-ej1-f54.google.com with SMTP id a640c23a62f3a-aa67333f7d2so1431907366b.0 for ; Tue, 31 Dec 2024 03:14:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=colorfullife-com.20230601.gappssmtp.com; s=20230601; t=1735643683; x=1736248483; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=MmObXSgsMQGxcaUjzd2u+bVoecE4js+QkvAZp6+RrvE=; b=JXcjPrnn/tCg46hE/WMnm0uP3rXsQi8DoXdj50U0+89neI9erjq599so7GR8Ip0MSK BqFCEw5HL2au6OAy9I3hV/dbTx9/jnuuCQR4H2z+TATP8kgpxxe5UfS4gbPSmjUtRNtm IUXqL+F+WZInJ4y27xeJez8Tnc2FDM5umA5WofCI3oScB0riZQT83Qj6AATk6yn0Igxp an5DyRcP60a8V5Kqq6umnqc0YXLSLDK0bK5C8NYuJBxBBky7E0YXQTx5//ul1kcZaPCE tLfT9o2ffgLGX5nI6aKq/DzOCx/D5hnFpZ++NAAelNv0jyvk4MnmkSvcArvuhMkIbB+A 66SA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1735643683; x=1736248483; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=MmObXSgsMQGxcaUjzd2u+bVoecE4js+QkvAZp6+RrvE=; b=wRZpJYLS9ijYNH23O8FmI8/kfjgszvd2biNDNH+4tu3dNdoP5CvK5x9l6Gr6wvqeNd cMFX6m5+9vM6G9Ky1touP82BDuyWUHH7zk0s2DWCzEjy/zsApEC809eQcTXrgojW42Sh ADcsatv1bZSBouqW34+X087NSOsXiDFP+mh2lWz76AjnzJj2/oe+DErOOZNu4voQjTD0 h65qrFctlp8RsTncjmcSYqBNNByfKn/k5yig6O7MM4oY/O8vx1Hi+j+1weRPD+UqGp1/ Yx4peehIvPp8hn06vXZkEwawpZTV7PxY3xJcYoPrWIZmt5i8cFwSm/HWUd9jJvHFVmDX gNaQ== X-Forwarded-Encrypted: i=1; AJvYcCUgPwoJ7tqyhIDPc1REqLe4Oktjbuut9Ghj7ZAKoE2qCtLfHyU6Kc9N7tCA408TcuiIPjk5n24iSulRtZE=@vger.kernel.org X-Gm-Message-State: AOJu0YwCCvn25xaaMzB03iRdOpOHRYABgGQuRO9xwaJMeY60vmvMky2/ 02Q9VC0kAaiqdTb20YuZbPJKRQi3AuY/JQzqSZDZQ6eN6hxBGqfLYhfHFiK2NQ== X-Gm-Gg: ASbGncuRRZ2wyyoXe2zMeTA5bM5AHKvXD84Nn5MSWN8ZNxOmRv6VkLc3pkx6t7F+C2a h2QdUupXdiVk9w/4Wy3GgfVenrzzOOIaQcYKEAK1yp2c5Iy7tkAZD6PBww5VXhQQL3xhTV0JaE7 tVrnN5C93GnSbx59JCPxn9dcdkOhNpHYYkbO77ztvbDmQHvGYVxDIaOvWtwtPs2PjVxyuoO08ef H2LE5bEGC3l/PYwyZhCQlT8J9wBtB2N/r0un1yrthay4uPprSwxKG1ENU7V4utJ2XgrQ7WsByAm BklfegPKXSa0FqjrWoLXBDqFwf/Pj57nMwgiEePYV1DKyPExFMqLcGy64ZcoBmg= X-Google-Smtp-Source: AGHT+IF2Rh/r6l9SuGGmJwEZqBUTjQ/v9V27hkd4TmS2UKd1ztjF1aEv68oCNfDmXfhxEH727T5mpw== X-Received: by 2002:a17:906:6a28:b0:aa5:391e:cadf with SMTP id a640c23a62f3a-aac334f637fmr3261763366b.42.1735643682798; Tue, 31 Dec 2024 03:14:42 -0800 (PST) Received: from localhost.localdomain (p200300d9971255001e3d0644a090dcde.dip0.t-ipconnect.de. [2003:d9:9712:5500:1e3d:644:a090:dcde]) by smtp.googlemail.com with ESMTPSA id 4fb4d7f45d1cf-5d806fedca8sm16233558a12.61.2024.12.31.03.14.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 31 Dec 2024 03:14:41 -0800 (PST) From: Manfred Spraul To: Oleg Nesterov Cc: Linus Torvalds , WangYuli , linux-fsdevel , Linux Kernel Mailing List , Christian Brauner , 1vier1@web.de, Manfred Spraul Subject: Re: [RESEND PATCH] fs/pipe: Introduce a check to skip sleeping processes during pipe read/write Date: Tue, 31 Dec 2024 12:14:28 +0100 Message-ID: <20241231111428.5510-1-manfred@colorfullife.com> X-Mailer: git-send-email 2.47.1 In-Reply-To: <20241230153844.GA15134@redhat.com> References: <20241230153844.GA15134@redhat.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Hi Oleg, just FYI, I did some quick tests with: - your changes to fs/pipe.c - my change, to skip locking in wake-up (and some smp_mb()) - statistics, to check how often wake_up is called/how often the list is wait queue is actually empty Known issue: Statistic printing every 10 seconds doesn't work, it prints at eratic times. And the comment in __wake_up is still wrong, the memory barrier would pair with smp_mb() after updating wq_head->head. Result: (all with a 2 core 4 thread i3, fully idle system) - your change has no impact on 'find /proc /sys | grep doesnotexist' (using busybox) - Running your test app for around 100 seconds - 3 wakeups with non-empty queue - 26 wakeup with empty queue - 2107 __add_wait_queue - find|grep produces insane numbers of wakeup. I've seen 20k, I've now seen 50k wakeup calls. With just around 2k __add_wait_queue, ... Thus, at least for pipe: Should we add the missing memory barriers and switch to wait_queue_active() in front of all wakeup calls? --- fs/pipe.c | 13 +++++++------ include/linux/wait.h | 5 +++++ kernel/sched/wait.c | 45 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 6 deletions(-) diff --git a/fs/pipe.c b/fs/pipe.c index 12b22c2723b7..27ffb650f131 100644 --- a/fs/pipe.c +++ b/fs/pipe.c @@ -253,7 +253,7 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to) size_t total_len = iov_iter_count(to); struct file *filp = iocb->ki_filp; struct pipe_inode_info *pipe = filp->private_data; - bool was_full, wake_next_reader = false; + bool wake_writer = false, wake_next_reader = false; ssize_t ret; /* Null read succeeds. */ @@ -271,7 +271,6 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to) * (WF_SYNC), because we want them to get going and generate more * data for us. */ - was_full = pipe_full(pipe->head, pipe->tail, pipe->max_usage); for (;;) { /* Read ->head with a barrier vs post_one_notification() */ unsigned int head = smp_load_acquire(&pipe->head); @@ -340,8 +339,10 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to) buf->len = 0; } - if (!buf->len) + if (!buf->len) { + wake_writer |= pipe_full(head, tail, pipe->max_usage); tail = pipe_update_tail(pipe, buf, tail); + } total_len -= chars; if (!total_len) break; /* common path: read succeeded */ @@ -377,7 +378,7 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to) * _very_ unlikely case that the pipe was full, but we got * no data. */ - if (unlikely(was_full)) + if (unlikely(wake_writer)) wake_up_interruptible_sync_poll(&pipe->wr_wait, EPOLLOUT | EPOLLWRNORM); kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT); @@ -391,14 +392,14 @@ pipe_read(struct kiocb *iocb, struct iov_iter *to) return -ERESTARTSYS; mutex_lock(&pipe->mutex); - was_full = pipe_full(pipe->head, pipe->tail, pipe->max_usage); wake_next_reader = true; + wake_writer = false; } if (pipe_empty(pipe->head, pipe->tail)) wake_next_reader = false; mutex_unlock(&pipe->mutex); - if (was_full) + if (wake_writer) wake_up_interruptible_sync_poll(&pipe->wr_wait, EPOLLOUT | EPOLLWRNORM); if (wake_next_reader) wake_up_interruptible_sync_poll(&pipe->rd_wait, EPOLLIN | EPOLLRDNORM); diff --git a/include/linux/wait.h b/include/linux/wait.h index 6d90ad974408..0fdad3c3c513 100644 --- a/include/linux/wait.h +++ b/include/linux/wait.h @@ -166,6 +166,7 @@ extern void add_wait_queue_exclusive(struct wait_queue_head *wq_head, struct wai extern void add_wait_queue_priority(struct wait_queue_head *wq_head, struct wait_queue_entry *wq_entry); extern void remove_wait_queue(struct wait_queue_head *wq_head, struct wait_queue_entry *wq_entry); +extern atomic_t g_add_count; static inline void __add_wait_queue(struct wait_queue_head *wq_head, struct wait_queue_entry *wq_entry) { struct list_head *head = &wq_head->head; @@ -177,6 +178,8 @@ static inline void __add_wait_queue(struct wait_queue_head *wq_head, struct wait head = &wq->entry; } list_add(&wq_entry->entry, head); + smp_mb(); + atomic_inc(&g_add_count); } /* @@ -192,6 +195,8 @@ __add_wait_queue_exclusive(struct wait_queue_head *wq_head, struct wait_queue_en static inline void __add_wait_queue_entry_tail(struct wait_queue_head *wq_head, struct wait_queue_entry *wq_entry) { list_add_tail(&wq_entry->entry, &wq_head->head); + smp_mb(); + atomic_inc(&g_add_count); } static inline void diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c index 51e38f5f4701..07487429dddf 100644 --- a/kernel/sched/wait.c +++ b/kernel/sched/wait.c @@ -110,6 +110,10 @@ static int __wake_up_common_lock(struct wait_queue_head *wq_head, unsigned int m return nr_exclusive - remaining; } +#if 1 +atomic_t g_add_count = ATOMIC_INIT(0); +#endif + /** * __wake_up - wake up threads blocked on a waitqueue. * @wq_head: the waitqueue @@ -124,6 +128,47 @@ static int __wake_up_common_lock(struct wait_queue_head *wq_head, unsigned int m int __wake_up(struct wait_queue_head *wq_head, unsigned int mode, int nr_exclusive, void *key) { +#if 1 +static atomic_t g_slow = ATOMIC_INIT(0); +static atomic_t g_mid = ATOMIC_INIT(0); +static atomic_t g_fast = ATOMIC_INIT(0); +static u64 printtime = 10*HZ; +#endif + 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) { +#if 1 + atomic_inc(&g_fast); +#endif + return 0; + } else { +#if 1 + atomic_inc(&g_mid); +#endif + } + } else { +#if 1 + atomic_inc(&g_slow); +#endif + } +#if 1 + if (get_jiffies_64() > printtime) { + printtime = get_jiffies_64() + 10*HZ; + pr_info("__wakeup: slow/obvious: %d, mid/nearly raced: %d, fast: %d, add: %d.\n", atomic_read(&g_slow), atomic_read(&g_mid), atomic_read(&g_fast), atomic_read(&g_add_count)); + } +#endif return __wake_up_common_lock(wq_head, mode, nr_exclusive, 0, key); } EXPORT_SYMBOL(__wake_up); -- 2.47.1