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 32CD7C27C53 for ; Thu, 13 Jun 2024 02:18:48 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B9B586B009C; Wed, 12 Jun 2024 22:18:47 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id B4A506B009D; Wed, 12 Jun 2024 22:18:47 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A11E66B009E; Wed, 12 Jun 2024 22:18:47 -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 82DC56B009C for ; Wed, 12 Jun 2024 22:18:47 -0400 (EDT) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 30386A18CC for ; Thu, 13 Jun 2024 02:18:47 +0000 (UTC) X-FDA: 82224257094.29.035D613 Received: from mail-lj1-f171.google.com (mail-lj1-f171.google.com [209.85.208.171]) by imf22.hostedemail.com (Postfix) with ESMTP id 479FEC0011 for ; Thu, 13 Jun 2024 02:18:45 +0000 (UTC) Authentication-Results: imf22.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=QI7BYA+D; spf=pass (imf22.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.171 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1718245124; 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=12rYDQECQwR2eWoCJ65xt9hG+fucIyD5JxR2TElJl4c=; b=gL8csIX3Mi4gc1o0+X+VICFKTgb2/doZICJSGPb2huxe3IBsNqTGLa/JrulgcTEd0jO3Af anTAdzNsMs3xHU/yD9qPOR+l8nGdG8CiISKSD8wxoOF3eyH8AoUfsFFfMj8tvp9fMMFiND iX37QvtiaJjD//g0ov5LgtH//xp9cO0= ARC-Authentication-Results: i=1; imf22.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=QI7BYA+D; spf=pass (imf22.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.171 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1718245124; a=rsa-sha256; cv=none; b=aiqAVfSusE0kEPfFmyqpPcTf6y/R0KJHjTHM0VzI0c5TSGXda5sZhTCYR/Snzq30Dke4m4 ljDiZnsgM909aKYq3xlfVwkVBMi0YBMEph3kpaOMbmew+SbaDX/bLsFEjlzFe4Cjcg/que lwaDaCcF/beNoBFhaampwFoj70/X+GY= Received: by mail-lj1-f171.google.com with SMTP id 38308e7fff4ca-2eaae2a6dc1so7889301fa.0 for ; Wed, 12 Jun 2024 19:18:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1718245123; x=1718849923; 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=12rYDQECQwR2eWoCJ65xt9hG+fucIyD5JxR2TElJl4c=; b=QI7BYA+DUoxdzxtZwClz81+cOftP21MgT1HrrtzYmhcL/xNoVvpDZ+3gJPvu44bzSL ONF9pwul8gGfv3+wDW7OITIXpUnck+1USKjWGQGjX5eYlb/BoyxM4FoRdxzoHHfXobBe NDHCdeIqKHxf5HahuzDWOmrgiRjBOD/IwsKJJ0dmQv9lS8RkCo7/cK9EsPKRrxxgAHnv UELtuZCp3Q5R0iilZH/tmGNcRTAH/ipcS9aUt/1OqMSEHTGZ4/+ibOEXdN3EdSPAWw4K ypyWxdZIOFuKI0abrpOoS+tmWm4qoEX9p/9nyhrQhF4NS25lKgYdB3P6P8MXl5HcSwmn bRYw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718245123; x=1718849923; 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=12rYDQECQwR2eWoCJ65xt9hG+fucIyD5JxR2TElJl4c=; b=cCJS8Mk60byG9jhxHLion4e9/vKZXLD2gtZ7Le68CX9R6gm3FvO+/QuhPr/4YWUKsA pFldsfs/Zlz+Lszg6buECLu0aUzf1HwM7E6qqh7wfzNhe7saNWx/mEygQ0LEXp/cFRv7 acZnrbpmVx2hQ/OzHUvtERinETTsmR9BkcwXnOj3nYVOWxik5Bgsad2PjB201N2fCq4a MkFkgAQBWWDDblIfpEmFTouXEZA5b2MUyRKI04oAKzhtAzHLwP48XclrtYHvqK5dhDbB 0Nrp1f2ap89frwc0kgjzkiU3Au9oiLqkfy3G5j889/Ca4UP/TduRd2VMsvDSCc8G7mXb l/2Q== X-Forwarded-Encrypted: i=1; AJvYcCWkBJXX8Nd2CQpTbu61Fuz36K/ciivVu98hG4TxOp0F5GFIsSPCjmmlUftxsHky2gbKRztJaSbE9hmW/gMCCuR2nkk= X-Gm-Message-State: AOJu0YyKxlcxQsSOGdWQHpy3jlLzLOIUXNLXb+zJmjQMsxRGpcr+c//J sYr9zVxKYK6xjZ7+rK/5LQxNDYG4zvfK5nR8DSpzVXvzdBIxtm+qLDUnqHHBhamjBLvgrm4wVIn n6ecYvacUqhVuwRuYGKr8bXvjkQd4H8aKzQmPtsfFZzslQft2IJUy9+m+ig== X-Google-Smtp-Source: AGHT+IFU4l3WY9wL81p+yVuTlpyUuiNnuA0ZnMTCC1qXPf1EmUjm/ZVtjaVB9KB3C5PIpAeWSfa1KdjCeGTyVRLq/zA= X-Received: by 2002:a05:651c:198c:b0:2eb:eb25:81f4 with SMTP id 38308e7fff4ca-2ebfc9ab264mr31400101fa.6.1718245123334; Wed, 12 Jun 2024 19:18:43 -0700 (PDT) MIME-Version: 1.0 References: <20240608155316.451600-1-flintglass@gmail.com> <20240608155316.451600-2-flintglass@gmail.com> In-Reply-To: From: Yosry Ahmed Date: Wed, 12 Jun 2024 19:18:05 -0700 Message-ID: Subject: Re: [PATCH v1 1/3] mm: zswap: fix global shrinker memcg iteration To: Takero Funaki Cc: Nhat Pham , Johannes Weiner , Chengming Zhou , Jonathan Corbet , Andrew Morton , Domenico Cerasuolo , linux-mm@kvack.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, Shakeel Butt Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam11 X-Rspamd-Queue-Id: 479FEC0011 X-Stat-Signature: 7b5oy5a18c3c8x4goqyntzi3k1yntqh6 X-Rspam-User: X-HE-Tag: 1718245125-917522 X-HE-Meta: U2FsdGVkX189dESk8yWiGuGcj8HY2C6bkdmaseKdI4Wy6QXgGMukHYOldALdmNNCujcYKwROHIDyfmu+f7jke929EtcPDIVzuDGv06HrfP4Ctwo9Od0L40u2pp9JdwrjTpSarfmXhBzCIviOZaFdAcHkCaw1EwnmQqHHDXqCyKOlpjQ2FdzYwmAGsG5Ntcjiia1GCe6hI8arIIDDxbAWOvjB8zDeOz7dOHb8KPWCerQ73teJBhsRtb/qt+oLueqWTcV86EDjP1YLqseZLNiuk7ikq+drhbnA6HTQKy8EnfFKEtPM241zkqqXZlE8R29CXFJROpsPN96VRh22R2RRnx4uCId6Yl01TffxiGKsVMVKyZNmKKNkpEM/5ujKYa39pN+n4pNU3W74NWRNJu3ubzMU7fOGsueUY0R8KOBteredJ7c9VrJgy0+FqHWxMB8Qy/xkIWMP2RF9qdfseBC0kJLXlvxo5A8PtUf0z71Lf1eT6gemG5GB3FWS/X6ozs0U1AU4rgjVfjzVFu9TNpvRiqYQ+reLC6W6ceYwBMzJjs9z1qSydDj0XGQU4J46mnVlw8sH1QNiblqOUMVY0kuAydR1qe/gM7cp4+EaP3k9JxJ1Sz2UuBRfS4gnIZDs2i8uLhCndTu8XbqUXLVm1APHYhXq9umN4PmDm840gpxdxxJ01QcXQ4iVk2Ldu6p5aIuZY6N608TxND/Su2Lz5qRYH4Ym05K6gxOdrytMehb4xolf4+3IRpFvVtADB3TkRzyVbmyARZa9GUniusBg52NNAvJQR5Rf+B56xPC/LaEi9bN60w+D245H3RQsqec/wnRSMx+kHZr3RF3G9w2tbo6TKSQBVBeIDLJ5t9lencFBQXYjWC49/3ySfsAwNVDuyDfWb8D8VFXluqgh+3VslBfVwz+//IHcGhaI2wbedRKg39I8MSZ+kVIjRCyscaAZm85ROE54fqkgIsZXLsmYN9S Q3xMsVjZ SB7t0WeJS619O3QSXB8p60/emCQqN+lNKn0bcv1iafz+gHTRgv1ZhVAp6UUzWy7+O+xSx43XnrZ090gsLhbsPJmWwh49yZBNNFMUla8aie3R3VmMbVsGZQChOtdVOJFDQdgZ0WDI+Cpv0dcyG4aT1ArBZyWqrxLLEv4OuIxrR+4NvDWGeFLZws8HtFD8ETAoOMjM0x/z+7aWkHirKn3mjxGQh7kp5sglRGeW3e/Nu1xL1FKx1x1K2SvgVGdRQiskCi+75JOc4QrW9z43TeWilG3QqXw== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000126, 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, Jun 12, 2024 at 7:13=E2=80=AFPM Takero Funaki wrote: > > 2024=E5=B9=B46=E6=9C=8813=E6=97=A5(=E6=9C=A8) 3:28 Yosry Ahmed : > > > > On Wed, Jun 12, 2024 at 11:16=E2=80=AFAM Takero Funaki wrote: > > > > > > 2024=E5=B9=B46=E6=9C=8812=E6=97=A5(=E6=B0=B4) 3:26 Nhat Pham : > > > > > > > > > > > As I have noted in v0, I think this is unnecessary and makes it mor= e confusing. > > > > > > > > > > Does spin_lock() ensure that compiler optimizations do not remove > > > memory access to an external variable? I think we need to use > > > READ_ONCE/WRITE_ONCE for shared variable access even under a spinlock= . > > > For example, > > > https://elixir.bootlin.com/linux/latest/source/mm/mmu_notifier.c#L234 > > > > In this example, it seems like mmu_interval_set_seq() updates > > interval_sub->invalidate_seq locklessly using WRITE_ONCE(). I think > > this is why READ_ONCE() is required in that particular case. > > > > > > > > isn't this a common use case of READ_ONCE? > > > ```c > > > bool shared_flag =3D false; > > > spinlock_t flag_lock; > > > > > > void somefunc(void) { > > > for (;;) { > > > spin_lock(&flag_lock); > > > /* check external updates */ > > > if (READ_ONCE(shared_flag)) > > > break; > > > /* do something */ > > > spin_unlock(&flag_lock); > > > } > > > spin_unlock(&flag_lock); > > > } > > > ``` > > > Without READ_ONCE, the check can be extracted from the loop by optimi= zation. > > > > According to Documentation/memory-barriers.txt, lock acquiring > > functions are implicit memory barriers. Otherwise, the compiler would > > be able to pull any memory access outside of the lock critical section > > and locking wouldn't be reliable. > > Ah, I understand now. The implicit barrier is sufficient as long as > all memory access occurs within the lock. It=E2=80=99s a fundamental rule= of > locking=E2=80=94facepalm. > > I misread a module code, like in the link, where a lockless write > could invade a critical section. My example was in the opposite > direction, just wrong. Thank you so much for clarifying my > misunderstanding. > > For now checking the patch, I suppose the locking mechanism itself is > not affected by my misunderstanding of READ_ONCE. > > The corrected version of the cleaner should be: > ```c > void zswap_memcg_offline_cleanup(struct mem_cgroup *memcg) > { > /* lock out zswap shrinker walking memcg tree */ > spin_lock(&zswap_shrink_lock); > if (zswap_next_shrink =3D=3D memcg) { > do { > zswap_next_shrink =3D mem_cgroup_iter(NULL, > zswap_next_shrink, NULL); > spin_unlock(&zswap_shrink_lock); > spin_lock(&zswap_shrink_lock); > if (!zswap_next_shrink) > break; > } while (!mem_cgroup_online(zswap_next_shrink)); > } > spin_unlock(&zswap_shrink_lock); > } > ``` Is the idea here to avoid moving the iterator to another offline memcg that zswap_memcg_offline_cleanup() was already called for, to avoid holding a ref on that memcg until the next run of zswap shrinking? If yes, I think it's probably worth doing. But why do we need to release and reacquire the lock in the loop above?