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 33FA7C021A9 for ; Mon, 17 Feb 2025 21:32:03 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8F09328003F; Mon, 17 Feb 2025 16:32:02 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 8C66B6B011C; Mon, 17 Feb 2025 16:32:02 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7682C28003F; Mon, 17 Feb 2025 16:32:02 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 55D966B0170 for ; Mon, 17 Feb 2025 16:32:02 -0500 (EST) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id D071D160ECE for ; Mon, 17 Feb 2025 21:32:01 +0000 (UTC) X-FDA: 83130734442.17.112CB19 Received: from mail-pj1-f53.google.com (mail-pj1-f53.google.com [209.85.216.53]) by imf13.hostedemail.com (Postfix) with ESMTP id D7DE620003 for ; Mon, 17 Feb 2025 21:31:59 +0000 (UTC) Authentication-Results: imf13.hostedemail.com; dkim=pass header.d=fromorbit-com.20230601.gappssmtp.com header.s=20230601 header.b=hC7H7+RI; spf=pass (imf13.hostedemail.com: domain of david@fromorbit.com designates 209.85.216.53 as permitted sender) smtp.mailfrom=david@fromorbit.com; dmarc=pass (policy=quarantine) header.from=fromorbit.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1739827920; 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=QhTfy1/ybvgYfqGEX8d/5gFaFMopC4YNbC7hGVBL/4c=; b=sFlwzPAhaBE38LV+cW6mKaOH0PxEH8ZBxAS7fLtRn5rt5hNA5r3Z2FmKlHPJM/ptwzRA3K Kt5Y4SZgy7z4v3n/Unkb96Si9nEQHOipu7KQw/45h9Ugk3iPNgaGr+Jl6GebzqXnhVyUQY CLhNPDuGKuWvHZOvlgunkZtPBHUJBOE= ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=pass header.d=fromorbit-com.20230601.gappssmtp.com header.s=20230601 header.b=hC7H7+RI; spf=pass (imf13.hostedemail.com: domain of david@fromorbit.com designates 209.85.216.53 as permitted sender) smtp.mailfrom=david@fromorbit.com; dmarc=pass (policy=quarantine) header.from=fromorbit.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1739827920; a=rsa-sha256; cv=none; b=sp/qB7c2Xp/5YpFCMzSUmLA7rIaRyNU+dqPi3+dgIwCo1cKa7eI96/sXrWA0NY4yYL09Xp FWGJrE8n1CUD4melSlSChmMUiJZYUuDrNipEXPpCTYOHtGWc71B41HyjI+qJNRbYkTP5op usno7qXuvZ8jUU5E0qMDz6JiiCJeHsI= Received: by mail-pj1-f53.google.com with SMTP id 98e67ed59e1d1-2fc0bd358ccso9586744a91.2 for ; Mon, 17 Feb 2025 13:31:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fromorbit-com.20230601.gappssmtp.com; s=20230601; t=1739827919; x=1740432719; 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=QhTfy1/ybvgYfqGEX8d/5gFaFMopC4YNbC7hGVBL/4c=; b=hC7H7+RIi3l4Ycx5maFKwivjxsamNaBia1fd8NfKNs0Vbm31V406xsLEgHi7+qNw/O amUFOYzUOYeJ9zl3e26NuSUxMiJt+WtohtztwGcVt10RZSBkd5Al3hnI45rWmswsmYNq R+O3e9HbNa+ogw1/ySP+lcNw9i6XanF+3M/Pjz/hNw+uixLYFJ9kTMXMzvut01Uo3uLL aNzvtSbWpsuNYEP4NpWAz2gdv9AE677CFPkLFAEBOdxWwYY5zplKvwWYAi6TGSYuPaZ6 ndOxo6DOyAdWTpGRQwIS/Ln5SBy3UydTBukINqNNhLrRyFQOTx6VZj5hurfcJNxTTW17 3+OA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1739827919; x=1740432719; 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=QhTfy1/ybvgYfqGEX8d/5gFaFMopC4YNbC7hGVBL/4c=; b=f2ZYuQfGJ4DI2n2esSQyHHKn/JvVQZxmaKBws5pZEzYo/BIDM7nlJ2sURIP7oz1gFn hQlFej+3C/c9S+hmCT+ovx6hYkQnAPeOpa8goJYt0czNpL5um8+1oz9TNHBEij74s0aj RCe7YipEMl8nhp7Tgi37VBx0I/9ivdSQ6kIzkWu00OKEZ5fgpj6U4fWdOfxUaPGeYnPF 2rywhtmCzdtwaTuN7KTStm3aE9TXmX2S5juKdJyWyZFgLpckLcNKjqohuY8gWVFqznVX PjxonMekTw/erhoGy+vu3lb8QW6xHsCUj4m9Lw8Q5y3i2NHbPQvVqTeDtLJMa2ifktmw SyYw== X-Forwarded-Encrypted: i=1; AJvYcCWAHvIpq6jLBsbItu0X+kwtwJ1D4uQMyAqK/97pcU8f3AkvuM0sn/wDUSlECqyUfqJ1o5cQVGEm4g==@kvack.org X-Gm-Message-State: AOJu0Yzf0a7F/nibC5vCqk6+kI0pJ3i0adXPWT3udgtBR+SD6kmtasz5 nKz88cQRZSkQ1YGz3pjLGmH89wV4W1pVeCJTycHGoJ6jX2ivr2S0Ho0audQwZ9s= X-Gm-Gg: ASbGncvWalb7xnRs0xYIdsnK/lXlH79TvtBJwvTUSt6L++cV0RlvuyensaZ5leXypj/ //mpcaNmyB0B2uk28NdzSoEUbzqFfrBs4oLvBaG1kPEEEvCJ5npRkTCqOLomSOekHVbCy+4ZDHJ +QDkJKXXC7AtDGltwcDs8k5KcX8i1m1nFQgWKF28477aDJ+smxLnXJlPbEDEsDokXyt6UPSeSkc jB8VwLe8inMBNY7Hj16mB+eFcWtDF4oC+r4q/8Vv4/wS4b8gpJK8O0zK4HDuw/lFLFbyTPomEfb OZmgSv50E4ErPa33qSeWMl+xwDoLCdC6nWo4QveT9J/oIOuwDVA9uQY53ooogGCCxS8= X-Google-Smtp-Source: AGHT+IGWbBJAtL2UtKSWCS6Jd9Ns/0Md8Y1s79IB++xb28sgasLU0+0/qaH46F3zNCw25wym7QqlJQ== X-Received: by 2002:a17:90b:3b4b:b0:2ee:fdf3:390d with SMTP id 98e67ed59e1d1-2fc4115087fmr15333822a91.31.1739827918683; Mon, 17 Feb 2025 13:31:58 -0800 (PST) Received: from dread.disaster.area (pa49-186-89-135.pa.vic.optusnet.com.au. [49.186.89.135]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-220d53491d4sm76091525ad.4.2025.02.17.13.31.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 17 Feb 2025 13:31:58 -0800 (PST) Received: from dave by dread.disaster.area with local (Exim 4.98) (envelope-from ) id 1tk8iZ-00000002XxN-2C1Y; Tue, 18 Feb 2025 08:31:55 +1100 Date: Tue, 18 Feb 2025 08:31:55 +1100 From: Dave Chinner To: Yunsheng Lin Cc: Yishai Hadas , Jason Gunthorpe , Shameer Kolothum , Kevin Tian , Alex Williamson , Chris Mason , Josef Bacik , David Sterba , Gao Xiang , Chao Yu , Yue Hu , Jeffle Xu , Sandeep Dhavale , Carlos Maiolino , "Darrick J. Wong" , Andrew Morton , Jesper Dangaard Brouer , Ilias Apalodimas , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Simon Horman , Trond Myklebust , Anna Schumaker , Chuck Lever , Jeff Layton , Neil Brown , Olga Kornievskaia , Dai Ngo , Tom Talpey , Luiz Capitulino , Mel Gorman , kvm@vger.kernel.org, virtualization@lists.linux.dev, linux-kernel@vger.kernel.org, linux-btrfs@vger.kernel.org, linux-erofs@lists.ozlabs.org, linux-xfs@vger.kernel.org, linux-mm@kvack.org, netdev@vger.kernel.org, linux-nfs@vger.kernel.org Subject: Re: [RFC] mm: alloc_pages_bulk: remove assumption of populating only NULL elements Message-ID: References: <20250217123127.3674033-1-linyunsheng@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250217123127.3674033-1-linyunsheng@huawei.com> X-Rspam-User: X-Rspamd-Server: rspam11 X-Rspamd-Queue-Id: D7DE620003 X-Stat-Signature: sbq6j58ucmeqygjt9cgijm5wogf443a1 X-HE-Tag: 1739827919-460924 X-HE-Meta: U2FsdGVkX1+Dcad1gyYXkj+RpC8LtWTqIrKNIFgfYE2+KyTkMIaA3yReLquwr2ZmgOzLr0svMJaxBA9RjJpm5xDT4kjT1lFBQsYRdlX/inFe0K9BNulR8yMvTJjyGQyVD+i+iI5B1FtvZyheRG/U7z4A9pqPA1XpaDq07nXH5iihYsJgNx1SoahgNIbnxfiWZj6haDff7Z9G/X/pfRGFsNyulgeI2pdObBlF9Ur/CVqIymbeeNB9DbGUMmngev+vyH1NZ2vwSBCSJQ21MTSwuqwhlW21mBiLwcO3fLgy/OPB3HNHJvgYfJj847wwLKPvCBLuUVD4Skj7srYbC0iiXvxbAgNf8xdBv0KrxNwlDSak9PO0532JPAWgTkos/4XpEng5XAxPl/dynA43g8X+OsPsklRvSkSgJUXg/5uWyDdbt8b4mjn2x/WkUjwMqIqwwu2MwViWdRZir0jj3wPIVl2trJwVGW7IxLLzAb4qnK2cB/js/SPWVW9RJW1FNDrewQfesF6pTnPxD3VU+Q/mJWn92+j2mxnuKTPuy+pYRkQyc1zGcYbBMdKPRTGWF5LMILwEkBhXaZo2ApwCrl3BGu3MSEg3l5LBXo2CmIMWC4vObsLGu6w7/u5sERueq3Gkv7WSzgRivqGOGhTpFPdGyM14Y1gevfEsiMPCeRLXDPrqHjGinXfW4eEJ5D3KsUAu0G6dl4NFrNEDeY0WClkqkpJ7s7raCBic6DKdRN/8wUT7TcqDAv04RLpNW7EPAk7dj3A/kPws+a5Z/BEC6kncYUzXer2Jm8C73emJqD48+bpNt4VpyAOj6rIgIh9kcN5EY5IiqmBuObwTLe3vx0vVo/thVxnPzzZkDfm/A+aR25ctE+bFMK4C+yZjyrjZ1ewDDxNOlXWMqxl72QAz+s+CxKl9IC8GeXsEnSkSsEaj5+emMGOl/7/a3laK9ovlRJm7Jcuz+WMoXT9BVgZENu/ X6j7xBUp m7yVYq5Nt9/xDMP6ZlXob4EERDlHblhov6YQ9WvwC50R0FNe+2TOnAGHyVafyOjqfPZ86shqwMFDpl6nngp4jKjEAS3vWz1X27PdXvfkmjobhd7QgTGwXdn2Pkh/NX8O7QBxyoL520oCfF37H0k0Gv/lEQUAGJPwu5BkBAS8p4hZQtwLZbhBTz6HW3g/5nXnr2ogrf3oij8UufuLgj4siO+gE5udG0hf8AGxcBGNgktmIcl7N8Qpdb1I+9jzgAzMu9+QJMxtoCs97pAYPiCBTfcwhTkRDuEqCgez0EwYu77s7GXTwqtlb3fogQ8fcg8/+X2lTa7cKxX8SovlgZNknqNdLKrOGdbCj+WIClJn5FFw59S5HMMxbI6D7rWoQVIKs/v/xQOjxArvLVouvLniEZzXw+FM4nW9kG5QszlbNKc/E3WPVNgoU5GfecdLncvSAxLtXfKem+P6+zvC6A+la+JWCIA== 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 Mon, Feb 17, 2025 at 08:31:23PM +0800, Yunsheng Lin wrote: > As mentioned in [1], it seems odd to check NULL elements in > the middle of page bulk allocating, and it seems caller can > do a better job of bulk allocating pages into a whole array > sequentially without checking NULL elements first before > doing the page bulk allocation. .... IMO, the new API is a poor one, and you've demonstrated it clearly in this patch. ..... > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index 15bb790359f8..9e1ce0ab9c35 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -377,16 +377,17 @@ xfs_buf_alloc_pages( > * least one extra page. > */ > for (;;) { > - long last = filled; > + long alloc; > > - filled = alloc_pages_bulk(gfp_mask, bp->b_page_count, > - bp->b_pages); > + alloc = alloc_pages_bulk(gfp_mask, bp->b_page_count - refill, > + bp->b_pages + refill); > + refill += alloc; > if (filled == bp->b_page_count) { > XFS_STATS_INC(bp->b_mount, xb_page_found); > break; > } > > - if (filled != last) > + if (alloc) > continue; You didn't even compile this code - refill is not defined anywhere. Even if it did complile, you clearly didn't test it. The logic is broken (what updates filled?) and will result in the first allocation attempt succeeding and then falling into an endless retry loop. i.e. you stepped on the API landmine of your own creation where it is impossible to tell the difference between alloc_pages_bulk() returning "memory allocation failed, you need to retry" and it returning "array is full, nothing more to allocate". Both these cases now return 0. The existing code returns nr_populated in both cases, so it doesn't matter why alloc_pages_bulk() returns with nr_populated != full, it is very clear that we still need to allocate more memory to fill it. The whole point of the existing API is to prevent callers from making stupid, hard to spot logic mistakes like this. Forcing callers to track both empty slots and how full the array is itself, whilst also constraining where in the array empty slots can occur greatly reduces both the safety and functionality that alloc_pages_bulk() provides. Anyone that has code that wants to steal a random page from the array and then refill it now has a heap more complex code to add to their allocator wrapper. IOWs, you just demonstrated why the existing API is more desirable than a highly constrained, slightly faster API that requires callers to get every detail right. i.e. it's hard to get it wrong with the existing API, yet it's so easy to make mistakes with the proposed API that the patch proposing the change has serious bugs in it. -Dave. -- Dave Chinner david@fromorbit.com