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 X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 64B5EC433E0 for ; Wed, 20 May 2020 03:32:47 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 2E22A207E8 for ; Wed, 20 May 2020 03:32:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2E22A207E8 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id AA38D80048; Tue, 19 May 2020 23:32:46 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A53F48002C; Tue, 19 May 2020 23:32:46 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9694D80048; Tue, 19 May 2020 23:32:46 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0253.hostedemail.com [216.40.44.253]) by kanga.kvack.org (Postfix) with ESMTP id 760B98002C for ; Tue, 19 May 2020 23:32:46 -0400 (EDT) Received: from smtpin09.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 2DD10181AEF31 for ; Wed, 20 May 2020 03:32:46 +0000 (UTC) X-FDA: 76835675532.09.hair34_5251e50f29126 X-HE-Tag: hair34_5251e50f29126 X-Filterd-Recvd-Size: 5314 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf17.hostedemail.com (Postfix) with ESMTP for ; Wed, 20 May 2020 03:32:45 +0000 (UTC) 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 BAA5630E; Tue, 19 May 2020 20:32:44 -0700 (PDT) Received: from [10.163.75.101] (unknown [10.163.75.101]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C8F2E3F52E; Tue, 19 May 2020 20:32:42 -0700 (PDT) Subject: Re: [RFC V2] mm/vmstat: Add events for PMD based THP migration without split To: John Hubbard , linux-mm@kvack.org Cc: Naoya Horiguchi , Zi Yan , Andrew Morton , linux-kernel@vger.kernel.org References: <1589784156-28831-1-git-send-email-anshuman.khandual@arm.com> <67be2597-c019-63c1-b551-d4571a44f1a5@nvidia.com> From: Anshuman Khandual Message-ID: <88aa238d-9de1-9f6a-a3b0-51fbe073090d@arm.com> Date: Wed, 20 May 2020 09:02:07 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <67be2597-c019-63c1-b551-d4571a44f1a5@nvidia.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US 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 05/19/2020 01:40 AM, John Hubbard wrote: > On 2020-05-17 23:42, Anshuman Khandual wrote: > ... >> diff --git a/include/linux/vm_event_item.h b/include/linux/vm_event_it= em.h >> index ffef0f279747..23d8f9884c2b 100644 >> --- a/include/linux/vm_event_item.h >> +++ b/include/linux/vm_event_item.h >> @@ -91,6 +91,10 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPO= UT, >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 THP_ZERO_PAGE_A= LLOC_FAILED, >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 THP_SWPOUT, >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 THP_SWPOUT_FALL= BACK, >> +#ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION >=20 >=20 > Hi Anshuman, >=20 > These new events look great to me. I have some nits below, plus one > lightly controversial suggestion which I'd really like to have someone > more experienced weigh in on, which is: >=20 > How about not being quite so granular on the THP config options, and > just guarding these events with the overall CONFIG_TRANSPARENT_HUGEPAGE > option, instead of the sub-option CONFIG_ARCH_ENABLE_THP_MIGRATION? >=20 > I tentatively think it's harmless and not really misleading to have > /proc/vmstat showing this in all THP-enabled configurations: >=20 > thp_pmd_migration_success 0 > thp_pmd_migration_failure 0 >=20 > ...if THP is enabled, and *whether or not* _THP_MIGRATION is enabled. > And this simplifies things a bit. Given how the .config options can get= , > I think simplifying would be nice. >=20 > However, I'm ready to be corrected on that, if it's a bad idea for > other API reasons perhaps.=C2=A0 Can anyone please comment? There is no THP migration events to track unless it is enabled. Why to show these statistics (as 0) when its not even possible. If the config simplicity is the only intended rationale here, it might not be the case either. These events and their tracking would still need to be wrapped with CONFIG_TRANSPARENT_HUGEPAGE otherwise. If your concern is more towards CONFIG_ARCH_ENABLE_THP_MIGRATION being unsuitable or with complex dependencies, then that is something how THP migration feature itself is implemented currently and adding VM events does not address that. A possible patch in the future patch could solve all these (together). But sure, let's hear it for what others have to say on this. >=20 >=20 >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 THP_PMD_MIGRATION_SUCCESS, >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 THP_PMD_MIGRATION_FAILURE, >> +#endif >> =C2=A0 #endif >> =C2=A0 #ifdef CONFIG_MEMORY_BALLOON >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 BALLOON_INFLATE= , >> diff --git a/mm/migrate.c b/mm/migrate.c >> index 7160c1556f79..5325700a3e90 100644 >> --- a/mm/migrate.c >> +++ b/mm/migrate.c >> @@ -1170,6 +1170,18 @@ static int __unmap_and_move(struct page *page, = struct page *newpage, >> =C2=A0 #define ICE_noinline >> =C2=A0 #endif >> =C2=A0 +#ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION >> +static inline void thp_migration_success(bool success) >=20 >=20 > I think this should be named >=20 > =C2=A0=C2=A0=C2=A0 thp_pmd_migration_success() >=20 > , since that's what you're really counting. Or, you could > name the events THP_MIGRATION_SUCCESS|FAILURE. Either way, > just so the function name matches the events it's counting. Makes sense but IMHO we should keep _pmd_ to be more specific. Will change the name here as thp_pmd_migration_success(). >=20 >=20 >> +{ >> +=C2=A0=C2=A0=C2=A0 if (success) >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 count_vm_event(THP_PMD_MIG= RATION_SUCCESS); >> +=C2=A0=C2=A0=C2=A0 else >> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 count_vm_event(THP_PMD_MIG= RATION_FAILURE); >> +} >> +#else >> +static inline void thp_migration_success(bool success) { } >=20 >=20 > This whole ifdef clause would disappear if my suggestion above is We will have to protect these with CONFIG_TRANSPARENT_HUGEPAGE as the events are still conditionally available. > accepted. However, if not, then I believe the convention for this > kind of situation is: >=20 > static inline void thp_migration_success(bool success) > { > } AFAIK, we have examples both ways but will change if this is preferred.