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 26573C3ABAA for ; Mon, 5 May 2025 16:54:14 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3A5E66B0085; Mon, 5 May 2025 12:54:11 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 32D166B0089; Mon, 5 May 2025 12:54:11 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1CD356B008A; Mon, 5 May 2025 12:54:11 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id EEE386B0085 for ; Mon, 5 May 2025 12:54:10 -0400 (EDT) Received: from smtpin22.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id E14621603A8 for ; Mon, 5 May 2025 16:54:12 +0000 (UTC) X-FDA: 83409451944.22.2149D60 Received: from mail-qt1-f174.google.com (mail-qt1-f174.google.com [209.85.160.174]) by imf17.hostedemail.com (Postfix) with ESMTP id 15D6E40007 for ; Mon, 5 May 2025 16:54:10 +0000 (UTC) Authentication-Results: imf17.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=PzuGcDTS; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf17.hostedemail.com: domain of surenb@google.com designates 209.85.160.174 as permitted sender) smtp.mailfrom=surenb@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1746464051; a=rsa-sha256; cv=none; b=ubaL5J2WLJVSKHAqYHj3KbsURtk7uo993lD7yr3LkIG22UxXeWxc79MAii9joG6zrU4H8r CDrSJTdR1mPGxpgDml3NJZNSWUHg9hFhYlvqZAJce5seQYKiTQHviydgQ7N0QIoo68HBgx 4cJ/Q16rhxeXl06yaPlbnBRVPE67eeY= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1746464051; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=BvkwbBAAy51gwoOy2GdGqciR4ki2e9eLZIkUV7w/43I=; b=qhqpb8ttq+b5z1S4zuiYJvqG+tvfZ2Ugm7uJUrlDeDVuRoX2bVLS69XvgxcC467LhjfE0x ohdkWPzz4VU8BVfqFgTXFG94t01o4Xks/yl+2q2VO0Fl0UjLWB/jHlTS9tUGc2koYkOmm6 Ln+OAcEjuaNxjyg6jAxS22PnifaLgQk= ARC-Authentication-Results: i=1; imf17.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=PzuGcDTS; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf17.hostedemail.com: domain of surenb@google.com designates 209.85.160.174 as permitted sender) smtp.mailfrom=surenb@google.com Received: by mail-qt1-f174.google.com with SMTP id d75a77b69052e-48b7747f881so4011cf.1 for ; Mon, 05 May 2025 09:54:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1746464050; x=1747068850; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=BvkwbBAAy51gwoOy2GdGqciR4ki2e9eLZIkUV7w/43I=; b=PzuGcDTSPUGdR87bz00k9XSJxFCg4tvQJNAXaOpuq6OoI6tdoU9dpnX4D4895hUr8s 5xTO1aQOnAqweIvlk+lAHq79flZhFf+coBehceXeb0gJ8TAQA6Yb2gUSX5bPgeVl9OOy PzAoM1esQUwhie5IRyVU8OJukB8P8Zn7pSsGZWNoDfJ4w+nXofXuq3lLYiHI98tuBFvX Pn7loa7YRUScp5gu1X0qBtba5raun/Yo/JF76w3rghxGU9Ro9t7iWg78Oe8jLBzx3aSN lC0aX6dv+geHusQ7ZhFOJbRyd+fA48taJSJUWLjayO3OAT8TqoXtomrWKevi1DqflRuW qPLA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1746464050; x=1747068850; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=BvkwbBAAy51gwoOy2GdGqciR4ki2e9eLZIkUV7w/43I=; b=fno5ZAYIh3dg7mz/IZmslhFUKznQRDHNorWNB8bM3dN/A7jIWYl7HyJNELbKqorN+k gB8pGCKu01mQC8vWrp/nIcKO++SGkWBm38BJKn2eBHLdFgcwh9Ad67vP2otuJ1ZkIChl SLYVemAY3Zik3+hmcC62IzHqK1Ss9LXw1fDWsZdicT/5In55V9pPX9ViFseZqiYgAkqQ we9f2caEHYEWVBcz8WCNvuRfppbRVN1WOKRkacbgtnB+93iGGJ1ZelM8G+7Pb0T2s5Wh M/EQVDDx6+vtoK1K+nmY/jLHJw9FUrh8cl5AxRzzUBAuEa9DC+x2IgfBhzjsiuiKl1v+ saWw== X-Forwarded-Encrypted: i=1; AJvYcCXW1OItVQCwiZIX3Jrl5K8/rQbZJyKyUZh3s6sG72vv6KvUB7qzqV9DQU9AG0pKS766wvY6ZgtBrw==@kvack.org X-Gm-Message-State: AOJu0YwMM+QuSnknsvr3lmJFinWPXDIs9+c0DEQkx3i5rFFAcWFPKHk1 RCNFP1CV+yMRjBCvhN6WrKvcpj/ssmv/RgMrzvGyi3mJooE4imzj9TtzxFd8bfe1zC4DkIIhZ6H MKQm59O0KfIc5LLwqxnM9WF3JH+jxQiUxAMAB X-Gm-Gg: ASbGncuGrJABINvAaoZHwdud3XvRpOP95nz4+T9JGmMhGR0d/D9wbaxTzqw11OAuwwG MGmQqlu45YhnyxNMW3FLidnpnZ4zzQS/hrTiy3qr0vPIgGOJ49ss7xihGkvdCn2gHHf65sOnb+l y3j9e+0KHVzzFBAINjKeRK X-Google-Smtp-Source: AGHT+IFYImIoddN52fXijvNt5y6xVdVxVQ9f+9nrzOlBgiGz/IPyWvzu/ePbKaCOYpESVtCd6w3BF/y++SOQRaJqoG4= X-Received: by 2002:a05:622a:1aa2:b0:479:1958:d81a with SMTP id d75a77b69052e-490cbca1fdfmr634351cf.6.1746464049875; Mon, 05 May 2025 09:54:09 -0700 (PDT) MIME-Version: 1.0 References: <20250504061923.66914-1-00107082@163.com> <8edbd2be-d495-4bfc-a9f3-6eaae7a66d91@suse.cz> <1da43908.3afc.196a0db7dc3.Coremail.00107082@163.com> <0feb4309-431f-4b74-83bf-e16198798c30@suse.cz> <3be93014.40dc.196a153f521.Coremail.00107082@163.com> In-Reply-To: <3be93014.40dc.196a153f521.Coremail.00107082@163.com> From: Suren Baghdasaryan Date: Mon, 5 May 2025 09:53:58 -0700 X-Gm-Features: ATxdqUHI1cER8ebejXFC0ZlXWgWhayyDJtXaCo0vHjoz4cOy8TogE8Lp7Fok_sE Message-ID: Subject: Re: [PATCH] mm/codetag: sub in advance when free non-compound high order pages To: David Wang <00107082@163.com> Cc: Vlastimil Babka , akpm@linux-foundation.org, mhocko@suse.com, jackmanb@google.com, hannes@cmpxchg.org, ziy@nvidia.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org, Shakeel Butt Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 15D6E40007 X-Stat-Signature: 9dcryadrk1bumo1e1q3egtajqrk7qy3c X-Rspam-User: X-HE-Tag: 1746464050-44911 X-HE-Meta: U2FsdGVkX19ZY72bEIv6Od1fWvcuyklfKipRtVLMJ6c3BMb5aOfTVKCFy/A1+R+l2rI5UykjLS297+tngaMrv8oMIBfZH66BkJzKTIL1Y6VCaLvNlZ0OIRM72f07hBSjaa/6VYsBHfB0f6l3kAdRHoV/ikYH+RyqZJ/yypOBttReezex7X/eQ5YvG/PcfYH5vLCd/bpXn7nKvSz8frrTIZh9Kp8k7iJsFX/Is8eQlbqNZPAel0+o7j2mPesIThSzhhlR2Qq4YtJAxeFwvqXi6YXA8Rv2XjdkUGHzKoBsZAT7iQ5gze0eGFAuai04ynjpYm0h71UO3Wo6Ff49AYIQlxAWhYNd7Etec+tHEQzWqnXemVi/OtkTW1CJOJoYCC3+Whc2lgVQutoSX5zdu38pmUwmw1GISJljlxBKro6GXGK4qu6zg+vD6+ZDfLCQkXpB70GIM/nT/63/8hvfd9Ditm1tX9yxQc1tZTQBD1KEmKKjrPf2PZKcRS9s/9zKHJUYjKGAIZcOySUpVBvV95UzMyUKKInE6mYvKKboOMe9PuI8WRf2wFWSNP4AOkIqnFhjNNMzW58iPVDxc9iaQA0VJ6QVnJ7XyAbUfkeOoduQ9VhD6aAgbIJjZymfO8bTq7edPQ+c+EA97qSNv7SxXqk9IxiAXLB6UdSRwGETqOg/4qEblvjyNat2yi5c2qXRU5qv9s6ijYAbZU858W179gyfIAfwHTQW1WFfINFPFNDx3UWsJki/j+Qfq9lmY9MIwkw8WHD7t12rdT/xTULDBGcJnXOobZc1efJky7Y0Dcy3azeOrQNxAeIvqxdpYRBGhXfQHLijh1Ewpv+8yiqK0BokvUiRum7czAApgYRxZh7Hga+HgBJvV/pl3UH5EM1pKAZSzvPBi3WcLSq9smSd+6pRbrkezbiWqUg6QK5ORl/Balx/2gL2MMsW2orGYZ226cq8esSaR4UoYX8noD5TIEE ZlGvzgYW l33vPtKZN1D46C63ucq193klplW0zsVIRJja5e2ppgBEa6VGGUPUq+4rVxy4R6FGWPl+F6qtK2zoMHDmOXa3pBWoRqP5AcZK6LYF/VEozBJrsI9BVEoR9rPGBfSFxk6sYURwOldrwB1jmWFGh3K+13FDzH1OccuZljbcRdduW0WABBze64RrYkvkpWpUxHkNRfrVDtTYPUVpWvHi901Tx5VWYmma4WjUjztg6cHh8/JkCSiwICFg+cTo96BOAQi5l+67xRUHHKZzZAytP6MZ4YTAxMNMPIVD5p9UIA20lre1AlPCbfhjt02xOLyaa7VJ0dltFIX2DlWWCD+E= 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, May 5, 2025 at 9:43=E2=80=AFAM David Wang <00107082@163.com> wrote: > > > > > At 2025-05-05 23:33:50, "Suren Baghdasaryan" wrote: > >On Mon, May 5, 2025 at 7:55=E2=80=AFAM Vlastimil Babka = wrote: > >> > >> On 5/5/25 16:31, David Wang wrote: > >> > > >> > > >> > At 2025-05-05 21:12:55, "Vlastimil Babka" wrote: > >> >>On 5/4/25 08:19, David Wang wrote: > >> >>> When page is non-compound, page[0] could be released by other > >> >>> thread right after put_page_testzero failed in current thread, > >> >>> pgalloc_tag_sub_pages afterwards would manipulate an invalid > >> >>> page for accounting remaining pages: > >> >>> > >> >>> [timeline] [thread1] [thread2] > >> >>> | alloc_page non-compound > >> >>> V > >> >>> | get_page, rf counter in= c > >> >>> V > >> >>> | in ___free_pages > >> >>> | put_page_testzero fails > >> >>> V > >> >>> | put_page, page released > >> >>> V > >> >>> | in ___free_pages, > >> >>> | pgalloc_tag_sub_pages > >> >>> | manipulate an invalid page > >> >>> V > >> >>> V > >> >>> > >> >>> Move the tag page accounting ahead, and only account remaining pag= es > >> >>> for non-compound pages with non-zero order. > >> >>> > >> >>> Signed-off-by: David Wang <00107082@163.com> > > > >Thanks for reporting! > > > >> >> > >> >>Hmm, I think the problem was introduced by 51ff4d7486f0 ("mm: avoid = extra > >> >>mem_alloc_profiling_enabled() checks"). Previously we'd get the tag = pointer > >> >>upfront and avoid the page use-after-free. > > > >Right, sorry I missed that. > > > >> > > >> > > >> > Oh, you're right. I forgot to check history...... > >> > > >> > > >> >> > >> >>It would likely be nicer to fix it by going back to that approach fo= r > >> >>___free_pages(), while hopefully keeping the optimisations of 51ff4d= 7486f0 > >> >>for the other call sites where it applies? > >> > > >> > After checking that commit, I kind of feels the changes in __free_pa= ges are > >> > the major optimization of the commit.... > >> > >> We could have both pgalloc_tag_get() to use in __free_page() as before > >> 51ff4d7486f0, and keep __pgalloc_tag_get() to use in pgalloc_tag_split= () and > >> pgalloc_tag_swap(). > > > >Yes, we can add back pgalloc_tag_get() which would call > >__pgalloc_tag_get() if mem_alloc_profiling_enabled() is true and > >change pgalloc_tag_sub_pages() back to use tags instead of pages. > >__free_pages() is the only user of that function, so that change > >should not affect anything else. > > > Adding back pgalloc_tag_get() seems just reverting 51ff4d7486f0..... Not quite. pgalloc_tag_split() and pgalloc_tag_swap() will still be using __pgalloc_tag_get(), so would avoid the extra checks. > Do you want me to do it or you take over and make further adjustments? The change I suggested should be simple, so take a stab at it and I'll review and ack. If you prefer me to make the change, let me know. > > > > > > >> > >> I think __free_page() didn't benefit from the stated purpose of "avoid= ing > >> mem_alloc_profiling_enabled() ... which is often called after that che= ck was > >> already done" > >> > >> > What about revert that commit and make optimization by condition che= cks, > >> > similar to what this patch did? > >> > >> The downside of the condition checks is they make the code more comple= x and > >> might actually increase overhead when mem_alloc_profiling_enabled() is > >> false, as those checks add non-static branches outside of the static b= ranch > >> that's mem_alloc_profiling_enabled(). > >> > >> I think __free_pages() before 51ff4d7486f0 was quite ok. > >> > >> - pgalloc_tag_get() is done unconditionally, but its code is all insid= e the > >> mem_alloc_profiling_enabled() static branch so that's a no-op when pro= filing > >> is not enabled > >> > >> - pgalloc_tag_sub_pages() is also all behind the static branch inside.= Also > >> it's a very rare path anyway, most freeing should go through the > >> put_page_testzero() being true. > > > >Yeah, the main goal of that change in __free_page() was to make > >__pgalloc_tag_get() a local function for alloc_tags and limiting the > >direct use of struct alloc_tag in the core mm code. Obviously I > >screwed up forgetting why we had to store the tag in the first place. > >An additional comment in __free_page() is probably a good idea to > >avoid confusion in the future. > >Thanks, > >Suren. > > > >> > >> > David > >> > > >>