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 14B38CF042D for ; Wed, 9 Oct 2024 00:55:00 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7B8446B00CD; Tue, 8 Oct 2024 20:54:59 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 771C46B00CF; Tue, 8 Oct 2024 20:54:59 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 607BA6B00D5; Tue, 8 Oct 2024 20:54:59 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 40FC76B00CD for ; Tue, 8 Oct 2024 20:54:59 -0400 (EDT) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 7C84980DEE for ; Wed, 9 Oct 2024 00:54:57 +0000 (UTC) X-FDA: 82652244276.01.1F5CC96 Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.19]) by imf11.hostedemail.com (Postfix) with ESMTP id 3757140003 for ; Wed, 9 Oct 2024 00:54:55 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=FOSxj2sd; spf=pass (imf11.hostedemail.com: domain of ying.huang@intel.com designates 192.198.163.19 as permitted sender) smtp.mailfrom=ying.huang@intel.com; dmarc=pass (policy=none) header.from=intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1728435162; 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=Sl0rfuX609reVT1IyGmxLn69QfDBwxtz74jsqlNj+Os=; b=ifLQWrsbDEkxWae/OFVwGlkLeHOiEy6W7mxN/OwRa3IZwxxP0j8dnoAkhbaNAQACaGEWph e/GGB6Vcx6B6ug+JAPhyVeGGFGq61DdvliRrIIyUxRZLAFKkCEUvhDicJUnawoq3JlT8XI kL0kVPlN3oCfou+tiJ8d9is80mbsOzQ= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1728435162; a=rsa-sha256; cv=none; b=tGDyyM2jiITOMPDUhwdgQmhlYEJ9bpgbWKGGOlNj7wCMHsIEDc7Hb0DJzUlkjr7tX/0ChX /WUAngxm230SRu9TxLs3HvqGCjLlOQjrpYg6dji4QmD5thauY3+zbSyocwCQmZUWD1k81y hRl9q8lGwUCwyevYObwxm/UINOcgDR4= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=FOSxj2sd; spf=pass (imf11.hostedemail.com: domain of ying.huang@intel.com designates 192.198.163.19 as permitted sender) smtp.mailfrom=ying.huang@intel.com; dmarc=pass (policy=none) header.from=intel.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1728435296; x=1759971296; h=from:to:cc:subject:in-reply-to:references:date: message-id:mime-version:content-transfer-encoding; bh=2wt8OZmZXZiHffk+E3soDo1i7Jyy9ZL3xsfV3wdF90c=; b=FOSxj2sdVA/6zoKCJ5/zxefrPfNNySirIy9bn3f3QbYZ5jnKJRWREMcR KsPrSYEnO7wR0lpv8z/5ZB+BMp6l2bnhRuenQjaZdDPlU9OZMyY35754C i5moVlpzWmAhm2zE9u2BB3JQBQHTafEEBeKJ+eWlhEZb7IcEn9nDUwz8b IQ72tbKGz1jGW2tqs6nJ2oBXTB1beOvV/f12vUMzYK5nMANimv14QcUgA YxZls5KIXZNOQ3esTzj4giztBudS9efBrNCzIwQ8N2/R+4wLYxz04myfb L42tC0TeGB8xODnVe+pq3ZSFKfDXbWtBz+Uy4p1HWIzXz4SuqOP7Fmslk Q==; X-CSE-ConnectionGUID: 1yBy7c4eRFu7pVU3kbBSTQ== X-CSE-MsgGUID: YQFRFfuQTYeSUNpdR6rgLg== X-IronPort-AV: E=McAfee;i="6700,10204,11219"; a="27182011" X-IronPort-AV: E=Sophos;i="6.11,188,1725346800"; d="scan'208";a="27182011" Received: from fmviesa001.fm.intel.com ([10.60.135.141]) by fmvoesa113.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Oct 2024 17:54:55 -0700 X-CSE-ConnectionGUID: RIUbv4x9RO6dI75ZOq+IpA== X-CSE-MsgGUID: GtXaVDiGTdG+m+d1KQ1olg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.11,188,1725346800"; d="scan'208";a="106816873" Received: from yhuang6-desk2.sh.intel.com (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.238.208.55]) by smtpauth.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Oct 2024 17:54:50 -0700 From: "Huang, Ying" To: Kairui Song Cc: akpm@linux-foundation.org, chrisl@kernel.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, Barry Song <21cnbao@gmail.com> Subject: Re: [PATCH] mm: avoid unconditional one-tick sleep when swapcache_prepare fails In-Reply-To: <20241008130807.40833-1-21cnbao@gmail.com> (Barry Song's message of "Tue, 8 Oct 2024 21:08:07 +0800") References: <87ikuani1f.fsf@yhuang6-desk2.ccr.corp.intel.com> <20241008130807.40833-1-21cnbao@gmail.com> Date: Wed, 09 Oct 2024 08:51:17 +0800 Message-ID: <87set6m73u.fsf@yhuang6-desk2.ccr.corp.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 3757140003 X-Stat-Signature: 3z6mqa7jnjxnfmyew8y1315neqo4fisi X-Rspamd-Server: rspam09 X-Rspam-User: X-HE-Tag: 1728435295-430623 X-HE-Meta: U2FsdGVkX1+P7++2LO8oAUXJXw4Q58G+QoN3IVFsriHTBkqodcMlWpwDG4KSaTv3WTPOixO/RksXBiHJDupLqovqeXwjO9UcGzIqcu7enXO7f/UR2EoBxXnl+XbvoMRJnW6EDxITViCk9HaUOvudVJEbLuNo5ty/NJpy88b30SbyQUIdKWzQhy6Cyu0d5nT6oqZxmARaJG+iacnEj7IJwx0M0ZDgSWwhqlTUBVI71xX3yQnPbzbvFy3ojIUiWrvHHCzIAL/LENivQTeyV1TYSXm1mStWQC1uPpyLoDS5jk19OLs42KBgQFtscY0FbwFHJtxZ7NOzJ+mUaspr5GTscrGwO6AQYjAPI15e77QtdWVqqyskBZ8ftJlft8UeZEJHfVDmKSaCmJUKYJpRtfgmGfXjiEONqIATWAUrL1P6MYKX9Y+yw0zlbLSomgEASMAg7Sw3UwotQMcN2nJ9mWIUbmr17gwTD4o8KbiibjigL1qW9KoaEyI/L4hWcoxhTA6X05UisbwV5w+ABxwt6VNBk+wWHU+m49lMPPdXFy4OmgIosvr8JDm6k7d4A6p46P7PjNn4VmRqT53RDTNGgy5ugLbBMHbHpEZB+Cnlq8CjCmMVw80IBKC84RDj37yLkXaRBknPN1ZEfA/QnasfVPO48BTJ7P9x8hVrPA0XUm6sPJ/TEsdOM957ottWh2fJPjsJE1ggMZAoKfYzVkImFvmsqJEJRRbd+v97FWhEhuyEkpHkXC/MsL1jrbXC7w1fz1sU2cS6Q4sx61fkxXFedNI4cSUgWjWWWOH4WK0Oq2MQJEewcCdwHHmZDAIE/c3dNV85/bVujZX7BpXAqavDXk6GtnbgScCuNrFQ/eFzVq17Np1JNAlskMqmoazOb//CIFXwtYok+d6TZDos+MQn2/LAyk5BjlnN39bvSR04qY6gjUJ4yrD45F2b6nB2RQ1bemH4aoJcUPwWAPmFkkBWUUe DVxXz15T GHq4Mct6jeiITHlqGInOxScs2SjjYIg4obx5TFToiW5fC38vWE9AYkxsMzBnSmXvtYCEhdsXsqUPavfhlMXB/xNJBtxsFfE2I7UCvvYKLdBr17V1tUxXjVzmR9WV+SVal/82Hykez1Ilv46cdZWB8MyxFGReWPzbd1+LfOr1JDt++0HII8sto7ahX8MHj58tYGKNfWu0LHBigm7Ifax1VKDzbbtXVEf00qxoVlZc4xoZY7nv38I9u+mQPawdusFF3iMKcfCxg29IzhqkHCQteTKCeSpvPNqQWHRcwIzQoWFMlolH93qgVl4I3vmOCnc4a29p4NygEOAiSMBmXvUPFXPmy+pQ5jhB/qiZG4CWepT0TAibObqN+Ay00Kg== 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: 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 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. =C2=A0Why not extend the s= olution to >> >> >> >> cover schedule_timeout_uninterruptible() in __read_swap_cache_a= sync() >> >> >> >> too? =C2=A0We can call wake_up() when we clear SWAP_HAS_CACHE. = =C2=A0To avoid >> >> >> > >> >> >> > 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. =C2=A0It'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. =C2=A0On systems with many CPUs (such servers), this = may cause >> >> >> severe lock contention. =C2=A0Even the cache ping-pong may hurt pe= rformance >> >> >> 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. =C2=A0Because swapcache_prepare() returns 0 mos= t times, >> >> the atomic variable is 0 most times. =C2=A0If 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? =C2=A0If s= o, that >> >> 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: >> >> Yes. =C2=A0waitqueue hash may help reduce lock contention. =C2=A0And, we= can have >> both waitqueue_active() and waitqueue hash if necessary. =C2=A0As 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 2001 > From: Barry Song > Date: Tue, 8 Oct 2024 20:18:27 +0800 > Subject: [PATCH] mm: wake_up only when swapcache_wq waitqueue is active > > wake_up() will acquire spinlock even waitqueue is empty. This might > involve cache sync overhead. Let's only call wake_up() when waitqueue > 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? -- Best Regards, Huang, Ying