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 3B192C2BD09 for ; Tue, 9 Jul 2024 07:55:18 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C04E76B0085; Tue, 9 Jul 2024 03:55:17 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id BB5206B0096; Tue, 9 Jul 2024 03:55:17 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A7CB66B009A; Tue, 9 Jul 2024 03:55:17 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 866726B0085 for ; Tue, 9 Jul 2024 03:55:17 -0400 (EDT) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 38B5E1A174D for ; Tue, 9 Jul 2024 07:55:17 +0000 (UTC) X-FDA: 82319453874.21.2314285 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf06.hostedemail.com (Postfix) with ESMTP id DA07A18000E for ; Tue, 9 Jul 2024 07:55:14 +0000 (UTC) Authentication-Results: imf06.hostedemail.com; dkim=none; spf=pass (imf06.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1720511700; 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; bh=EiC3DjVGI4y3MVF4Fu/oIXyw3UbyfdPYwkWWjcDQGrc=; b=gX0CM1n/MX8YksUVewW6grp+WXDT38KFvOWDRW3B07kUala0FYV5n2CWeN5WvOCzVxb3K9 cei3lhSCnOjexTYj3j/IKm9eOtqslI9Rzlu6+GoYdkJMIQTleuEsr5E0+t/Ic4tosMMBe/ neVg8J2bzJmFj7fMbjof2qjuEUJ88FA= ARC-Authentication-Results: i=1; imf06.hostedemail.com; dkim=none; spf=pass (imf06.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1720511700; a=rsa-sha256; cv=none; b=mk5T+VrQGMAT5uu02rWuKoftDs9sJQuj4v/3KJm16S1S0kztt+PnFD27ARRO+1kuTZu/9I Mhe5NCn4pN/jQCRiRf6zs9vmkBKBZtlm+Lpum+oGBAU5mGssb3Lanh6IXNWmaV2DIUJ1dD 14eLEVe5YmGwGVVSBYsyefVSbQw1dvc= Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id B30971042; Tue, 9 Jul 2024 00:55:38 -0700 (PDT) Received: from [10.57.76.194] (unknown [10.57.76.194]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 3EC7C3F762; Tue, 9 Jul 2024 00:55:11 -0700 (PDT) Message-ID: <70ac8a5d-f6cf-41e6-b295-cd40b81fa85e@arm.com> Date: Tue, 9 Jul 2024 08:55:09 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v1] mm: shmem: Rename mTHP shmem counters Content-Language: en-GB To: Barry Song <21cnbao@gmail.com> Cc: akpm@linux-foundation.org, baohua@kernel.org, baolin.wang@linux.alibaba.com, corbet@lwn.net, da.gomez@samsung.com, david@redhat.com, hughd@google.com, ioworker0@gmail.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org, willy@infradead.org, ziy@nvidia.com References: <744749c3-4506-40d9-ac48-0dbc59689f92@arm.com> <20240709014413.18044-1-21cnbao@gmail.com> From: Ryan Roberts In-Reply-To: <20240709014413.18044-1-21cnbao@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspamd-Server: rspam03 X-Rspam-User: X-Rspamd-Queue-Id: DA07A18000E X-Stat-Signature: c6kfi681dzomokp68i4toub87fuihbji X-HE-Tag: 1720511714-224044 X-HE-Meta: U2FsdGVkX19ZxVzc32XyLOUIAW8PgESIt53ZhBhbgROEJL+J9JDrF/HKAXsSeMPNQFVlOxHdUHucoE24ESRoJf4E86DfDTsGZquWbIwBnLbImD9F4eHSZ4sHP4kRbd+ISyF9GEr6LUmcc7jiV7vgH+EcrW1zREcT6MgdZEGLKe4svzFkR2AOwPfbcsOjfhnTACXaptclCdAx1JcQqc5yjgP64vbiWBM2LUOflNRqT3FketXnK/XtH9XVax45Y2VxYyDtUGKKAaFkO5KzH4O1vb6iIGZzqcDQp9vnZiU0qU9ACgrI7g8gf8i2CVQcFxOWWC66sKvcs3Y0FmOqShupLRE1p22ZytR+awx1BqJoOKb1AbfVde0UtX3Ac/+pkIWdbiiN6iQYtqtQMIUzWhKKJLnv41qbW/chrgkzthMe7zqoIH8XSONKTIFL7HhYOXpOhcExbkYQs1lCK1JDEqYFMxOTERUKXCPXH2BexBm7b8X6doI7po8Dk1Dp2iIJoldzcVk16FeT3ik/p8pbALLqEroQM0awpGpbnUjR0Br+f7QhmFgrfCcSUmG/5dAyPddO/0buLGZ7r+zezk1ZIHDAdN8K1i8klxzcJ7mF+gDQm+viRU7VHC8CZ7vQXMKnq4mIDGHE3frROzG9oer/2NrY3qL0KoAk+hHrXTXvG/Ti7xchDjDQKl3R8Ht0WwvIIS/w6J+L8rWlTSWgZEiS2F5rVMF23fHVIzpLbV2x9bvrYhEoj5AIFZ+CUoGgy5UR47uU/ljeudQf/d+uV37M6Z7hDweGIfvenLLET0Dd2p6LhzaXDxQaTIU4fIuxs3L4mDymOmt4CRqJIRk2jTNxkcjjcosMU3xkmo/usFninrLPH7Nb3pt5PGXpM5uREJD50GWEZZlmZ9fGNc+6ldchXVOuBrxpeR7LBq2iFkNDThdxAOLjtxKNE6D6BYxqorDosUEkkOofPluaMsAquFgXJj9 A1BtDDuH k0LUOFjj695KfOFf39Xr0rirx/k+qR31twZjO1YkOTBEJEXBulldsKsxuJqQhPtAdAJ9mAq9sapiRhs4sbisHKK73Mu8dCu4SjkMmqKa4IQXIGxatilbyGIvyO/mbmxn6fOi3LCe0IMoNLXxbBfFw1lbQ5pcpJ/z4fuskUIlSWigh+QFQtKu0oljQdviqIXWOILqcrfrKHMjcwAr23pm/rPiL9ThxwhIqpptOpvwXiL15RUfGq+H9uoo1Hh/TF0wTed2g+ksU99WHHcuu5RT5/iWm8uVfWXOpFCtQV9uxq5ADQg3FO3iQ5N5rmA== 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 09/07/2024 02:44, Barry Song wrote: > On Tue, Jul 9, 2024 at 12:30 AM Ryan Roberts wrote: >> >> On 08/07/2024 12:36, Barry Song wrote: >>> On Mon, Jul 8, 2024 at 11:24 PM Ryan Roberts wrote: >>>> >>>> The legacy PMD-sized THP counters at /proc/vmstat include >>>> thp_file_alloc, thp_file_fallback and thp_file_fallback_charge, which >>>> rather confusingly refer to shmem THP and do not include any other types >>>> of file pages. This is inconsistent since in most other places in the >>>> kernel, THP counters are explicitly separated for anon, shmem and file >>>> flavours. However, we are stuck with it since it constitutes a user ABI. >>>> >>>> Recently, commit 66f44583f9b6 ("mm: shmem: add mTHP counters for >>>> anonymous shmem") added equivalent mTHP stats for shmem, keeping the >>>> same "file_" prefix in the names. But in future, we may want to add >>>> extra stats to cover actual file pages, at which point, it would all >>>> become very confusing. >>>> >>>> So let's take the opportunity to rename these new counters "shmem_" >>>> before the change makes it upstream and the ABI becomes immutable. >>> >>> Personally, I think this approach is much clearer. However, I recall >>> we discussed this >>> before [1], and it seems that inconsistency is a concern? >> >> Embarrassingly, I don't recall that converstation at all :-| but at least what I >> said then is consistent with what I've done in this patch. >> >> I think David's conclusion from that thread was to call them FILE_, and add both >> shmem and pagecache counts to those counters, meaning we can keep the same name >> as legacy THP counters. But those legacy THP counters only count shmem, and I >> don't think we would get away with adding pagecache counts to those at this >> point? (argument: they have been around for long time and there is a risk that >> user space relies on them and if they were to dramatically increase due to >> pagecache addition now that could break things). In that case, there is still >> inconsistency, but its worse; the names are consistent but the semantics are >> inconsistent. >> >> So my vote is to change to SHMEM_ as per this patch :) > > I have no objections. However, I dislike the documentation for > thp_file_*. Perhaps we can clean it all up together ? I agree that we should clean this documentation up and I'm happy to roll it into v2. However, I don't think what you have suggested is quite right. thp_file_alloc, thp_file_fallback and thp_file_fallback_charge *only* count shmem. They don't count pagecache. So perhaps the change should be "...every time a shmem huge page (dispite being named after "file", the counter measures only shmem) is..."? thp_file_mapped includes both file and shmem, so agree with your change there. What do you think? > > diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst > index 709fe10b60f4..65df48cb3bbb 100644 > --- a/Documentation/admin-guide/mm/transhuge.rst > +++ b/Documentation/admin-guide/mm/transhuge.rst > @@ -417,21 +417,22 @@ thp_collapse_alloc_failed > the allocation. > > thp_file_alloc > - is incremented every time a file huge page is successfully > - allocated. > + is incremented every time a file (including shmem) huge page is > + successfully allocated. > > thp_file_fallback > - is incremented if a file huge page is attempted to be allocated > - but fails and instead falls back to using small pages. > + is incremented if a file (including shmem) huge page is attempted > + to be allocated but fails and instead falls back to using small > + pages. > > thp_file_fallback_charge > - is incremented if a file huge page cannot be charged and instead > - falls back to using small pages even though the allocation was > - successful. > + is incremented if a file (including shmem) huge page cannot be > + charged and instead falls back to using small pages even though > + the allocation was successful. > > thp_file_mapped > - is incremented every time a file huge page is mapped into > - user address space. > + is incremented every time a file (including shmem) huge page is > + mapped into user address space. > > thp_split_page > is incremented every time a huge page is split into base > >> >>> >>> [1] https://lore.kernel.org/linux-mm/05d0096e4ec3e572d1d52d33a31a661321ac1551.1713755580.git.baolin.wang@linux.alibaba.com/ >>> >>> >>>> >>>> Signed-off-by: Ryan Roberts >>>> --- >>>> >>>> Hi All, >>>> >>>> Applies on top of today's mm-unstable (2073cda629a4) and tested with mm >>>> selftests; no regressions observed. >>>> >>>> The backstory here is that I'd like to introduce some counters for regular file >>>> folio allocations to observe how often large folio allocation succeeds, but >>>> these shmem counters are named "file" which is going to make things confusing. >>>> So hoping to solve that before commit 66f44583f9b6 ("mm: shmem: add mTHP >>>> counters for anonymous shmem") goes upstream (it is currently in mm-stable). >>>> >>>> Admittedly, this change means the mTHP stat names are not the same as the legacy >>>> PMD-size THP names, but I think that's a smaller issue than having "file_" mTHP >>>> stats that only count shmem, then having to introduce "file2_" or "pgcache_" >>>> stats for the regular file memory, which is even more inconsistent IMHO. I guess >>>> the alternative is to count both shmem and file in these mTHP stats (that's how >>>> they were documented anyway) but I think it's better to be able to consider them >>>> separately like we do for all the other counters. >>>> >>>> Thanks, >>>> Ryan >>>> >>>>  Documentation/admin-guide/mm/transhuge.rst | 12 ++++++------ >>>>  include/linux/huge_mm.h                    |  6 +++--- >>>>  mm/huge_memory.c                           | 12 ++++++------ >>>>  mm/shmem.c                                 |  8 ++++---- >>>>  4 files changed, 19 insertions(+), 19 deletions(-) >>>> >>>> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst >>>> index 747c811ee8f1..8b891689fc13 100644 >>>> --- a/Documentation/admin-guide/mm/transhuge.rst >>>> +++ b/Documentation/admin-guide/mm/transhuge.rst >>>> @@ -496,16 +496,16 @@ swpout_fallback >>>>         Usually because failed to allocate some continuous swap space >>>>         for the huge page. >>>> >>>> -file_alloc >>>> -       is incremented every time a file huge page is successfully >>>> +shmem_alloc >>>> +       is incremented every time a shmem huge page is successfully >>>>         allocated. >>>> >>>> -file_fallback >>>> -       is incremented if a file huge page is attempted to be allocated >>>> +shmem_fallback >>>> +       is incremented if a shmem huge page is attempted to be allocated >>>>         but fails and instead falls back to using small pages. >>>> >>>> -file_fallback_charge >>>> -       is incremented if a file huge page cannot be charged and instead >>>> +shmem_fallback_charge >>>> +       is incremented if a shmem huge page cannot be charged and instead >>>>         falls back to using small pages even though the allocation was >>>>         successful. >>>> >>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h >>>> index acb6ac24a07e..cff002be83eb 100644 >>>> --- a/include/linux/huge_mm.h >>>> +++ b/include/linux/huge_mm.h >>>> @@ -269,9 +269,9 @@ enum mthp_stat_item { >>>>         MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE, >>>>         MTHP_STAT_SWPOUT, >>>>         MTHP_STAT_SWPOUT_FALLBACK, >>>> -       MTHP_STAT_FILE_ALLOC, >>>> -       MTHP_STAT_FILE_FALLBACK, >>>> -       MTHP_STAT_FILE_FALLBACK_CHARGE, >>>> +       MTHP_STAT_SHMEM_ALLOC, >>>> +       MTHP_STAT_SHMEM_FALLBACK, >>>> +       MTHP_STAT_SHMEM_FALLBACK_CHARGE, >>>>         MTHP_STAT_SPLIT, >>>>         MTHP_STAT_SPLIT_FAILED, >>>>         MTHP_STAT_SPLIT_DEFERRED, >>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >>>> index 9ec64aa2be94..f9696c94e211 100644 >>>> --- a/mm/huge_memory.c >>>> +++ b/mm/huge_memory.c >>>> @@ -568,9 +568,9 @@ DEFINE_MTHP_STAT_ATTR(anon_fault_fallback, MTHP_STAT_ANON_FAULT_FALLBACK); >>>>  DEFINE_MTHP_STAT_ATTR(anon_fault_fallback_charge, MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE); >>>>  DEFINE_MTHP_STAT_ATTR(swpout, MTHP_STAT_SWPOUT); >>>>  DEFINE_MTHP_STAT_ATTR(swpout_fallback, MTHP_STAT_SWPOUT_FALLBACK); >>>> -DEFINE_MTHP_STAT_ATTR(file_alloc, MTHP_STAT_FILE_ALLOC); >>>> -DEFINE_MTHP_STAT_ATTR(file_fallback, MTHP_STAT_FILE_FALLBACK); >>>> -DEFINE_MTHP_STAT_ATTR(file_fallback_charge, MTHP_STAT_FILE_FALLBACK_CHARGE); >>>> +DEFINE_MTHP_STAT_ATTR(shmem_alloc, MTHP_STAT_SHMEM_ALLOC); >>>> +DEFINE_MTHP_STAT_ATTR(shmem_fallback, MTHP_STAT_SHMEM_FALLBACK); >>>> +DEFINE_MTHP_STAT_ATTR(shmem_fallback_charge, MTHP_STAT_SHMEM_FALLBACK_CHARGE); >>>>  DEFINE_MTHP_STAT_ATTR(split, MTHP_STAT_SPLIT); >>>>  DEFINE_MTHP_STAT_ATTR(split_failed, MTHP_STAT_SPLIT_FAILED); >>>>  DEFINE_MTHP_STAT_ATTR(split_deferred, MTHP_STAT_SPLIT_DEFERRED); >>>> @@ -581,9 +581,9 @@ static struct attribute *stats_attrs[] = { >>>>         &anon_fault_fallback_charge_attr.attr, >>>>         &swpout_attr.attr, >>>>         &swpout_fallback_attr.attr, >>>> -       &file_alloc_attr.attr, >>>> -       &file_fallback_attr.attr, >>>> -       &file_fallback_charge_attr.attr, >>>> +       &shmem_alloc_attr.attr, >>>> +       &shmem_fallback_attr.attr, >>>> +       &shmem_fallback_charge_attr.attr, >>>>         &split_attr.attr, >>>>         &split_failed_attr.attr, >>>>         &split_deferred_attr.attr, >>>> diff --git a/mm/shmem.c b/mm/shmem.c >>>> index 921d59c3d669..f24dfbd387ba 100644 >>>> --- a/mm/shmem.c >>>> +++ b/mm/shmem.c >>>> @@ -1777,7 +1777,7 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf, >>>>                         if (pages == HPAGE_PMD_NR) >>>>                                 count_vm_event(THP_FILE_FALLBACK); >>>>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE >>>> -                       count_mthp_stat(order, MTHP_STAT_FILE_FALLBACK); >>>> +                       count_mthp_stat(order, MTHP_STAT_SHMEM_FALLBACK); >>>>  #endif >>>>                         order = next_order(&suitable_orders, order); >>>>                 } >>>> @@ -1804,8 +1804,8 @@ static struct folio *shmem_alloc_and_add_folio(struct vm_fault *vmf, >>>>                                 count_vm_event(THP_FILE_FALLBACK_CHARGE); >>>>                         } >>>>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE >>>> -                       count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_FALLBACK); >>>> -                       count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_FALLBACK_CHARGE); >>>> +                       count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK); >>>> +                       count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_FALLBACK_CHARGE); >>>>  #endif >>>>                 } >>>>                 goto unlock; >>>> @@ -2181,7 +2181,7 @@ static int shmem_get_folio_gfp(struct inode *inode, pgoff_t index, >>>>                         if (folio_test_pmd_mappable(folio)) >>>>                                 count_vm_event(THP_FILE_ALLOC); >>>>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE >>>> -                       count_mthp_stat(folio_order(folio), MTHP_STAT_FILE_ALLOC); >>>> +                       count_mthp_stat(folio_order(folio), MTHP_STAT_SHMEM_ALLOC); >>>>  #endif >>>>                         goto alloced; >>>>                 } >>>> -- >>>> 2.43.0 >>>> >>> > > Thanks > Barry