From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-185.mta0.migadu.com (out-185.mta0.migadu.com [91.218.175.185]) (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 837141A9F97 for ; Tue, 30 Jun 2026 01:00:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.218.175.185 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1782781203; cv=none; b=lwPZ0S9HF8JkcN9v4jffZBYQRqBI82FGa7o9MmS6sMaPWCXhNiqCabYJcB1Bj4si5y3eqWVPJJdxPbZttNnkqZNX6j7/emDFStfWWybJP+W6yjVabqOJilvHceCYIu0jo7Lig4O7hq2lq53BNhunGhqrwwNP/8+lduiUsOobBJM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1782781203; c=relaxed/simple; bh=2GoPVn//CBUBoyyWGAr139cML42u0stanDfnxcJQgX0=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=otX2owF6/Sm10Gd0gHcBWqrbIqPIgb6E7cS/Lix11GW9kBFQyjk0VCK/uFEdIDi0Bw20R7k0NKcJvD3ii4J9p0f+DjI2EZ+fukm3t6aSfCxUtBS/YhyKQLw9QbaLb6nqupDrW2fKnKUFM1Aoqssnv2qLbdovHHwLDVHRrz9hS6w= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=w7ejOGDo; arc=none smtp.client-ip=91.218.175.185 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="w7ejOGDo" Message-ID: <29d9caca-5637-49c1-978c-83550430c538@linux.dev> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1782781198; h=from:from: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; bh=G+5yOIRk93e36AuhpitdlOf7eS3TS9OTL2poou9R2Nk=; b=w7ejOGDog1rBi7CMTMbxBtiPSX1aljWTgVWoM9o7Y29SiAkzCiLmQyVUCwsCyMirgyVDvP vQX7Lw0vvbTwMlUYiVYXpMIh2TXB2aT79pd7hbvrY7T66I1rrJg6mFuOzc5eRe3a4MP1sl 3cXVz0W/jhqLl3ZCCHwLRqxTAp0wREM= Date: Tue, 30 Jun 2026 08:59:47 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Subject: Re: [PATCH v2 2/6] mm/page_owner: use MIGRATE_REASON_NONE instead of -1 for last_migrate_reason To: "Vlastimil Babka (SUSE)" , Zi Yan , Andrew Morton Cc: Suren Baghdasaryan , Michal Hocko , Brendan Jackman , Johannes Weiner , linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20260626024550.25677-1-ye.liu@linux.dev> <20260626024550.25677-3-ye.liu@linux.dev> <407fa67f-3212-4c5a-ac8c-7c5d41479cc9@linux.dev> Content-Language: en-US X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Ye Liu In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT 在 2026/6/29 16:00, Vlastimil Babka (SUSE) 写道: > On 6/29/26 04:20, Ye Liu wrote: >> >> >> 在 2026/6/27 02:45, Zi Yan 写道: >>> On Thu Jun 25, 2026 at 10:45 PM EDT, Ye Liu wrote: >>>> The last_migrate_reason field uses -1 as a sentinel value to mean "no >>>> migration has happened". Replace the four bare -1 occurrences with a >>>> local MIGRATE_REASON_NONE define so the intent is explicit and the >>>> magic number is eliminated. >>>> >>>> No functional change. >>>> >>>> Signed-off-by: Ye Liu >>>> --- >>>> mm/page_owner.c | 15 +++++++++++---- >>>> 1 file changed, 11 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/mm/page_owner.c b/mm/page_owner.c >>>> index 342549891a8d..ebafa9d7ff07 100644 >>>> --- a/mm/page_owner.c >>>> +++ b/mm/page_owner.c >>>> @@ -21,6 +21,13 @@ >>>> */ >>>> #define PAGE_OWNER_STACK_DEPTH (16) >>>> >>>> +/* >>>> + * Used to indicate that a page has never been migrated, as the valid >>>> + * migrate_reason values are non-negative enum members (MR_* in >>>> + * include/linux/migrate_mode.h). >>> >>> Why not change last_migrate_reason to enum migrate_reason and functions >>> pass migrate reasons? MR_TYPES probably can be used to indicate no >>> migration has happened if we do not want to add MR_NO_MIGRATION. > > Indeed. I'd just add a proper MR_NEVER option with a comment. > >>> folio_set_owner_migrate_reason(), >>> __folio_set_owner_migrate_reason(), >>> move_hugetlb_state(), and >>> __update_page_owner_handle() need to update their signatures for this. > > They should ideally, but do they "need" as part of this series? > But I guess it's not a big deal. > >> Thanks for the review. >> >> Using MR_TYPES as a sentinel is problematic - it's used kernel-wide as an >> array-size marker (migrate_reason_names[MR_TYPES]). Overloading it to also >> mean "never migrated" conflates two unrelated semantics. It also has a > > Well it's just for tracepoint and dumping page owner. Nothing prevents us > assigning a value to MR_NEVER that's not -1 (just make MR_NEVER the first or > last option of the enum) and a give it a name e.g. "never_migrated" in > include/trace/events/migrate.h > >> subtle out-of-bounds risk: if anyone ever uses the field as an array index >> without the guard check, MR_TYPES silently points past the end of the array, >> while -1 faults immediately. > > That wouldn't be a problem either with a proper value. > >> Adding MR_NONE = -1 to the enum is the right long-term fix, but that needs >> proper enum definition changes and cascading signature updates across multiple >> headers — better done as a separate series. >> The current patch is a minimal improvement: it kills the magic number and >> makes intent explicit without touching any header. > > Minimal fixes are sometimes necessary for actual bugs, for stable backport > etc. Nothing prevents us doing it properly here. > Indeed, let me do it properly. In v3 I'll split this into two patches: 1. Add MR_NEVER (value 10) to enum migrate_reason in migrate_mode.h, add the corresponding "never_migrated" string in the MIGRATE_REASON trace macro, and replace the four bare -1 with MR_NEVER in page_owner.c. This removes the local MIGRATE_REASON_NONE define entirely. 2. A follow-up cleanup that converts all 'int reason' parameters in the callchain to 'enum migrate_reason'. The affected functions are: 2. page_owner: __folio_set_owner_migrate_reason(), folio_set_owner_migrate_reason() migrate: migrate_pages(), migrate_pages_sync(), migrate_pages_batch(), migrate_folios_move(), migrate_hugetlbs(), unmap_and_move_huge_page() hugetlb: move_hugetlb_state(), htlb_allow_alloc_fallback() trace: mm_migrate_pages / mm_migrate_pages_start events 2. The 'short last_migrate_reason' struct field and the __update_page_owner_handle() parameter are intentionally left as 'short' since struct page_owner is per-page metadata and the size matters there. Thanks for the review. >>> >>>> + */ >>>> +#define MIGRATE_REASON_NONE (-1) >>>> + >>>> struct page_owner { >>>> unsigned short order; >>>> short last_migrate_reason; >>>> @@ -339,7 +346,7 @@ noinline void __set_page_owner(struct page *page, unsigned short order, >>>> depot_stack_handle_t handle; >>>> >>>> handle = save_stack(gfp_mask); >>>> - __update_page_owner_handle(page, handle, order, gfp_mask, -1, >>>> + __update_page_owner_handle(page, handle, order, gfp_mask, MIGRATE_REASON_NONE, >>>> ts_nsec, current->pid, current->tgid, >>>> current->comm); >>>> inc_stack_record_count(handle, gfp_mask, 1 << order); >>>> @@ -596,7 +603,7 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, >>>> if (ret >= count) >>>> goto err; >>>> >>>> - if (page_owner->last_migrate_reason != -1) { >>>> + if (page_owner->last_migrate_reason != MIGRATE_REASON_NONE) { >>>> ret += scnprintf(kbuf + ret, count - ret, >>>> "Page has been migrated, last migrate reason: %s\n", >>>> migrate_reason_names[page_owner->last_migrate_reason]); >>>> @@ -667,7 +674,7 @@ void __dump_page_owner(const struct page *page) >>>> stack_depot_print(handle); >>>> } >>>> >>>> - if (page_owner->last_migrate_reason != -1) >>>> + if (page_owner->last_migrate_reason != MIGRATE_REASON_NONE) >>>> pr_alert("page has been migrated, last migrate reason: %s\n", >>>> migrate_reason_names[page_owner->last_migrate_reason]); >>>> page_ext_put(page_ext); >>>> @@ -826,7 +833,7 @@ static void init_pages_in_zone(struct zone *zone) >>>> >>>> /* Found early allocated page */ >>>> __update_page_owner_handle(page, early_handle, 0, 0, >>>> - -1, local_clock(), current->pid, >>>> + MIGRATE_REASON_NONE, local_clock(), current->pid, >>>> current->tgid, current->comm); >>>> count++; >>>> ext_put_continue: >>> >>> >>> >>> >> > -- Thanks, Ye Liu