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 30DE2D149F2 for ; Fri, 25 Oct 2024 21:13:14 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A627F6B00B2; Fri, 25 Oct 2024 17:13:13 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A124B6B00B3; Fri, 25 Oct 2024 17:13:13 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 866456B00B8; Fri, 25 Oct 2024 17:13:13 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 35B696B00B2 for ; Fri, 25 Oct 2024 17:13:13 -0400 (EDT) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 382891A0FC7 for ; Fri, 25 Oct 2024 21:12:37 +0000 (UTC) X-FDA: 82713374058.07.7E15F0B Received: from mail-wm1-f45.google.com (mail-wm1-f45.google.com [209.85.128.45]) by imf11.hostedemail.com (Postfix) with ESMTP id 58D844001E for ; Fri, 25 Oct 2024 21:12:46 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=EaqIwxbs; spf=pass (imf11.hostedemail.com: domain of usamaarif642@gmail.com designates 209.85.128.45 as permitted sender) smtp.mailfrom=usamaarif642@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=1729890635; 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=fvdti8gIxrkcQJ6mTK811/x8l2PD48doQkoU0rQK6Rw=; b=BePyHmEjFjdFt2Y59XjXlcTj5d3yQ5EgC2OJjkIyanrN24XeqInijWkTSKu5YZGc8cpFtI b21ndpjYIqdYIzbGgFJC5CLFSrfYkcZwEb6Ixn1wQ9T8W/o0ybi8k+P7eNdV0gM5b65DT0 FQykCieA25WIaQWGnMSNn91ZoCyF92E= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1729890635; a=rsa-sha256; cv=none; b=gZaWWUHX0TFEihMK7wAead65Idrk/XSsEDYSPJGoXXxCm1ZoHqAjpZtSAB/nZzlMXXiuHR gEjMqhLMRgkWQU9SnMQkP7ke7eEikxaJwmhqrYG1aK/pN3VJur+UXYoez33L227ZxPQce9 a8mSs8tjZLamaR+p6OWd4yFon0hxHY0= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=EaqIwxbs; spf=pass (imf11.hostedemail.com: domain of usamaarif642@gmail.com designates 209.85.128.45 as permitted sender) smtp.mailfrom=usamaarif642@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-wm1-f45.google.com with SMTP id 5b1f17b1804b1-4314b316495so22216285e9.2 for ; Fri, 25 Oct 2024 14:13:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1729890789; x=1730495589; darn=kvack.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=fvdti8gIxrkcQJ6mTK811/x8l2PD48doQkoU0rQK6Rw=; b=EaqIwxbszBaYNJ9TwHFIAATT/nVmuSfVP24ounQOZPvUFxbPRsygFLtbas6K18Kxde Fh+8kJ5GggTyJlhXCgApjfNKjuZ3lhvha/sTYofCnsJzG5ADTJBcQD2VFUEfbP3mX+jN CHQxpRbnuLpnTTWkCo5LgYONemvARNR6Alza8lrMDuVJjF+luoQeBkN1j7iaYwgUd5cb cL1RaygvTq4fjFpE95GzpFbjRf0n7enhCjCR09kv628aRkOfrb87lqRkrFmqdBt3vYqJ Fe9WgCP1QuM7lwZ9VwGS8oh2phK+nFjTFdKLkDDfhtrfSo1qt60RUKeEmCCGGnDIAHp2 Q7/A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1729890789; x=1730495589; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=fvdti8gIxrkcQJ6mTK811/x8l2PD48doQkoU0rQK6Rw=; b=hNk9DZLGLfsJJFVE+zGoHkKwJYhDl4ibyMFyc1NoMXPNFEy9Hm6GRY32KrGrGICSv0 pQnOWbnqK0d2q7eVri+XEG4aB2jpcRC78t3HiDsA0ju+WnIV4776x91BplNJ78eHgTjd j4qvZCgSKpgK2RV0mcdR8Z9KJxD5cp2wvQBgeH2VawZv1eE0+ENJ2YjkqP75EqW5mtXw dO/pTG6j1Im25f0oH8F1Ll7Je8QElbxj+buxo/0Kf+b6+LcooW9w6WGz20lIvEmlX7/5 eyBUy8MlpxFTwJhTE7UYPTpptZLZEWR9fc8BkJSweUgkRa0CyUeBYHTODW+0GCGDQ3BE do+g== X-Forwarded-Encrypted: i=1; AJvYcCXtWpvAyNhMzEk9MiSZz2YgHS4eAW3kKEzVyTqUtFvFzD3MtS45N7a5UMvUKgJNp6GD5qnY7sGM7Q==@kvack.org X-Gm-Message-State: AOJu0YzvTBcXqNXUuiL4/qjHnCd4KMyxvBHzOGoTHH9hNEP+MLlXxkxt vsWlnjvSI5m7Hd01SL2Ia84q0ALLDVPhUAKot2irBZIWbB4rPIQo X-Google-Smtp-Source: AGHT+IGtOU+iczD6ytqQxRU4L5qjWVAVKCoI0728+qskbvdLIuvdxzbwZ4VLCdp8G9oIDIKIyx98Ng== X-Received: by 2002:a05:600c:3c9a:b0:431:5194:1687 with SMTP id 5b1f17b1804b1-4319acb12bamr3175105e9.18.1729890788972; Fri, 25 Oct 2024 14:13:08 -0700 (PDT) Received: from ?IPV6:2a02:6b67:d751:7400:c2b:f323:d172:e42a? ([2a02:6b67:d751:7400:c2b:f323:d172:e42a]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4318b5430absm58625475e9.6.2024.10.25.14.13.08 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 25 Oct 2024 14:13:08 -0700 (PDT) Message-ID: <7334b158-8567-4c66-8176-57b879ff8882@gmail.com> Date: Fri, 25 Oct 2024 22:13:08 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 5/6] mm/list_lru: split the lock to per-cgroup scope To: Kairui Song , linux-mm@kvack.org Cc: Andrew Morton , Matthew Wilcox , Johannes Weiner , Roman Gushchin , Waiman Long , Shakeel Butt , Michal Hocko , Chengming Zhou , Qi Zheng , Muchun Song , Nhat Pham , Yosry Ahmed References: <20240925171020.32142-1-ryncsn@gmail.com> <20240925171020.32142-6-ryncsn@gmail.com> Content-Language: en-US From: Usama Arif In-Reply-To: <20240925171020.32142-6-ryncsn@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Stat-Signature: xb6grnmexqj59iz9kjsg39rwo55njqjb X-Rspamd-Queue-Id: 58D844001E X-Rspam-User: X-Rspamd-Server: rspam08 X-HE-Tag: 1729890766-548893 X-HE-Meta: U2FsdGVkX1+GAETYL+IPK9OHROxrNtrEthJY12kraK36K106MkJNBpCDvyLcTcQA0hdSitchuxf6SCSPmyCGUOetz8LPjsIjPBM61BcQM5gy+j5b7vX4K6a+KkhKXCTVHMGhm5TnzHNfry12xTWSTKAvrmHTSCb0b7ZTZFElf8jppZ6rQX8CuPFE2siQpQsM1+rmxTVWaAJnRZN3dw/sRRLGp0jbV3VQ8idky9+YoqZXmnaZjcWlzj6eY8Y6kd26h063Akckh0oUzIPm1XbusQTM1uV2DmH9iLg/oU8g0CuD967pXm81I4vU44VkPUPj49mIN5yYUMjNTfy8HWMZJW6YvqbzLMghko2kGucOBm++KzWV7/l5rkze7/j5MRsEa46b7zqj0cFhYxZc5C9z636nYEMMezSLonO2G//UpfXUKIKNxpm4hYOuSrlJl0D+bNDidrCcniYyaEkdk4hkmw6RJy4T9e0iDt23Cs7p3l7ArWuIEwWBGmIPK0fsQ9y/MYyoB+vVF/skrmVQH3g1BFfduEO/IzqW1XQm/kGBZiETi8sXFWuWJ/YXosfSdh2Dm4dGK5bJQY4sKsMeTaft1RYUGoc5zX/9ifAm1IbynBJwXx1snHl8aLr0Lx3RPxIrec+cxS7BTEBvwn1iKQyHFYcphsykjjy0OWnOn6UtVe6bD0XD5fJt8+2G5Qj1k3XPR13C1DzXbGlR++5uMwWebiit1I/PSM4+gzaAl/wWSEs5ONpQLJ+4iyLdW/92OkIJ+GgsdTIf8vV337OVW9p7hdxplqxNykHcuV3X3pt6NH9hn0EdGm1snZdnzAbbRANQB/vcEEbHWicLB49H4KXgcLi7jokKFguxQd9k4960fzAsggH4T+Wf8OvLtEv1yk5EnzJnCrzueIIXeqmkCWA3ORgiOBcuNxSsd/v0EAzsRB3SM7AUBd6L/LxlPX6WYOCh5gxU0/mRyWQjdaeMiDi 7PQir+mz l8f30eiXCP2je8rJi+gTxprfVFGdcwfmZJqoVWXltQ9clKycB9aWckF/CeFfh0pV7226rsyaqN+upbTDDSzGhhsYaaREcY+csUtuaJzPzjcSlBoFz5wjIV+de8sjMJGk8mfMiGgkCXejHGYkGfnGUVhTSO3FrerxDraH4WEL+a+9+S4mgcIIzN60id4S+HSskcWO2znu+ac2BqZd1kPS+SxM0bPpxrJ5UuGFLeZvRgNPNWUPLx6eK5nAMx6zlzbYnxzqfY+qOE6CTq1jw8ysN58SYAFf3Sx3AX63UbVuhGinU0ZhsWwku4rGr8GWFkR0WtDxurjsuAmK2pGO2DAkB3RPCDy9xCIi3FHtJfl5QfEe0k1fgXqWcA1RSzMT73weoEq3qd/jGqABhMEQphe9NP7YCbvxzC9yMlqT5LacZNKt5TvymtDw2JgJaP8S3F/yh8avdB2P20MBwZhSfw9rySiYNdhkd8LwpCNnmV3AZOMWyQ4yHkfbpiLKuHlL5F7vjbEcmzFjIc8RceCJixsBKKpxFrjMLSIJPlq3y 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 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=1 rd_size=33554432 > 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_alloc.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 list_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 list_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 nid, 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_cgroup *memcg, > + bool irq, bool skip_empty) > { > struct list_lru_one *l; > + rcu_read_lock(); > again: > l = list_lru_from_memcg_idx(lru, nid, memcg_kmem_id(memcg)); > - if (likely(l)) > - return l; > - > - memcg = 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) != 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 = parent_mem_cgroup(memcg); > goto again; > } > + > +static inline void unlock_list_lru(struct list_lru_one *l, bool irq_off) > +{ > + 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_cgroup *memcg, > + bool irq, bool skip_empty) > { > - return &lru->node[nid].lru; > + struct list_lru_one *l = &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_off) > +{ > + 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 nid, > - struct mem_cgroup *memcg) > + struct mem_cgroup *memcg) > { > struct list_lru_node *nlru = &lru->node[nid]; > struct list_lru_one *l; > > - spin_lock(&nlru->lock); > + l = lock_list_lru_of_memcg(lru, nid, memcg, false, false); > + if (!l) > + return false; > if (list_empty(item)) { > - l = 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 nid, > - struct mem_cgroup *memcg) > + struct mem_cgroup *memcg) > { > struct list_lru_node *nlru = &lru->node[nid]; > struct list_lru_one *l; > - > - spin_lock(&nlru->lock); > + l = lock_list_lru_of_memcg(lru, nid, memcg, false, false); > + if (!l) > + return false; > if (!list_empty(item)) { > - l = 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 = &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 = &lru->node[nid]; > - struct list_lru_one *l; > + struct list_lru_one *l = NULL; > struct list_head *item, *n; > unsigned long isolated = 0; > > restart: > - l = list_lru_from_memcg_idx(lru, nid, memcg_idx); > + l = 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 = isolate(item, l, &nlru->lock, cb_arg); > + ret = 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 == 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 traversal is > - * now invalid and so we have to restart from scratch. > - */ > - 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, struct mem_cgroup *memcg, > list_lru_walk_cb isolate, void *cb_arg, > unsigned long *nr_to_walk) > { > - struct list_lru_node *nlru = &lru->node[nid]; > - unsigned long ret; > - > - spin_lock(&nlru->lock); > - ret = __list_lru_walk_one(lru, nid, memcg_kmem_id(memcg), isolate, > - 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 nid, struct mem_cgroup *memcg, > list_lru_walk_cb isolate, void *cb_arg, > unsigned long *nr_to_walk) > { > - struct list_lru_node *nlru = &lru->node[nid]; > - unsigned long ret; > - > - spin_lock_irq(&nlru->lock); > - ret = __list_lru_walk_one(lru, nid, memcg_kmem_id(memcg), isolate, > - 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 = &lru->node[nid]; > - > - spin_lock(&nlru->lock); > - isolated += __list_lru_walk_one(lru, nid, index, > + rcu_read_lock(); > + memcg = mem_cgroup_from_id(index); > + if (!mem_cgroup_tryget(memcg)) { > + rcu_read_unlock(); > + continue; > + } > + rcu_read_unlock(); > + isolated += __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 <= 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 = 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_one(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_lru *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 = &lru->node[nid]; > + int dst_idx = 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 = list_lru_from_memcg_idx(lru, nid, memcg_kmem_id(dst_memcg)); > + spin_lock_irq(&src->lock); > + dst = 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 += src->nr_items; > set_shrinker_bit(dst_memcg, nid, lru_shrinker_id(lru)); > - src->nr_items = 0; > } > - spin_unlock_irq(&nlru->lock); > + /* Mark the list_lru_one dead */ > + src->nr_items = LONG_MIN; > + > + spin_unlock(&dst->lock); > + spin_unlock_irq(&src->lock); > } > > void memcg_reparent_list_lrus(struct mem_cgroup *memcg, struct mem_cgroup *parent) > @@ -448,7 +485,7 @@ void memcg_reparent_list_lrus(struct mem_cgroup *memcg, 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 guaranteed > @@ -497,7 +534,7 @@ int memcg_list_lru_alloc(struct mem_cgroup *memcg, struct list_lru *lru, > parent = parent_mem_cgroup(pos); > } > > - mlru = memcg_init_list_lru_one(gfp); > + mlru = 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 memcg_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 = 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_lru > + * 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 list_head *item, > ret = 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. The > + * 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. The > + * 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_lru. > * > * Similar reasoning holds for list_lru_del(). > @@ -1173,7 +1173,6 @@ static enum lru_status shrink_memcg_cb(struct list_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 might 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-00084-g278bd01cdaf1 #276 [ 130.052595] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 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 41 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: 0000000000000005 [ 130.057407] RDX: 0000000000000000 RSI: 0000000000000022 RDI: ffff888008a26400 [ 130.057794] RBP: ffff88805dedf6d0 R08: 0000000000000402 R09: 0000000000000001 [ 130.058579] R10: ffffc90004a2b7e8 R11: 0000000000000000 R12: ffffffff81342930 [ 130.058962] R13: ffff888017532ca0 R14: ffffc90004a2bae8 R15: ffff8880175322c8 [ 130.059773] FS: 00007ff3f1e21f00(0000) GS:ffff88807dd00000(0000) knlGS:0000000000000000 [ 130.060242] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 130.060563] CR2: 00007f428e2e2ed8 CR3: 0000000067db6001 CR4: 0000000000770ef0 [ 130.060952] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 130.061658] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 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 40 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: 0000000000000001 [ 130.071522] RDX: 00000000000005a8 RSI: 0000000000000000 RDI: 00007ff3efac9000 [ 130.072146] RBP: 00007ffc5c8180c0 R08: 0000000003007320 R09: 0000000000000007 [ 130.072594] R10: 0000000003007320 R11: 0000000000000012 R12: 00007ff3f1f0e000 [ 130.072981] R13: 000000007ffa1e74 R14: 00000000000005a8 R15: 00000000000000b5 [ 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 41 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: 0000000000000005 [ 130.076342] RDX: 0000000000000000 RSI: 0000000000000022 RDI: ffff888008a26400 [ 130.076739] RBP: ffff88805dedf6d0 R08: 0000000000000402 R09: 0000000000000001 [ 130.077192] R10: ffffc90004a2b7e8 R11: 0000000000000000 R12: ffffffff81342930 [ 130.077739] R13: ffff888017532ca0 R14: ffffc90004a2bae8 R15: ffff8880175322c8 [ 130.078149] FS: 00007ff3f1e21f00(0000) GS:ffff88807dd00000(0000) knlGS:0000000000000000 [ 130.078764] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 130.079095] CR2: 00007f428e2e2ed8 CR3: 0000000067db6001 CR4: 0000000000770ef0 [ 130.079521] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 130.080009] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 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