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 0B849CF8849 for ; Fri, 4 Oct 2024 15:35:55 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7F8EB6B009E; Fri, 4 Oct 2024 11:35:54 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 7A8B36B015C; Fri, 4 Oct 2024 11:35:54 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 649986B00EB; Fri, 4 Oct 2024 11:35:54 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 457646B03FA for ; Fri, 4 Oct 2024 11:35:54 -0400 (EDT) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id ABCECA1A05 for ; Fri, 4 Oct 2024 15:35:53 +0000 (UTC) X-FDA: 82636320186.27.E972F2F Received: from mail-vk1-f180.google.com (mail-vk1-f180.google.com [209.85.221.180]) by imf08.hostedemail.com (Postfix) with ESMTP id E203C16000E for ; Fri, 4 Oct 2024 15:35:50 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=lbVNeAi2; spf=pass (imf08.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.221.180 as permitted sender) smtp.mailfrom=21cnbao@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1728056019; 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=Nw2sq7oLbYRyM54E/JZVt3qqEmIecjILj6ka5iJnvA8=; b=F5TB2qJ66A28VpvmV9Feuzj624d7aVXFNv250tJ8qeVneo554Bgjq+W+BEhaomL3NRz97z sbHQgbel4LjkDPmkK9ji5Tm2qYR4EypERH2aE5f7bSWaB+/+nHF2gk5Q9JTyC0zKJkxmIo 23e6PPDEHRAszx28htlxreSDnNq3X4Y= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1728056020; a=rsa-sha256; cv=none; b=V3u9JIg6LRQ7gKX/0G+213TGmVlgxwTvWzdybVsFY12nPPUr7yncWVKYCTp4TB17XHxXEX nQYSZkZSi/K8UbyFvcyq0/O812XWwxcV8cJ/0iFhjA+JP3Z6Unt5bEg0xk+zj3kl4fT9tc sKLyuk0ZKA//LRyKeFbveZSJf9fB/VA= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=lbVNeAi2; spf=pass (imf08.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.221.180 as permitted sender) smtp.mailfrom=21cnbao@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-vk1-f180.google.com with SMTP id 71dfb90a1353d-50a87472832so620457e0c.3 for ; Fri, 04 Oct 2024 08:35:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1728056150; x=1728660950; 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=Nw2sq7oLbYRyM54E/JZVt3qqEmIecjILj6ka5iJnvA8=; b=lbVNeAi24VLSxO5D8NoJRS2ToDpPZCR1V7KRrOK/2/mGbqgba7qmzEEhmyAocYhFm5 pgNNRNJD4vkRbZfeOA47fspjIz093w3c6+saC6KM7F9ummnR+5coHr7khAxfEEK8kLFP NsUbDJawE3Pi8ELNhsTLvmnQBPNtf+bFRVyo894aoJ6ps4E8yXhZS1SHEmUqTrYhjAF2 /P3398mmXYHJx5Vxx1kAKtGM7PaPuzZKDrqamouIT5Unazl2/p4601n2mHdT6LkV31zd 5WwSCURYnZ3ZUdvEJp1O4/L2Ewot1YE9Bk40UErzTRY42+CFA2CIjog3+OWZRi82kJgi ggRQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1728056150; x=1728660950; 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=Nw2sq7oLbYRyM54E/JZVt3qqEmIecjILj6ka5iJnvA8=; b=kJjBR3yCsGtAnLkazM93XC0hGIkMImjrZ2Fj5uua7mQVIlW3P9g2MwQRfDKEFe9XZ2 hBNz4dNK50P/eS9ytBKtjpOXZO7SmLt+FgI1BNBm4S73I7jR0UK/LVTYtiU5jNP7uwN7 NY8BHgFfL0RmVd9T1wt5hdb43+HoSlvotAG1XFvlT0G7P88ux2L/tSw77ZgX1Xyvo+UP En1P0fv17lUfUJt7BYMNaZA4nn18VW92SobWkwhQCAhhox3QzVNLD6xhRgdZVEtltSlr /SgUJ7DFhesuoAOWXR4GnaGZebs62CJlOfRRQtbXPw0vOtqDBCxezK6Eufuyrie7I793 wGew== X-Forwarded-Encrypted: i=1; AJvYcCWFAZecqXngtd1jw8FCGMhwtGzWHmxzGZXRPaljXn8j1+PPITWoUD8sX0TxExLs3I4r38Riv5zfNQ==@kvack.org X-Gm-Message-State: AOJu0Yy2Fi7/I0SRvZpHBtYYsm/+7dKetLt3xSo/eeA2AT6+L10saD52 D+RtRd1vftWVjsHzgnxCYIoB+p/VpfsVOo+BCXGgyXkQIY5xE/84w1qVSDr6gWQSPrI/BCL4TnP 65tP38iJkyQoo4Xx3y0dXcV8hGG4= X-Google-Smtp-Source: AGHT+IEX6UhKKZvWjPQzKs3x120qKfcrb4jvMUBiNStNVz5nW9YCWNdPLZvRmnNxntvKi09plSGnjExnW8RMzBcETcs= X-Received: by 2002:a05:6122:1829:b0:50a:b8c6:b689 with SMTP id 71dfb90a1353d-50c854487abmr2562637e0c.3.1728056149818; Fri, 04 Oct 2024 08:35:49 -0700 (PDT) MIME-Version: 1.0 References: <87y137nxqs.fsf@yhuang6-desk2.ccr.corp.intel.com> <20241002015754.969-1-21cnbao@gmail.com> In-Reply-To: From: Barry Song <21cnbao@gmail.com> Date: Fri, 4 Oct 2024 23:35:06 +0800 Message-ID: Subject: Re: [PATCH] mm: avoid unconditional one-tick sleep when swapcache_prepare fails To: Chris Li Cc: ying.huang@intel.com, akpm@linux-foundation.org, david@redhat.com, hannes@cmpxchg.org, hughd@google.com, kaleshsingh@google.com, kasong@tencent.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-Stat-Signature: gu56dsiumh64hswga5cy39eh8gbrxrs7 X-Rspamd-Queue-Id: E203C16000E X-Rspam-User: X-Rspamd-Server: rspam08 X-HE-Tag: 1728056150-999505 X-HE-Meta: U2FsdGVkX1+B1qU97B7AbCIhTj3L7ZsCF3KsQWygKsfx6Tle6YS6esbDOjHxKjeXuo8tp1pX3wg9Z4gUzW5om04e+825SetmUcn32Cm0f/Yg1KfRf/FXmRth6i32L5AxIzASXExSkQFKalTYG062buR7R9kBoWoegvHsvV8pZzBuS9ycjLlSPfgKuSbrt0TjUUer3rbG+bacTmddW+ORRjNdwhuk4nD/MMOjiPPD7nHNh0eWNvPRtN5Fybhp1C689ipku3kQwgQ3iIMp5ZvuSDAR3+Yn9fCUUTXxqYa0cIKkzoHcP15QHhQHuPNXm+kvSYVHib4MnKQMo9ynaFA8Ez+YYZDWQvzlcNiauPEIqBPAQNHqjBqjdzXhJQ4BotXDZi8M8gQbwrabmLSw+Z2eLN8EjF1GcVfSUT3JUqqMNCsTNSU9beBcPwsoxWHwsU/5FNDEga1zRDxBBYnSIFQh6cOYMScOHiOZVfqGxbCxZ5ZvKvp3L/6c9FKHmMTq0dXCjCT7+TEAv5x7GGom+f8Si/r3jtYTCjUgo9dGEa072OxqKTLx3wj1ez1XAQmE5c/DH8jDJCVwtaI89CRxPTKn6Tq8CUj4+NM5psmWJkZI58x8gW8MYBuAurNZ8p+s1/CcEaChEEXBSEdzpeleFUDQDwvx5B1UWhGshCNaP7QetAE9LXJdEgxLb4gG7vS5OrUCNXy/K5d53XjvtBTCdIgYJWFN2ksNCQ0ZnWccM2g3LCfC7595aRyMxPYg+8+s7VFw/MQgyeHwMkQUquU4Ofo8JCNkQQ222tk4Tb35OV0oH8o7abdKf1/BurtJUSAc9orPyOVtDRf3IokGepwhJ1B9jLHXUZvY4YVBmojN795nsTTPXD2goYdCvRTSF8bw4Yf6SbKj5rSRCR3bZlmp6UJwzJM5GGfwh0/nUIE6za9zl2pcSxLuy1wD7Okxj0BjD0AeeHYI5hxHoQxct/sENlG BdYApx+p fETNO7HNl7wFVtpjNPt0AgmJX4jLnTz0MkdttjgS3LSc5Zvc6WM17Jj9bLGTtnEK5MdqiXzfcO39zICi63ukYSHAVpZYqIJm3lQ5naSTQfJl1Nm0h+nMHOSCn/7QvXpTzlzC9UUch1WujEua74+fkFb7JQJpe4203dIaXExdwgDG+6MQco+ZOJBM4Ri8hgLHQaRy78fz244ixuN6vHFhiIzdfz2STwQqGV3AMXfDK+Gr1X5Nd2N1IghVOmGehU+izQjG2h2VCfoEnsTkIPuOB88TcvuVGNaE2YWsopYFYy05gBDDkKb7RbiMRVp4M49tlTUtigoi9zUCwzYqY3DJUohfcLSwPCs9DiHqT6dlrEk3PfMFwA9bwDS7EukHBAj1HAX3Vq4cAVhFemLaw1RS72ATY0fwR+Rpb2y/MP7GzLdVWxfpHxvE7fn2D03JRO3QrTkOPkCQ51Blkx3kwq9485G9XLBN4sSs1HDIXSficXV8LS6NhrxZHQVBs14MC/AfLNdSQKXUNFdu6E64= 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 Fri, Oct 4, 2024 at 6:53=E2=80=AFAM Chris Li wrote: > > On Tue, Oct 1, 2024 at 6:58=E2=80=AFPM Barry Song <21cnbao@gmail.com> wro= te: > > > > 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 swapcac= he") > > > >> >> > introduced an unconditional one-tick sleep when `swapcache_pr= epare()` > > > >> >> > fails, which has led to reports of UI stuttering on latency-s= ensitive > > > >> >> > Android devices. To address this, we can use a waitqueue to w= ake up > > > >> >> > tasks that fail `swapcache_prepare()` sooner, instead of alwa= ys > > > >> >> > sleeping for a full tick. While tasks may occasionally be wok= en by an > > > >> >> > unrelated `do_swap_page()`, this method is preferable to two = scenarios: > > > >> >> > rapid re-entry into page faults, which can cause livelocks, a= nd > > > >> >> > multiple millisecond sleeps, which visibly degrade user exper= ience. > > > >> >> > > > >> >> In general, I think that this works. Why not extend the soluti= on to > > > >> >> cover schedule_timeout_uninterruptible() in __read_swap_cache_a= sync() > > > >> >> too? We can call wake_up() when we clear SWAP_HAS_CACHE. To a= void > > > >> > > > > >> > Hi Ying, > > > >> > Thanks for your comments. > > > >> > I feel extending the solution to __read_swap_cache_async() shoul= d be done > > > >> > in a separate patch. On phones, I've never encountered any issue= s reported > > > >> > on that path, so it might be better suited for an optimization r= ather 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, we can= use an > > > >> >> atomic to count waiting tasks. > > > >> > > > > >> > I'm not sure it's worth adding the complexity, as wake_up() on a= n empty > > > >> > waitqueue should have a very low cost on its own? > > > >> > > > >> wake_up() needs to call spin_lock_irqsave() unconditionally on a g= lobal > > > >> shared lock. On systems with many CPUs (such servers), this may c= ause > > > >> severe lock contention. Even the cache ping-pong may hurt perform= ance > > > >> much. > > > > > > > > I understand that cache synchronization was a significant issue bef= ore > > > > qspinlock, but it seems to be less of a concern after its implement= ation. > > > > > > Unfortunately, qspinlock cannot eliminate cache ping-pong issue, as > > > discussed in the following thread. > > > > > > https://lore.kernel.org/lkml/20220510192708.GQ76023@worktop.programmi= ng.kicks-ass.net/ > > > > > > > However, using a global atomic variable would still trigger cache b= roadcasts, > > > > correct? > > > > > > We can only change the atomic variable to non-zero when > > > swapcache_prepare() returns non-zero, and call wake_up() when the ato= mic > > > variable is non-zero. Because swapcache_prepare() returns 0 most tim= es, > > > the atomic variable is 0 most times. If we don't change the value of > > > atomic variable, cache ping-pong will not be triggered. > > > > yes. this can be implemented by adding another atomic variable. > > > > > > > > Hi, Kairui, > > > > > > Do you have some test cases to test parallel zram swap-in? If so, th= at > > > can be used to verify whether cache ping-pong is an issue and whether= 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: > > > > diff --git a/mm/memory.c b/mm/memory.c > > index 2366578015ad..aae0e532d8b6 100644 > > --- a/mm/memory.c > > +++ b/mm/memory.c > > @@ -4192,6 +4192,23 @@ static struct folio *alloc_swap_folio(struct vm_= fault *vmf) > > } > > #endif /* CONFIG_TRANSPARENT_HUGEPAGE */ > > > > +/* > > + * Alleviating the 'thundering herd' phenomenon using a waitqueue hash > > + * when multiple do_swap_page() operations occur simultaneously. > > + */ > > +#define SWAPCACHE_WAIT_TABLE_BITS 5 > > +#define SWAPCACHE_WAIT_TABLE_SIZE (1 << SWAPCACHE_WAIT_TABLE_BITS) > > +static wait_queue_head_t swapcache_wqs[SWAPCACHE_WAIT_TABLE_SIZE]; > > + > > +static int __init swapcache_wqs_init(void) > > +{ > > + for (int i =3D 0; i < SWAPCACHE_WAIT_TABLE_SIZE; i++) > > + init_waitqueue_head(&swapcache_wqs[i]); > > + > > + return 0; > > +} > > +late_initcall(swapcache_wqs_init); > > + > > /* > > * We enter with non-exclusive mmap_lock (to exclude vma changes, > > * but allow concurrent faults), and pte mapped but not yet locked. > > @@ -4204,6 +4221,8 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > > { > > struct vm_area_struct *vma =3D vmf->vma; > > struct folio *swapcache, *folio =3D NULL; > > + DECLARE_WAITQUEUE(wait, current); > > + wait_queue_head_t *swapcache_wq; > > struct page *page; > > struct swap_info_struct *si =3D NULL; > > rmap_t rmap_flags =3D RMAP_NONE; > > @@ -4297,12 +4316,16 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > > * undetectable as pte_same() returns t= rue due > > * to entry reuse. > > */ > > + swapcache_wq =3D &swapcache_wqs[hash_lo= ng(vmf->address & PMD_MASK, > > + SWAPCACHE_WAIT_= TABLE_BITS)]; > > It is better to hash against the swap entry value rather than the > fault address. Same swap entries can map to different parts of the > page table. I am not sure this is triggerable in the SYNC_IO page > fault path, hash against the swap entries is more obviously correct. > i am not convinced swap entry offset is a correct key here. 1. do_swap_page() is always for anon pages, there is no possibility for anon pages to have different mapped virtual address; shmem will never execute a different code path. 2. considering a mTHP swap-in case, the aligned virtual address is the only reliable value for hash. if we only consider small folios swap-in, it is fine to use swap entry value. > Chris > > > if (swapcache_prepare(entry, nr_pages))= { > > /* > > * Relax a bit to prevent rapid > > * repeated page faults. > > */ > > + add_wait_queue(swapcache_wq, &w= ait); > > schedule_timeout_uninterruptibl= e(1); > > + remove_wait_queue(swapcache_wq,= &wait); > > goto out_page; > > } > > need_clear_cache =3D true; > > @@ -4609,8 +4632,10 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > > pte_unmap_unlock(vmf->pte, vmf->ptl); > > out: > > /* Clear the swap cache pin for direct swapin after PTL unlock = */ > > - if (need_clear_cache) > > + if (need_clear_cache) { > > swapcache_clear(si, entry, nr_pages); > > + wake_up(swapcache_wq); > > + } > > if (si) > > put_swap_device(si); > > return ret; > > @@ -4625,8 +4650,10 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) > > folio_unlock(swapcache); > > folio_put(swapcache); > > } > > - if (need_clear_cache) > > + if (need_clear_cache) { > > swapcache_clear(si, entry, nr_pages); > > + wake_up(swapcache_wq); > > + } > > if (si) > > put_swap_device(si); > > return ret; > > -- > > 2.34.1 > > > > > -- > > > Best Regards, > > > Huang, Ying > > Thanks Barry