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 0BD3DC282EC for ; Tue, 11 Mar 2025 21:01:26 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1F185280002; Tue, 11 Mar 2025 17:01:24 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 17D27280001; Tue, 11 Mar 2025 17:01:24 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 04316280002; Tue, 11 Mar 2025 17:01:23 -0400 (EDT) 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 99EC0280001 for ; Tue, 11 Mar 2025 17:01:23 -0400 (EDT) Received: from smtpin02.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 2CF3C140686 for ; Tue, 11 Mar 2025 21:01:24 +0000 (UTC) X-FDA: 83210490888.02.5EC31DE Received: from nyc.source.kernel.org (nyc.source.kernel.org [147.75.193.91]) by imf18.hostedemail.com (Postfix) with ESMTP id F22CB1C0017 for ; Tue, 11 Mar 2025 21:01:21 +0000 (UTC) Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=BadLIGob; dmarc=pass (policy=quarantine) header.from=kernel.org; spf=pass (imf18.hostedemail.com: domain of sj@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=sj@kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1741726882; a=rsa-sha256; cv=none; b=S70yE4l7JPf2Ar+ODaCg0U1Hw+U1/DjcYIlSWWTy0B1fdOfsAfCLGgPpKhcdUivUEeWLSz 9Xn2jyeLiVjjbMr1LnOsvFBejXhjtJlCP/IPyux0DVTvNnIsjPyyK2AKKS/od2xQFwCIF8 9f2nWlxJWm7lAWC4rx+q5oJmV1uE/9w= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=BadLIGob; dmarc=pass (policy=quarantine) header.from=kernel.org; spf=pass (imf18.hostedemail.com: domain of sj@kernel.org designates 147.75.193.91 as permitted sender) smtp.mailfrom=sj@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1741726882; 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-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=Gjd7e9GS2xY/URXSAM56iQC8ALdhza+wyQCo0kmRmz0=; b=5W+35yXxHZgQAn/872RXKzMZKrMi+Gom+Fftk+cdgPzASw+RU+WdYtBLo++axT8pw2Qi7m AwEi7GR239VFYtDHadE1hKDw+LLFbYkcJPn77fBm0oOil0dJKZdGJYKg5XLwE4skFKEvTb x/aVS9sQo/jXZtiw6EG2MLmEdsvw81U= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id BADA5A46979; Tue, 11 Mar 2025 20:55:50 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 438D0C4CEE9; Tue, 11 Mar 2025 21:01:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1741726880; bh=+NTNS0fhln7wAifm0+ONt7nOeyWyL6zedv9NQ16rf7Y=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=BadLIGob3p2Avw2SasIiOnPmJSmwhz0lacvAgjfidbEk8OEOtXASvEluZ/cqnq5mT GvcwXgfk33hNWLXtVTEKA/GDMBxBCoCT/EuhqD4yan2WD0SFKQ9qa7pVENJiQgKHxd uI83wfED6XDpnbvPAJHy/0Tr+FhXuSlrB7VI297Eo2rlFMbbEIysx+7wF2WDXwkkdR /+JxI+bDN2wwlIKmDecB3rpwXEVR5a1V/mLpcR9yXQgIC3V7xv1kSugcSKSkjt9uqg kWtWvil3XEUjirL97/X54Ekw76Aolae3VKw562TMpGf1ujkhnY+u9gv2XhO2iyMB7V QHQXd8RezjEwQ== From: SeongJae Park To: Lorenzo Stoakes Cc: SeongJae Park , Andrew Morton , "Liam R. Howlett" , David Hildenbrand , Shakeel Butt , Vlastimil Babka , kernel-team@meta.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org, Rik van Riel Subject: Re: [PATCH 8/9] mm/madvise: batch tlb flushes for [process_]madvise(MADV_{DONTNEED[_LOCKED],FREE}) Date: Tue, 11 Mar 2025 14:01:18 -0700 Message-Id: <20250311210118.85501-1-sj@kernel.org> X-Mailer: git-send-email 2.39.5 In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Rspamd-Server: rspam07 X-Rspam-User: X-Stat-Signature: 1dguiyikhmmqxju4r5qastc9bmtpcp1e X-Rspamd-Queue-Id: F22CB1C0017 X-HE-Tag: 1741726881-299155 X-HE-Meta: U2FsdGVkX1/vkTcVLq2ua8QAEPP1fr5iGm6ZZoARAQjGWpR4tN0wbuvz2Rz9R+hsrS1LzqlwtvWXmRIEGOgKFvvrch7UVUYlskV+AwGb4wsz9HIaUtG3gRbNwLciHkFR+KHof8QgP9vXP+wRHHIL1IR1vtTIOUcaJc2ZfJ83/97+TUY9+PZfEyRLZcHArKGXkA6isniUOCeppslT60mQqbNf2GOoT+dDv/GR4ooHKk/xHgeqZsDOkvZjQiXP5IGx0m7sgQ8gaNubvZXortog2fd9nLV7vBjyBGl+vBO2QU0qt7J+72v7pHCxXdI+Anzw6nNZ/mWEtT/IFYNcCsI+xO60tMPcNDVlUz6jA4NtmyyP25Dn0oONkeNXSgLUhazwAXJy3NVgpfGIMBMEqScwq8fhSRecm1fvVmiRYGhq1EFhn22Jv+/Cuu91cy3263f5ISguYV8n8SfcIvuMI5KMq2JhysHHs1PdVUkeOFoKu1VRdTvHZ1BaPec7LRBbfaEaanYemA71QxxaU2Nt/MUFW1fKww6n+fAHfS+6MbtQ/Db4Q6E5m8jv8/qx3xDwt5TCbH1NvPKg++SNo2X46l67IH1kNigPCWy8vZ04VtUn0cpnY0EEiKEMShh4qoh63FXuGJucsZf66h2bZ6+S0CoexRxQzpOGqMGaKlDHegofMdDDfNY0YkNBK76nopGw55XcifuLBcLmWjYI4V/9uXyYGL4EBdxd0nA+6eEc13XmvwEivaUPa6aOKTJOOJUXoFhQ5gnmbwQdZeh6qYPUgtSC/bgPewSoxZr16LW+QneATFLnZobTDYWYDsqLU1sFgye6Zq7HG4Y48DeyrhqUgrGutw/fGNOiVcuc87YFBe/KQ5tYtYCs4atqm2Qb3Fj7Xd7V0OrKPIhdec1IDlUL4tRfylZaouDG/xb26PAFrG6WJtDymNQmYhPfg4ZdxenduVQTTC1ILHJqPyEBZ1wsEUo t54xfPT9 HOHB6qovxQJAYtF9h9+i+vtgDVMD/55wRTlcKvFGTcDa83jITDypMWQjlG0DOlhOI0Ivz/BHH3k8UEUFFCuEMJ7k30RjNTdNxd0IwstBhucp9QKXXt0IorAI9f2cRy+5OtVJwLoCSIOrg34OsjF5Bir/5pxxbDi477Zh7tfNv94F0809FchJTfO63euccWpiSqfv+e9XkGjWOjyf8ZlbO/iqlvkaYfe8v3kvDWG232k/9YGAKM96H4IuFtMaFAlu5lZ7ZBPnsB1QVQRGyf815PhWawzaLG1oXK1gIQcb6+u78BRc= 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 Tue, 11 Mar 2025 13:59:10 +0000 Lorenzo Stoakes wrote: > +cc Rik on this, as he's working on TLB flush-related stuff. Maybe worth > cc-ing him on series respins too? Unless Rik objects of course :P > > Again, nit, but your subject line/first line of commit message is > definitely too long here! :) I will reduce. > > On Mon, Mar 10, 2025 at 10:23:17AM -0700, SeongJae Park wrote: > > MADV_DONTNEED[_LOCKED] and MADV_FREE internal logics for > > [process_]madvise() can be invoked with batched tlb flushes. Update > > vector_madvise() and do_madvise(), which are called for the two system > > calls respectively, to use those in the efficient way. Initialize an > > mmu_gather object before starting the internal works, and flush the > > gathered tlb entries at once after all the internal works are done. > > super nit but logics -> logic and works -> work :) > > I think we need more here as to why you're restricting to > MADV_DONTNEED_LOCKED and MADV_FREE. I see pageout initialises a tlb gather > object, so does cold, etc. etc.? Good point. I'm just trying to start from small things. I will clarify this on the next spin. > > > > > Signed-off-by: SeongJae Park > > This is really nice, I love how we're able to evolve this towards batching > flushes. > > Overall though I'd like you to address some of the concerns here before > giving tags... :) Thank you for nice comments! :) > > > --- > > mm/madvise.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++---- > > 1 file changed, 47 insertions(+), 4 deletions(-) > > > > diff --git a/mm/madvise.c b/mm/madvise.c > > index d7ea71c6422c..d5f4ce3041a4 100644 > > --- a/mm/madvise.c > > +++ b/mm/madvise.c > > @@ -905,6 +905,7 @@ static bool madvise_dontneed_free_valid_vma(struct vm_area_struct *vma, > > > > struct madvise_behavior { > > int behavior; > > + struct mmu_gather *tlb; > > }; > > Aha! Good :) > > I see in 9/9 you actually pull the caller_tlb stuff out, I still feel like > we should be threading this state through further, if possible, rather than > passing in behavior->tlb as a parameter. Yes, I will do so. > > But this is nitty I suppose! > > > > > static long madvise_dontneed_free(struct vm_area_struct *vma, > > @@ -964,9 +965,11 @@ static long madvise_dontneed_free(struct vm_area_struct *vma, > > } > > > > if (behavior == MADV_DONTNEED || behavior == MADV_DONTNEED_LOCKED) > > - return madvise_dontneed_single_vma(NULL, vma, start, end); > > + return madvise_dontneed_single_vma( > > + madv_behavior->tlb, vma, start, end); > > else if (behavior == MADV_FREE) > > - return madvise_free_single_vma(NULL, vma, start, end); > > + return madvise_free_single_vma( > > + madv_behavior->tlb, vma, start, end); > > Yeah as I said above be nice to just pass madv_behavior, makes things more > flexible to pass a pointer to the helper struct through, as you can Yes. > > > else > > return -EINVAL; > > } > > @@ -1639,6 +1642,32 @@ static void madvise_unlock(struct mm_struct *mm, int behavior) > > mmap_read_unlock(mm); > > } > > > > +static bool madvise_batch_tlb_flush(int behavior) > > +{ > > + switch (behavior) { > > + case MADV_DONTNEED: > > + case MADV_DONTNEED_LOCKED: > > + return true; > > + default: > > + return false; > > + } > > +} > > I kind of hate this madvise_ prefix stuff, like we're in mm/madvise.c, it's > pretty obvious static functions are related to madvise :) but this is a > pre-existing thing, not your fault, and it's actually right to maintain > consistency with this. > > So this is purely a whine that can be >/dev/null. Thank you for understanding :) > > > + > > +static void madvise_init_tlb(struct madvise_behavior *madv_behavior, > > + struct mm_struct *mm) > > +{ > > + if (!madvise_batch_tlb_flush(madv_behavior->behavior)) > > + return; > > + tlb_gather_mmu(madv_behavior->tlb, mm); > > +} > > + > > +static void madvise_finish_tlb(struct madvise_behavior *madv_behavior) > > +{ > > + if (!madvise_batch_tlb_flush(madv_behavior->behavior)) > > + return; > > + tlb_finish_mmu(madv_behavior->tlb); > > +} > > + > > Nitty, but for both of these, usually I like the guard clause pattern, but > since it's such a trivial thing I think it reads better as: > > if (madvise_batch_tlb_flush(madv_behavior->behavior)) > tlb_gather_mmu(madv_behavior->tlb, mm); > > and: > > if (madvise_batch_tlb_flush(madv_behavior->behavior)) > tlb_finish_mmu(madv_behavior->tlb); Totally agreed, thank you for catching this. Thanks, SJ [...]