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 6831CC19F32 for ; Wed, 5 Mar 2025 23:56:39 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7271A280003; Wed, 5 Mar 2025 18:56:38 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 6D6DB280002; Wed, 5 Mar 2025 18:56:38 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 59E31280003; Wed, 5 Mar 2025 18:56:38 -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 3C3FA280002 for ; Wed, 5 Mar 2025 18:56:38 -0500 (EST) Received: from smtpin23.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 6E1D9A9189 for ; Wed, 5 Mar 2025 23:56:38 +0000 (UTC) X-FDA: 83189159676.23.18AAE1E Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf30.hostedemail.com (Postfix) with ESMTP id B5A7B80009 for ; Wed, 5 Mar 2025 23:56:36 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=sp0K89Em; spf=pass (imf30.hostedemail.com: domain of sj@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=sj@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1741218996; 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=M/cz/1oNQq7RaRVODV3glA/KDbwtrnRgGWPyfXH/WoQ=; b=ae2k9RhNomsrmkTZrOCgj3tUHmb/dMER9+/jrouggpSwfnmoheW8/ohfVTBW4kzkKwPvie OP9qHP7e1o5m5SwE01u8jkW5CsyltnS1rtmviw0LkAbMJMt6X8AVytofnJ/5DoQOc8SKAP dmtUYrfYwagwa7u4LFLL5cxOX9hKOVw= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=sp0K89Em; spf=pass (imf30.hostedemail.com: domain of sj@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=sj@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1741218996; a=rsa-sha256; cv=none; b=3f6GCpwiZIZmKZeiqXMSsLksrC3w7LQfvApgauU1k0SU71viHwr1aESPhfeV2L4Gg7kyyk /wbKrAt1sfbE/KPdjAv4Zp215Sw1xhoeObXe/+N5Q1BOESJYkdZ/VU7XOZZZh7cQHkQYWw EuMnkQO+cX0UO7MMUszKNrTm6w/GmJo= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id E17155C6DB2; Wed, 5 Mar 2025 23:54:18 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 42D62C4CED1; Wed, 5 Mar 2025 23:56:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1741218995; bh=Hzjlp30dCTBYdo66QorfE8yUiVB8YP6kBzBJxDX0R+A=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=sp0K89EmUnXwOfMGrDv2rVmOfvJa34KCMUlf5QQdeNyCarMumtlKYuy73SpMyL5+D nTG99D69EXqv2S0UY53v75I4CQxpOdKD2srh6pI5SQKuBuW5T3/Kw0AinZePl9IQ2W 2nV/z2whuFiDh7Zx8Hr3OkbmFSB8IlV17nu2pYMEMImG6AnBy095LbkmF0iZoEmBDT 0ULEJvXR1S8bNdW5tzg3gyaV0lpdorVRHwugWq+UVPdPKPL3en7sjTNCajT6l3M9GJ fVwoFr2wfUnaTJTCfoPWTVTd342LAUO1HUddYQsGGZXSiZ1pFMZ2NC/INYoISYS/Vy ay+SYFquH2kdw== From: SeongJae Park To: Shakeel Butt Cc: SeongJae Park , "Liam R. Howlett" , Andrew Morton , David Hildenbrand , Lorenzo Stoakes , Vlastimil Babka , kernel-team@meta.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: Re: [RFC PATCH 05/16] mm/madvise: define and use madvise_behavior struct for madvise_do_behavior() Date: Wed, 5 Mar 2025 15:56:32 -0800 Message-Id: <20250305235632.137169-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-Stat-Signature: oq4yuc5nm1ep59yseygs3gyewezuaek5 X-Rspamd-Queue-Id: B5A7B80009 X-Rspamd-Server: rspam06 X-Rspam-User: X-HE-Tag: 1741218996-827723 X-HE-Meta: U2FsdGVkX1+Za461CzYPnVy0LQ1jNzxY/9LkRMTYT4N0cyku0FuPj0urXzGQcxses8S1lly1AsMILmDsRjZhuB8zuQGE0o0r0kJkclXIzjnrblVIu9mdcwtCtExEePO3GCCFT/qPFnxC2MM1seIWQcWYc0Dv4WeRP36DdHT4JrjC2GfM82P4r/1cP3O/k4YM0fxiuWinG4ZwOHsXy7TfQ6Jvi0/MvuVm5KCk50Ttmrnw4DWSMyrdLKONqZMkcdzbCv3tEHaRb74nG5lOPP7a4JDEVFBzQWt+RBNR6dvkpJlYqTF1IuTSX451nNY3DPK5NN2pylio8IRer7g9bCgZJ/769uqmgvP5RR+2kRrAuLgidx+y6KcJY1Vzvf+NOMkNLqou4WXtreU146C9yTuEXTDxX/Wf+96bgPpX1U+8ZiErqhLz1XwZMiWZQgpfqTs77BK8rbDghy4wvVC5cfT47h/aSHc2LmUXxAAKSEhhUehFtwpryBXod0dKLQncrlgYCtw3mpjodv8Yp0MPbeLFk6TS+/zJaqPTIxhEl1iOOCMpWP3g4gdqf9I3GEvZ+rpN7mztOUsvohHPTemVpn2sLkjkeE/4H+pp7M7Y70wR790U6ZUn4hfgsVXWriWWYbHRewrW0N6ndimf9T720yt3hTaV6F5rruZg9DX4Gs8fRa5g9lbR0TWrR6DO14DMT1dWXbZB3HXRUwZmtYrmBwLolsq2V2ji/5b2iqOi4cb7NO9Lu475ql9bJs5OvLdCeHztXhrhP4KsOxvYezeWCADb/MCLJEsPRIu2a3P72at9xCW1z/88yp5nl15JA9baMZhRQL13ZtBpxtRfcBTWGX6dm0mS+BjGVUqz2My2T0YpespAfv2xkP/awW7q7Exbnh9WGZUinOfL/uxgTXGN8xjStZvfowWhAPEcYzaZPzxX+86p4UbG+xHaYu2QXAoSv3/Dj3/8NCZb9G2LUsQTa5J e2UYR+8u iaRoHSA5mkvtYeMOQ5o0Jr8fuXXgMKpprhXaZcsNEW4BKXV95B7Lu7zGHkqGRHNPdimYlvRDqObXSl9k/OTwJIRl0B0pjnUkV5qZiY64/x6z1uCCYUUlGTKoTqKeRNgxiJYl2oC33TBKzOMj65pUSRnLE+XCJOwzMmRX1T4Rgrrt4sRhTtFXg6S/1hTnz2r1b3wQNmglkUM2R1EzzjAO+WJxp1dH2p/tKwQv6rWkHJV7inRz++PZVQ9Evtrn8Mx5Oqg3uH02mCSq3znmoW5pBEH7Bmg== 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 Wed, 5 Mar 2025 13:40:17 -0800 Shakeel Butt wrote: > On Wed, Mar 05, 2025 at 01:02:15PM -0800, Shakeel Butt wrote: > > On Wed, Mar 05, 2025 at 10:16:00AM -0800, SeongJae Park wrote: > > > To implement batched tlb flushes for MADV_DONTNEED[_LOCKED] and > > > MADV_FREE, an mmu_gather object in addition to the behavior integer need > > > to be passed to the internal logics. Define a struct for passing such > > > information together with the behavior value without increasing number > > > of parameters of all code paths towards the internal logic. > > > > > > Signed-off-by: SeongJae Park > > > --- > > > mm/madvise.c | 15 ++++++++++++--- > > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > > > diff --git a/mm/madvise.c b/mm/madvise.c > > > index c5e1a4d1df72..3346e593e07d 100644 > > > --- a/mm/madvise.c > > > +++ b/mm/madvise.c > > > @@ -1665,9 +1665,15 @@ static bool is_memory_populate(int behavior) > > > } > > > } > > > > > > +struct madvise_behavior { > > > + int behavior; > > > +}; > > > > I think the patch 5 to 8 are just causing churn and will be much better > > to be a single patch. I agree. I will do so in the next version. > > Also why not make the above struct a general > > madvise visit param struct which can be used by both > > madvise_vma_anon_name() and madvise_vma_behavior()? I was also thinking we can further extend the struct for more clean and efficient code, so agree to your fundamental point. I ended up desiring to focus on tlb flushes batching at the moment, though. However, what you are suggesting is bit different from what I was thinking. To me, madvise_walk_vmas() is for general purposes and hence the visit functions should be able to recieve an argument of arbitrary types. It is true that there are only two visit functions, but they seems doing very different purpose works to me, so having a same type argument doesn't seem very straightforward to understand its usage, nor efficient to my humble viewpoint. > > Something like following: > > diff --git a/mm/madvise.c b/mm/madvise.c > index c5e1a4d1df72..cbc3817366a6 100644 > --- a/mm/madvise.c > +++ b/mm/madvise.c > @@ -890,11 +890,17 @@ static bool madvise_dontneed_free_valid_vma(struct vm_area_struct *vma, > return true; > } > > +struct madvise_walk_param { > + int behavior; > + struct anon_vma_name *anon_name; > +}; Only madvise_vma_behavior() will use 'behavior' field. And only madvise_vma_anon_name() will use 'anon_name' field. But I will have to assume both function _might_ use both fields when reading the madvise_walk_vmas() function code. That doesn't make my humble code reading that simple and straightforward. Also populating and passing a data structure that has data that would not really be used seems not very efficient to me. [...] > @@ -1666,8 +1674,10 @@ static bool is_memory_populate(int behavior) > } > > static int madvise_do_behavior(struct mm_struct *mm, > - unsigned long start, size_t len_in, int behavior) > + unsigned long start, size_t len_in, > + struct madvise_walk_param *arg) > { > + int behavior = arg->behavior; > struct blk_plug plug; > unsigned long end; > int error; > @@ -1681,7 +1691,7 @@ static int madvise_do_behavior(struct mm_struct *mm, > if (is_memory_populate(behavior)) > error = madvise_populate(mm, start, end, behavior); 'arg' is for madvise_walk_vmas() visit function, but we're using it as a capsule for passing an information that will be used for madvise_do_behavior(). This also seems not very straightforward to my humble perspective. I have no strong opinion and maybe my humble taste is too peculiar. But, if this is not a blocker for tlb flushes batcing, I'd like to suggest keep this part as is for now, and revisit for more code cleanup later. What do you think, Shakeel? Thanks, SJ