From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 96152146018; Fri, 5 Jul 2024 09:34:48 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1720172090; cv=none; b=grYUWa1H3YrUM7P1flSg30EX/e6V+CWQeI54L67CVh9odz8O+qy20UuFSthTpBqpLAjeWMabOmwvPuC1Bp/U2SY8WxGdiYKwS7wj6gGhK5tq/yiDmyXEcf/urqJAYM5QhlBr6o2INi0CrjuodTmdmSfUkwdEqDHmTYHvxgoUI2E= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1720172090; c=relaxed/simple; bh=ClpXwmxX/iI8KJIciRRFV8mBW7xSc8xL23a+yxJ4flI=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=BuP1zJ4pWDmKYwuQ01ypIJ9HFXaE3tM2OTBfF5wiZlyOkiKwdfyZ2saVVNnjfHoHgTRgeOIZwUrpfDd5M79tR5ZBvXZ05Z7GWRAV7J5FhM1R4NOwm7N/sVVoAt3ah5RWl8bNxKsyp9ciw4Naj8ME+KpfoyU2Pdvxpeqbrwy9Drg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com 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 1DF55367; Fri, 5 Jul 2024 02:35:13 -0700 (PDT) Received: from [10.57.74.223] (unknown [10.57.74.223]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 569DD3F762; Fri, 5 Jul 2024 02:34:45 -0700 (PDT) Message-ID: Date: Fri, 5 Jul 2024 10:34:43 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] mm: Fix khugepaged activation policy Content-Language: en-GB To: Andrew Morton Cc: Jonathan Corbet , David Hildenbrand , Barry Song , Baolin Wang , Lance Yang , Yang Shi , linux-mm@kvack.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org References: <20240704091051.2411934-1-ryan.roberts@arm.com> <20240704113325.fb9f1b04f99abaac315b5c88@linux-foundation.org> From: Ryan Roberts In-Reply-To: <20240704113325.fb9f1b04f99abaac315b5c88@linux-foundation.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 04/07/2024 19:33, Andrew Morton wrote: > On Thu, 4 Jul 2024 10:10:50 +0100 Ryan Roberts wrote: > >> Since the introduction of mTHP, the docuementation has stated that >> khugepaged would be enabled when any mTHP size is enabled, and disabled >> when all mTHP sizes are disabled. There are 2 problems with this; 1. >> this is not what was implemented by the code and 2. this is not the >> desirable behavior. >> >> Desirable behavior is for khugepaged to be enabled when any PMD-sized >> THP is enabled, anon or file. (Note that file THP is still controlled by >> the top-level control so we must always consider that, as well as the >> PMD-size mTHP control for anon). khugepaged only supports collapsing to >> PMD-sized THP so there is no value in enabling it when PMD-sized THP is >> disabled. So let's change the code and documentation to reflect this >> policy. >> >> Further, per-size enabled control modification events were not >> previously forwarded to khugepaged to give it an opportunity to start or >> stop. Consequently the following was resulting in khugepaged eroneously >> not being activated: >> >> echo never > /sys/kernel/mm/transparent_hugepage/enabled >> echo always > /sys/kernel/mm/transparent_hugepage/hugepages-2048kB/enabled >> >> ... >> >> -static inline bool hugepage_flags_enabled(void) >> +static inline bool hugepage_pmd_enabled(void) >> { >> /* >> - * We cover both the anon and the file-backed case here; we must return >> - * true if globally enabled, even when all anon sizes are set to never. >> - * So we don't need to look at huge_anon_orders_inherit. >> + * We cover both the anon and the file-backed case here; file-backed >> + * hugepages, when configured in, are determined by the global control. >> + * Anon pmd-sized hugepages are determined by the pmd-size control. >> */ >> - return hugepage_global_enabled() || >> - READ_ONCE(huge_anon_orders_always) || >> - READ_ONCE(huge_anon_orders_madvise); >> + return (IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) && hugepage_global_enabled()) || >> + test_bit(PMD_ORDER, &huge_anon_orders_always) || >> + test_bit(PMD_ORDER, &huge_anon_orders_madvise) || >> + (test_bit(PMD_ORDER, &huge_anon_orders_inherit) && hugepage_global_enabled()); >> } > > That's rather a mouthful. Is this nicer? Sure, I'll take your version into v3. > > static inline bool hugepage_pmd_enabled(void) > { > /* > * We cover both the anon and the file-backed case here; file-backed > * hugepages, when configured in, are determined by the global control. > * Anon pmd-sized hugepages are determined by the pmd-size control. > */ > if (IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) && > hugepage_global_enabled()) > return true; > if (test_bit(PMD_ORDER, &huge_anon_orders_always)) > return true; > if (test_bit(PMD_ORDER, &huge_anon_orders_madvise)) > return true; > if (test_bit(PMD_ORDER, &huge_anon_orders_inherit) && > hugepage_global_enabled()) > return true; > return false; > } > > Also, that's a pretty large function to be inlined. It could be a > non-inline function static to khugepaged.c. But I suppose that's a > separate patch. Yeah fair point. I'll respin it now as a static in khugepaged.c.