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 5FC2ED5B854 for ; Tue, 29 Oct 2024 00:31:30 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D18AD6B00BA; Mon, 28 Oct 2024 20:31:29 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id CC82B6B00BC; Mon, 28 Oct 2024 20:31:29 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BB7566B00C0; Mon, 28 Oct 2024 20:31:29 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 9E7746B00BA for ; Mon, 28 Oct 2024 20:31:29 -0400 (EDT) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 0D46B1C6E21 for ; Tue, 29 Oct 2024 00:31:29 +0000 (UTC) X-FDA: 82724760342.23.1048D49 Received: from fanzine2.igalia.com (fanzine.igalia.com [178.60.130.6]) by imf27.hostedemail.com (Postfix) with ESMTP id C01664000A for ; Tue, 29 Oct 2024 00:31:02 +0000 (UTC) Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=igalia.com header.s=20170329 header.b=Wp2Qm4Rz; spf=pass (imf27.hostedemail.com: domain of mcanal@igalia.com designates 178.60.130.6 as permitted sender) smtp.mailfrom=mcanal@igalia.com; dmarc=pass (policy=none) header.from=igalia.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1730161676; 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:dkim-signature; bh=AARMfSu/7CX+ky7XhzUJm5Khwsm5g7vV8ZMifTOXCPw=; b=Kn7gjB4TKOhc6anPRnbATf91A6NE4QqZDnBzunEGBUUgHbwjez3AHeS7D9rLPfdOPfX1iK 8EViRBfM2a4A4vTB/MRSRZ5kg1cqOCeMWixGmsR4cxlG1VckSR/4YrCLOFyvNSez4e/BV+ a7nfWwJBQS8PLCTZqH83rdjOxK/CGZI= ARC-Authentication-Results: i=1; imf27.hostedemail.com; dkim=pass header.d=igalia.com header.s=20170329 header.b=Wp2Qm4Rz; spf=pass (imf27.hostedemail.com: domain of mcanal@igalia.com designates 178.60.130.6 as permitted sender) smtp.mailfrom=mcanal@igalia.com; dmarc=pass (policy=none) header.from=igalia.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1730161676; a=rsa-sha256; cv=none; b=sKsk6brKALT36QSndWkz4BMETBSq0lo0g0LhpSc22GtxxiuftL+u3J+cKC9ki1NrszOH9v Cia6U54PswwjRP0UcYndFUGOwqv825jSx7lRFnjem6k+qras0kMjFI1HNTZweA2W5xs3mg rBxu0PGORlYiqW3Zfx0jhvuurWmlwEo= DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=igalia.com; s=20170329; h=Content-Transfer-Encoding:Content-Type:In-Reply-To:From: References:Cc:To:Subject:MIME-Version:Date:Message-ID:Sender:Reply-To: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=AARMfSu/7CX+ky7XhzUJm5Khwsm5g7vV8ZMifTOXCPw=; b=Wp2Qm4RzkRSaKidVyWnMNoMX5q vyRaBdc/tVdhpRFsVJUqWELXf3vXny3uWc+l60FRx1YJRFVeyuRbDrdz35FV7LL6HH1r7MlhToFR5 VmGNk4zIlZ1dPQY73mEzFwsR1KM04TiA8mo6ezUJNOcayyG8/TmPg6K4LWzQXvGkMzCcZ5gnsHXFu Sc05fnF8SNn5Gkh75KtifPyyjYpSJ6D4AoT23+QrxoED0rxV//O6C6mdHTzF8CUM/ml5d2bpJ9s7L 5UEp1r8/3cI2VWfKhIdRPcvuZ8L6BAeb0rqeYAI+St7OhJ3xYx6FQH6c4NiLcSSGuYuBJSjNdYxFz n4vN3xXg==; Received: from [187.36.213.55] (helo=[192.168.1.103]) by fanzine2.igalia.com with esmtpsa (Cipher TLS1.3:ECDHE_X25519__RSA_PSS_RSAE_SHA256__AES_128_GCM:128) (Exim) id 1t5a8k-00GIzd-1R; Tue, 29 Oct 2024 01:31:18 +0100 Message-ID: <654236eb-945c-4540-bc52-9c99898deed8@igalia.com> Date: Mon, 28 Oct 2024 21:31:11 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 3/3] mm: shmem: override mTHP shmem default with a kernel parameter To: Barry Song <21cnbao@gmail.com> Cc: Jonathan Corbet , Andrew Morton , Hugh Dickins , David Hildenbrand , Ryan Roberts , Baolin Wang , Lance Yang , linux-mm@kvack.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-dev@igalia.com References: <20241027175743.1056710-1-mcanal@igalia.com> <20241027175743.1056710-4-mcanal@igalia.com> <2505d52c-3454-4892-8c90-e3d9b2f0c84f@igalia.com> Content-Language: en-US From: =?UTF-8?Q?Ma=C3=ADra_Canal?= In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: C01664000A X-Stat-Signature: pb8pwtweb9uu9zqm9j7aae1yxztehcsk X-Rspam-User: X-HE-Tag: 1730161862-662757 X-HE-Meta: U2FsdGVkX18xzKl3AQXCBJiuA2DbDU57NnG1LYMixAbwT2z0rHo/3b8R5nwvQvIi8xu0mUCkekRRDTW5Y2oCupTopzwE9yhXQr+i75XXaAWv6CuK6UrHuvpstJjVRMjipz7ogTzBAcucAaEehxKWgwMOJWUHf/+n3v+0lHZbAkSl3KVjcOw/VsUepylrmpr0jVWGuEMV4V6+hraym6ScuTyyPtF8DgzAd5on68HF2KNpW/ckgSUiLWERFlAkuXc5kXFydDVt/1vxZMhoBNkaIbkUZCmEzxxAgAdU+m0/WXYBJwu/uxg5HSvBFDYXTd43w/xEOZnZmIjg2L0aZXNSyBkSv3Zk6/fkSJ/f4M9UZpxuTsmlHCeWuAli8blW5P/POWyf+F/UQY29MTktw8oVAgIAL8Io8wA2ruZeDB7jEDXhzE/TU1aZikhaEK98YWVyKh17rUi+DM1DNd24jklVO0juk1mDU/YnYg9q1aCN9dsE8s9khmOQX3fxESFeXIvSHkHD7Px7wvJpqdyi0/9Xrjs2CnzID3vBDMFxG8RT6NOecGxp7LAuH+AnM7LLk8qgd/o17g9mQqRN2/XxqZEeGf+UQ1UNzEcdh+CRzMmeQUjLKLY7YUt8AjHz1ecV4M09KeKA5hIT+2SJbrQVyzxsWF2yMNWIXGusKwB/hvQFI5C+Adzx2FnAzlUSS1lnLhoh4p2HX1ccUw8sTtzxAATYuRTW313qj7SjE+q9e74qCPAqhEITXlQHstELAmlBd4atd+NZ3wPg7xDFqAXw9xKXr8vF9esFKYCGGYngaeh/av2Sw17Jp3VDQ4D2BLUR5XcBWbdt/MdDlIRsYLZ5C1JuCh8LW/zs61hAPCMIzB3g9w3m93bsGVf9w7NiaMyRVenRTuR2ADcCoSVlX2hrd7rgOVWxo4MxRDmNygE1LOQ1d1ci15bin2hu17Wz5IMChMzisF7+3UitCQ3/FcPyX9Z 4A3kiSsz WEretRWTq23pxSuOjmU+AK4TudRsPN0B9/vpfBD+HoABPtfWFipPngaka+zy/GIWQA74U7X2UHGelvUGH2lqQbICHHty/wh7H/0w5WZ5jV/JfhPKPHN+73N4let6LsaLDQ42AnmP7d5qfypro/lf6l4kBb9XXgiZwx6oaHYf5IhKpupOO+753/gUUcaveyjPNH1fnGH2THIf2ZJ8SP9SpcpLI95FVxc0vEzmW6qNRbmNSmh54IJ64Ptrs4F1jMeh09QlCq1E67xLXxh/HlMSdAGOxXUq0jPj7j69XoNl8pcmjnAh0zz2dgW/anXOAxMIv4RNoXDyKYAPb1Ysg1L6hI6Yf/+6GH9alnEgFsPR1tXSBMQuv1a8d35JMgYISZq3DjiLexSfF868Gkkw= 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: Hi Barry, On 28/10/24 19:35, Barry Song wrote: > On Mon, Oct 28, 2024 at 7:34 PM Maíra Canal wrote: >> >> Hi Barry, >> >> On 28/10/24 08:09, Barry Song wrote: >>> On Mon, Oct 28, 2024 at 6:10 PM Maíra Canal wrote: >>>> >>>> Hi Barry, >>>> >>>> On 27/10/24 18:54, Barry Song wrote: >>>>> On Mon, Oct 28, 2024 at 6:58 AM Maíra Canal wrote: >>>>>> >>>>>> Add the ``thp_shmem=`` kernel command line to allow specifying the >>>>>> default policy of each supported shmem hugepage size. The kernel parameter >>>>>> accepts the following format: >>>>>> >>>>>> thp_shmem=[KMG],[KMG]:;[KMG]-[KMG]: >>>>>> >>>>>> For example, >>>>>> >>>>>> thp_shmem=16K-64K:always;128K,512K:inherit;256K:advise;1M-2M:never;4M-8M:within_size >>>>>> >>>>>> By configuring the default policy of several shmem huge pages, the user >>>>>> can take advantage of mTHP before it's been configured through sysfs. >>>>>> >>>>>> Signed-off-by: Maíra Canal >>>>>> --- >>>>>> .../admin-guide/kernel-parameters.txt | 10 ++ >>>>>> Documentation/admin-guide/mm/transhuge.rst | 17 +++ >>>>>> mm/shmem.c | 109 +++++++++++++++++- >>>>>> 3 files changed, 135 insertions(+), 1 deletion(-) >>>>>> >>>>> >>>>> Hi Maíra, >>>>> >>>>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt >>>>>> index acabb04d0dd4..595fa096e28b 100644 >>>>>> --- a/Documentation/admin-guide/kernel-parameters.txt >>>>>> +++ b/Documentation/admin-guide/kernel-parameters.txt >>>>>> @@ -6700,6 +6700,16 @@ >>>>>> Force threading of all interrupt handlers except those >>>>>> marked explicitly IRQF_NO_THREAD. >>>>>> >>>>>> + shmem_anon= [KNL] >>>>>> + Format: [KMG],[KMG]:;[KMG]-[KMG]: >>>>>> + Control the default policy of each hugepage size for the >>>>>> + internal shmem mount. is one of policies available >>>>>> + for the shmem mount ("always", "inherit", "never", "within_size", >>>>>> + and "advise"). >>>>>> + It can be used multiple times for multiple shmem THP sizes. >>>>>> + See Documentation/admin-guide/mm/transhuge.rst for more >>>>>> + details. >>>>> >>>>> I'm not sure this is the right name. How about "thp_shmem"? >>>> >>>> Oops, sorry about that. >>>> >>>>> >>>>>> + >>>>>> topology= [S390,EARLY] >>>>>> Format: {off | on} >>>>>> Specify if the kernel should make use of the cpu >>>>>> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst >>>>>> index 9b5b02c4d1ab..47e7fc30e22d 100644 >>>>>> --- a/Documentation/admin-guide/mm/transhuge.rst >>>>>> +++ b/Documentation/admin-guide/mm/transhuge.rst >>>>>> @@ -332,6 +332,23 @@ allocation policy for the internal shmem mount by using the kernel parameter >>>>>> seven valid policies for shmem (``always``, ``within_size``, ``advise``, >>>>>> ``never``, ``deny``, and ``force``). >>>>>> >>>>>> +In the same manner as ``thp_anon`` controls each supported anonymous THP >>>>>> +size, ``thp_shmem`` controls each supported shmem THP size. ``thp_shmem`` >>>>>> +has the same format as ``thp_anon``, but also supports the policy >>>>>> +``within_size``. >>>>>> + >>>>>> +``thp_shmem=`` may be specified multiple times to configure all THP sizes >>>>>> +as required. If ``thp_shmem=`` is specified at least once, any shmem THP >>>>>> +sizes not explicitly configured on the command line are implicitly set to >>>>>> +``never``. >>>>>> + >>>>>> +``transparent_hugepage_shmem`` setting only affects the global toggle. If >>>>>> +``thp_shmem`` is not specified, PMD_ORDER hugepage will default to >>>>>> +``inherit``. However, if a valid ``thp_shmem`` setting is provided by the >>>>>> +user, the PMD_ORDER hugepage policy will be overridden. If the policy for >>>>>> +PMD_ORDER is not defined within a valid ``thp_shmem``, its policy will >>>>>> +default to ``never``. >>>>>> + >>>>>> Hugepages in tmpfs/shmem >>>>>> ======================== >>>>>> >>>>>> diff --git a/mm/shmem.c b/mm/shmem.c >>>>>> index 24cdeafd8260..0a7a7d04f725 100644 >>>>>> --- a/mm/shmem.c >>>>>> +++ b/mm/shmem.c >> >> [...] >> >>>>>> static int __init setup_transparent_hugepage_shmem(char *str) >>>>>> { >>>>>> int huge, ret = 0; >>>>>> @@ -5206,6 +5228,91 @@ static int __init setup_transparent_hugepage_shmem(char *str) >>>>>> } >>>>>> __setup("transparent_hugepage_shmem=", setup_transparent_hugepage_shmem); >>>>>> >>>>>> +static char str_dup[PAGE_SIZE] __initdata; >>>>>> +static int __init setup_thp_shmem(char *str) >>>>>> +{ >>>>>> + char *token, *range, *policy, *subtoken; >>>>>> + unsigned long always, inherit, madvise, within_size; >>>>>> + char *start_size, *end_size; >>>>>> + int start, end, nr; >>>>>> + char *p; >>>>>> + >>>>>> + if (!str || strlen(str) + 1 > PAGE_SIZE) >>>>>> + goto err; >>>>>> + strcpy(str_dup, str); >>>>>> + >>>>>> + always = huge_shmem_orders_always; >>>>>> + inherit = huge_shmem_orders_inherit; >>>>>> + madvise = huge_shmem_orders_madvise; >>>>>> + within_size = huge_shmem_orders_within_size; >>>>>> + p = str_dup; >>>>>> + while ((token = strsep(&p, ";")) != NULL) { >>>>>> + range = strsep(&token, ":"); >>>>>> + policy = token; >>>>>> + >>>>>> + if (!policy) >>>>>> + goto err; >>>>>> + >>>>>> + while ((subtoken = strsep(&range, ",")) != NULL) { >>>>>> + if (strchr(subtoken, '-')) { >>>>>> + start_size = strsep(&subtoken, "-"); >>>>>> + end_size = subtoken; >>>>>> + >>>>>> + start = get_order_from_str(start_size); >>>>>> + end = get_order_from_str(end_size); >>>>>> + } else { >>>>>> + start = end = get_order_from_str(subtoken); >>>>>> + } >>>>>> + >>>>>> + if (start < 0 || end < 0 || start > end) >>>>>> + goto err; >>>>>> + >>>>>> + nr = end - start + 1; >>>>>> + if (!strcmp(policy, "always")) { >>>>>> + bitmap_set(&always, start, nr); >>>>>> + bitmap_clear(&inherit, start, nr); >>>>>> + bitmap_clear(&madvise, start, nr); >>>>>> + bitmap_clear(&within_size, start, nr); >>>>>> + } else if (!strcmp(policy, "advise")) { >>>>>> + bitmap_set(&madvise, start, nr); >>>>>> + bitmap_clear(&inherit, start, nr); >>>>>> + bitmap_clear(&always, start, nr); >>>>>> + bitmap_clear(&within_size, start, nr); >>>>>> + } else if (!strcmp(policy, "inherit")) { >>>>>> + bitmap_set(&inherit, start, nr); >>>>>> + bitmap_clear(&madvise, start, nr); >>>>>> + bitmap_clear(&always, start, nr); >>>>>> + bitmap_clear(&within_size, start, nr); >>>>>> + } else if (!strcmp(policy, "within_size")) { >>>>>> + bitmap_set(&within_size, start, nr); >>>>>> + bitmap_clear(&inherit, start, nr); >>>>>> + bitmap_clear(&madvise, start, nr); >>>>>> + bitmap_clear(&always, start, nr); >>>>>> + } else if (!strcmp(policy, "never")) { >>>>>> + bitmap_clear(&inherit, start, nr); >>>>>> + bitmap_clear(&madvise, start, nr); >>>>>> + bitmap_clear(&always, start, nr); >>>>>> + bitmap_clear(&within_size, start, nr); >>>>>> + } else { >>>>>> + pr_err("invalid policy %s in thp_shmem boot parameter\n", policy); >>>>>> + goto err; >>>>>> + } >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + huge_shmem_orders_always = always; >>>>>> + huge_shmem_orders_madvise = madvise; >>>>>> + huge_shmem_orders_inherit = inherit; >>>>>> + huge_shmem_orders_within_size = within_size; >>>>>> + shmem_orders_configured = true; >>>>>> + return 1; >>>>>> + >>>>>> +err: >>>>>> + pr_warn("thp_shmem=%s: error parsing string, ignoring setting\n", str); >>>>>> + return 0; >>>>>> +} >>>>> >>>>> Can we share source code with thp_anon since there's a lot of duplication? >>>> >>>> I'm not a regular mm contributor and I'm most usually around drivers, so >>>> I don't know exactly here I could add shared code. Should I add the >>>> headers to "internal.h"? >>> >>> My comment isn't related to drivers or memory management. It's solely about >>> avoiding code duplication. For example, we could create a shared function to >>> handle both controls, reducing redundant code :-) >> >> Let me rephrase it. >> >> I completely agree that we should avoid code duplication. I'm asking >> where is the best place to add the headers of the shared functions. >> "linux/shmem_fs.h" doesn't look appropriate to me, so I believe the >> remaining options would be "linux/huge_mm.h" or "internal.h". > > Both locations seem quite odd. I have a feeling that these boot command > elements are purely internal, yet internal.h contains something that is > actually 'external' to mm. The shared code isn't 'external' enough to belong > in internal.h. > > I didn't realize that shmem has placed these controls in its own file; > I thought they > were also located in mm/huge_memory.c. Given the current situation, I would > prefer to keep the code as it is and tolerate the code duplication. > > Unless we are going to place controls for shmem and other thp controls in > one place, I feel your code is better than having a shared function either in > internal.h or linux/huge_mm.h. Sorry, I only catch your e-mail after sending v2. If possible, please, take a look on v2 [1] and let me know if you still prefer to duplicate the code. [1] https://lore.kernel.org/linux-mm/20241029002324.1062723-1-mcanal@igalia.com/T/ Best Regards, - Maíra > >> >> I would like to know your opinion about those two options. >> >> Best Regards, >> - Maíra >> >>> >>>> >>>> Best Regards, >>>> - Maíra >>>> >>>>> >>>>>> +__setup("thp_shmem=", setup_thp_shmem); >>>>>> + >>>>>> #endif /* CONFIG_TRANSPARENT_HUGEPAGE */ >>>>>> >>>>>> #else /* !CONFIG_SHMEM */ >>>>>> -- >>>>>> 2.46.2 >>>>>> >>>>> > > Thanks > barry