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 212C6D13578 for ; Sun, 27 Oct 2024 17:26:52 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 337EF6B0083; Sun, 27 Oct 2024 13:26:52 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2E73A6B0088; Sun, 27 Oct 2024 13:26:52 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 113B68D0001; Sun, 27 Oct 2024 13:26:52 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id DF35C6B0083 for ; Sun, 27 Oct 2024 13:26:51 -0400 (EDT) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id B14FCA1C13 for ; Sun, 27 Oct 2024 17:26:11 +0000 (UTC) X-FDA: 82720061382.06.E1C0AEF Received: from mail-lj1-f172.google.com (mail-lj1-f172.google.com [209.85.208.172]) by imf07.hostedemail.com (Postfix) with ESMTP id 66F6F40006 for ; Sun, 27 Oct 2024 17:26:18 +0000 (UTC) Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=YE5w8Q9r; spf=pass (imf07.hostedemail.com: domain of ryncsn@gmail.com designates 209.85.208.172 as permitted sender) smtp.mailfrom=ryncsn@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=1730049930; 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=H5eeYEUvk3nFhsJrmbvN68VUwhL7j585EH8/XaDDJGc=; b=KXKjuHcsI7y3WmcTYftqhhkQ7azD6hAHj9D9kimHYqnt+yL+fvQ2bBqqRI5DqEUfZBiORn 0NPHhV1zLcvP710O3oGwVnY8yWkvI4W5DKueJcL3tofBQTO09pzy+nEGQazPHIkelJNwnu Ce/sc8BR2SUuupMG2LVIOJauvlGwo6c= ARC-Authentication-Results: i=1; imf07.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=YE5w8Q9r; spf=pass (imf07.hostedemail.com: domain of ryncsn@gmail.com designates 209.85.208.172 as permitted sender) smtp.mailfrom=ryncsn@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1730049930; a=rsa-sha256; cv=none; b=5Yj3WzPnCFjG7bzODyPZMGOInF61biBtQul1QDLqFyk4Dq7xj5TWPsSFbwGnbIctwG2zsZ MFQWusbHB1FHB04d79evIlShf1jyo2kdpJ8IvBWKv6CPtsxCheR/nYzziGObo83Uhq5MYw KGw0N/Dh2UlS9QMW8jholM9Vc3zl1Jo= Received: by mail-lj1-f172.google.com with SMTP id 38308e7fff4ca-2fb388e64b0so33720321fa.0 for ; Sun, 27 Oct 2024 10:26:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1730050007; x=1730654807; 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=H5eeYEUvk3nFhsJrmbvN68VUwhL7j585EH8/XaDDJGc=; b=YE5w8Q9rvVDTtiC0fzBEtyHU/TxZKIkV6zRCKBuYiV8gqH7KX37GIIIJQhBXe25LIa wei12upS8sdFHf+QTHAymDNid2QTCzKT/Qa99b41XyVY1lzFUZI5rmWY72SLb+8eRbT7 G4KeDqoYVfvdqyLa7006CZJvu0BdAKI0v8cuAEo4N/IeND6SspgF9zMeUN7O3mJAXtKw oAJjWHFKmzWcDthPdCDRUR/tWU53njuzdqNnnxrlMvxCaGvdKnRFEBFwjCJUvN1orpXq 6g0N7Ms+KmN3gDAqtsqTERdsAkmWrHmKpdl59I7yLizQV/7eQhdYQrb8AZKP2EHI2eyb fzHg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1730050007; x=1730654807; 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=H5eeYEUvk3nFhsJrmbvN68VUwhL7j585EH8/XaDDJGc=; b=ZMYseIJn6VC679QKqutxexZCkw7QJzzLJn5upAFNAinrRQkijSplsY+qGCjFEZFUwt o9157FWWNs9bL1lvwUIaeTNmOZ52CvmzmqFEsnN06c/JB1wT8UUxKC2HPQbOn9Rca81E V7cPoTJ4qe5a+VjwKi9ejTdIzomfzTC8lKHlsADssOYuQIju4TnEGUCsEi06pocOdzbq 2Dg/Wf8/KvTVCZV82mLebv+ffMfHKwke0HUPOKeaqdMIgxxUVwng6Hl8mUVqXc/YFucl Rvo6XlpN8F2jLHfAnVNKQ1c3cK1JNnTE9ulgr9QU9nw91MByhR5nbZzvi4rDn4XQ+UPB NoJQ== X-Gm-Message-State: AOJu0YyrYGr2G0Dq9FbM1UdNMmr6MtuN7y8dqoiNJUE2zTzhENyQU52A mNqUzIMYm/2b5/UADXO3pZONewnFapdZFM0iF9Dwp46/JneVEReX8vvdtQJHzjcv445vpQhMMHu 6Wclxhu16lPdhUzPDLFHugeRgG6I= X-Google-Smtp-Source: AGHT+IG6qyzaR4e9hzxioDMaVIGuLnpcfABTGlqKnE72BDr9sGWfBtQmP8ZGzIjYVMaKh2+mSHE1R97kBHJWk9ZbYs4= X-Received: by 2002:a2e:97c8:0:b0:2f9:c337:aca9 with SMTP id 38308e7fff4ca-2fcbe0997fcmr16241521fa.44.1730050007030; Sun, 27 Oct 2024 10:26:47 -0700 (PDT) MIME-Version: 1.0 References: <20240925171020.32142-1-ryncsn@gmail.com> <20240925171020.32142-6-ryncsn@gmail.com> <7334b158-8567-4c66-8176-57b879ff8882@gmail.com> In-Reply-To: <7334b158-8567-4c66-8176-57b879ff8882@gmail.com> From: Kairui Song Date: Mon, 28 Oct 2024 01:26:30 +0800 Message-ID: Subject: Re: [PATCH v2 5/6] mm/list_lru: split the lock to per-cgroup scope To: Usama Arif Cc: linux-mm@kvack.org, Andrew Morton , Matthew Wilcox , Johannes Weiner , Roman Gushchin , Waiman Long , Shakeel Butt , Michal Hocko , Chengming Zhou , Qi Zheng , Muchun Song , Nhat Pham , Yosry Ahmed Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam03 X-Rspam-User: X-Rspamd-Queue-Id: 66F6F40006 X-Stat-Signature: r3znqmyk795uxamf4qqw9sz5hykcje7m X-HE-Tag: 1730049978-402414 X-HE-Meta: U2FsdGVkX1/m8eyfPwtKpkB6IFk4uJeDI9DugqoVgnln9lIu9D1BXNLn3g1Dq3kUJwt+5OUO1kVgY78jewjyHrzQ12scwKOQY19LwdQXAyhbFn1nWT3LITXfK1tOfoKLIhnpKip8WXFQlg0YJm8VMSfqtvmB2YXEQE45MNH+Yz1IYmV2AT0GAUgJ1VG220s8R8ABwco8Tsgbn/hwLdQl7g5i1muV94DJgL1AqaBZui4oju/tBnp6Eb36fWvO/Rg6CoP0I8lVrRx1qat7TQUYe2sO2H3TJySImdgBU33EWhqQIk8F31yHKzlha2v2QDErMcslfLdxIUsovzyk09s4CkDDWS9/0AiP69+eICdy5K56wEJS/5Gt8yQbUKETbD62nWu13su0ncTAyyba9YLxJQm3HdUJMkkMwmBefN6VOQogHlgJ/aBUIVOGvPXZNaOHNtnY2df0HCtEQqy+bpP8dtrIWu/umz3LdO+9UqWJWbazkYGFBjPc2mRBYsI/1XJ5XXHj+sEhknkFjMo8bDocUvMQVaUcZpTALB2PNyFD+S2XKQqHepY22/LoTNzNcGPAj4hsCq8vqCzzcgqYoNoFTaMio3179dTdOmjzyvHE/oALd+ILMCi/uA6li8jIimsIlVg/0p2wp0vQXcJ4ah0FWb2tqdhxXd4KPRdlBSkS+ekBfmR7LOjPegIy/0mckXpUi4wQ6TxHd9sCyWjKyKUmBGcME6Mozwdpvo2G6SipJvv6iKgiwej0gUf1c2OMrthucN21cQn/UKwxdzpSdIBKFmuBqOP1dgX73kPTHn4WYBGZUhKaxjzUlJKVRu3+rk48omigzYwCVD0zV8Ns9L97B28BagmSMDkuIdqwDchtgc3y3TT88WZpAgu9HQbN3SkqHdoJiNxQ5PWzILTn5z8Md/Q0VGZDcklL0/iZ7/Fz6pyO/hFWE2pkbIxHo9BcBhyKPpGVUhrvb5DP3y0Z79F N9MDDS+L jzcdF9c+iBEvwMOfHkAjw3XSbZ1PQz52othNqSf02Pp6mtf+XCVp688IfcepSFHt6WqRw76F9UPvPtJm0oVexTgQYHB2gUNBLqYqT5CfUBEYXKUrJf7DKHBFI/2M0RpeGGZx0JJGN2hP4O32CzRpu4zZzRgI9xfsvCjht8JFNm3u21xrLkLBijX108Mz4/++eZaMCzkf8eB8G7pfxeK1iXIafjIP4CAzEYLrIr8L9OT1fsrGf9RL1xXSi5+yHL2JaGRFODnEZPjI2O8cskrYl1ja4wgZIYxCy5H4li83oKCKg9lKjQmFvOnmkuw65kSePAyGvAYwCxxLgmfMg1M5Z9IRvkydU56HvTOzT0Q4jEYnOU6DnXWO8WBbjTeVjmBS5ql+fknUP6EjrPF+tEKl+z8mvB5SU/0fjzbsR 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: Hi Usama, On Sat, Oct 26, 2024 at 5:16=E2=80=AFAM Usama Arif = wrote: > > > > On 25/09/2024 18:10, Kairui Song wrote: > > From: Kairui Song > > > > Currently, every list_lru has a per-node lock that protects adding, > > deletion, isolation, and reparenting of all list_lru_one instances > > belonging to this list_lru on this node. This lock contention is > > heavy when multiple cgroups modify the same list_lru. > > > > This lock can be split into per-cgroup scope to reduce contention. > > > > To achieve this, we need a stable list_lru_one for every cgroup. > > This commit adds a lock to each list_lru_one and introduced a > > helper function lock_list_lru_of_memcg, making it possible to pin > > the list_lru of a memcg. Then reworked the reparenting process. > > > > Reparenting will switch the list_lru_one instances one by one. > > By locking each instance and marking it dead using the nr_items > > counter, reparenting ensures that all items in the corresponding > > cgroup (on-list or not, because items have a stable cgroup, see below) > > will see the list_lru_one switch synchronously. > > > > Objcg reparent is also moved after list_lru reparent so items will have > > a stable mem cgroup until all list_lru_one instances are drained. > > > > The only caller that doesn't work the *_obj interfaces are direct > > calls to list_lru_{add,del}. But it's only used by zswap and > > that's also based on objcg, so it's fine. > > > > This also changes the bahaviour of the isolation function when > > LRU_RETRY or LRU_REMOVED_RETRY is returned, because now releasing the > > lock could unblock reparenting and free the list_lru_one, isolation > > function will have to return withoug re-lock the lru. > > > > prepare() { > > mkdir /tmp/test-fs > > modprobe brd rd_nr=3D1 rd_size=3D33554432 > > mkfs.xfs -f /dev/ram0 > > mount -t xfs /dev/ram0 /tmp/test-fs > > for i in $(seq 1 512); do > > mkdir "/tmp/test-fs/$i" > > for j in $(seq 1 10240); do > > echo TEST-CONTENT > "/tmp/test-fs/$i/$j" > > done & > > done; wait > > } > > > > do_test() { > > read_worker() { > > sleep 1 > > tar -cv "$1" &>/dev/null > > } > > read_in_all() { > > cd "/tmp/test-fs" && ls > > for i in $(seq 1 512); do > > (exec sh -c 'echo "$PPID"') > "/sys/fs/cgroup/benchmark/$i/= cgroup.procs" > > read_worker "$i" & > > done; wait > > } > > for i in $(seq 1 512); do > > mkdir -p "/sys/fs/cgroup/benchmark/$i" > > done > > echo +memory > /sys/fs/cgroup/benchmark/cgroup.subtree_control > > echo 512M > /sys/fs/cgroup/benchmark/memory.max > > echo 3 > /proc/sys/vm/drop_caches > > time read_in_all > > } > > > > Above script simulates compression of small files in multiple cgroups > > with memory pressure. Run prepare() then do_test for 6 times: > > > > Before: > > real 0m7.762s user 0m11.340s sys 3m11.224s > > real 0m8.123s user 0m11.548s sys 3m2.549s > > real 0m7.736s user 0m11.515s sys 3m11.171s > > real 0m8.539s user 0m11.508s sys 3m7.618s > > real 0m7.928s user 0m11.349s sys 3m13.063s > > real 0m8.105s user 0m11.128s sys 3m14.313s > > > > After this commit (about ~15% faster): > > real 0m6.953s user 0m11.327s sys 2m42.912s > > real 0m7.453s user 0m11.343s sys 2m51.942s > > real 0m6.916s user 0m11.269s sys 2m43.957s > > real 0m6.894s user 0m11.528s sys 2m45.346s > > real 0m6.911s user 0m11.095s sys 2m43.168s > > real 0m6.773s user 0m11.518s sys 2m40.774s > > > > Signed-off-by: Kairui Song > > --- > > drivers/android/binder_alloc.c | 1 - > > fs/inode.c | 1 - > > fs/xfs/xfs_qm.c | 1 - > > include/linux/list_lru.h | 6 +- > > mm/list_lru.c | 215 +++++++++++++++++++-------------- > > mm/memcontrol.c | 7 +- > > mm/workingset.c | 1 - > > mm/zswap.c | 5 +- > > 8 files changed, 134 insertions(+), 103 deletions(-) > > > > diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_al= loc.c > > index b3acbc4174fb..86bbe40f4bcd 100644 > > --- a/drivers/android/binder_alloc.c > > +++ b/drivers/android/binder_alloc.c > > @@ -1106,7 +1106,6 @@ enum lru_status binder_alloc_free_page(struct lis= t_head *item, > > mmput_async(mm); > > __free_page(page_to_free); > > > > - spin_lock(lock); > > return LRU_REMOVED_RETRY; > > > > err_invalid_vma: > > diff --git a/fs/inode.c b/fs/inode.c > > index 471ae4a31549..b95d80cb140b 100644 > > --- a/fs/inode.c > > +++ b/fs/inode.c > > @@ -932,7 +932,6 @@ static enum lru_status inode_lru_isolate(struct lis= t_head *item, > > mm_account_reclaimed_pages(reap); > > } > > inode_unpin_lru_isolating(inode); > > - spin_lock(lru_lock); > > return LRU_RETRY; > > } > > > > diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c > > index 7e2307921deb..665d26990b78 100644 > > --- a/fs/xfs/xfs_qm.c > > +++ b/fs/xfs/xfs_qm.c > > @@ -496,7 +496,6 @@ xfs_qm_dquot_isolate( > > trace_xfs_dqreclaim_busy(dqp); > > XFS_STATS_INC(dqp->q_mount, xs_qm_dqreclaim_misses); > > xfs_dqunlock(dqp); > > - spin_lock(lru_lock); > > return LRU_RETRY; > > } > > > > diff --git a/include/linux/list_lru.h b/include/linux/list_lru.h > > index eba93f6511f3..10ba9a54d42c 100644 > > --- a/include/linux/list_lru.h > > +++ b/include/linux/list_lru.h > > @@ -32,6 +32,8 @@ struct list_lru_one { > > struct list_head list; > > /* may become negative during memcg reparenting */ > > long nr_items; > > + /* protects all fields above */ > > + spinlock_t lock; > > }; > > > > struct list_lru_memcg { > > @@ -41,11 +43,9 @@ struct list_lru_memcg { > > }; > > > > struct list_lru_node { > > - /* protects all lists on the node, including per cgroup */ > > - spinlock_t lock; > > /* global list, used for the root cgroup in cgroup aware lrus */ > > struct list_lru_one lru; > > - long nr_items; > > + atomic_long_t nr_items; > > } ____cacheline_aligned_in_smp; > > > > struct list_lru { > > diff --git a/mm/list_lru.c b/mm/list_lru.c > > index 172b16146e15..6d1c187d9019 100644 > > --- a/mm/list_lru.c > > +++ b/mm/list_lru.c > > @@ -61,18 +61,48 @@ list_lru_from_memcg_idx(struct list_lru *lru, int n= id, int idx) > > } > > > > static inline struct list_lru_one * > > -list_lru_from_memcg(struct list_lru *lru, int nid, struct mem_cgroup *= memcg) > > +lock_list_lru_of_memcg(struct list_lru *lru, int nid, struct mem_cgrou= p *memcg, > > + bool irq, bool skip_empty) > > { > > struct list_lru_one *l; > > + rcu_read_lock(); > > again: > > l =3D list_lru_from_memcg_idx(lru, nid, memcg_kmem_id(memcg)); > > - if (likely(l)) > > - return l; > > - > > - memcg =3D parent_mem_cgroup(memcg); > > + if (likely(l)) { > > + if (irq) > > + spin_lock_irq(&l->lock); > > + else > > + spin_lock(&l->lock); > > + if (likely(READ_ONCE(l->nr_items) !=3D LONG_MIN)) { > > + WARN_ON(l->nr_items < 0); > > + rcu_read_unlock(); > > + return l; > > + } > > + if (irq) > > + spin_unlock_irq(&l->lock); > > + else > > + spin_unlock(&l->lock); > > + } > > + /* > > + * Caller may simply bail out if raced with reparenting or > > + * may iterate through the list_lru and expect empty slots. > > + */ > > + if (skip_empty) { > > + rcu_read_unlock(); > > + return NULL; > > + } > > VM_WARN_ON(!css_is_dying(&memcg->css)); > > + memcg =3D parent_mem_cgroup(memcg); > > goto again; > > } > > + > > +static inline void unlock_list_lru(struct list_lru_one *l, bool irq_of= f) > > +{ > > + if (irq_off) > > + spin_unlock_irq(&l->lock); > > + else > > + spin_unlock(&l->lock); > > +} > > #else > > static void list_lru_register(struct list_lru *lru) > > { > > @@ -99,31 +129,48 @@ list_lru_from_memcg_idx(struct list_lru *lru, int = nid, int idx) > > } > > > > static inline struct list_lru_one * > > -list_lru_from_memcg(struct list_lru *lru, int nid, int idx) > > +lock_list_lru_of_memcg(struct list_lru *lru, int nid, struct mem_cgrou= p *memcg, > > + bool irq, bool skip_empty) > > { > > - return &lru->node[nid].lru; > > + struct list_lru_one *l =3D &lru->node[nid].lru; > > + > > + if (irq) > > + spin_lock_irq(&l->lock); > > + else > > + spin_lock(&l->lock); > > + > > + return l; > > +} > > + > > +static inline void unlock_list_lru(struct list_lru_one *l, bool irq_of= f) > > +{ > > + if (irq_off) > > + spin_unlock_irq(&l->lock); > > + else > > + spin_unlock(&l->lock); > > } > > #endif /* CONFIG_MEMCG */ > > > > /* The caller must ensure the memcg lifetime. */ > > bool list_lru_add(struct list_lru *lru, struct list_head *item, int ni= d, > > - struct mem_cgroup *memcg) > > + struct mem_cgroup *memcg) > > { > > struct list_lru_node *nlru =3D &lru->node[nid]; > > struct list_lru_one *l; > > > > - spin_lock(&nlru->lock); > > + l =3D lock_list_lru_of_memcg(lru, nid, memcg, false, false); > > + if (!l) > > + return false; > > if (list_empty(item)) { > > - l =3D list_lru_from_memcg(lru, nid, memcg); > > list_add_tail(item, &l->list); > > /* Set shrinker bit if the first element was added */ > > if (!l->nr_items++) > > set_shrinker_bit(memcg, nid, lru_shrinker_id(lru)= ); > > - nlru->nr_items++; > > - spin_unlock(&nlru->lock); > > + unlock_list_lru(l, false); > > + atomic_long_inc(&nlru->nr_items); > > return true; > > } > > - spin_unlock(&nlru->lock); > > + unlock_list_lru(l, false); > > return false; > > } > > > > @@ -146,24 +193,23 @@ EXPORT_SYMBOL_GPL(list_lru_add_obj); > > > > /* The caller must ensure the memcg lifetime. */ > > bool list_lru_del(struct list_lru *lru, struct list_head *item, int ni= d, > > - struct mem_cgroup *memcg) > > + struct mem_cgroup *memcg) > > { > > struct list_lru_node *nlru =3D &lru->node[nid]; > > struct list_lru_one *l; > > - > > - spin_lock(&nlru->lock); > > + l =3D lock_list_lru_of_memcg(lru, nid, memcg, false, false); > > + if (!l) > > + return false; > > if (!list_empty(item)) { > > - l =3D list_lru_from_memcg(lru, nid, memcg); > > list_del_init(item); > > l->nr_items--; > > - nlru->nr_items--; > > - spin_unlock(&nlru->lock); > > + unlock_list_lru(l, false); > > + atomic_long_dec(&nlru->nr_items); > > return true; > > } > > - spin_unlock(&nlru->lock); > > + unlock_list_lru(l, false); > > return false; > > } > > -EXPORT_SYMBOL_GPL(list_lru_del); > > > > bool list_lru_del_obj(struct list_lru *lru, struct list_head *item) > > { > > @@ -220,25 +266,24 @@ unsigned long list_lru_count_node(struct list_lru= *lru, int nid) > > struct list_lru_node *nlru; > > > > nlru =3D &lru->node[nid]; > > - return nlru->nr_items; > > + return atomic_long_read(&nlru->nr_items); > > } > > EXPORT_SYMBOL_GPL(list_lru_count_node); > > > > static unsigned long > > -__list_lru_walk_one(struct list_lru *lru, int nid, int memcg_idx, > > +__list_lru_walk_one(struct list_lru *lru, int nid, struct mem_cgroup *= memcg, > > list_lru_walk_cb isolate, void *cb_arg, > > - unsigned long *nr_to_walk) > > + unsigned long *nr_to_walk, bool irq_off) > > { > > struct list_lru_node *nlru =3D &lru->node[nid]; > > - struct list_lru_one *l; > > + struct list_lru_one *l =3D NULL; > > struct list_head *item, *n; > > unsigned long isolated =3D 0; > > > > restart: > > - l =3D list_lru_from_memcg_idx(lru, nid, memcg_idx); > > + l =3D lock_list_lru_of_memcg(lru, nid, memcg, irq_off, true); > > if (!l) > > - goto out; > > - > > + return isolated; > > list_for_each_safe(item, n, &l->list) { > > enum lru_status ret; > > > > @@ -250,19 +295,20 @@ __list_lru_walk_one(struct list_lru *lru, int nid= , int memcg_idx, > > break; > > --*nr_to_walk; > > > > - ret =3D isolate(item, l, &nlru->lock, cb_arg); > > + ret =3D isolate(item, l, &l->lock, cb_arg); > > switch (ret) { > > + /* > > + * LRU_RETRY and LRU_REMOVED_RETRY will drop the lru lock= , > > + * the list traversal will be invalid and have to restart= from > > + * scratch. > > + */ > > + case LRU_RETRY: > > + goto restart; > > case LRU_REMOVED_RETRY: > > - assert_spin_locked(&nlru->lock); > > fallthrough; > > case LRU_REMOVED: > > isolated++; > > - nlru->nr_items--; > > - /* > > - * If the lru lock has been dropped, our list > > - * traversal is now invalid and so we have to > > - * restart from scratch. > > - */ > > + atomic_long_dec(&nlru->nr_items); > > if (ret =3D=3D LRU_REMOVED_RETRY) > > goto restart; > > break; > > @@ -271,21 +317,15 @@ __list_lru_walk_one(struct list_lru *lru, int nid= , int memcg_idx, > > break; > > case LRU_SKIP: > > break; > > - case LRU_RETRY: > > - /* > > - * The lru lock has been dropped, our list traver= sal is > > - * now invalid and so we have to restart from scr= atch. > > - */ > > - assert_spin_locked(&nlru->lock); > > - goto restart; > > case LRU_STOP: > > - assert_spin_locked(&nlru->lock); > > + assert_spin_locked(&l->lock); > > goto out; > > default: > > BUG(); > > } > > } > > out: > > + unlock_list_lru(l, irq_off); > > return isolated; > > } > > > > @@ -294,14 +334,8 @@ list_lru_walk_one(struct list_lru *lru, int nid, s= truct mem_cgroup *memcg, > > list_lru_walk_cb isolate, void *cb_arg, > > unsigned long *nr_to_walk) > > { > > - struct list_lru_node *nlru =3D &lru->node[nid]; > > - unsigned long ret; > > - > > - spin_lock(&nlru->lock); > > - ret =3D __list_lru_walk_one(lru, nid, memcg_kmem_id(memcg), isola= te, > > - cb_arg, nr_to_walk); > > - spin_unlock(&nlru->lock); > > - return ret; > > + return __list_lru_walk_one(lru, nid, memcg, isolate, > > + cb_arg, nr_to_walk, false); > > } > > EXPORT_SYMBOL_GPL(list_lru_walk_one); > > > > @@ -310,14 +344,8 @@ list_lru_walk_one_irq(struct list_lru *lru, int ni= d, struct mem_cgroup *memcg, > > list_lru_walk_cb isolate, void *cb_arg, > > unsigned long *nr_to_walk) > > { > > - struct list_lru_node *nlru =3D &lru->node[nid]; > > - unsigned long ret; > > - > > - spin_lock_irq(&nlru->lock); > > - ret =3D __list_lru_walk_one(lru, nid, memcg_kmem_id(memcg), isola= te, > > - cb_arg, nr_to_walk); > > - spin_unlock_irq(&nlru->lock); > > - return ret; > > + return __list_lru_walk_one(lru, nid, memcg, isolate, > > + cb_arg, nr_to_walk, true); > > } > > > > unsigned long list_lru_walk_node(struct list_lru *lru, int nid, > > @@ -332,16 +360,21 @@ unsigned long list_lru_walk_node(struct list_lru = *lru, int nid, > > #ifdef CONFIG_MEMCG > > if (*nr_to_walk > 0 && list_lru_memcg_aware(lru)) { > > struct list_lru_memcg *mlru; > > + struct mem_cgroup *memcg; > > unsigned long index; > > > > xa_for_each(&lru->xa, index, mlru) { > > - struct list_lru_node *nlru =3D &lru->node[nid]; > > - > > - spin_lock(&nlru->lock); > > - isolated +=3D __list_lru_walk_one(lru, nid, index= , > > + rcu_read_lock(); > > + memcg =3D mem_cgroup_from_id(index); > > + if (!mem_cgroup_tryget(memcg)) { > > + rcu_read_unlock(); > > + continue; > > + } > > + rcu_read_unlock(); > > + isolated +=3D __list_lru_walk_one(lru, nid, memcg= , > > isolate, cb_arg, > > - nr_to_walk); > > - spin_unlock(&nlru->lock); > > + nr_to_walk, false= ); > > + mem_cgroup_put(memcg); > > > > if (*nr_to_walk <=3D 0) > > break; > > @@ -353,14 +386,19 @@ unsigned long list_lru_walk_node(struct list_lru = *lru, int nid, > > } > > EXPORT_SYMBOL_GPL(list_lru_walk_node); > > > > -static void init_one_lru(struct list_lru_one *l) > > +static void init_one_lru(struct list_lru *lru, struct list_lru_one *l) > > { > > INIT_LIST_HEAD(&l->list); > > + spin_lock_init(&l->lock); > > l->nr_items =3D 0; > > +#ifdef CONFIG_LOCKDEP > > + if (lru->key) > > + lockdep_set_class(&l->lock, lru->key); > > +#endif > > } > > > > #ifdef CONFIG_MEMCG > > -static struct list_lru_memcg *memcg_init_list_lru_one(gfp_t gfp) > > +static struct list_lru_memcg *memcg_init_list_lru_one(struct list_lru = *lru, gfp_t gfp) > > { > > int nid; > > struct list_lru_memcg *mlru; > > @@ -370,7 +408,7 @@ static struct list_lru_memcg *memcg_init_list_lru_o= ne(gfp_t gfp) > > return NULL; > > > > for_each_node(nid) > > - init_one_lru(&mlru->node[nid]); > > + init_one_lru(lru, &mlru->node[nid]); > > > > return mlru; > > } > > @@ -398,28 +436,27 @@ static void memcg_destroy_list_lru(struct list_lr= u *lru) > > xas_unlock_irq(&xas); > > } > > > > -static void memcg_reparent_list_lru_node(struct list_lru *lru, int nid= , > > - struct list_lru_one *src, > > - struct mem_cgroup *dst_memcg) > > +static void memcg_reparent_list_lru_one(struct list_lru *lru, int nid, > > + struct list_lru_one *src, > > + struct mem_cgroup *dst_memcg) > > { > > - struct list_lru_node *nlru =3D &lru->node[nid]; > > + int dst_idx =3D dst_memcg->kmemcg_id; > > struct list_lru_one *dst; > > > > - /* > > - * Since list_lru_{add,del} may be called under an IRQ-safe lock, > > - * we have to use IRQ-safe primitives here to avoid deadlock. > > - */ > > - spin_lock_irq(&nlru->lock); > > - dst =3D list_lru_from_memcg_idx(lru, nid, memcg_kmem_id(dst_memcg= )); > > + spin_lock_irq(&src->lock); > > + dst =3D list_lru_from_memcg_idx(lru, nid, dst_idx); > > + spin_lock_nested(&dst->lock, SINGLE_DEPTH_NESTING); > > > > list_splice_init(&src->list, &dst->list); > > - > > if (src->nr_items) { > > dst->nr_items +=3D src->nr_items; > > set_shrinker_bit(dst_memcg, nid, lru_shrinker_id(lru)); > > - src->nr_items =3D 0; > > } > > - spin_unlock_irq(&nlru->lock); > > + /* Mark the list_lru_one dead */ > > + src->nr_items =3D LONG_MIN; > > + > > + spin_unlock(&dst->lock); > > + spin_unlock_irq(&src->lock); > > } > > > > void memcg_reparent_list_lrus(struct mem_cgroup *memcg, struct mem_cgr= oup *parent) > > @@ -448,7 +485,7 @@ void memcg_reparent_list_lrus(struct mem_cgroup *me= mcg, struct mem_cgroup *paren > > * safe to reparent. > > */ > > for_each_node(i) > > - memcg_reparent_list_lru_node(lru, i, &mlru->node[= i], parent); > > + memcg_reparent_list_lru_one(lru, i, &mlru->node[i= ], parent); > > > > /* > > * Here all list_lrus corresponding to the cgroup are gua= ranteed > > @@ -497,7 +534,7 @@ int memcg_list_lru_alloc(struct mem_cgroup *memcg, = struct list_lru *lru, > > parent =3D parent_mem_cgroup(pos); > > } > > > > - mlru =3D memcg_init_list_lru_one(gfp); > > + mlru =3D memcg_init_list_lru_one(lru, gfp); > > if (!mlru) > > return -ENOMEM; > > xas_set(&xas, pos->kmemcg_id); > > @@ -544,14 +581,8 @@ int __list_lru_init(struct list_lru *lru, bool mem= cg_aware, struct shrinker *shr > > if (!lru->node) > > return -ENOMEM; > > > > - for_each_node(i) { > > - spin_lock_init(&lru->node[i].lock); > > -#ifdef CONFIG_LOCKDEP > > - if (lru->key) > > - lockdep_set_class(&lru->node[i].lock, lru->key); > > -#endif > > - init_one_lru(&lru->node[i].lru); > > - } > > + for_each_node(i) > > + init_one_lru(lru, &lru->node[i].lru); > > > > memcg_init_list_lru(lru, memcg_aware); > > list_lru_register(lru); > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index 8e90aa026c47..f421dfcfe8a1 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -3098,8 +3098,13 @@ static void memcg_offline_kmem(struct mem_cgroup= *memcg) > > if (!parent) > > parent =3D root_mem_cgroup; > > > > - memcg_reparent_objcgs(memcg, parent); > > memcg_reparent_list_lrus(memcg, parent); > > + > > + /* > > + * Objcg's reparenting must be after list_lru's, make sure list_l= ru > > + * helpers won't use parent's list_lru until child is drained. > > + */ > > + memcg_reparent_objcgs(memcg, parent); > > } > > > > #ifdef CONFIG_CGROUP_WRITEBACK > > diff --git a/mm/workingset.c b/mm/workingset.c > > index df3937c5eedc..8c4b6738dcad 100644 > > --- a/mm/workingset.c > > +++ b/mm/workingset.c > > @@ -777,7 +777,6 @@ static enum lru_status shadow_lru_isolate(struct li= st_head *item, > > ret =3D LRU_REMOVED_RETRY; > > out: > > cond_resched(); > > - spin_lock_irq(lru_lock); > > return ret; > > } > > > > diff --git a/mm/zswap.c b/mm/zswap.c > > index 95a28301b7cd..cb37a25fd00c 100644 > > --- a/mm/zswap.c > > +++ b/mm/zswap.c > > @@ -706,9 +706,9 @@ static void zswap_lru_add(struct list_lru *list_lru= , struct zswap_entry *entry) > > * Note that it is safe to use rcu_read_lock() here, even in the = face of > > * concurrent memcg offlining: > > * > > - * 1. list_lru_add() is called before list_lru_memcg is erased. T= he > > + * 1. list_lru_add() is called before list_lru_one is dead. The > > * new entry will be reparented to memcg's parent's list_lru. > > - * 2. list_lru_add() is called after list_lru_memcg is erased. Th= e > > + * 2. list_lru_add() is called after list_lru_one is dead. The > > * new entry will be added directly to memcg's parent's list_l= ru. > > * > > * Similar reasoning holds for list_lru_del(). > > @@ -1173,7 +1173,6 @@ static enum lru_status shrink_memcg_cb(struct lis= t_head *item, struct list_lru_o > > zswap_written_back_pages++; > > } > > > > - spin_lock(lock); > > return ret; > > } > > > > Hi Kairui, > > I was testing zswap writeback in mm-unstable, and I think this patch migh= t be breaking things. > > I have added the panic below > > 130.051024] ------------[ cut here ]------------ > [ 130.051489] kernel BUG at mm/list_lru.c:321! > [ 130.051732] Oops: invalid opcode: 0000 [#1] SMP > [ 130.052133] CPU: 1 UID: 0 PID: 4976 Comm: cc1 Not tainted 6.12.0-rc1-0= 0084-g278bd01cdaf1 #276 > [ 130.052595] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIO= S 1.16.3-2.el9 04/01/2014 > [ 130.053276] RIP: 0010:__list_lru_walk_one+0x1ae/0x1b0 > [ 130.053983] Code: 7c 24 78 00 74 03 fb eb 00 48 89 d8 48 83 c4 40 5b 4= 1 5c 41 5d 41 5e 41 5f 5d c3 41 c6 07 00 eb e8 41 c6 07 00 fb eb e1 0f 0b <= 0f> 0b 0f 1f 44 00 00 6a 01 e8 44 fe ff ff 48 83 c4 08 c3 66 2e 0f > [ 130.055557] RSP: 0000:ffffc90004a2b9a0 EFLAGS: 00010246 > [ 130.056084] RAX: ffff88805dedf6e8 RBX: 0000000000000071 RCX: 000000000= 0000005 > [ 130.057407] RDX: 0000000000000000 RSI: 0000000000000022 RDI: ffff88800= 8a26400 > [ 130.057794] RBP: ffff88805dedf6d0 R08: 0000000000000402 R09: 000000000= 0000001 > [ 130.058579] R10: ffffc90004a2b7e8 R11: 0000000000000000 R12: ffffffff8= 1342930 > [ 130.058962] R13: ffff888017532ca0 R14: ffffc90004a2bae8 R15: ffff88801= 75322c8 > [ 130.059773] FS: 00007ff3f1e21f00(0000) GS:ffff88807dd00000(0000) knlG= S:0000000000000000 > [ 130.060242] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 130.060563] CR2: 00007f428e2e2ed8 CR3: 0000000067db6001 CR4: 000000000= 0770ef0 > [ 130.060952] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 000000000= 0000000 > [ 130.061658] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 000000000= 0000400 > [ 130.062425] PKRU: 55555554 > [ 130.062578] Call Trace: > [ 130.062720] > [ 130.062941] ? __die_body+0x66/0xb0 > [ 130.063145] ? die+0x88/0xb0 > [ 130.063309] ? do_trap+0x9d/0x170 > [ 130.063499] ? __list_lru_walk_one+0x1ae/0x1b0 > [ 130.063745] ? __list_lru_walk_one+0x1ae/0x1b0 > [ 130.063995] ? handle_invalid_op+0x65/0x80 > [ 130.064223] ? __list_lru_walk_one+0x1ae/0x1b0 > [ 130.064467] ? exc_invalid_op+0x2f/0x40 > [ 130.064681] ? asm_exc_invalid_op+0x16/0x20 > [ 130.064912] ? zswap_shrinker_count+0x1c0/0x1c0 > [ 130.065172] ? __list_lru_walk_one+0x1ae/0x1b0 > [ 130.065417] list_lru_walk_one+0xc/0x20 > [ 130.065630] zswap_shrinker_scan+0x4b/0x80 > [ 130.065856] do_shrink_slab+0x15f/0x2f0 > [ 130.066075] shrink_slab+0x2bf/0x3d0 > [ 130.066276] shrink_node+0x4f0/0x8a0 > [ 130.066477] do_try_to_free_pages+0x131/0x4d0 > [ 130.066717] try_to_free_mem_cgroup_pages+0x143/0x220 > [ 130.067000] try_charge_memcg+0x22a/0x610 > [ 130.067224] __mem_cgroup_charge+0x74/0x100 > [ 130.068060] do_pte_missing+0xaa8/0x1020 > [ 130.068280] handle_mm_fault+0x75d/0x1120 > [ 130.068502] do_user_addr_fault+0x1c2/0x6f0 > [ 130.068802] exc_page_fault+0x4f/0xb0 > [ 130.069014] asm_exc_page_fault+0x22/0x30 > [ 130.069240] RIP: 0033:0x7ff3f19ede49 > [ 130.069441] Code: c9 62 e1 7f 29 7f 00 c3 66 0f 1f 84 00 00 00 00 00 4= 0 0f b6 c6 48 89 d1 48 89 fa f3 aa 48 89 d0 c3 48 3b 15 c9 a3 06 00 77 e7 <= 62> e1 fe 28 7f 07 62 e1 fe 28 7f 47 01 48 81 fa 80 00 00 00 76 89 > [ 130.070477] RSP: 002b:00007ffc5c818078 EFLAGS: 00010283 > [ 130.070830] RAX: 00007ff3efac9000 RBX: 00007ff3f02d1940 RCX: 000000000= 0000001 > [ 130.071522] RDX: 00000000000005a8 RSI: 0000000000000000 RDI: 00007ff3e= fac9000 > [ 130.072146] RBP: 00007ffc5c8180c0 R08: 0000000003007320 R09: 000000000= 0000007 > [ 130.072594] R10: 0000000003007320 R11: 0000000000000012 R12: 00007ff3f= 1f0e000 > [ 130.072981] R13: 000000007ffa1e74 R14: 00000000000005a8 R15: 000000000= 00000b5 > [ 130.073369] > [ 130.073496] Modules linked in: > [ 130.073701] ---[ end trace 0000000000000000 ]--- > [ 130.073960] RIP: 0010:__list_lru_walk_one+0x1ae/0x1b0 > [ 130.074319] Code: 7c 24 78 00 74 03 fb eb 00 48 89 d8 48 83 c4 40 5b 4= 1 5c 41 5d 41 5e 41 5f 5d c3 41 c6 07 00 eb e8 41 c6 07 00 fb eb e1 0f 0b <= 0f> 0b 0f 1f 44 00 00 6a 01 e8 44 fe ff ff 48 83 c4 08 c3 66 2e 0f > [ 130.075564] RSP: 0000:ffffc90004a2b9a0 EFLAGS: 00010246 > [ 130.075897] RAX: ffff88805dedf6e8 RBX: 0000000000000071 RCX: 000000000= 0000005 > [ 130.076342] RDX: 0000000000000000 RSI: 0000000000000022 RDI: ffff88800= 8a26400 > [ 130.076739] RBP: ffff88805dedf6d0 R08: 0000000000000402 R09: 000000000= 0000001 > [ 130.077192] R10: ffffc90004a2b7e8 R11: 0000000000000000 R12: ffffffff8= 1342930 > [ 130.077739] R13: ffff888017532ca0 R14: ffffc90004a2bae8 R15: ffff88801= 75322c8 > [ 130.078149] FS: 00007ff3f1e21f00(0000) GS:ffff88807dd00000(0000) knlG= S:0000000000000000 > [ 130.078764] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 130.079095] CR2: 00007f428e2e2ed8 CR3: 0000000067db6001 CR4: 000000000= 0770ef0 > [ 130.079521] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 000000000= 0000000 > [ 130.080009] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 000000000= 0000400 > [ 130.080402] PKRU: 55555554 > [ 130.080713] Kernel panic - not syncing: Fatal exception > [ 130.081198] Kernel Offset: disabled > [ 130.081396] ---[ end Kernel panic - not syncing: Fatal exception ]--- > > Thanks, > Usama > Thanks for the report. I converted list_lru_walk callback to keep the list unlocked when LRU_RETRY and LRU_REMOVED_RETRY is returned, but didn't notice shrink_memcg_cg in zswap.c could return LRU_STOP after it unlocked the list. The fix should be simple, is it easy to reproduce? Can you help verify? diff --git a/mm/list_lru.c b/mm/list_lru.c index 79c2d21504a2..1a3caf4c4e14 100644 --- a/mm/list_lru.c +++ b/mm/list_lru.c @@ -298,9 +298,9 @@ __list_lru_walk_one(struct list_lru *lru, int nid, struct mem_cgroup *memcg, ret =3D isolate(item, l, cb_arg); switch (ret) { /* - * LRU_RETRY and LRU_REMOVED_RETRY will drop the lru lock, - * the list traversal will be invalid and have to restart f= rom - * scratch. + * LRU_RETRY, LRU_REMOVED_RETRY and LRU_STOP will drop the = lru + * lock, the list traversal will be invalid and have to res= tart + * from scratch. */ case LRU_RETRY: goto restart; @@ -318,14 +318,13 @@ __list_lru_walk_one(struct list_lru *lru, int nid, struct mem_cgroup *memcg, case LRU_SKIP: break; case LRU_STOP: - assert_spin_locked(&l->lock); goto out; default: BUG(); } } -out: unlock_list_lru(l, irq_off); +out: return isolated; }