From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id E29F14F881 for ; Thu, 5 Mar 2026 12:30:49 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772713850; cv=none; b=BS9EYHRY7Yc4WD57TRdtxUxTX3Vu0QRKzAl264rXxzswB/xcozFu/nlJhqsERpfD+4Rs0cXrA0AumUtWdsY9lmgkNM7VK/p0VoB+vJYa4HrHj9/dAm1iNHgYlA9jOP1wTR/Ii+bdsVAQGSR6gsgqWHlSrAuw/pRnW9EF1rLFtO0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1772713850; c=relaxed/simple; bh=ou6P22AaUMwtzK/lZK4+cuILdeYptAQivhv6iGAoAfI=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=prnOzGraJG9VFsc6Cw12N/OJXl1/5rOmTrlGaXU9yW2C0Negqmtz2tTydZq/pixp3CsHcwbHrzQ3L63PhCAq6YHpyIg/fGe8as4HZWQXzE9zH+91fTQyV0VRvZjBwOYcpTD1VyTpuqHTqSjStRuF05xQU9a9MjXPau7z4DBxgI4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=tmwqJizx; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="tmwqJizx" Received: by smtp.kernel.org (Postfix) with ESMTPSA id D708DC116C6; Thu, 5 Mar 2026 12:30:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1772713849; bh=ou6P22AaUMwtzK/lZK4+cuILdeYptAQivhv6iGAoAfI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=tmwqJizxPJNI+D0Sw/K02tGdZCnZDkDoG5eim6r8+3PRJSn0+EI2QzONUMRGd4IWX gQOtFqxdl4Kz9hWMoR0DCcegkXIjnMJJOsl8E5CAcfTi7cLTOFnoqc183Z4rzi4Y4q qB0wqhH4rD5Yc+YD9avyfRhhYGMV0fX4y4c1fso04d2AQ+naaGlJNBxUBSBiaOVSCD Skv68MX2O7k3kLPhe1KiHJV/7x9PfEMoqTta/KBp1TBp5nR/JbaZCCxoMGpSFd68Af PdS/M7ZD6xXioibwlbYdsb30XhzDxVeX6x8Nw1zi4GwHyjyQiI17gruAuKmC7yAqWn tLEPI+JKnzMRQ== Date: Thu, 5 Mar 2026 12:30:40 +0000 From: "Lorenzo Stoakes (Oracle)" To: Breno Leitao Cc: Andrew Morton , David Hildenbrand , Lorenzo Stoakes , Zi Yan , Baolin Wang , "Liam R. Howlett" , Nico Pache , Ryan Roberts , Dev Jain , Barry Song , Lance Yang , Vlastimil Babka , Suren Baghdasaryan , Michal Hocko , Brendan Jackman , Johannes Weiner , linux-mm@kvack.org, linux-kernel@vger.kernel.org, usamaarif642@gmail.com, kas@kernel.org, kernel-team@meta.com Subject: Re: [PATCH 1/2] mm: thp: avoid calling start_stop_khugepaged() in anon_enabled_store() Message-ID: <6eaeeace-7b0e-4465-8bb0-88cce2b13bb9@lucifer.local> References: <20260304-thp_logs-v1-0-59038218a253@debian.org> <20260304-thp_logs-v1-1-59038218a253@debian.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Thu, Mar 05, 2026 at 03:48:07AM -0800, Breno Leitao wrote: > On Wed, Mar 04, 2026 at 04:40:22PM +0000, Lorenzo Stoakes (Oracle) wrote: > > On Wed, Mar 04, 2026 at 02:22:33AM -0800, Breno Leitao wrote: > > > Writing "never" (or any other value) multiple times to > > > /sys/kernel/mm/transparent_hugepage/hugepages-*/enabled calls > > > start_stop_khugepaged() each time, even when nothing actually changed. > > > This causes set_recommended_min_free_kbytes() to run unconditionally, > > > which is unnecessary and floods the printk buffer with "raising > > > min_free_kbytes" messages. Example: > > > > > > # for i in $(seq 100); do > > > # echo never > /sys/kernel/mm/transparent_hugepage/enabled > > > # done > > > > > > # dmesg | grep "min_free_kbytes is not updated" | wc -l > > > 100 > > > > > > Use test_and_set_bit()/test_and_clear_bit() instead of the plain > > > variants to detect whether any bit actually flipped, and skip the > > > start_stop_khugepaged() call entirely when the configuration is > > > unchanged. > > > > > > With this patch, redoing the same operation becomes a no-op. > > > > > > Signed-off-by: Breno Leitao > > > > General concept is sensible, but let's improve this code please. > > Ack! Thanks for the suggestions. No problem, thanks for the patch! :) > > > > spin_unlock(&huge_anon_orders_lock); > > > } else > > > ret = -EINVAL; > > > > > > - if (ret > 0) { > > > + if (ret > 0 && changed) { > > > int err; > > > > > > err = start_stop_khugepaged(); > > > > There's a caveat here as mentioned in reply to Kiryl - I'm concerned users > > might rely on the set recommended min kbytes even when things don't change. > > > > Not sure how likely that is, but it's a user-visible change in how this behaves. > > Is there any motivation that users are retouching > /sys/kernel/mm/transparent_hugepage just to trigger > set_recommended_min_free_kbytes() ? That seems weird, but, I will keep it in > the change. I mean I can't really think of any, but I don't want to risk breaking (weird) userspace. > > > From cb2c4c8bf183ef0d10068cfd12c12d19cb17a241 Mon Sep 17 00:00:00 2001 > > From: "Lorenzo Stoakes (Oracle)" > > Date: Wed, 4 Mar 2026 16:37:20 +0000 > > Subject: [PATCH] idea > > > > Signed-off-by: Lorenzo Stoakes (Oracle) > > Thanks for the idea. Let me hack on top of it, and propose a v2. Thanks! > > > --- > > mm/huge_memory.c | 74 ++++++++++++++++++++++++++++++------------------ > > 1 file changed, 46 insertions(+), 28 deletions(-) > > > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > > index 0df1f4a17430..97dabbeb9112 100644 > > --- a/mm/huge_memory.c > > +++ b/mm/huge_memory.c > > @@ -515,46 +515,64 @@ static ssize_t anon_enabled_show(struct kobject *kobj, > > return sysfs_emit(buf, "%s\n", output); > > } > > > > +enum huge_mode { > > + HUGE_ALWAYS, > > + HUGE_INHERIT, > > + HUGE_MADVISE, > > + HUGE_NUM_MODES, > > + HUGE_NEVER, > > +}; > > + > > +static bool change_anon_orders(int order, enum huge_mode mode) > > +{ > > + static unsigned long *orders[] = { > > + &huge_anon_orders_always, > > + &huge_anon_orders_inherit, > > + &huge_anon_orders_madvise, > > + }; > > + bool changed = false; > > + int i; > > + > > + spin_lock(&huge_anon_orders_lock); > > + for (i = 0; i < HUGE_NUM_MODES; i++) { > > > + if (i == mode) > > + changed |= !test_and_set_bit(order, orders[mode]); > > + else > > + changed |= test_and_clear_bit(order, orders[mode]); > > I suppose we want s/mode/i in the test_and_{clear,set}_bit() here: > > if (i == mode) > // set for mode > changed |= !test_and_set_bit(order, orders[i]); > else > // clear for !mode > changed |= test_and_clear_bit(order, orders[i]); > > For two reasons: > * you want to unset "i" when i != mode. > * you would have an OOB when accessing orders[HUGE_NEVER == 4] > > > > static ssize_t anon_enabled_store(struct kobject *kobj, > > struct kobj_attribute *attr, > > const char *buf, size_t count) > > { > > int order = to_thpsize(kobj)->order; > > ssize_t ret = count; > > + bool changed; > > + > > + if (sysfs_streq(buf, "always")) > > + changed = change_anon_orders(order, HUGE_ALWAYS); > > + else if (sysfs_streq(buf, "inherit")) > > + changed = change_anon_orders(order, HUGE_INHERIT); > > + else if (sysfs_streq(buf, "madvise")) > > + changed = change_anon_orders(order, HUGE_MADVISE); > > + else if (sysfs_streq(buf, "never")) > > + changed = change_anon_orders(order, HUGE_NEVER); > > + else > > + return -EINVAL; > > I think we can simplify anon_enabled_store() even more, by leveraging sysfs_match_string(). > Something like: > > static const char *const anon_mode_strings[] = { > [HUGE_ALWAYS] = "always", > [HUGE_INHERIT] = "inherit", > [HUGE_MADVISE] = "madvise", > [HUGE_NEVER] = "never", > NULL, > }; > > and then > > static ssize_t anon_enabled_store(struct kobject *kobj, > struct kobj_attribute *attr, > const char *buf, size_t count) > { > int order = to_thpsize(kobj)->order; > int mode; > > mode = sysfs_match_string(enabled_mode_strings, buf); > if (mode < 0) > return mode; Nice! > > if (change_anon_orders(order, mode)) { > int err = start_stop_khugepaged(); > > if (err) > return err; > } else { > /* Users expect this even if unchanged. TODO: Put in header... */ > //set_recommended_min_free_kbytes(); > } > return count; > } > > > > Anyway, I like this approach, thanks!. Let me hack a v2 based on it. Great, thanks! Note that my code seemed to introduce a splat, so it's buggy, make sure to check it carefully (that 'untested' proviso was apposite, it turns out! :) > > --breno Cheers, Lorenzo