From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 753D7D2E008 for ; Wed, 23 Oct 2024 02:32:48 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D78F36B00B8; Tue, 22 Oct 2024 22:32:47 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D29F96B00B9; Tue, 22 Oct 2024 22:32:47 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BA1266B00BA; Tue, 22 Oct 2024 22:32:47 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 9B1886B00B8 for ; Tue, 22 Oct 2024 22:32:47 -0400 (EDT) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id DAF071606C3 for ; Wed, 23 Oct 2024 02:32:27 +0000 (UTC) X-FDA: 82703292840.17.F1D439B Received: from mail-vs1-f44.google.com (mail-vs1-f44.google.com [209.85.217.44]) by imf19.hostedemail.com (Postfix) with ESMTP id 912FD1A0003 for ; Wed, 23 Oct 2024 02:32:24 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=CYT0P80D; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf19.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.217.44 as permitted sender) smtp.mailfrom=21cnbao@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1729650598; h=from:from:sender: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:dkim-signature; bh=znxApOC+uNv7EYMj9gabTY3S89oruXYjfhDj/QrnRGM=; b=efdGN5e8kilt1FCdLng52jrRg0O02DsCR5nCI0DRMlcMX/cUXMh7hoo3FEGuOje6h6NGQk /Xq82gRA4yrSCcNNPe31N79Xe3/zYKyTMk6RJCRX2yvgav9N4LgZm2/puILiroBZHyPr0O NkWqFgEWtqKzW435CWYlQwMmp0/dU3k= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1729650598; a=rsa-sha256; cv=none; b=v+s15dHtuAJSPCs0fUXEhviK1OosFrsvQ3Pts1pBwlB589M2nKW+ocnJZyjgfho9AB6wg4 DDWc72NVuowEzgmnaEILNTeUtoh+/BMzaPeGEf25gqwGQd/8ChvGc3gzqUDftdzHFs4GhP xKid8VmCtC/zKyBE0QW5yN2yQWh1Uo8= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=CYT0P80D; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf19.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.217.44 as permitted sender) smtp.mailfrom=21cnbao@gmail.com Received: by mail-vs1-f44.google.com with SMTP id ada2fe7eead31-4a47fff85d2so1980669137.0 for ; Tue, 22 Oct 2024 19:32:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1729650764; x=1730255564; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=znxApOC+uNv7EYMj9gabTY3S89oruXYjfhDj/QrnRGM=; b=CYT0P80D78l7fhKeSNtg7TGLygPpq2SlwhnyviUw3lU7gHtU/i1nw84lIScBKCX5D8 /4qRLqkDwJCGEu3a7HmP/q0VGZYi10WSik9KGQWbHb8IOa0IrKqFI4GCQFHx4sAAV6mi Hihq2Xdsv53VWI6vqodYji8E5VbZjEah9v7m7Q+4OFLFOZJTpww5f3RVxckMusw75GwE //mVYYH8O4bTRrUbvNTseK1lyzrq4Leif7x9E2AZy/1ect4sI+RqlFnnfbF6i+oqvl4f our04cHExEAP4OdV10NHk64QZtVB3QvVXJm19EdwmWbyrfw9bqq1544ZCzBK8MEeh+Mk py+w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1729650764; x=1730255564; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=znxApOC+uNv7EYMj9gabTY3S89oruXYjfhDj/QrnRGM=; b=nw6fK3uekR3yKTS0+EDlqnsCGeIPfEXADC6hxwj6axpvy1Ea+b7UGk8M3cILFKke3v Z29Rp9ZmZ+WrjM9q23UwDlbVmQ57rFUYWV/IYiyTIcpTKLzc3pQ6DgH/YNHa94Ebkeim tT6Fw1Ch2CWcbbwpDvJhMyiKlLjUNKy1DnfwlOyDxSlqdx9H+OBUdgcnxmG/a22/lHE7 gWlQv6V0q9/pX4XBxQPVFx9/6PeU1PM5bAKKNNvIrZkyj7IXUXhq2TrcigMF0cYkCZ2l 5YkZVtJbCt+Wqb/u4TLbmAmZH8aRdQqW7fGZLp77d/XWgidJCOEXr0tr3mwj/kzAGQC1 sE8Q== X-Forwarded-Encrypted: i=1; AJvYcCWhepezH0+9ks9Bg5oljWgnNN6+dI8B6UXnijDYIfvX8rvFHZMz7CSdXTNDaAvOyHsT4hO0IHjVnQ==@kvack.org X-Gm-Message-State: AOJu0Yy+6ybgQB1FQOq2IBGWAKRSAE3PVoW9K6QvpIqA0s/cZaoiaxPw YhR9lvn30Ujdt+OuWfcnwr+vp+4/iYWeuod7A6eo5zRGzXnAghaUbB5MHeDjMfayUYNPskSyS74 W532P9TLapsJmdS7VITD/dni93d8= X-Google-Smtp-Source: AGHT+IFeWPj+ZmewRQE8kwIwSh3ID+aLKdiBrIR2ByPOBmY9r1G4coEjq1Y0ZZKHFvz23lZO4QvWAP+46cFsqTRS83M= X-Received: by 2002:a05:6102:3a0d:b0:4a5:b5db:ec5e with SMTP id ada2fe7eead31-4a751cb4ef0mr1363993137.27.1729650764351; Tue, 22 Oct 2024 19:32:44 -0700 (PDT) MIME-Version: 1.0 References: <87ikuani1f.fsf@yhuang6-desk2.ccr.corp.intel.com> <20241008130807.40833-1-21cnbao@gmail.com> <87set6m73u.fsf@yhuang6-desk2.ccr.corp.intel.com> <87iktj4m3u.fsf@yhuang6-desk2.ccr.corp.intel.com> In-Reply-To: <87iktj4m3u.fsf@yhuang6-desk2.ccr.corp.intel.com> From: Barry Song <21cnbao@gmail.com> Date: Wed, 23 Oct 2024 15:32:33 +1300 Message-ID: Subject: Re: [PATCH] mm: avoid unconditional one-tick sleep when swapcache_prepare fails To: "Huang, Ying" Cc: Kairui Song , akpm@linux-foundation.org, chrisl@kernel.org, david@redhat.com, hannes@cmpxchg.org, hughd@google.com, kaleshsingh@google.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org, liyangouwen1@oppo.com, mhocko@suse.com, minchan@kernel.org, sj@kernel.org, stable@vger.kernel.org, surenb@google.com, v-songbaohua@oppo.com, willy@infradead.org, yosryahmed@google.com, yuzhao@google.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: 912FD1A0003 X-Stat-Signature: gs8ecuzg9swza7hm9qatoz3nua8jmqh4 X-Rspam-User: X-HE-Tag: 1729650744-562264 X-HE-Meta: U2FsdGVkX1/8pMKP40RagThNy25TY3dEE2VbfyBWrjZgIOvOdYg9zBBs29fUCdbyckzdZPYaO29R8X4fGAUxEw42NixpHku5S+XII0eb0hKVpvH7m0brxQoAlXNKD81rLAvbDADHzXWnScNsFbgUTN6CAUX+7CdaO4asjnuJSOViP2lzMljzD6oTV3EOJLA3iHfM0oQZ78EE8t8G6RhYKTX0UdSxBjvZTPRw3uzf/j7w5eVQucY/v2Da1luSnoEbxm2n9f2dFYiV46ZXdVDzucER7Yh+nSkhP4rem48UpnbUCLdnqvJFmnb6NkUG2gDoPg9qeB6dPKz+uZ4HxebyIMd9raMf6VE7FuqdWgCcDqt6O+xVgFPZguFScs/Bo6DMN0lnn2ERe2KJl7fO/yxgFUnH6b0+3y49nCpePaTBZrdVCwicYaZzA8/xT9w9XEeMe8Di9XCUgjIADLc+L2Qfo8emCJMeRjVrQF6vCIifUn7SAhLBXDeQ4QVrUEpmiOTw56hwvXj7wg4fO89yomWtmqUAn+3UhfazodL8W3LytBJxrw+4n3F6ToXAs+JkLB+s/1/B0Rp+ZDA5dJiQl7kry5d2HBsIS1NzVSjhOvl2e88hAZRNuuOxjjpREGv8Fz/+wNMKUmSoAvidlD7PPgEa9SRwGojPlUS6xxOZR/IsQicd9Gc6nFVVFYFcpF8Y0PyUvhAFJoob/ihibOGgOw8nPY0rnEw85vfc+dlti4l3KnYDot5pAGD+zdJIVzWi68Ad8T5ytGKMA3cgJ8AyMYGNZZeRStAILuzjU+hijiMQ/0XmXlcaf6LKqYbIi6T05+QcPcnLwrLqjSM8mhKW6Jnl7U+ADxYQCuDhtBJUvdtX3CqxaZmrweYfakcokErZ80kvDTfIwiLR27mrSD6A4x8MJ2eihVoD21fBcV9QTk+CoNG2b03hInkRt/RMICOlN9YARSesDQkcCSzhr25Tlpp f5RqkvVa VQjrTEk/UEE1B9QUP1Vy3QBBZZt0pT5Cp96lBkDJrdxEOxya4cagVxCOzHV/awcY2PgtZt4U6fada/oo49tN5nTzoJ4BGcoGmO/iNfq+kftA5JMUNzjmnBR4cQ8vPpzwVd94pj3ltgYxSfl19kVk/X3k3T0Zt/HtKSWpOc1QsbLao4KqMwJsO9qF8Ts/FQAX1zAYcgAleBsM4ZuT+6GNqpDBlDswhwx8kEynkWDvmjx83u1tgwkz8yx3EwoLrHvAWxeXduCfZrZvoVblYGEc1glHORoTssLkkhjuAEcJ5wglnRkp/wMdBw+APlJp/QQ20r97oPqNAxR3Y5E2efKQnQFJN7ketrEVmEhhrSFq3BhWAjFLj4Ua4AW+WkwIxOWyUWCLLSKlxuLmzCZdV1dW5AzWcyxAgloBsr6GGW109aBiBbzlmpag8D0VpoeLCTSx9VqeMEe8AKF7RqVJ+5zDjeE6rS2Kz05zf9FLaf6Z8pVgozIRFkO9/Br2ryihpC9HHsYOjgjBzOyxswss= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Wed, Oct 23, 2024 at 3:01=E2=80=AFPM Huang, Ying = wrote: > > Kairui Song writes: > > > On Wed, Oct 9, 2024 at 8:55=E2=80=AFAM Huang, Ying wrote: > >> > >> Barry Song <21cnbao@gmail.com> writes: > >> > >> > On Thu, Oct 3, 2024 at 8:35=E2=80=AFAM Huang, Ying wrote: > >> >> > >> >> Barry Song <21cnbao@gmail.com> writes: > >> >> > >> >> > On Wed, Oct 2, 2024 at 8:43=E2=80=AFAM Huang, Ying wrote: > >> >> >> > >> >> >> Barry Song <21cnbao@gmail.com> writes: > >> >> >> > >> >> >> > On Tue, Oct 1, 2024 at 7:43=E2=80=AFAM Huang, Ying wrote: > >> >> >> >> > >> >> >> >> Barry Song <21cnbao@gmail.com> writes: > >> >> >> >> > >> >> >> >> > On Sun, Sep 29, 2024 at 3:43=E2=80=AFPM Huang, Ying wrote: > >> >> >> >> >> > >> >> >> >> >> Hi, Barry, > >> >> >> >> >> > >> >> >> >> >> Barry Song <21cnbao@gmail.com> writes: > >> >> >> >> >> > >> >> >> >> >> > From: Barry Song > >> >> >> >> >> > > >> >> >> >> >> > Commit 13ddaf26be32 ("mm/swap: fix race when skipping sw= apcache") > >> >> >> >> >> > introduced an unconditional one-tick sleep when `swapcac= he_prepare()` > >> >> >> >> >> > fails, which has led to reports of UI stuttering on late= ncy-sensitive > >> >> >> >> >> > Android devices. To address this, we can use a waitqueue= to wake up > >> >> >> >> >> > tasks that fail `swapcache_prepare()` sooner, instead of= always > >> >> >> >> >> > sleeping for a full tick. While tasks may occasionally b= e woken by an > >> >> >> >> >> > unrelated `do_swap_page()`, this method is preferable to= two scenarios: > >> >> >> >> >> > rapid re-entry into page faults, which can cause liveloc= ks, and > >> >> >> >> >> > multiple millisecond sleeps, which visibly degrade user = experience. > >> >> >> >> >> > >> >> >> >> >> In general, I think that this works. Why not extend the s= olution to > >> >> >> >> >> cover schedule_timeout_uninterruptible() in __read_swap_ca= che_async() > >> >> >> >> >> too? We can call wake_up() when we clear SWAP_HAS_CACHE. = To avoid > >> >> >> >> > > >> >> >> >> > Hi Ying, > >> >> >> >> > Thanks for your comments. > >> >> >> >> > I feel extending the solution to __read_swap_cache_async() = should be done > >> >> >> >> > in a separate patch. On phones, I've never encountered any = issues reported > >> >> >> >> > on that path, so it might be better suited for an optimizat= ion rather than a > >> >> >> >> > hotfix? > >> >> >> >> > >> >> >> >> Yes. It's fine to do that in another patch as optimization. > >> >> >> > > >> >> >> > Ok. I'll prepare a separate patch for optimizing that path. > >> >> >> > >> >> >> Thanks! > >> >> >> > >> >> >> >> > >> >> >> >> >> overhead to call wake_up() when there's no task waiting, w= e can use an > >> >> >> >> >> atomic to count waiting tasks. > >> >> >> >> > > >> >> >> >> > I'm not sure it's worth adding the complexity, as wake_up()= on an empty > >> >> >> >> > waitqueue should have a very low cost on its own? > >> >> >> >> > >> >> >> >> wake_up() needs to call spin_lock_irqsave() unconditionally o= n a global > >> >> >> >> shared lock. On systems with many CPUs (such servers), this = may cause > >> >> >> >> severe lock contention. Even the cache ping-pong may hurt pe= rformance > >> >> >> >> much. > >> >> >> > > >> >> >> > I understand that cache synchronization was a significant issu= e before > >> >> >> > qspinlock, but it seems to be less of a concern after its impl= ementation. > >> >> >> > >> >> >> Unfortunately, qspinlock cannot eliminate cache ping-pong issue,= as > >> >> >> discussed in the following thread. > >> >> >> > >> >> >> https://lore.kernel.org/lkml/20220510192708.GQ76023@worktop.prog= ramming.kicks-ass.net/ > >> >> >> > >> >> >> > However, using a global atomic variable would still trigger ca= che broadcasts, > >> >> >> > correct? > >> >> >> > >> >> >> We can only change the atomic variable to non-zero when > >> >> >> swapcache_prepare() returns non-zero, and call wake_up() when th= e atomic > >> >> >> variable is non-zero. Because swapcache_prepare() returns 0 mos= t times, > >> >> >> the atomic variable is 0 most times. If we don't change the val= ue of > >> >> >> atomic variable, cache ping-pong will not be triggered. > >> >> > > >> >> > yes. this can be implemented by adding another atomic variable. > >> >> > >> >> Just realized that we don't need another atomic variable for this, = just > >> >> use waitqueue_active() before wake_up() should be enough. > >> >> > >> >> >> > >> >> >> Hi, Kairui, > >> >> >> > >> >> >> Do you have some test cases to test parallel zram swap-in? If s= o, that > >> >> >> can be used to verify whether cache ping-pong is an issue and wh= ether it > >> >> >> can be fixed via a global atomic variable. > >> >> >> > >> >> > > >> >> > Yes, Kairui please run a test on your machine with lots of cores = before > >> >> > and after adding a global atomic variable as suggested by Ying. I= am > >> >> > sorry I don't have a server machine. > >> >> > > >> >> > if it turns out you find cache ping-pong can be an issue, another > >> >> > approach would be a waitqueue hash: > >> >> > >> >> Yes. waitqueue hash may help reduce lock contention. And, we can = have > >> >> both waitqueue_active() and waitqueue hash if necessary. As the fi= rst > >> >> step, waitqueue_active() appears simpler. > >> > > >> > Hi Andrew, > >> > If there are no objections, can you please squash the below change? = Oven > >> > has already tested the change and the original issue was still fixed= with > >> > it. If you want me to send v2 instead, please let me know. > >> > > >> > From a5ca401da89f3b628c3a0147e54541d0968654b2 Mon Sep 17 00:00:00 20= 01 > >> > From: Barry Song > >> > Date: Tue, 8 Oct 2024 20:18:27 +0800 > >> > Subject: [PATCH] mm: wake_up only when swapcache_wq waitqueue is act= ive > >> > > >> > wake_up() will acquire spinlock even waitqueue is empty. This might > >> > involve cache sync overhead. Let's only call wake_up() when waitqueu= e > >> > is active. > >> > > >> > Suggested-by: "Huang, Ying" > >> > Signed-off-by: Barry Song > >> > --- > >> > mm/memory.c | 6 ++++-- > >> > 1 file changed, 4 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/mm/memory.c b/mm/memory.c > >> > index fe21bd3beff5..4adb2d0bcc7a 100644 > >> > --- a/mm/memory.c > >> > +++ b/mm/memory.c > >> > @@ -4623,7 +4623,8 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > >> > /* Clear the swap cache pin for direct swapin after PTL unlock= */ > >> > if (need_clear_cache) { > >> > swapcache_clear(si, entry, nr_pages); > >> > - wake_up(&swapcache_wq); > >> > + if (waitqueue_active(&swapcache_wq)) > >> > + wake_up(&swapcache_wq); > >> > } > >> > if (si) > >> > put_swap_device(si); > >> > @@ -4641,7 +4642,8 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > >> > } > >> > if (need_clear_cache) { > >> > swapcache_clear(si, entry, nr_pages); > >> > - wake_up(&swapcache_wq); > >> > + if (waitqueue_active(&swapcache_wq)) > >> > + wake_up(&swapcache_wq); > >> > } > >> > if (si) > >> > put_swap_device(si); > >> > >> Hi, Kairui, > >> > >> Do you have time to give this patch (combined with the previous patch > >> from Barry) a test to check whether the overhead introduced in the > >> previous patch has been eliminated? > > > > Hi Ying, Barry > > > > I did a rebase on mm tree and run more tests with the latest patch: > > > > Before the two patches: > > make -j96 (64k): 33814.45 35061.25 35667.54 36618.30 37381.60 37678.75 > > make -j96: 20456.03 20460.36 20511.55 20584.76 20751.07 20780.79 > > make -j64:7490.83 7515.55 7535.30 7544.81 7564.77 7583.41 > > > > After adding workqueue: > > make -j96 (64k): 33190.60 35049.57 35732.01 36263.81 37154.05 37815.50 > > make -j96: 20373.27 20382.96 20428.78 20459.73 20534.59 20548.48 > > make -j64: 7469.18 7522.57 7527.38 7532.69 7543.36 7546.28 > > > > After adding workqueue with workqueue_active() check: > > make -j96 (64k): 33321.03 35039.68 35552.86 36474.95 37502.76 37549.04 > > make -j96: 20601.39 20639.08 20692.81 20693.91 20701.35 20740.71 > > make -j64: 7538.63 7542.27 7564.86 7567.36 7594.14 7600.96 > > > > So I think it's just noise level performance change, it should be OK > > in either way. Thanks for Kairui's testing. > > Thanks for your test results. There should be bottlenecks in other > places. Exactly. I=E2=80=99d expect cache ping-pong to become noticeable only when the spinlock is highly contended=E2=80=94such as when many threads simultaneously follow the pattern below: spin_lock short-time operations spin_unlock But we=E2=80=99re likely dealing with a different pattern, as shown below: long-time operations spin_lock short-time operations spin_unlock > > -- > Best Regards, > Huang, Ying Thanks Barry