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 68EA3C4345F for ; Sat, 13 Apr 2024 00:51:13 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D59C96B00A6; Fri, 12 Apr 2024 20:51:12 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D09276B00A7; Fri, 12 Apr 2024 20:51:12 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BA91F6B00A8; Fri, 12 Apr 2024 20:51:12 -0400 (EDT) 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 9CFB96B00A6 for ; Fri, 12 Apr 2024 20:51:12 -0400 (EDT) Received: from smtpin15.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 304E2A105F for ; Sat, 13 Apr 2024 00:51:12 +0000 (UTC) X-FDA: 82002679584.15.73B043A Received: from mail-ed1-f54.google.com (mail-ed1-f54.google.com [209.85.208.54]) by imf02.hostedemail.com (Postfix) with ESMTP id 585EE80002 for ; Sat, 13 Apr 2024 00:51:10 +0000 (UTC) Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=R3J+PCQp; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf02.hostedemail.com: domain of shy828301@gmail.com designates 209.85.208.54 as permitted sender) smtp.mailfrom=shy828301@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1712969470; 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=kbYS3aC9uGvkjYk6/cLeqdOFTEci2uZlTd3WzZKqoXw=; b=I84adUH1slZFdwF3V0Wj5BeSjvPpfjhmIRaXDqX6bq3/v1ys/QW9NneWDoUh4lZP2rGXuX NwWfX9MaAG6IJgDiTw3bsvCzuDWZFCHRmVuyZvNZ8tsJZ8KEasxOBs2Cv5Fc84zGzFMVgM rT5oqtBWi9Jng6xsd4axzacQckPbhjk= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=R3J+PCQp; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf02.hostedemail.com: domain of shy828301@gmail.com designates 209.85.208.54 as permitted sender) smtp.mailfrom=shy828301@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1712969470; a=rsa-sha256; cv=none; b=GbcEG61dAlEtJVrt4KvtYq3S/+CLS2bFMx3QrVNOElt3neppOi/BsSTOLbNC31gCqhLsKm 19ZCLclrYmVVOTXgV5txPD1FbnnHxL/+Z1v0y33LRmcKCZdYIh7fy0PMTdP5bGyk205jB2 UsM9acU2yAALMSOrttG1FPvBrtXws1M= Received: by mail-ed1-f54.google.com with SMTP id 4fb4d7f45d1cf-55a179f5fa1so1491580a12.0 for ; Fri, 12 Apr 2024 17:51:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1712969469; x=1713574269; 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=kbYS3aC9uGvkjYk6/cLeqdOFTEci2uZlTd3WzZKqoXw=; b=R3J+PCQpVr0oRx9hwJYZ+UwbGYvg+ofE8zAbW3/INl8UNpeZfJ6EPEwZpKNAcCQN2q sLclfFhD8LOx0x/DSAuVg3JVEwG5trgTbFMnz4tp2sRoHmHfG/jRh8JYMOB+gmlKmXQ2 LD+sS6NSLSnoBojKIKaUu6cyU2Cyf4NahR4OumSSsjkbWLEUZaj3Lwmhze/xzpFGfgmU aBEmATd5fxLAw/wvAdYaK9YWrb9aqWxgCNezFkJtnVK8+Pn9oAFV3ijLucQjlJ79Sh3l FMb9hp/DXQf1hPZr0dbzhAOm20wfSaSIatPGZG0NW/ypb3zzqD/2RWThjElRuzps0heC J4wQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712969469; x=1713574269; 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=kbYS3aC9uGvkjYk6/cLeqdOFTEci2uZlTd3WzZKqoXw=; b=NeVTBEXB/CLFxdKCsVwAG1IhEvuadCPYHDBCoVKF6OmlzjL2PQ+5DhyZhC9T3rZHyB HoJW1T3xjYnBk4wLU/G2SDO8ltUB1PnYOet65CTDsMvSgd80I2B1uhB5HOa0mBgwnECO KjBXHWrkQMVz1edlv53HAt4gPb0ZxennFYrIiWUpxayk40sVCCSd+BjQ1ZuWz9kSswE0 2W0SvuKfcHyieAFbHv5AQlH8z7QJGpibYqRRCGuHn+p+y65LpIlpFsnWnwna54UyCPpO I90on5Tq93id8hyylQCTkKeYFooedDUbopdMmUTUtp7adlvv6LOCE6NwfhXdlAT62oDZ tLbQ== X-Forwarded-Encrypted: i=1; AJvYcCU7F35P5fg6ii7MvtWCRTqK2EWiRTtF80S9lWwg57Jkxur61ENaMFFyjFycj0Q9MppPqCzjLbgIkB3Y8cagcvUA22M= X-Gm-Message-State: AOJu0Yymf8hqo7XsvVjUlsi0JLzqEvpJATJec+yvuWV/fB/BWrIkdqBl pCJuwacLEbGM74TxmMNRPcuTezlbVlc2DKYn95ryQvcUJYXG/plXBNhf0eBbgSXoresHooy6shR w7/Ru5ynklbA0SrJrCSFlNHxr5Z4= X-Google-Smtp-Source: AGHT+IFK+kfekIP4WtA4fPXnB2LXRkQRlXC+1IOk8rSIah8aqbpb9YgJ0njZnDg9FhY2WCvVs8umWiOrLp+Hvq6ZvMM= X-Received: by 2002:a17:907:972a:b0:a51:827d:c99b with SMTP id jg42-20020a170907972a00b00a51827dc99bmr3143961ejc.14.1712969468567; Fri, 12 Apr 2024 17:51:08 -0700 (PDT) MIME-Version: 1.0 References: <20240411153232.169560-1-zi.yan@sent.com> <2C698A64-268C-4E43-9EDE-6238B656A391@nvidia.com> <2BE605BB-F474-48E3-A54F-1E9371BF59E5@nvidia.com> In-Reply-To: <2BE605BB-F474-48E3-A54F-1E9371BF59E5@nvidia.com> From: Yang Shi Date: Fri, 12 Apr 2024 17:50:56 -0700 Message-ID: Subject: Re: [PATCH] mm/rmap: do not add fully unmapped large folio to deferred split list To: Zi Yan Cc: David Hildenbrand , linux-mm@kvack.org, Andrew Morton , Matthew Wilcox , Ryan Roberts , Barry Song <21cnbao@gmail.com>, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 585EE80002 X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: hdwa54ar1g7gnt6qrg9qsw41wf6hi5jg X-HE-Tag: 1712969470-444679 X-HE-Meta: U2FsdGVkX1/RoUvaxzePAOpoqxUlkq9+3tyf+2wser56VEzrGRA+LIcxyRnZM4v7XDgbflLT99zA5egYy8FY9dMH7TYixxUCc9Iyob9vn3et9gKeTSTRoRow0ZgyNUPwJX9D+VJuWjmBSfh8Mpm7pckT5hY+3Ai+NznGWF9D9r2g0Ne9tx80SGYMrHIb/QSn+nDaN6FvYe2We/B3hT/neXq7XmdqucUpvzsxR8lf1dis1B6nAWpb0Ur718KskuBiL2rx9oInGR5M0mBauhm4SkIQcwXrP/uD6iyV8CXfx9mfBoY1dy3Ud0Ni7vo8sdYd4Ov4QEL2+wxssiN38NE/NoXy2v6bOHBGeujiu7/zrh9YZAD/4rK1b9LvRYEeOHohNSL4oN1X1CSkbT2PSTCgu8x8NveE3sGsYaMWhbBFxI33R1IGTPIPW57BDozxO4rEAuoumb9U6Z/1RX4vtJOTkLRQ9xyPtQ/EzlctB9gWrPg16B8dtpM57VzIaWNSpygxIkFxZV4OEDvT7xR9yieTS0IQQns3iv08JEBaUC6yQqTxKNA2f20chT47NSy9ovdWjCH7N2Q2MZoraBqKGdVspD7mRzQDLR+C6gDSwxdwxm8sSNXH9aT3iH2mYdoEObX0AnCOMHx0s+VwQXq/FUO0Hg+F8ZAoQVrAIGDjCzCaAxp6gW5yVX/WJBsc7F4B44ZXBMaF+udnN1xucWh2bu11KhTelCrMTOWUwg9VSvseIdSh7IipZLtS+r2IOBrlgOzbA+H5BqxfXroDRHRrWV7CThLD9RwDT09lXmrrmo98tE/qpn1/aFWqcFvNrMilDYeZ1xilDj6BBLjt3ZZgeZ4+A+nd1gbCR9vbR0qSF5O6of4XqBdTdWd5Lr7daY1agxMgvtcSK8CYlrSpXw0AdKYSN2eMB0nntVp1hn+BtZoCL7ElONGqgtHjnIrwEq2VC58u6UE4V6V7IEf/08x2pnO t0GtkaX4 gJ7WetWYHYTvruyvYSaC3o7FoSBu2Rc0q7PPkc7G7nnoUgpCZBSMGk9e/yt4DkCu42JVRjFg4naaehsy9D9XeTlubJOUSbi5tIqMDxxW3VrKSHSmpYIDymPdxmyqRDVGGZIcPjvuwIazOtorlygGLVUJx710NCMyplfVznFDSgXbRH6A1bUcexYNgGvdhJqy171aZbkuXoJmjDqgyoieuGh10O9uh7h5T2LeRiIwih8Sgn2RHBNF4EslhWfFy0iRCXNYVsgsUSQTBX/iAiU5pQ4kueBqEnDCQ4cTIAXXnHcJELzYAaU/nYi6slfaJ5RzpKACrkoRTDsB39WUuw4GrnRIRwHPw9uWQOliH72I6fR5B5pbqwAiKBJVqqA== 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, Apr 12, 2024 at 3:59=E2=80=AFPM Zi Yan wrote: > > On 12 Apr 2024, at 18:29, Yang Shi wrote: > > > On Fri, Apr 12, 2024 at 2:06=E2=80=AFPM Zi Yan wrote: > >> > >> On 12 Apr 2024, at 15:32, David Hildenbrand wrote: > >> > >>> On 12.04.24 16:35, Zi Yan wrote: > >>>> On 11 Apr 2024, at 11:46, David Hildenbrand wrote: > >>>> > >>>>> On 11.04.24 17:32, Zi Yan wrote: > >>>>>> From: Zi Yan > >>>>>> > >>>>>> In __folio_remove_rmap(), a large folio is added to deferred split= list > >>>>>> if any page in a folio loses its final mapping. It is possible tha= t > >>>>>> the folio is unmapped fully, but it is unnecessary to add the foli= o > >>>>>> to deferred split list at all. Fix it by checking folio mapcount b= efore > >>>>>> adding a folio to deferred split list. > >>>>>> > >>>>>> Signed-off-by: Zi Yan > >>>>>> --- > >>>>>> mm/rmap.c | 9 ++++++--- > >>>>>> 1 file changed, 6 insertions(+), 3 deletions(-) > >>>>>> > >>>>>> diff --git a/mm/rmap.c b/mm/rmap.c > >>>>>> index 2608c40dffad..d599a772e282 100644 > >>>>>> --- a/mm/rmap.c > >>>>>> +++ b/mm/rmap.c > >>>>>> @@ -1494,7 +1494,7 @@ static __always_inline void __folio_remove_r= map(struct folio *folio, > >>>>>> enum rmap_level level) > >>>>>> { > >>>>>> atomic_t *mapped =3D &folio->_nr_pages_mapped; > >>>>>> - int last, nr =3D 0, nr_pmdmapped =3D 0; > >>>>>> + int last, nr =3D 0, nr_pmdmapped =3D 0, mapcount =3D 0; > >>>>>> enum node_stat_item idx; > >>>>>> __folio_rmap_sanity_checks(folio, page, nr_pages, level= ); > >>>>>> @@ -1506,7 +1506,8 @@ static __always_inline void __folio_remove_r= map(struct folio *folio, > >>>>>> break; > >>>>>> } > >>>>>> - atomic_sub(nr_pages, &folio->_large_mapcount); > >>>>>> + mapcount =3D atomic_sub_return(nr_pages, > >>>>>> + &folio->_large_mapcount) += 1; > >>>>> > >>>>> That becomes a new memory barrier on some archs. Rather just re-rea= d it below. Re-reading should be fine here. > >>>> > >>>> Would atomic_sub_return_relaxed() work? Originally I was using atomi= c_read(mapped) > >>>> below, but to save an atomic op, I chose to read mapcount here. > >>> > >>> Some points: > >>> > >>> (1) I suggest reading about atomic get/set vs. atomic RMW vs. atomic > >>> RMW that return a value -- and how they interact with memory barriers= . > >>> Further, how relaxed variants are only optimized on some architecture= s. > >>> > >>> atomic_read() is usually READ_ONCE(), which is just an "ordinary" mem= ory > >>> access that should not be refetched. Usually cheaper than most other = stuff > >>> that involves atomics. > >> > >> I should have checked the actual implementation instead of being foole= d > >> by the name. Will read about it. Thanks. > >> > >>> > >>> (2) We can either use folio_large_mapcount() =3D=3D 0 or !atomic_read= (mapped) > >>> to figure out if the folio is now completely unmapped. > >>> > >>> (3) There is one fundamental issue: if we are not batch-unmapping the= whole > >>> thing, we will still add the folios to the deferred split queue. Migr= ation > >>> would still do that, or if there are multiple VMAs covering a folio. > >>> > >>> (4) We should really avoid making common operations slower only to ma= ke > >>> some unreliable stats less unreliable. > >>> > >>> > >>> We should likely do something like the following, which might even be= a bit > >>> faster in some cases because we avoid a function call in case we unma= p > >>> individual PTEs by checking _deferred_list ahead of time > >>> > >>> diff --git a/mm/rmap.c b/mm/rmap.c > >>> index 2608c40dffad..356598b3dc3c 100644 > >>> --- a/mm/rmap.c > >>> +++ b/mm/rmap.c > >>> @@ -1553,9 +1553,11 @@ static __always_inline void __folio_remove_rma= p(struct folio *folio, > >>> * page of the folio is unmapped and at least one pag= e > >>> * is still mapped. > >>> */ > >>> - if (folio_test_large(folio) && folio_test_anon(folio)= ) > >>> - if (level =3D=3D RMAP_LEVEL_PTE || nr < nr_pm= dmapped) > >>> - deferred_split_folio(folio); > >>> + if (folio_test_large(folio) && folio_test_anon(folio)= && > >>> + (level =3D=3D RMAP_LEVEL_PTE || nr < nr_pmdmapped= ) && > >>> + atomic_read(mapped) && > >>> + data_race(list_empty(&folio->_deferred_list))) > >> > >> data_race() might not be needed, as Ryan pointed out[1] > >> > >>> + deferred_split_folio(folio); > >>> } > >>> > >>> I also thought about handling the scenario where we unmap the whole > >>> think in smaller chunks. We could detect "!atomic_read(mapped)" and > >>> detect that it is on the deferred split list, and simply remove it > >>> from that list incrementing an THP_UNDO_DEFERRED_SPLIT_PAGE event. > >>> > >>> But it would be racy with concurrent remapping of the folio (might ha= ppen with > >>> anon folios in corner cases I guess). > >>> > >>> What we can do is the following, though: > >>> > >>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c > >>> index dc30139590e6..f05cba1807f2 100644 > >>> --- a/mm/huge_memory.c > >>> +++ b/mm/huge_memory.c > >>> @@ -3133,6 +3133,8 @@ void folio_undo_large_rmappable(struct folio *f= olio) > >>> ds_queue =3D get_deferred_split_queue(folio); > >>> spin_lock_irqsave(&ds_queue->split_queue_lock, flags); > >>> if (!list_empty(&folio->_deferred_list)) { > >>> + if (folio_test_pmd_mappable(folio)) > >>> + count_vm_event(THP_UNDO_DEFERRED_SPLIT_PAGE); > >>> ds_queue->split_queue_len--; > >>> list_del_init(&folio->_deferred_list); > >>> } > >>> > >>> Adding the right event of course. > >>> > >>> > >>> Then it's easy to filter out these "temporarily added to the list, bu= t never split > >>> before the folio was freed" cases. > >> > >> So instead of making THP_DEFERRED_SPLIT_PAGE precise, use > >> THP_DEFERRED_SPLIT_PAGE - THP_UNDO_DEFERRED_SPLIT_PAGE instead? That s= hould work. > > > > It is definitely possible that the THP on the deferred split queue are > > freed instead of split. For example, 1M is unmapped for a 2M THP, then > > later the remaining 1M is unmapped, or the process exits before memory > > pressure happens. So how come we can tell it is "temporarily added to > > list"? Then THP_DEFERRED_SPLIT_PAGE - THP_UNDO_DEFERRED_SPLIT_PAGE > > actually just counts how many pages are still on deferred split queue. > > It may be useful. However the counter is typically used to estimate > > how many THP are partially unmapped during a period of time. So we > > just need to know the initial value and the value when we read it > > again. > > > >> > >> I wonder what THP_DEFERRED_SPLIT_PAGE counts. If it counts THP deferre= d > >> splits, why not just move the counter to deferred_split_scan(), where = the actual > >> split happens. Or the counter has a different meaning? > > > > The deferred_split_scan() / deferred_split_count() just can return the > > number of pages on a specific queue (a specific node with a specific > > memcg). But THP_DEFERRED_SPLIT_PAGE is a global counter. Did I miss > > something? Or you mean traverse all memcgs and all nodes? It sounds > > too overkilling. > > I mean instead of increasing THP_DEFERRED_SPLIT_PAGE when a folio is adde= d > to the split list, increase it when a folio is split in deferred_split_sc= an(), > regardless which list the folio is on. It will have overlap with thp_split_page. And what if memory pressure doesn't happen? The counter will be 0 even though thousands THP have been partially unmapped. > > -- > Best Regards, > Yan, Zi