From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 943B5C02198 for ; Fri, 14 Feb 2025 21:26:56 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0FB9A280002; Fri, 14 Feb 2025 16:26:56 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 0AB54280001; Fri, 14 Feb 2025 16:26:56 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EB460280002; Fri, 14 Feb 2025 16:26:55 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id CD2C5280001 for ; Fri, 14 Feb 2025 16:26:55 -0500 (EST) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 3ED72140A6E for ; Fri, 14 Feb 2025 21:26:55 +0000 (UTC) X-FDA: 83119835190.18.6AC29E0 Received: from mail-qt1-f176.google.com (mail-qt1-f176.google.com [209.85.160.176]) by imf03.hostedemail.com (Postfix) with ESMTP id 0B51520003 for ; Fri, 14 Feb 2025 21:26:52 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=cmpxchg-org.20230601.gappssmtp.com header.s=20230601 header.b="V/aQLP1W"; spf=pass (imf03.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.160.176 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org; dmarc=pass (policy=none) header.from=cmpxchg.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1739568413; h=from:from:sender: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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=Cg1ey3aipCNJUc79bD6lrhDndah42SIkSCRK4mNzles=; b=pP1g9JldYN3RYUxwqt/XO0KQoaLt4pMfNOToz2yBycw2W7dcKq9sglA4fgZKQNlreZuwPv yIsIXsAxqpt88BWwh2u8/yeth9F81EnlhBZx5LBkx7QvfTFSKbZU+6xfQE/xy2HraAKN/A VoAwh5WlcnVMNqC+188QT5otUn6ZS7M= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1739568413; a=rsa-sha256; cv=none; b=1oHbcSvNVv7lMqCosyVEFcMCHL76Igi67Q6BACglF5gnbiYsyF/d2YoX+Q15ZsDZEUiDO8 EK8dwUZ/gt67uciydDS5CV1HDrG+fNCFVmJ5t9ahuEfiEh7lOedBJkxU0gjHevGFOl2vGY SVabrOwJsV0YBqwiAMdPa5DKqmOR7T8= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=cmpxchg-org.20230601.gappssmtp.com header.s=20230601 header.b="V/aQLP1W"; spf=pass (imf03.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.160.176 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org; dmarc=pass (policy=none) header.from=cmpxchg.org Received: by mail-qt1-f176.google.com with SMTP id d75a77b69052e-471cdc14e99so14005631cf.1 for ; Fri, 14 Feb 2025 13:26:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cmpxchg-org.20230601.gappssmtp.com; s=20230601; t=1739568412; x=1740173212; darn=kvack.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=Cg1ey3aipCNJUc79bD6lrhDndah42SIkSCRK4mNzles=; b=V/aQLP1Wa9Z1LHxuECgubA76hBfnrWEbgBmlENUglwU7/loXDCBkwEtPu9zonhoiGB 3iTcldRPuMtIhC4KWWAWPhqnuVBVqCFHEZEcIjSkIJP93YvD/sMHtdfPe38fVirL2/mx mB5fvggXhL+a6kn+rpzFT6M+mV+M/qtS1Vat1sekIp8AhWGB3eLk8Knf1SOZbJ4bUGdi upUPU9Xd/dfwqHC6T2IdMtUq3bsK1ZNSEyygWBTcsJ7YDcuL85oTSDU1LnYv+gNSq3AG Tsjy66DSOPyVx3GdEFA6bCs+N1X3CCMY+cKTWqYWbB/Fos4toJbOe1HKBEY0OjPcHWA8 ZPtw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1739568412; x=1740173212; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=Cg1ey3aipCNJUc79bD6lrhDndah42SIkSCRK4mNzles=; b=gQ+IR8gVLzg0nKNIuB3UsVti0EC4PcyiOd2Je7pFZxdP1ZC9aZA+G4xvQgN9LztdvF u2RIyK+CcGuuH49pCGlykhY+zGlo3ClktTix0iQDta64ZEL9TQwBgFQZ/V7ZHiZs1XDA yaP7WcmnACHhhnqrEqInPuVgWNV77WJp9kPLpbgBZzNZVm7Gf9zx669sE5ICga+XP4t2 y10tgKh1vGtSpHBAoEzaHh03qwwcM9dre3LXBr93l21kV2TBq+5FTHdqttc7YMeNh2io JbjtTDjAw1+FCq6YgSMN8CiUlt2ILSJ4Bt7p3VPqaLjwayyCWXqFYsu0WgGsweQAQsfV eLEA== X-Forwarded-Encrypted: i=1; AJvYcCU7WTLrJ89otwbrSnMhzvKCfJLDP9V5FFlA+TOesT3j0SuGiUV1DrFOmGOdcMLE4oHbsrkepzz6og==@kvack.org X-Gm-Message-State: AOJu0Ywq5wMzkdPjYVO+w3XYTo9+rUYDMwB+L4EaS8qmJr+rHMNYfWf+ N1//dj/u5FdDOrhvrq6Xl3F38ULEyq8rHhAsxzlJzsBC7+XxWjFIIQK2m31ubARWirXGIeISmjd 4 X-Gm-Gg: ASbGncs/+Tu967nRYq5XbquHyZ/zw4bnYCzlq6/6+wHtE6bUJOAFYae3oXkCOJWkYXJ KPepwQmzD0iM9+uYz6RV7jZndT8ZNV3N1js/St6T3tRE3aZROKBKJycwP7Vc/egPAi6WRnkwHx+ +CId/w7aBqaET8/Nii6wUJmOik64tG7EzGOFPh7Z9sx+V16bgVWRQIFKxv+MMJdm94F5KoQJcXN KNdRc0CzD0DdwPof3/TjfdoAvX+tqcFXuvW141Akrt7nrsKne91LV0vjHOyosDvPFLiasmSzsX8 UdjrNaWSXGjkBA== X-Google-Smtp-Source: AGHT+IGKobUtXHluY1sE6yvKOwaoaHY7us7ziVqjKoQwwTgWBzyrPdqWETaIUMUOpE8su1LCqMuAhA== X-Received: by 2002:a05:622a:1801:b0:471:887e:fe4 with SMTP id d75a77b69052e-471dc14081amr12129771cf.5.1739568411804; Fri, 14 Feb 2025 13:26:51 -0800 (PST) Received: from localhost ([2603:7000:c01:2716:da5e:d3ff:fee7:26e7]) by smtp.gmail.com with UTF8SMTPSA id d75a77b69052e-471c2a12436sm21514891cf.23.2025.02.14.13.26.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 14 Feb 2025 13:26:51 -0800 (PST) Date: Fri, 14 Feb 2025 16:26:47 -0500 From: Johannes Weiner To: Brendan Jackman Cc: Andrew Morton , Vlastimil Babka , Mel Gorman , Michal Hocko , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Yosry Ahmed Subject: Re: [PATCH] mm/page_alloc: Clarify some migratetype fallback code Message-ID: <20250214212647.GB233399@cmpxchg.org> References: <20250214-clarify-steal-v1-1-79dc5adf1b79@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250214-clarify-steal-v1-1-79dc5adf1b79@google.com> X-Rspam-User: X-Rspamd-Queue-Id: 0B51520003 X-Rspamd-Server: rspam07 X-Stat-Signature: ux7u16k7sopej7resqeasyoe85fs7hyy X-HE-Tag: 1739568412-110348 X-HE-Meta: U2FsdGVkX19ysHEcN3iQQZ6v2bqmUFO29jz1QIvjBFzoyiEXIWNGX9sBvjOP9SY5KLjuzEead8vVfk2HLIEAYCZ2UZjnID+wtR6IfQ5TgjW15M8aL7Pi7LkU0jjy/9Z/+eCJYPeey9Gsv2hLLYtf9OYo6+CRcPmBpb0PDe4AwJhCZuE9+RHldHhuEXjjor6M/nH3AaFRZlGkiF0geosLPFKBh40PCimIJxqLqmP9AU67SEzQHUIPP3B8LbxyyEXvAlcsOLjbnd3an0OFDYFpYiah4tggMT+WBml0VXUENC8zT9bpCWjjUtUipaZliUur9FClF8Emw7NlfYYABbfUxcxkV3lS1QbKzZJtjMdhensQIr/GGJyDaPAWkY2e0MZ+DsU8Qf3JIys5M7Y7uF7wqLrBfAm0/1n9MYMDja2/+L2204ysMp/FvWrhCK8ZfXXbRF9plMBMZBhdqNomrKOEjJR1C+xkaIxNOneAfAARMTycLS3dKsNtuu3iFIh31gx+9eGZThL4LnX62mflamCIQpmC4feG+PqPcAHhK+9dPUep8vAsTcc31wfJFr3uxg32X0LpgbXLIRnX6cn+aH4V7LS5K1zCLxGH1W2ad22D5wgc/WdWbdxKJpWDsJyj3rcqtZNb/0Qk9l0iKs85q1tzhb+tGfJKBJqHvvtgsAkJuGM8PkQP35WEKLRN5s5G6uauqSXHl8zSzHcVINldR5flqgyKEoXQVcvmoD8+78HFESyTAO/hQgkMGl8hLFnP99ZIymKFRsVyy+A9K6m5uIl/TPR/PZMXp8w/5EZLyUoSxffZX7FURKZ6d0WiYuc6dGMF3Rz+rc3IXU1NPmFVvhOD7UEA/oy0bnC3vb3YZFFVUSz6ZczV3NAOqs3iHkMe9dmxeEFv6DdAeYfSbmHYojI3H09XOegja5Gcsnp0X7x2524ZiWx5rvkr8LAlVoaoGQDhw/m7NAZAiDqA9uBRnIC fLavi9k2 r91UxQFXBaROcKXnK03SBmbzlvb+bg+cQQoq4SUPt6NjO/1+IUHDSeDFpBWN8fRcMUMxcL81GjIo/2zb9zzzTc0wkVyqeKxmgSKB5rj4DQoThtQ2xtphGDRtYmTb/vrfOYi8t8t7RVI1tWIEok0gtp18a0e9yZhCYassGT6DrEfaaplat+JdECAUwyjtmVHrHYwyi1liaFcjzc7D1XR0RKlTN8HM9VSFCAP+FhxAULfUkaZfKhQ6ITJgbM3a2xsXPK+bH6HcCSFl4XaP+J2/4lJn993fVLUxfGhngZa7CcxnTs9fpDkLx/O/uJtt3pGXLLkeD16nYl3ewl4q6a3+PVocfpznB4tksmK1W5wLVZJFxUt5XInIrx4dwAaejrQNYOhmw/Ayd1vm2Eorrf3Kc0G8Lxz5h9N/KNXNBJ9evr6bO2QvuAxl8jM77zHiGg5OLMlmDqHduLWH1hrq7fxib2tcI9yJ11FutNaFIUZFxjXiOr19Er4kjvc3SbwlC7jOIsyTv9Xv6aowSPprD7d42fE8VVw== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Fri, Feb 14, 2025 at 06:14:01PM +0000, Brendan Jackman wrote: > This code is rather confusing because: > > 1. "Steal" is sometimes used to refer to the general concept of > allocating from a from a block of a fallback migratetype > (steal_suitable_fallback()) but sometimes it refers specifically to > converting a whole block's migratetype (can_steal_fallback()). Yes, that's ambiguous. > 2. can_steal_fallback() sounds as though it's answering the question "am > I functionally permitted to allocate from that other type" but in > fact it is encoding a heuristic preference. Here I don't see that nuance tbh. > 3. The same piece of data has different names in different places: > can_steal vs whole_block. This reinforces point 2 because it looks > like the different names reflect a shift in intent from "am I > allowed to steal" to "do I want to steal", but no such shift exists. > > Fix 1. by avoiding the term "steal" in ambiguous contexts. This fixes > 3. automatically since the natural name for can_steal is whole_block. I'm not a fan of whole_block because it loses the action verb. It makes sense in the context of steal_suitable_fallback(), but becomes ominous in find_suitable_fallback(). Maybe @block_claimable would be better? > Fix 2. by using "should" instead of "can", and also rename its > parameters and add some commentary to make it more explicit what they > mean. > > Signed-off-by: Brendan Jackman > --- > mm/compaction.c | 4 ++-- > mm/internal.h | 2 +- > mm/page_alloc.c | 42 ++++++++++++++++++++++-------------------- > 3 files changed, 25 insertions(+), 23 deletions(-) > > diff --git a/mm/compaction.c b/mm/compaction.c > index 12ed8425fa175c5dec50bac3dddb13499abaaa11..8dccb2e388f128dd134ec6f24c924c118c9c93bb 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -2332,7 +2332,7 @@ static enum compact_result __compact_finished(struct compact_control *cc) > ret = COMPACT_NO_SUITABLE_PAGE; > for (order = cc->order; order < NR_PAGE_ORDERS; order++) { > struct free_area *area = &cc->zone->free_area[order]; > - bool can_steal; > + bool whole_block; > > /* Job done if page is free of the right migratetype */ > if (!free_area_empty(area, migratetype)) > @@ -2349,7 +2349,7 @@ static enum compact_result __compact_finished(struct compact_control *cc) > * other migratetype buddy lists. > */ > if (find_suitable_fallback(area, order, migratetype, > - true, &can_steal) != -1) > + true, &whole_block) != -1) This one e.g. would look clearer with &block_claimable. Not that it's actually used... > @@ -1948,7 +1948,7 @@ steal_suitable_fallback(struct zone *zone, struct page *page, > if (boost_watermark(zone) && (alloc_flags & ALLOC_KSWAPD)) > set_bit(ZONE_BOOSTED_WATERMARK, &zone->flags); > > - /* We are not allowed to try stealing from the whole block */ > + /* No point in stealing from the whole block */ The original comment actually makes more sense to me. Why is there no point? It could well find enough free+alike pages to steal the block... It's just not allowed to. I will say, the current code is pretty hard to reason about: On one hand we check the block size statically in can_steal_fallback; on the other hand, we do that majority scan for compatible pages in steal_suitable_fallback(). The effective outcomes are hard to discern, and I'm honestly not convinced they're all intentional. For example, if we're allowed to steal the block because of this in can_steal_fallback(): order >= pageblock_order/2 surely, we'll always satisfy this in steal_suitable_fallback() free_pages + alike_pages >= (1 << (pageblock_order-1) on free_pages alone. And if the order is less than half a block, we're only allowed an attempt at stealing it if this is true in can_steal_fallback(): start_type == MIGRATE_RECLAIMABLE || start_type == MIGRATE_UNMOVABLE So why is the majority scan in steal_suitable_fallback() checking: if (start_type == MIGRATE_MOVABLE) alike_pages = movable_pages Here is how I read the effective rules: - We always steal the block if at least half of it is free. - If less than half is free, but more than half is compatible (free + alike), we currently do movable -> non-movable conversions. We don't do non-movable -> movable (won't get to the majority scan). This seems reasonable to me, as there seems to be little value in making a new pre-polluted movable block. - However, we do non-movable -> movable conversion if more than half is free. This is seemingly in conflict with the previous point. Then there is compaction, which currently uses only the find_suitable_fallback() subset of the rules. Namely, for kernel allocations, compaction stops as soon as there is an adequately sized fallback. Even if the allocator won't convert the block due to the majority scan. For movable requests, we'll stop if there is half a block to fall back to. I suppose that's reasonable - the old utilization vs. fragmentation debate aside... Did I miss one? We should be able to encode all this more concisely. > @@ -1995,12 +1995,14 @@ steal_suitable_fallback(struct zone *zone, struct page *page, > > /* > * Check whether there is a suitable fallback freepage with requested order. > - * If only_stealable is true, this function returns fallback_mt only if > - * we can steal other freepages all together. This would help to reduce > + * Sets *whole_block to instruct the caller whether it should convert a whole > + * pageblock to the returned migratetype. > + * If need_whole_block is true, this function returns fallback_mt only if > + * we would do this whole-block stealing. This would help to reduce > * fragmentation due to mixed migratetype pages in one pageblock. > */ > int find_suitable_fallback(struct free_area *area, unsigned int order, > - int migratetype, bool only_stealable, bool *can_steal) > + int migratetype, bool need_whole_block, bool *whole_block) > { > int i; > int fallback_mt; > @@ -2008,19 +2010,19 @@ int find_suitable_fallback(struct free_area *area, unsigned int order, > if (area->nr_free == 0) > return -1; > > - *can_steal = false; > + *whole_block = false; > for (i = 0; i < MIGRATE_PCPTYPES - 1 ; i++) { > fallback_mt = fallbacks[migratetype][i]; > if (free_area_empty(area, fallback_mt)) > continue; > > - if (can_steal_fallback(order, migratetype)) > - *can_steal = true; > + if (should_steal_whole_block(order, migratetype)) > + *whole_block = true; > > - if (!only_stealable) > + if (!need_whole_block) > return fallback_mt; > > - if (*can_steal) > + if (*whole_block) > return fallback_mt; > } This loop is quite awkward, but I think it actually gets more awkward with the new names. Consider this instead: *block_claimable = can_claim_block(order, migratetype); if (*block_claimable || !need_whole_block) return fallback_mt; Or better yet, inline that function completely. There are no other callers, and consolidating the rules into fewer places would IMO go a long way of making it easier to follow: if (order >= pageblock_order/2 || start_mt == MIGRATE_RECLAIMABLE || start_mt == MIGRATE_UNMOVABLE) *block_claimable = true; if (*block_claimable || !need_whole_block) return fallback_mt;