Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From:   David Hildenbrand <david@redhat.com>
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH v2] mm/page_alloc: count CMA pages per zone and print them in /proc/zoneinfo
Date:   Thu, 28 Jan 2021 23:30:33 +0100
Message-Id: <2CF5220A-5452-4913-AFCB-41E1C642E521@redhat.com>
References: <89e1dbcd-605d-6a7b-361-c130f7eb9d8c@google.com>
Cc:     David Hildenbrand <david@redhat.com>, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        "Peter Zijlstra (Intel)" <peterz@infradead.org>,
        Mike Rapoport <rppt@kernel.org>,
        Oscar Salvador <osalvador@suse.de>,
        Michal Hocko <mhocko@kernel.org>,
        Wei Yang <richard.weiyang@linux.alibaba.com>,
        linux-api@vger.kernel.org
In-Reply-To: <89e1dbcd-605d-6a7b-361-c130f7eb9d8c@google.com>
To:     David Rientjes <rientjes@google.com>
Precedence: bulk


> Am 28.01.2021 um 23:28 schrieb David Rientjes <rientjes@google.com>:
>=20
> =EF=BB=BFOn Thu, 28 Jan 2021, David Hildenbrand wrote:
>=20
>>> =EF=BB=BFOn Thu, 28 Jan 2021, David Hildenbrand wrote:
>>>=20
>>>> diff --git a/mm/vmstat.c b/mm/vmstat.c
>>>> index 7758486097f9..957680db41fa 100644
>>>> --- a/mm/vmstat.c
>>>> +++ b/mm/vmstat.c
>>>> @@ -1650,6 +1650,11 @@ static void zoneinfo_show_print(struct seq_file *=
m, pg_data_t *pgdat,
>>>>          zone->spanned_pages,
>>>>          zone->present_pages,
>>>>          zone_managed_pages(zone));
>>>> +#ifdef CONFIG_CMA
>>>> +    seq_printf(m,
>>>> +           "\n        cma      %lu",
>>>> +           zone->cma_pages);
>>>> +#endif
>>>>=20
>>>>   seq_printf(m,
>>>>          "\n        protection: (%ld",
>>>=20
>>> Hmm, not sure about this.  If cma is only printed for CONFIG_CMA, we can=
't=20
>>> distinguish between (1) a kernel without your patch without including so=
me=20
>>> version checking and (2) a kernel without CONFIG_CMA enabled.  IOW,=20
>>> "cma 0" carries value: we know immediately that we do not have any CMA=20=

>>> pages on this zone, period.
>>>=20
>>> /proc/zoneinfo is also not known for its conciseness so I think printing=
=20
>>> "cma 0" even for !CONFIG_CMA is helpful :)
>>>=20
>>> I think this #ifdef should be removed and it should call into a=20
>>> zone_cma_pages(struct zone *zone) which returns 0UL if disabled.
>>>=20
>>=20
>> Yeah, that=E2=80=99s also what I proposed in a sub-thread here.
>>=20
>=20
> Ah, I certainly think your original intuition was correct.
>=20
>> The last option would be going the full mile and not printing nr_free_cma=
. Code might get a bit uglier though, but we could also remove that stats co=
unter ;)
>>=20
>> I don=E2=80=98t particularly care, while printing =E2=80=9E0=E2=80=9C mig=
ht be easier, removing nr_free_cma might be cleaner.
>>=20
>> But then, maybe there are tools that expect that value to be around on an=
y kernel?
>>=20
>=20
> Yeah, that's probably undue risk, the ship has sailed and there's no=20
> significant upside.
>=20
> I still think "cma 0" in /proc/zoneinfo carries value, though, especially=20=

> for NUMA and it looks like this is how it's done in linux-next.  With a=20=

> single read of the file, userspace can make the determination what CMA=20
> pages exist on this node.
>=20
> In general, I think the rule-of-thumb is that the fewer ifdefs in=20
> /proc/zoneinfo, the easier it is for userspace to parse it.

Makes sense, I=E2=80=98ll send an updated version tomorrow - thanks!


>=20
> (I made that change to /proc/zoneinfo to even print non-existant zones for=
=20
> each node because otherwise you cannot determine what the indices of=20
> things like vm.lowmem_reserve_ratio represent.)