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 66A5AC282D1 for ; Thu, 6 Mar 2025 04:18:19 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1F8496B008A; Wed, 5 Mar 2025 23:18:17 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 1A9786B0093; Wed, 5 Mar 2025 23:18:17 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 070DC6B0095; Wed, 5 Mar 2025 23:18:17 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id D9CC06B008A for ; Wed, 5 Mar 2025 23:18:16 -0500 (EST) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 6E197B8041 for ; Thu, 6 Mar 2025 04:18:17 +0000 (UTC) X-FDA: 83189819034.16.D865953 Received: from nyc.source.kernel.org (nyc.source.kernel.org [147.75.193.91]) by imf08.hostedemail.com (Postfix) with ESMTP id C88AA160002 for ; Thu, 6 Mar 2025 04:18:15 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=s15WA362; spf=pass (imf08.hostedemail.com: domain of sj@kernel.org designates 147.75.193.91 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=1741234695; 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=gFRhk5gxmiXRq4GzzE7b276LfbSZfeM0E76nEeeE3nE=; b=erjnqZ2nIniY3o7IsQNI7HttP9SnnaMOQtLKYEYyr2Bzz31LhJGdVM15Ptwx7bHgxFdgLo brY2eWVlI21p1opTw5QhzAR/9yS3HODcJTI/SIr2w90nt5//ANc+cuBXsEMfq5izKHwZmM rTg9u1soiFhTF3vqc6IqLcjmw1iYtBk= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=s15WA362; spf=pass (imf08.hostedemail.com: domain of sj@kernel.org designates 147.75.193.91 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=1741234695; a=rsa-sha256; cv=none; b=7BScjQbqQJqKA5PC7TqZGRsWXZ3NoWj/O3xClsK37EFgjgSpzWBZqNAidRlCPpSC0cZucW PNXkYyNyV1NqYk/kah4+j9GnLeHkd+1Oq5NtTm0FkEPUIylFrLIlSnsdKAC24xm0GrFHA9 sWdNMOGcyYYopZMjks0PpMVDs0ZocJ4= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id 3D928A450ED; Thu, 6 Mar 2025 04:12:44 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 6F1F3C4CEE4; Thu, 6 Mar 2025 04:18:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1741234694; bh=VgXJGwKGRvF0z0k8Etl8Xec0K7y3Rvi04P7d0jCoAb4=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=s15WA362Z9PWIBmTNg6hAKO0dU1Zgc4ojZ64aPwmNckMZQbyi/sj15Q3tAGSJ2PZd AwHhX3rdTCUOPJQ8FahaOyyaiwVQbfxItHQ51tBurXZkQBZsv4oToYM6J/i14lxGIT y9iUIgoBbqKsgakAxEnoen5MbYLsRd45M9v3TuDLTDpu4f4uIRYCr1NUAX3R3TF9th fvsf1ePefiiPjuKEyvqmGpYvLJv0lqeZlcO+UWZPc0qlcXKqwOjjHAOFEiWAQwzIsg wkf2Jodshy+BrZRO17UqHJNQi0jScHOX1vvAVDeSEev85J7JTvqwyHedBZEM3x51ku wD/zjguTmyEpg== 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 20:18:12 -0800 Message-Id: <20250306041812.59243-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-Rspam-User: X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: C88AA160002 X-Stat-Signature: xfu3m9y7gb8utunt3ikathcm851faroe X-HE-Tag: 1741234695-41333 X-HE-Meta: U2FsdGVkX1+yDs49rsFDz2ZrlggKqM/0+hyyQmYrT+vLiuJZ9EKu26MyY77HZRzKTWQg+0R0xMSXSazBC4TlmsTHsUCkY1mzUkSa8UdLWWxpnVGhNPDbcfZCZO1kfE1NVW63IjyaCq3c2LNq/qGfSi7fF9H0KX7zMtwIFF3YLWTnjAj+f0IHJ/F9umoAH+BaP6KGa5sY37QkwIe0pcq0BNCK0oXmV7OyRA8OUqN5ZMN4f6OHLjiCyjIOj0fQwxetlYQvx/5382xB8uT57HgMIfvV8do6oQjvNaI7s0f7ktDzmwNMYDvM9V8kaBnoIMD6jGQ3Pxx06V9sar+4ovShq5zRiX/QvnWT4Ws0BMOiF4dtP/fvXTsHrbBsbN5bIlOkXHjL86HW72GCCuqWHA/1hNrnq2a7kU7TP3NengtqE6l9zNYVfqgAZUw9ReW2sqKVNgGfmOS1UBsr0BFMiOLQq2Bp8tsJYpEvujYd2qej1GgrFTYiZtFsrdT2HPZSHkkEr0mwRpNxeFdR+HQBjDYRCOtVPraJOI3indPn9oOUdSqkwsY3TpCSf3qoTQoiAXTO1mPtAvK2LylpIjKs5a/0NYNuHfuo9GpXr64A4JmWUVyGrQOM44BsjdYy3I3bNptC0bzy8bxZ/aUXfWlNXmgIJLqxRXDyhPFMAgKeJikG4R7ckePsqjcDRHFYANG8yyDHbyTmAyHcx6z+FsSkL9+kYedPPhXpwb44jzPT5YINhg69JJvVX36P5F/rCKBcMoGM/NsrRYCE9ncAdU6XnhjdZs5/FGFrlxONew2EWC2GxwDsUIXT47fhmA2ag25rriAQgq09BkqGLvl+P/aOZKN3huhhdALu5LXxLH5pWB/xHl0sP/n654A3bBXdBQ2tzUUU5aXTGf5+ChXcvXitvn3zFzB3crZQR9xYONH+QQVYNPMD0E6h+VM+/IV/quHPvfCpyWZRF8A7YD7cdlWrzIA nfrh43d6 tPBvENJBSHW1+N94clY2COe3UPJTccsPze5joB+hdMNxT7g2hslDjmepgGiMOLZyxVEevmO+S7wYAY2CBqVGhjTqTUKiDyX6I3vdgKn1paKs1KQSC2Hp2y/BhxHhz++H2wAOP7y7HDzKj5iYTRNTWPDfsocZ3mr2pyQKXBNjUEgHaAB0DOwYSEu7T48enkQJJaAqc/Fp2SJ57cP1jHur7SoY15ZB4KsGZ1QFoaTB3ViDhu+beSkUwQKjC3RMV0pzhx50F 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 19:37:28 -0800 Shakeel Butt wrote: > On Wed, Mar 05, 2025 at 03:56:32PM -0800, SeongJae Park wrote: > > On Wed, 5 Mar 2025 13:40:17 -0800 Shakeel Butt wrote: > > > > > > > > +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. > > > > This is not a new pattern. You can find a lot of examples in kernel > where a struct encapsulates multiple fields and its pointer is passed > around rather than those fields (or subset of them). You can check out > zap_details, vm_fault, fs_parameter. If some fields are mutually > exclusive you can have union in the struct. You're right. But we do so for readability and/or efficiency, right? And what I'm saying is that I'm not very sure if this change is making code much easier to read and/or efficient. > Also I am not sure what do > you mean by "not efficient" here. Inefficient in what sense? Unused > memory or something else? I was meaning unused memory in this context. It is a negligible extent, of course. Nevertheless, what I wanted to say is not that the change will degrade efficiency too much, but I'm not clearly seeing the efficiency benefit that explains why the change is something that should be made. > > > [...] > > > @@ -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. > > Here we can keep behavior as parameter to madvise_walk_vmas() and define > struct madvise_walk_param inside it with the passed behavior. Anyways > this is a very common pattern in kernel. > > > > > 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? > > > > Squashing patches 5 to 8 into one is the main request from me. My other > suggestion you can ignore but let's see what other says. Makes sense. Thank you for your inputs :) Thanks, SJ