From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from stravinsky.debian.org (stravinsky.debian.org [82.195.75.108]) (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 9DFBD1D9A54 for ; Mon, 16 Mar 2026 10:13:32 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=82.195.75.108 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773656013; cv=none; b=aWgQnZAJPC6H8G9FC5Ou3S9pv5jO0MmKlXJK17T4Pxf25KGLywxB+UsAhuIY/gLxAgzOMhb6zFNlGVBpc9sxoAcCZn21VQ08JOYV8QNFRbCBkcQbySuQsdw3bDOTNJ00jHJaBUtRiJDnNN34lhF5Ibh5FO0+Uta84POc3buiSyo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773656013; c=relaxed/simple; bh=dBh90J3yam/GgTcLfPaoYfbSD5ctNzjE1A429h0jSv4=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=a9IitMNiuaKvVmxzkWRi1IL2FEQB/+qckFiuMcsH2Liyc0y/zZOemuVKyuh3WcOm+5u886R71rV47/vUys4ETR1n4ffcc+RHfMbI8zJQKV9E6lODDKi6RTh2iK0t2LnsWwtw62gKmfyB2kNqYAAP1CuKZko5zDSAyUE/AszQEW4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=debian.org; spf=none smtp.mailfrom=debian.org; dkim=pass (2048-bit key) header.d=debian.org header.i=@debian.org header.b=GtR6ulxl; arc=none smtp.client-ip=82.195.75.108 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=debian.org Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=debian.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=debian.org header.i=@debian.org header.b="GtR6ulxl" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=debian.org; s=smtpauto.stravinsky; h=X-Debian-User:In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=PMGqjhYGheXdeL0YxuFwQSpgFTVcLscLtzpDIsQK/28=; b=GtR6ulxlBEWy60LKh5iSh4feQM O6HFJZ2Z5VjZcqKplOhTD1WR/F3FMVeUWRx3iBHAuiG0E2Yo75X5f9UqISpKQEs+C8aTPeWcjyJaj iQOg8opYbm5Yj0mOQJ5vq6z2aCUjQBCmJaqBPGwYOsHusSTbKWiu8GI1MLRAfH2IH6e3xrNgOse9G R70Nv3Y7az4lQrQd+hSxkcgmqyjs2H/H+WYUVPjgAQQxQamtihIDbaF46Nqqr5opWyBoitlKsg/0A 0SI8pGGPFEeETZ6qNirdJJx+F4YkaUsGc37bRhlyT7btpp5wc2pSKLlKu/Ocu6gKZfOWu4YgQlLB5 QWOW9GMA==; Received: from authenticated user by stravinsky.debian.org with esmtpsa (TLS1.3:ECDHE_X25519__RSA_PSS_RSAE_SHA256__AES_256_GCM:256) (Exim 4.94.2) (envelope-from ) id 1w24wV-0022S5-Fz; Mon, 16 Mar 2026 10:12:58 +0000 Date: Mon, 16 Mar 2026 03:12:51 -0700 From: Breno Leitao To: Sohil Mehta 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 , Mike Rapoport , linux-mm@kvack.org, linux-kernel@vger.kernel.org, usamaarif642@gmail.com, kas@kernel.org, kernel-team@meta.com, "Lorenzo Stoakes (Oracle)" , Wei Yang , "Accardi, Kristen C" Subject: Re: [PATCH v6 3/4] mm: huge_memory: refactor enabled_store() with change_enabled() Message-ID: References: <20260311-thp_logs-v6-0-421e30d881e0@debian.org> <20260311-thp_logs-v6-3-421e30d881e0@debian.org> <322f6f2f-840e-462f-96b0-b603b9c88582@intel.com> 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: <322f6f2f-840e-462f-96b0-b603b9c88582@intel.com> X-Debian-User: leitao Hello Sohil, On Fri, Mar 13, 2026 at 03:31:25PM -0700, Sohil Mehta wrote: > Hello, > > On 3/11/2026 3:17 AM, Breno Leitao wrote: > > > +static bool set_global_enabled_mode(enum global_enabled_mode mode) > > +{ > > + static const unsigned long thp_flags[] = { > > + TRANSPARENT_HUGEPAGE_FLAG, > > + TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG, > > + }; > > + enum global_enabled_mode m; > > + bool changed = false; > > + > > + for (m = 0; m < ARRAY_SIZE(thp_flags); m++) { > > + if (m == mode) > > + changed |= !__test_and_set_bit(thp_flags[m], > > + &transparent_hugepage_flags); > > + else > > + changed |= __test_and_clear_bit(thp_flags[m], > > + &transparent_hugepage_flags); > > I am not a mm expert and typically do not follow the mm list. Is there > an issue with the usage of non-atomic variants here? The commit message > says this uses the same pattern as set_anon_enabled_mode(). > > However, set_anon_enabled_mode() has a spinlock=>huge_anon_orders_lock > protecting the access. But, transparent_hugepage_flags seems to be > unprotected in that regard. I don't think that the atomic vs non-atomic will not help much, given this is a compoud operation. Independently if this is atomic or not, it is racy with anyone changing these fields (transparent_hugepage_flags). In other words, Atomic ops make each individual bit flip safe, but set_global_enabled_mode() and defrag_store() need to flip multiple bits as a group. With atomic ops, two concurrent writers can still interleave and leave the flags in an invalid state. That said, Although I don't think this patch is making it worse, I think the is a racy issue here that we can make better. My suggestion is to move the rest of the helpers (defrag_store()) to use sysfs_match_string(), and then create a thp_flags_lock spinlock to protect operations against transparent_hugepage_flags. Any concern about this approach? Thanks for reviewing it, --breno