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 589E7C7EE22 for ; Wed, 3 May 2023 08:53:02 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C0D3F6B0071; Wed, 3 May 2023 04:53:01 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id BBD476B0072; Wed, 3 May 2023 04:53:01 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A84516B0074; Wed, 3 May 2023 04:53:01 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from mail-ej1-f41.google.com (mail-ej1-f41.google.com [209.85.218.41]) by kanga.kvack.org (Postfix) with ESMTP id 552F86B0071 for ; Wed, 3 May 2023 04:53:01 -0400 (EDT) Received: by mail-ej1-f41.google.com with SMTP id a640c23a62f3a-959a3e2dd27so941808566b.3 for ; Wed, 03 May 2023 01:53:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20221208; t=1683103980; x=1685695980; 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=TbaAQ5UWf5QMYXvokGZmB71CDacil8tkLbI7wOrpl80=; b=Mxc2JCREE1yPHrD1YutRLpWerZjanLYglyXectkAajv+vGFjCFjVon+/cuIVG2lqYG AvBMg9a0frcRzu4xyDO08YhORm1wporFKAGPJu+/qvKnf2D+SGnJ97sPlAMDkIY57Ubv IoMSuTbLlDsvoCKfP+1vONSYT4IOkVAxBZenIDCdKBMQ3SpT+Rr1RUVmj+i4OS46I3ro 08ZQWmHjb+R2AswP26CLJE7jEW0gJJyjgbvcxSYu6BKJJmizPL7E45f8nRF0FG5k8Mqt foeHu56la/jy3m0su/xTMX2pNY3KdU66A2I5jzZ7SzNTu6v+p0TYxTrvkM6uD+TCJWga mRzg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683103980; x=1685695980; 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=TbaAQ5UWf5QMYXvokGZmB71CDacil8tkLbI7wOrpl80=; b=P8F/A6a88YB3dN+dfVuQ4Aqq1qGdk5pODvwIMbdIHSX0nazMkJPKZyQSieu1vUp8Lm DBtw80zgXQpwSV9EtKSpN9BkzU34vO57KlG5TPIIVICP1TnHca7OrQ3jtZFjCoYvoJfI J1HMuG+dRgZHw1wEeUxOOnSPQlatnEhJKCj2S5IGTiMfUzbidHxQlNCXTzjGojzKQ/AE rcvJ6i6k/kLZCMMhw/zstlD+o7uz/iUeTBcnW4YllLjnPiBVE/qtXeHyPR6c+tNHlFp6 0xIp11FVUFv47C5BTEJqgWl9Gv8Y57aZNpB6zfPCQ+yMJK5n0iWExEd3LdCf8WCs03Qd +xDA== X-Gm-Message-State: AC+VfDy+o3D/2qyNue1BWVbXCUWN9YFdM88FCLjrkWZ2M4x4JapEuK0V VJ5n+i6z3drMt1Vc2lCvC+PC1N5GhxFyrcHvto5MnQ== X-Google-Smtp-Source: ACHHUZ7PuEUATe/1giIF1EdKEXipkccdXkYVo+3JX0K5LcbIyR19TSYZhd2ZMEaZUr5444r5qjoFRdXoehdgkyjLeG4= X-Received: by 2002:a17:907:6d25:b0:961:8fcd:53c6 with SMTP id sa37-20020a1709076d2500b009618fcd53c6mr2390114ejc.66.1683103980340; Wed, 03 May 2023 01:53:00 -0700 (PDT) MIME-Version: 1.0 References: <20230428132406.2540811-1-yosryahmed@google.com> <20230428132406.2540811-3-yosryahmed@google.com> In-Reply-To: From: Yosry Ahmed Date: Wed, 3 May 2023 01:52:23 -0700 Message-ID: Subject: Re: [PATCH v2 2/2] memcg: dump memory.stat during cgroup OOM for v1 To: Michal Hocko Cc: Johannes Weiner , Roman Gushchin , Shakeel Butt , Andrew Morton , Muchun Song , Sergey Senozhatsky , Steven Rostedt , Petr Mladek , Chris Li , cgroups@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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: On Wed, May 3, 2023 at 1:50=E2=80=AFAM Michal Hocko wrote= : > > On Fri 28-04-23 13:24:06, Yosry Ahmed wrote: > > Commit c8713d0b2312 ("mm: memcontrol: dump memory.stat during cgroup > > OOM") made sure we dump all the stats in memory.stat during a cgroup > > OOM, but it also introduced a slight behavioral change. The code used t= o > > print the non-hierarchical v1 cgroup stats for the entire cgroup > > subtree, now it only prints the v2 cgroup stats for the cgroup under > > OOM. > > > > For cgroup v1 users, this introduces a few problems: > > (a) The non-hierarchical stats of the memcg under OOM are no longer > > shown. > > (b) A couple of v1-only stats (e.g. pgpgin, pgpgout) are no longer > > shown. > > (c) We show the list of cgroup v2 stats, even in cgroup v1. This list o= f > > stats is not tracked with v1 in mind. While most of the stats seem to b= e > > working on v1, there may be some stats that are not fully or correctly > > tracked. > > > > Although OOM log is not set in stone, we should not change it for no > > reason. When upgrading the kernel version to a version including > > commit c8713d0b2312 ("mm: memcontrol: dump memory.stat during cgroup > > OOM"), these behavioral changes are noticed in cgroup v1. > > > > The fix is simple. Commit c8713d0b2312 ("mm: memcontrol: dump memory.st= at > > during cgroup OOM") separated stats formatting from stats display for > > v2, to reuse the stats formatting in the OOM logs. Do the same for v1. > > > > Move the v2 specific formatting from memory_stat_format() to > > memcg_stat_format(), add memcg1_stat_format() for v1, and make > > memory_stat_format() select between them based on cgroup version. > > Since memory_stat_show() now works for both v1 & v2, drop > > memcg_stat_show(). > > > > Signed-off-by: Yosry Ahmed > > Acked-by: Michal Hocko > Thanks Thanks Michal! > > > --- > > mm/memcontrol.c | 60 ++++++++++++++++++++++++++++--------------------- > > 1 file changed, 35 insertions(+), 25 deletions(-) > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index 5922940f92c9..2b492f8d540c 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -1551,7 +1551,7 @@ static inline unsigned long memcg_page_state_outp= ut(struct mem_cgroup *memcg, > > return memcg_page_state(memcg, item) * memcg_page_state_unit(item= ); > > } > > > > -static void memory_stat_format(struct mem_cgroup *memcg, struct seq_bu= f *s) > > +static void memcg_stat_format(struct mem_cgroup *memcg, struct seq_buf= *s) > > { > > int i; > > > > @@ -1604,6 +1604,17 @@ static void memory_stat_format(struct mem_cgroup= *memcg, struct seq_buf *s) > > WARN_ON_ONCE(seq_buf_has_overflowed(s)); > > } > > > > +static void memcg1_stat_format(struct mem_cgroup *memcg, struct seq_bu= f *s); > > + > > +static void memory_stat_format(struct mem_cgroup *memcg, struct seq_bu= f *s) > > +{ > > + if (cgroup_subsys_on_dfl(memory_cgrp_subsys)) > > + memcg_stat_format(memcg, s); > > + else > > + memcg1_stat_format(memcg, s); > > + WARN_ON_ONCE(seq_buf_has_overflowed(s)); > > +} > > + > > #define K(x) ((x) << (PAGE_SHIFT-10)) > > /** > > * mem_cgroup_print_oom_context: Print OOM information relevant to > > @@ -4078,9 +4089,8 @@ static const unsigned int memcg1_events[] =3D { > > PGMAJFAULT, > > }; > > > > -static int memcg_stat_show(struct seq_file *m, void *v) > > +static void memcg1_stat_format(struct mem_cgroup *memcg, struct seq_bu= f *s) > > { > > - struct mem_cgroup *memcg =3D mem_cgroup_from_seq(m); > > unsigned long memory, memsw; > > struct mem_cgroup *mi; > > unsigned int i; > > @@ -4095,18 +4105,18 @@ static int memcg_stat_show(struct seq_file *m, = void *v) > > if (memcg1_stats[i] =3D=3D MEMCG_SWAP && !do_memsw_accoun= t()) > > continue; > > nr =3D memcg_page_state_local(memcg, memcg1_stats[i]); > > - seq_printf(m, "%s %lu\n", memcg1_stat_names[i], > > + seq_buf_printf(s, "%s %lu\n", memcg1_stat_names[i], > > nr * memcg_page_state_unit(memcg1_stats[i])); > > } > > > > for (i =3D 0; i < ARRAY_SIZE(memcg1_events); i++) > > - seq_printf(m, "%s %lu\n", vm_event_name(memcg1_events[i])= , > > - memcg_events_local(memcg, memcg1_events[i])); > > + seq_buf_printf(s, "%s %lu\n", vm_event_name(memcg1_events= [i]), > > + memcg_events_local(memcg, memcg1_events[i]= )); > > > > for (i =3D 0; i < NR_LRU_LISTS; i++) > > - seq_printf(m, "%s %lu\n", lru_list_name(i), > > - memcg_page_state_local(memcg, NR_LRU_BASE + i)= * > > - PAGE_SIZE); > > + seq_buf_printf(s, "%s %lu\n", lru_list_name(i), > > + memcg_page_state_local(memcg, NR_LRU_BASE = + i) * > > + PAGE_SIZE); > > > > /* Hierarchical information */ > > memory =3D memsw =3D PAGE_COUNTER_MAX; > > @@ -4114,11 +4124,11 @@ static int memcg_stat_show(struct seq_file *m, = void *v) > > memory =3D min(memory, READ_ONCE(mi->memory.max)); > > memsw =3D min(memsw, READ_ONCE(mi->memsw.max)); > > } > > - seq_printf(m, "hierarchical_memory_limit %llu\n", > > - (u64)memory * PAGE_SIZE); > > + seq_buf_printf(s, "hierarchical_memory_limit %llu\n", > > + (u64)memory * PAGE_SIZE); > > if (do_memsw_account()) > > - seq_printf(m, "hierarchical_memsw_limit %llu\n", > > - (u64)memsw * PAGE_SIZE); > > + seq_buf_printf(s, "hierarchical_memsw_limit %llu\n", > > + (u64)memsw * PAGE_SIZE); > > > > for (i =3D 0; i < ARRAY_SIZE(memcg1_stats); i++) { > > unsigned long nr; > > @@ -4126,19 +4136,19 @@ static int memcg_stat_show(struct seq_file *m, = void *v) > > if (memcg1_stats[i] =3D=3D MEMCG_SWAP && !do_memsw_accoun= t()) > > continue; > > nr =3D memcg_page_state(memcg, memcg1_stats[i]); > > - seq_printf(m, "total_%s %llu\n", memcg1_stat_names[i], > > + seq_buf_printf(s, "total_%s %llu\n", memcg1_stat_names[i]= , > > (u64)nr * memcg_page_state_unit(memcg1_stats[i= ])); > > } > > > > for (i =3D 0; i < ARRAY_SIZE(memcg1_events); i++) > > - seq_printf(m, "total_%s %llu\n", > > - vm_event_name(memcg1_events[i]), > > - (u64)memcg_events(memcg, memcg1_events[i])); > > + seq_buf_printf(s, "total_%s %llu\n", > > + vm_event_name(memcg1_events[i]), > > + (u64)memcg_events(memcg, memcg1_events[i])= ); > > > > for (i =3D 0; i < NR_LRU_LISTS; i++) > > - seq_printf(m, "total_%s %llu\n", lru_list_name(i), > > - (u64)memcg_page_state(memcg, NR_LRU_BASE + i) = * > > - PAGE_SIZE); > > + seq_buf_printf(s, "total_%s %llu\n", lru_list_name(i), > > + (u64)memcg_page_state(memcg, NR_LRU_BASE += i) * > > + PAGE_SIZE); > > > > #ifdef CONFIG_DEBUG_VM > > { > > @@ -4153,12 +4163,10 @@ static int memcg_stat_show(struct seq_file *m, = void *v) > > anon_cost +=3D mz->lruvec.anon_cost; > > file_cost +=3D mz->lruvec.file_cost; > > } > > - seq_printf(m, "anon_cost %lu\n", anon_cost); > > - seq_printf(m, "file_cost %lu\n", file_cost); > > + seq_buf_printf(s, "anon_cost %lu\n", anon_cost); > > + seq_buf_printf(s, "file_cost %lu\n", file_cost); > > } > > #endif > > - > > - return 0; > > } > > > > static u64 mem_cgroup_swappiness_read(struct cgroup_subsys_state *css, > > @@ -4998,6 +5006,8 @@ static int mem_cgroup_slab_show(struct seq_file *= m, void *p) > > } > > #endif > > > > +static int memory_stat_show(struct seq_file *m, void *v); > > + > > static struct cftype mem_cgroup_legacy_files[] =3D { > > { > > .name =3D "usage_in_bytes", > > @@ -5030,7 +5040,7 @@ static struct cftype mem_cgroup_legacy_files[] = =3D { > > }, > > { > > .name =3D "stat", > > - .seq_show =3D memcg_stat_show, > > + .seq_show =3D memory_stat_show, > > }, > > { > > .name =3D "force_empty", > > -- > > 2.40.1.495.gc816e09b53d-goog > > -- > Michal Hocko > SUSE Labs