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 5AC1DC531DC for ; Fri, 16 Aug 2024 09:47:23 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E701E8D0061; Fri, 16 Aug 2024 05:47:22 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E20FE8D0002; Fri, 16 Aug 2024 05:47:22 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CE83B8D0061; Fri, 16 Aug 2024 05:47:22 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id ADD008D0002 for ; Fri, 16 Aug 2024 05:47:22 -0400 (EDT) Received: from smtpin22.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 56F841A0E5D for ; Fri, 16 Aug 2024 09:47:22 +0000 (UTC) X-FDA: 82457630724.22.4301B76 Received: from mail-vk1-f172.google.com (mail-vk1-f172.google.com [209.85.221.172]) by imf03.hostedemail.com (Postfix) with ESMTP id 8A96820027 for ; Fri, 16 Aug 2024 09:47:20 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="IvGM5hG/"; spf=pass (imf03.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.221.172 as permitted sender) smtp.mailfrom=21cnbao@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1723801627; 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=juhozGP38pQUisELDGJeljBYJ302hnOpTsrGh2pGnFU=; b=PsK5+jua0TZIv7bz4kNaBVF5tbJZzXlgcjIuCHxM7kq5HQgArZtHIf6BiZEBfqDTSFpmM3 z3VEUas1UrTapIJr5P33qGqqFZI1+5N6+ZckWRnqWkb3St0dUQ2wK0UFtpbDy4Yh+GMSlP TlVFuYn7zTnID1TlZC2pt9tj30n12gI= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b="IvGM5hG/"; spf=pass (imf03.hostedemail.com: domain of 21cnbao@gmail.com designates 209.85.221.172 as permitted sender) smtp.mailfrom=21cnbao@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1723801627; a=rsa-sha256; cv=none; b=qdkGHZkRvTIzO1bj/q+WUykjgl2zYmMUMlL0UNAKPfa4UzZt5HhbwX/BTeJx7JOcpWnqid 1Go8Je7pZcktBWH5i5kE9jwZmDR7IkqNYVEnmOYV89rNO2baEQK5Kzu2pWkV1c/j2Uiqgm tKWQDCOOs14yibxhEVrK46rQCp2YIHs= Received: by mail-vk1-f172.google.com with SMTP id 71dfb90a1353d-4f89c9a1610so679235e0c.3 for ; Fri, 16 Aug 2024 02:47:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1723801639; x=1724406439; darn=kvack.org; 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=juhozGP38pQUisELDGJeljBYJ302hnOpTsrGh2pGnFU=; b=IvGM5hG/2okrLvWFnjvpV+d0v1AAtfZfhCaR8gWBSGd3ErPw9UyuJwSrbB4C3Qc7JD s+bffcMa8Kl16cMSmndzEXRb31O5ZLB59yfEVaw2uwadaiO4WeLLYqQmDTjGbRGxftNt rjEd1L9upFWUvRaHXGmMDlRIZBKLrWg0l9oBJ59XGdCbevBO38MFLcosetn0VI/B9wGZ khzc3WjLKiuvE8Dtr/yTfISyXjP1dS6sdPq6pnjQCxK5fD08jWg7I4lL6JGspMDfEO/8 cxbKMSCeofm6+auOteDCdD+eR3im8igX4W1JL89/fHAcDEpDjYv5pgLhKwYFg5VB49F8 wgaA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723801639; x=1724406439; 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=juhozGP38pQUisELDGJeljBYJ302hnOpTsrGh2pGnFU=; b=dV8oKtrn+eRugUl5HGLWEAObnw9AYPVcZOYyZSSkBk4qLRCnVcExyJH2pcwYWRFofz 7IIOS5G1xNPR8a6xoXRd+FuBZnOvpJGcNpScCvP6dWn8My0ZuPVxCP5sj8vNbF+ypVT7 yStDbUycCOcpSzLyYJ+woDg6qaYsPzbpYzOllIww75XnxLZ9TgZ+a3zxr8cduMGN7gmJ cvad4YKbHKGAxjJFnwypcI8IDSy3OltofxorXDeFs7w388gvJX5wbNMZFxSgZBXW+NDj Z81dlttG+pY3qDCpQHqoIlC42DH1eNoTBIJL5No1ld5ZoZNnQhoZwAUTZmxPCeIJoPMW /CvQ== X-Forwarded-Encrypted: i=1; AJvYcCUi7kyu18DEk8lpE5NE8/nKcfs9dCdowrHW75sYCcFAwey1piJcJQxL3e2C3JJ4qe3zToDQgevPIzx5ID/XO26shu4= X-Gm-Message-State: AOJu0YyLDLeN5GSClhN1ufbtdkOaiWhI6M3mlBZTuaHWPlf2KwPhyvD/ mRKpjgyjN5Z9trWHRQV8CWmeq7qZHai5jm+sJjqijVl9iQ5Faf3IzhepQeivCfPBsgCsSMMA5fI OW7njJ0M7v3NNCUSKYmiAJ7FUGfY= X-Google-Smtp-Source: AGHT+IFusvZlG+6uxNbC781BcbgG4URcYQBQQubc0DVfKShhPEmzVUwcnzvWiiwll63ZAFQD4RIzAxDvo3J/61pAK0A= X-Received: by 2002:a05:6122:29c9:b0:4f5:2b7f:f1a7 with SMTP id 71dfb90a1353d-4fc6cc4428emr2758053e0c.13.1723801639436; Fri, 16 Aug 2024 02:47:19 -0700 (PDT) MIME-Version: 1.0 References: <6e23c705-3d67-419b-b085-f19f5101124c@redhat.com> <20240815235001.96624-1-21cnbao@gmail.com> In-Reply-To: From: Barry Song <21cnbao@gmail.com> Date: Fri, 16 Aug 2024 21:47:04 +1200 Message-ID: Subject: Re: [PATCH v4] mm: Override mTHP "enabled" defaults at kernel cmdline To: David Hildenbrand Cc: akpm@linux-foundation.org, baolin.wang@linux.alibaba.com, corbet@lwn.net, ioworker0@gmail.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org, ryan.roberts@arm.com, v-songbaohua@oppo.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Stat-Signature: of5hu7b84hd7b9cy75xhwgdrkocm6qfg X-Rspamd-Queue-Id: 8A96820027 X-Rspamd-Server: rspam11 X-HE-Tag: 1723801640-616305 X-HE-Meta: U2FsdGVkX19mT2bOqKCHXSJ6r0tP74W7x2U6IEE1jE2tdrvpvV7DmngFbJlqlaK46NFgBv4YSM7O4vctmw9jF4Uhd/cMPoGldk8r/M/heEX2ZwjZAVjfPX+np/bx55B3KiPJaE2iUEMtgIZjfF5LtskdRQTtO+LO95c8aTB6OThIBV+pKz42xBAzQKT1EwjlSwY1l1aXbvVL+iihg0qkGKDCUjm04X88v0eehSYFKJDirQH9JaVjdG7F00CLhQOAqg7+75OB8h1mRaSd9FVVcYf5iO0ihz81C7ZSYau1cij50ZmF2QpxslvGtHG+KqjN4UO2xmgbUz/y2weK16hhL3WenOJUQ/9dMpScPRFACIwWl2sUCOtLcc1LnEbvYXSF0a2/7OD3xOIi8PtZxtQaRu7bSMyr1yEA9muB83zFRCJAs7Q3Mb8XFLHaC6zD5hKk5Qj7pPbsUdNK/gFSP4gHWzcCrJxO2Yu6AgJh9b90zx71XiUmKqMzoNJDJialW9nxA51AbLcZ4PtWDZ8TWRsNCkp+2az8a762/jhGLo6hAHVg7CgUVIOwVyVaDVoEhjQIcyg1Tmlquuva3fhay/jtW4A9d0MMXs8xC3dF6WUeLrHnuyvCIQ+4lm4QoD7D9HBQUCpMLJVlGB0TrSrGPLBhAjKmslZZGgxVcbScW3syH4B+7alpe8xjsZfM/IKXJ4gJLgjSaykGzHCOtV2pJYcDKyYWidW82yAwVPzwqb+fpXDnAQeV5UmvPIcg76CLBuVdc/3Tve64btd4Oiv8Vrw8ZeQyV4ktssHeVY/Oh/p0wiJ8DrbELxZlrOdoP+WTR5Zstj/3YdR+SI6nGS+pgEnAx6fBkRh+534YMqxHQHr8O9IaTRN20NGLwvLlIL9en6uYErP1ovrh0zWJon8fXQX0kfhO1r0Bq9nrQUKQMJupT+4X6ssee9ygbv+MZHunO/Rlz388gDh9aYvnY794gzI fYD0m9Vp 0zyTmmbDOB8QPlBv1MFcRl7iin9LCk77W/6GyWebUlBpU1gHckbyZobAIQ8Hq7s6buiMYECqwxTJ5FKuAHdblZhdVR0GJawYp1/bvSN0CLVLszwgSFXO9i8hHAfAxv+QRyWPqte7p1Y2cMOGEbf2LPBcEbgps94YTIsYUWUD3lcT/FlAGAzn1nb1dd/HUIE7YFx4CPe0Lis4G50tqHsVfJ3P1iVs0DTx4fyiDYkD8MoGFw38If7xWDJUa/l0FymCn7eCfVZmcgUHFrDou/Gj/tjjpYSgoxyvorYNcmpeHoAEvBJrwDM0/fAkJA/37s5Krsshw 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 Fri, Aug 16, 2024 at 9:33=E2=80=AFPM David Hildenbrand wrote: > > On 16.08.24 01:50, Barry Song wrote: > > On Thu, Aug 15, 2024 at 10:26=E2=80=AFPM David Hildenbrand wrote: > >> > >>>>> +static inline int get_order_from_str(const char *size_str) > >>>>> +{ > >>>>> + unsigned long size; > >>>>> + char *endptr; > >>>>> + int order; > >>>>> + > >>>>> + size =3D memparse(size_str, &endptr); > >>>> > >>>> Do we have to also test if is_power_of_2(), and refuse if not? For > >>>> example, what if someone would pass 3K, would the existing check cat= ch it? > >>> > >>> no, the existing check can't catch it. > >>> > >>> I passed thp_anon=3D15K-64K:always, then I got 16K enabled: > >>> > >>> / # cat /sys/kernel/mm/transparent_hugepage/hugepages-16kB/enabled > >>> [always] inherit madvise never > >>> > >> > >> Okay, so we should document then that start/end of the range must be > >> valid THP sizes. > > > > Ack > > > >> > >>> I can actually check that by: > >>> > >>> static inline int get_order_from_str(const char *size_str) > >>> { > >>> unsigned long size; > >>> char *endptr; > >>> int order; > >>> > >>> size =3D memparse(size_str, &endptr); > >>> > >>> if (!is_power_of_2(size >> PAGE_SHIFT)) > >> > >> No need for the shift. > >> > >> if (!is_power_of_2(size)) > >> > >> Is likely even more correct if someone would manage to pass something > >> stupid like > >> > >> 16385 (16K + 1) > > > > Ack > > > >> > >>> goto err; > >>> order =3D get_order(size); > >>> if ((1 << order) & ~THP_ORDERS_ALL_ANON) > >>> goto err; > >>> > >>> return order; > >>> err: > >>> pr_err("invalid size %s in thp_anon boot parameter\n", size_st= r); > >>> return -EINVAL; > >>> } > >>> > >>>> > >>>>> + order =3D fls(size >> PAGE_SHIFT) - 1; > >>>> > >>>> Is this a fancy way of writing > >>>> > >>>> order =3D log2(size >> PAGE_SHIFT); > >>>> > >>>> ? :) > >>> > >>> I think ilog2 is implemented by fls ? > >> > >> Yes, so we should have used that instead. But get_order() > >> is even better. > >> > >>> > >>>> > >>>> Anyhow, if get_order() wraps that, all good. > >>> > >>> I guess it doesn't check power of 2? > >>> > >>>> > >>>>> + if ((1 << order) & ~THP_ORDERS_ALL_ANON) { > >>>>> + pr_err("invalid size %s(order %d) in thp_anon boot pa= rameter\n", > >>>>> + size_str, order); > >>>>> + return -EINVAL; > >>>>> + } > >>>>> + > >>>>> + return order; > >>>>> +} > >>>> > >>>> Apart from that, nothing jumped at me. > >>> > >>> Please take a look at the new get_order_from_str() before I > >>> send v5 :-) > >> > >> Besides the shift for is_power_of_2(), LGTM, thanks! > > > > Thanks, David! > > > > Hi Andrew, > > > > Apologies for sending another squash request. If you'd > > prefer me to send a new v5 that includes all the changes, > > please let me know. > > > > > > Don't shift the size, as it can still detect invalid sizes > > like 16K+1. Also, document that the size must be a valid THP > > size. > > > > diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation= /admin-guide/mm/transhuge.rst > > index 15404f06eefd..4468851b6ecb 100644 > > --- a/Documentation/admin-guide/mm/transhuge.rst > > +++ b/Documentation/admin-guide/mm/transhuge.rst > > @@ -294,8 +294,9 @@ kernel command line. > > > > Alternatively, each supported anonymous THP size can be controlled by > > passing ``thp_anon=3D,[KMG]:;-[KMG]:``, > > -where ```` is the THP size and ```` is one of ``always``, > > -``madvise``, ``never`` or ``inherit``. > > +where ```` is the THP size (must be a power of 2 of PAGE_SIZE an= d > > +supported anonymous THP) and ```` is one of ``always``, ``madv= ise``, > > +``never`` or ``inherit``. > > > > For example, the following will set 16K, 32K, 64K THP to ``always``, > > set 128K, 512K to ``inherit``, set 256K to ``madvise`` and 1M, 2M > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > > index d6dade8ac5f6..903b47f2b2db 100644 > > --- a/mm/huge_memory.c > > +++ b/mm/huge_memory.c > > @@ -953,7 +953,7 @@ static inline int get_order_from_str(const char *si= ze_str) > > > > size =3D memparse(size_str, &endptr); > > > > - if (!is_power_of_2(size >> PAGE_SHIFT)) > > + if (!is_power_of_2(size)) > > goto err; > > > Reading your documentation above, do we also want to test "if (size < > PAGE_SIZE)", or is that implicitly covered? (likely not I assume?) as we also check the order is valid. so size > PAGE_SHIFT)) goto err; order =3D get_order(size); if ((1 << order) & ~THP_ORDERS_ALL_ANON) goto err; return order; err: pr_err("invalid size %s in thp_anon boot parameter\n", size_str); return -EINVAL; } > > I assume it's implicitly covered: if we pass "1k" , it would be mapped > to "4k" (order-0) and that is not a valid mTHP size, right? > > I would appreciate a quick v5, just so can see the final result more > easily :) sure. > > -- > Cheers, > > David / dhildenb >