From: Qi Zheng <zhengqi.arch@bytedance.com>
To: Kirill Tkhai <tkhai@ya.ru>
Cc: sultan@kerneltoast.com, dave@stgolabs.net,
penguin-kernel@I-love.SAKURA.ne.jp, paulmck@kernel.org,
linux-mm@kvack.org, linux-kernel@vger.kernel.org,
Andrew Morton <akpm@linux-foundation.org>,
Johannes Weiner <hannes@cmpxchg.org>,
Shakeel Butt <shakeelb@google.com>,
Michal Hocko <mhocko@kernel.org>,
Roman Gushchin <roman.gushchin@linux.dev>,
Muchun Song <muchun.song@linux.dev>,
David Hildenbrand <david@redhat.com>,
Yang Shi <shy828301@gmail.com>
Subject: Re: [PATCH 2/5] mm: vmscan: make memcg slab shrink lockless
Date: Wed, 22 Feb 2023 16:21:42 +0800 [thread overview]
Message-ID: <b9d344d0-9d27-034d-d59e-9e880318906b@bytedance.com> (raw)
In-Reply-To: <39cf7fa7-5dd6-0424-23de-1c33e948a201@bytedance.com>
On 2023/2/22 16:16, Qi Zheng wrote:
> Hi Kirill,
>
> On 2023/2/22 05:43, Kirill Tkhai wrote:
>> On 20.02.2023 12:16, Qi Zheng wrote:
>>> Like global slab shrink, since commit 1cd0bd06093c<...>
>>> static bool cgroup_reclaim(struct scan_control *sc)
>>> @@ -891,15 +905,14 @@ static unsigned long shrink_slab_memcg(gfp_t
>>> gfp_mask, int nid,
>>> {
>>> struct shrinker_info *info;
>>> unsigned long ret, freed = 0;
>>> + int srcu_idx;
>>> int i;
>>> if (!mem_cgroup_online(memcg))
>>> return 0;
>>> - if (!down_read_trylock(&shrinker_rwsem))
>>> - return 0;
>>> -
>>> - info = shrinker_info_protected(memcg, nid);
>>> + srcu_idx = srcu_read_lock(&shrinker_srcu);
>>> + info = shrinker_info_srcu(memcg, nid);
>>> if (unlikely(!info))
>>> goto unlock;
>>
>> There is shrinker_nr_max dereference under this hunk. It's not in the
>> patch:
>>
>> for_each_set_bit(i, info->map, shrinker_nr_max) {
>>
>> Since shrinker_nr_max may grow in parallel, this leads to access
>> beyond allocated memory :(
>
> Oh, indeed.
>
>>
>> It looks like we should save size of info->map as a new member of
>> struct shrinker_info.
>
> Agree, then we only traverse info->map_size each time. Like below:
>
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index b6eda2ab205d..f1b5d2803007 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -97,6 +97,7 @@ struct shrinker_info {
> struct rcu_head rcu;
> atomic_long_t *nr_deferred;
> unsigned long *map;
> + int map_size;
> };
>
> struct lruvec_stats_percpu {
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index f94bfe540675..dd07eb107915 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -239,14 +239,20 @@ static void free_shrinker_info_rcu(struct rcu_head
> *head)
> kvfree(container_of(head, struct shrinker_info, rcu));
> }
>
> -static int expand_one_shrinker_info(struct mem_cgroup *memcg,
> - int map_size, int defer_size,
> - int old_map_size, int old_defer_size)
> +static int expand_one_shrinker_info(struct mem_cgroup *memcg, int
> new_nr_max,
> + int old_nr_max)
> {
> + int map_size, defer_size, old_map_size, old_defer_size;
> struct shrinker_info *new, *old;
> struct mem_cgroup_per_node *pn;
> int nid;
> - int size = map_size + defer_size;
> + int size;
> +
> + map_size = shrinker_map_size(new_nr_max);
> + defer_size = shrinker_defer_size(new_nr_max);
> + old_map_size = shrinker_map_size(shrinker_nr_max);
> + old_defer_size = shrinker_defer_size(shrinker_nr_max);
Perhaps these should still be calculated outside the loop as before.
> + size = map_size + defer_size;
>
> for_each_node(nid) {
> pn = memcg->nodeinfo[nid];
> @@ -261,6 +267,7 @@ static int expand_one_shrinker_info(struct
> mem_cgroup *memcg,
>
> new->nr_deferred = (atomic_long_t *)(new + 1);
> new->map = (void *)new->nr_deferred + defer_size;
> + new->map_size = new_nr_max;
>
> /* map: set all old bits, clear all new bits */
> memset(new->map, (int)0xff, old_map_size);
> @@ -310,6 +317,7 @@ int alloc_shrinker_info(struct mem_cgroup *memcg)
> }
> info->nr_deferred = (atomic_long_t *)(info + 1);
> info->map = (void *)info->nr_deferred + defer_size;
> + info->map_size = shrinker_nr_max;
> rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_info,
> info);
> }
> mutex_unlock(&shrinker_mutex);
> @@ -327,8 +335,6 @@ static int expand_shrinker_info(int new_id)
> {
> int ret = 0;
> int new_nr_max = new_id + 1;
> - int map_size, defer_size = 0;
> - int old_map_size, old_defer_size = 0;
> struct mem_cgroup *memcg;
>
> if (!need_expand(new_nr_max))
> @@ -339,15 +345,9 @@ static int expand_shrinker_info(int new_id)
>
> lockdep_assert_held(&shrinker_mutex);
>
> - map_size = shrinker_map_size(new_nr_max);
> - defer_size = shrinker_defer_size(new_nr_max);
> - old_map_size = shrinker_map_size(shrinker_nr_max);
> - old_defer_size = shrinker_defer_size(shrinker_nr_max);
> -
> memcg = mem_cgroup_iter(NULL, NULL, NULL);
> do {
> - ret = expand_one_shrinker_info(memcg, map_size, defer_size,
> - old_map_size,
> old_defer_size);
> + ret = expand_one_shrinker_info(memcg, new_nr_max,
> shrinker_nr_max);
> if (ret) {
> mem_cgroup_iter_break(NULL, memcg);
> goto out;
> @@ -912,7 +912,7 @@ static unsigned long shrink_slab_memcg(gfp_t
> gfp_mask, int nid,
> if (unlikely(!info))
> goto unlock;
>
> - for_each_set_bit(i, info->map, shrinker_nr_max) {
> + for_each_set_bit(i, info->map, info->map_size) {
> struct shrink_control sc = {
> .gfp_mask = gfp_mask,
> .nid = nid,
>
> I will send the v2.
>
> Thanks,
> Qi
>
>>
>>> @@ -949,14 +962,9 @@ static unsigned long shrink_slab_memcg(gfp_t
>>> gfp_mask, int nid,
>>> set_shrinker_bit(memcg, nid, i);
>>> }
>>> freed += ret;
>>> -
>>> - if (rwsem_is_contended(&shrinker_rwsem)) {
>>> - freed = freed ? : 1;
>>> - break;
>>> - }
>>> }
>>> unlock:
>>> - up_read(&shrinker_rwsem);
>>> + srcu_read_unlock(&shrinker_srcu, srcu_idx);
>>> return freed;
>>> }
>>> #else /* CONFIG_MEMCG */
>>
>
--
Thanks,
Qi
next prev parent reply other threads:[~2023-02-22 8:21 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-20 9:16 [PATCH 0/5] make " Qi Zheng
2023-02-20 9:16 ` [PATCH 1/5] mm: vmscan: make global " Qi Zheng
2023-02-20 9:16 ` [PATCH 2/5] mm: vmscan: make memcg " Qi Zheng
2023-02-21 21:28 ` Kirill Tkhai
2023-02-22 7:32 ` Qi Zheng
2023-02-22 19:58 ` Kirill Tkhai
2023-02-23 4:36 ` Qi Zheng
2023-02-21 21:43 ` Kirill Tkhai
2023-02-22 8:16 ` Qi Zheng
2023-02-22 8:21 ` Qi Zheng [this message]
2023-02-22 20:05 ` Kirill Tkhai
2023-02-23 4:37 ` Qi Zheng
2023-02-20 9:16 ` [PATCH 3/5] mm: shrinkers: make count and scan in shrinker debugfs lockless Qi Zheng
2023-02-20 13:22 ` kernel test robot
2023-02-20 9:16 ` [PATCH 4/5] mm: vmscan: remove shrinker_rwsem from synchronize_shrinkers() Qi Zheng
2023-02-20 9:16 ` [PATCH 5/5] mm: shrinkers: convert shrinker_rwsem to mutex Qi Zheng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b9d344d0-9d27-034d-d59e-9e880318906b@bytedance.com \
--to=zhengqi.arch@bytedance.com \
--cc=akpm@linux-foundation.org \
--cc=dave@stgolabs.net \
--cc=david@redhat.com \
--cc=hannes@cmpxchg.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@kernel.org \
--cc=muchun.song@linux.dev \
--cc=paulmck@kernel.org \
--cc=penguin-kernel@I-love.SAKURA.ne.jp \
--cc=roman.gushchin@linux.dev \
--cc=shakeelb@google.com \
--cc=shy828301@gmail.com \
--cc=sultan@kerneltoast.com \
--cc=tkhai@ya.ru \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox