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 2209B365A1C for ; Tue, 17 Mar 2026 11:24:14 +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=1773746656; cv=none; b=LhrKvCZsBX7T2s17XNG9bun4x3sdDk3BUvQbumRf+V9BmBd+UxtLVANJKTWtdk7wzEAlA22Ic6xL5faV+0u98wgkvsZIOM/j6JHoNSru77chzzhnOHauKb6sqUVQumwHhg8i6xV666P1J9sHh8YoQgzWwZad23ZMWww/KcK/XVk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1773746656; c=relaxed/simple; bh=TthdpDCzlVtbgPk5u6D9Nzp8iEsmpQ9Ft3PYUo1Qql0=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Ri5+8nTNF/5Z4ZPkBdyqab3MQ1LL/v/aRfjLHFPWPQjycF1KjYz3EKwyqX4mkF/eWXxHDGXqiFgMMzMKG6CGK/igKGCkaHgqH9eeer7GM4OjDAIxbeheQEP/z/OR+wIVozEXCvAy8qYWND3CpcmYviAfcUI2iOSBBT22bObsSk0= 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=CfuU8sp8; 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="CfuU8sp8" 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=1HYSPSrBr6XMwpPFaT+qL1xDiWXaJ1imWzB15lXKN0c=; b=CfuU8sp8RV8jQo+WBsuSxW6xLc qT6oatdKGhOsEFIePrg9fySJCSPiVDj6wc7fv9mtJP/yZ9yFxhqaqufxoGtIkqhitJ+g/RyiyTmGP HNwuKYDriAvjjZJ+iLDi30N0189eNMdg05VTllYHaBsSqq4djgqDojZwO/Eimz/B3lJiH7eayAYUg rIChz3+THC5NmZkyzYw+YgsCjFmT3/gUrICB6jJsjmzHXU/qiYRTdeuiUOA7IZns1QIhuTYBORJHS NkwFYcbgS26+8GY3JvWheDTMgH1tcEhakMtJpgG/vQsEG0KKeK/EjdCoBJn7/ypxmRh8BlY/C9LaF YEoNHI4w==; 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 1w2SWX-002qgq-NI; Tue, 17 Mar 2026 11:23:44 +0000 Date: Tue, 17 Mar 2026 04:23:37 -0700 From: Breno Leitao To: "Lorenzo Stoakes (Oracle)" Cc: Sohil Mehta , 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, 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> <1b7d1bdf-c002-4543-b858-09fd2b1b73f9@lucifer.local> 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: <1b7d1bdf-c002-4543-b858-09fd2b1b73f9@lucifer.local> X-Debian-User: leitao On Tue, Mar 17, 2026 at 09:37:39AM +0000, Lorenzo Stoakes (Oracle) wrote: > On Mon, Mar 16, 2026 at 04:26:30PM -0700, Sohil Mehta wrote: > > On 3/16/2026 3:12 AM, Breno Leitao wrote: > > > > >> 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. > > > > You are right it is a compound operation. So, there is an existing issue > > with two concurrent writers which can leave the flags in an invalid state. > > > > But, I was wondering if there is a slightly different issue now due to > > the non-atomic part. Some updates could be lost depending on the timing > > of the operation. > > > > For example, > > > > CPU1 does: CPU2 does: > > set_global_enabled_mode("always") defrag_store("always") > > > > ___test_and_set_bit(): > > // Trying to set bit 1 > > > > old = *p > > // reads flags, sees defrag bit=0 > > > > set_bit(DEFRAG_DIRECT_FLAG) > > // atomic: sets bit 3 > > > > > > *p = old | TRANSPARENT_HUGEPAGE_FLAG; > > > // writes back old value with bit 1 set > > // DEFRAG_DIRECT_FLAG is lost (reverted to 0) > > > > IIUC, this issue didn't exist before. I think it might be safer to use > > the atomic test_and_set_bit() to be compatible with the older code. > > Though, I'll leave it up to you as I don't have expertise here. > > No, it's up to the maintainers :) > > Given the above I think we should switch back to the atomic accessors. > > We can address the broader issues with this horrible code in a separate > series. Ack. let me respin then. > > Overall, as you mentioned below, protecting transparent_hugepage_flags > > with a spinlock seems like a better, long-term solution to me as well. > > Yeah, let's look at doing a follow up that cleans this up in general and > address that then. Sure. I am planning to improve defrag_store() as the next work, and then come up with this additional spinlock for transparent_hugepage_flags.