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 D6E18C02180 for ; Wed, 15 Jan 2025 06:42:02 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7499F280002; Wed, 15 Jan 2025 01:42:02 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 6FA0C6B008A; Wed, 15 Jan 2025 01:42:02 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5C13A280002; Wed, 15 Jan 2025 01:42:02 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 3CA9A6B0089 for ; Wed, 15 Jan 2025 01:42:02 -0500 (EST) Received: from smtpin30.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id D03831207FC for ; Wed, 15 Jan 2025 06:42:01 +0000 (UTC) X-FDA: 83008741242.30.E14AA3F Received: from invmail4.hynix.com (exvmail4.hynix.com [166.125.252.92]) by imf19.hostedemail.com (Postfix) with ESMTP id 4140C1A0005 for ; Wed, 15 Jan 2025 06:41:58 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=none; spf=pass (imf19.hostedemail.com: domain of honggyu.kim@sk.com designates 166.125.252.92 as permitted sender) smtp.mailfrom=honggyu.kim@sk.com; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1736923320; 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: in-reply-to:in-reply-to:references:references; bh=WB5SbjaWS7ACLhogx2vNDofs+szFb4jrpIPEwrZgghM=; b=sdExibSbPIDyOeqSho/81ublJHFOt3rJXdg2bi91bieg7Jr89fZF7FFIUZrnUynLJ/WUQ6 5m809KLmAFOQx65WazM01IpobZzBEcpQbuad3Y4j1FTVePMgB5cJnsDyiXid+tL0zIENNC VGyU/g/Vbut3mN+0wETZH5xDCRdv5Ig= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=none; spf=pass (imf19.hostedemail.com: domain of honggyu.kim@sk.com designates 166.125.252.92 as permitted sender) smtp.mailfrom=honggyu.kim@sk.com; dmarc=none ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1736923320; a=rsa-sha256; cv=none; b=12Ir8YadLr+iemsEy4OTH6mQxDTtkw8luhgXACFpQg2Wkj1aS/nWnsdbBv5iGt+9rwkZ8b EDY8CV9dX/dpVgNClUr9ShzABsbTdXjuygBCEq3xDHg2Ic4tsLzlfvBPFxekJdDXx6I3wi zK2NJUVuY1kEefu59GZ9/TbcVH/qsyk= X-AuditID: a67dfc5b-3e1ff7000001d7ae-c0-678758b39940 Content-Type: multipart/alternative; boundary="------------bDWX8kwjX3hF0imyFS49bR2E" Message-ID: <4d3c4a37-2678-47b3-9095-c71c45481484@sk.com> Date: Wed, 15 Jan 2025 15:41:54 +0900 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Cc: kernel_team@skhynix.com, Andrew Morton , Jens Axboe , Pavel Begunkov , damon@lists.linux.dev, io-uring@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, lorenzo.stoakes@oracle.com Subject: Re: [RFC PATCH] mm/madvise: remove redundant mmap_lock operations from process_madvise() Content-Language: ko To: SeongJae Park References: <20250115061910.58938-1-sj@kernel.org> From: Honggyu Kim In-Reply-To: <20250115061910.58938-1-sj@kernel.org> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprPIsWRmVeSWpSXmKPExsXC9ZZnoe7miPZ0g1V3eS3mrF/DZjFn1TZG i9V3+9ksnvz/zWrxrvUci8XlXXPYLO6t+c9qcXLWShaLw1/fMDlweuycdZfd4/LZUo9NqzrZ PDZ9msTucWLGbxaPF5tnMnp8fHqLxePzJrkAjigum5TUnMyy1CJ9uwSujGUHZ7IWvLSo6Ls6 j6mBcY5eFyMnh4SAicSurgOMMPb7tn9gNrNAmMS5L23MIDavgKXEkT3bmUBsFgFVic//Olgg 4oISJ2c+AbNFBeQl7t+awd7FyAXUO4lJ4tWGmewgCWGBZInpdw6yQQwVkZjdCTFUREBR4tzj i6wgtpCAkcTJ32vABrEJqElceTkJaBkHB6eAscT3f/UQt71nk3h+rhLClpQ4uOIGywRGgVlI Tp2F5KRZSLZB2GYSXVu7GCFseYntb+dAxdUkbm+7yo4svoCRfRWjUGZeWW5iZo6JXkZlXmaF XnJ+7iZGYLQtq/0TvYPx04XgQ4wCHIxKPLwX4tvShVgTy4orcw8xSnAwK4nwLmFrTRfiTUms rEotyo8vKs1JLT7EKM3BoiTOa/StPEVIID2xJDU7NbUgtQgmy8TBKdXAKKEeH2Odse3cZqG3 FfzMbA12bqs1V77n3Krc7W82/UwJS9W5o2Y3qu2drv/4wyTkWiBfLlXLZdxpq3xPp3nuJ/2N 1/5+5pJf7bv/Usb3gAeWGssDV02o05ya/HJS/Gm9vN3lLIK/bA9/WrZo4S6nl/ESqonmYWek y3sM5e0C3gseetEQKS+uxFKckWioxVxUnAgAKYMVAbICAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFmpnkeLIzCtJLcpLzFFi42LhmqGlp7s5oj3d4OEMZos569ewWcxZtY3R YvXdfjaLJ/9/s1q8az3HYnF47klWi8u75rBZ3Fvzn9Xi5KyVQLGvb5gcuDx2zrrL7nH5bKnH plWdbB6bPk1i9zgx4zeLx4vNMxk9Pj69xeKx+MUHJo/Pm+QCOKO4bFJSczLLUov07RK4MpYd nMla8NKiou/qPKYGxjl6XYycHBICJhLv2/4xgtjMAmES5760MYPYvAKWEkf2bGcCsVkEVCU+ /+tggYgLSpyc+QTMFhWQl7h/awZ7FyMXUO8kJolXG2aygySEBZIlpt85yAYxVERidifEUBEB RYlzjy+ygthCAkYSJ3+vARvEJqAmceXlJKBlHBycAsYS3//VT2DknYXkpFlIVs9CMhXCNpPo 2trFCGHLS2x/OwcqriZxe9tVdmTxBYzsqxhFMvPKchMzc0z1irMzKvMyK/SS83M3MQKjZ1nt n4k7GL9cdj/EKMDBqMTDeyKiLV2INbGsuDL3EKMEB7OSCO8SttZ0Id6UxMqq1KL8+KLSnNTi Q4zSHCxK4rxe4akJQgLpiSWp2ampBalFMFkmDk6pBsai3rf3j3jwsN9i8lYtb2kukD5XH9vC 6lWXoVq3gd07ffU8vy2e+yT4C6dss7hpLTPHvytuZSuP5JzHUfLHVjyZ+j3J/qYzr3Ka7rLV 657qsxQuldU1tNSR0rM4u636nfD8uHeBV/+InNtW2V0S9vvlM2e2CskepUvN6mbrHdaxbj3K KZwrocRSnJFoqMVcVJwIAIBKqRiaAgAA X-CFilter-Loop: Reflected X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 4140C1A0005 X-Rspam-User: X-Stat-Signature: 93ttezwcfm6dknqe7oikyeasa77usy6c X-HE-Tag: 1736923318-833591 X-HE-Meta: U2FsdGVkX18HHroy/XtyD8YqaUp0oKzckf6dtdP8OpFiUTpFcJ/nczFH9S2YyGL+YzLcCNYcIeLBGQdu7X27T3E33dd3TN+BoBpgeiR7F1Dyus3P2mp9sLOE362vQnV2z7LZa27BzWNwpNQN45/91Raw4DDuL5CTJrb4dx8MWcX1UGUPRz7NlDOfC7iPHzZYskk+XsySD3YG3LQlEKe25lSsBEhzZCnZ9/RYqDEFbV+D5RXnps/AoNGEDsW26yr+qBjpLOvthCMEId5xLxHsxph/2ZoXRuoKK6j2Oey5aTjqjrThnVPT5+pTJQtfXF5sUFXMMnprbRAUM8ktFQSH53/qOf+iQynMihdp75pxj9xdk7LP5h7xJYEXJYYzrTA6+OBz+YurANGVIFbHYo1sCgI7KT8YFBh5o5ZUfBrUznTqt082MR82/QRBQsUX9VUV3ljOh79fJ/M86ANFq9j+nBbB4YHy70xzAuXAFXyR8CGZLGNnUIGHfdC5VWQZ7K/yVTeM1jEu/KY6lRD6DWKryBbcKHoLbkgBq186sUKKGqnep+QRD8+7VPmFBOlII9WLKEjuA/kcIjKZ4BQXhzM0jgb7uNrLYgzbmm30zb47uin8Ka8bqRYvLksl1U4xv5k9JoOdtF192+JynxzB+bnO5Q1BYtlrkCvMtCcRiW56rmVzmhiUkNvgr3QhVKJ4s+Hd2vPvmSXH8LRl6qXONWAzEmQhZ5zEBtGh3T/w8KgrTDZHSQ/g/rnJAeKzouk1QKAqbyyGJ1m6KTmsUWJf8aC1rJX5EbfEhsNlWYZanIwAoC3MC2MQjfLXRYZEcpYP5711zUkYXwsTivuhAd73FVamvanY+Q/fV7U/RikzvaEL62tAPwa+sjvoZXJjyNzOOqtCKKI9gmtE7/Gy3G/R+JuLe4m4cJHk5nvtHfzoupYcZlaz38wR9RuqBVOOqkL9Q1Zwq5jsBoSFzZ93aj1ZV4Q VRwkV94K aAY62JU0GH+EW3hhNejy4wITLfxm8/U2Owt19RcP/nT2pdI7qdlYduq9P0cYe6bfjmVFkI8qTUCdR+mZ6NzpWK+wiEXskkdXP+iiQFwUxak9t1tXrMMZ6JS21ArFBRUunpT3HGg2HwytNelei0VJOR/tCvzl6Xs0rHp1/Mgcl9hiYJz9FpDbKdAr9WPI5Jh/dmbfppoHLtrttd0k1XuYXiDbkCPZ5NR+7mu7WPbeXehqbD2mInsSA0dZoWinYgikbM0ibz0ggweT076KJ8jhyabClJixjzBXewmHX5qaCridiAukeWd5V1DHGuXbgBQTohj92ya/wxCoHm/A0o6axodaYlq2VE7qOwIdhxHpGntZW+pym9wXezzTLY3F7MRKDMgzB 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: This is a multi-part message in MIME format. --------------bDWX8kwjX3hF0imyFS49bR2E Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 1/15/2025 3:19 PM, SeongJae Park wrote: > Hi Honggyu, > > On Wed, 15 Jan 2025 13:35:48 +0900 Honggyu Kim wrote: > >> Hi SeongJae, >> >> I have a simple comment on this. >> >> On 1/11/2025 9:46 AM, SeongJae Park wrote: >>> process_madvise() calls do_madvise() for each address range. Then, each >>> do_madvise() invocation holds and releases same mmap_lock. Optimize the >>> redundant lock operations by doing the locking in process_madvise(), and >>> inform do_madvise() that the lock is already held and therefore can be >>> skipped. > [...] >>> --- >>> include/linux/mm.h | 3 ++- >>> io_uring/advise.c | 2 +- >>> mm/damon/vaddr.c | 2 +- >>> mm/madvise.c | 54 +++++++++++++++++++++++++++++++++++----------- >>> 4 files changed, 45 insertions(+), 16 deletions(-) >>> >>> diff --git a/include/linux/mm.h b/include/linux/mm.h >>> index 612b513ebfbd..e3ca5967ebd4 100644 >>> --- a/include/linux/mm.h >>> +++ b/include/linux/mm.h >>> @@ -3459,7 +3459,8 @@ int do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma, >>> unsigned long end, struct list_head *uf, bool unlock); >>> extern int do_munmap(struct mm_struct *, unsigned long, size_t, >>> struct list_head *uf); >>> -extern int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int behavior); >>> +extern int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, >>> + int behavior, bool lock_held); >>> >>> #ifdef CONFIG_MMU >>> extern int __mm_populate(unsigned long addr, unsigned long len, >>> diff --git a/io_uring/advise.c b/io_uring/advise.c >>> index cb7b881665e5..010b55d5a26e 100644 >>> --- a/io_uring/advise.c >>> +++ b/io_uring/advise.c >>> @@ -56,7 +56,7 @@ int io_madvise(struct io_kiocb *req, unsigned int issue_flags) >>> >>> WARN_ON_ONCE(issue_flags & IO_URING_F_NONBLOCK); >>> >>> - ret = do_madvise(current->mm, ma->addr, ma->len, ma->advice); >>> + ret = do_madvise(current->mm, ma->addr, ma->len, ma->advice, false); >> I feel like this doesn't look good in terms of readability. Can we >> introduce an enum for this? > I agree that's not good to read. Liam alos pointed out a similar issue Right. I didn't carefully read his comment. Thanks for the info. > but suggested splitting functions with clear names[1]. I think that also fairly > improves readability, and I slightly prefer that way, since it wouldn't > introduce a new type for only a single use case. Would that also work for your > concern, or do you have a different opinion? > > [1]https://lore.kernel.org/20250115041750.58164-1-sj@kernel.org I don't have any other concern. Thanks, Honggyu > Thanks, > SJ > > [...] --------------bDWX8kwjX3hF0imyFS49bR2E Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit


On 1/15/2025 3:19 PM, SeongJae Park wrote:
Hi Honggyu,

On Wed, 15 Jan 2025 13:35:48 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:


Hi SeongJae,

I have a simple comment on this.

On 1/11/2025 9:46 AM, SeongJae Park wrote:

process_madvise() calls do_madvise() for each address range.  Then, each
do_madvise() invocation holds and releases same mmap_lock.  Optimize the
redundant lock operations by doing the locking in process_madvise(), and
inform do_madvise() that the lock is already held and therefore can be
skipped.

[...]

---
  include/linux/mm.h |  3 ++-
  io_uring/advise.c  |  2 +-
  mm/damon/vaddr.c   |  2 +-
  mm/madvise.c       | 54 +++++++++++++++++++++++++++++++++++-----------
  4 files changed, 45 insertions(+), 16 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 612b513ebfbd..e3ca5967ebd4 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3459,7 +3459,8 @@ int do_vmi_align_munmap(struct vma_iterator *vmi, struct vm_area_struct *vma,
  		    unsigned long end, struct list_head *uf, bool unlock);
  extern int do_munmap(struct mm_struct *, unsigned long, size_t,
  		     struct list_head *uf);
-extern int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int behavior);
+extern int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in,
+		int behavior, bool lock_held);
  
  #ifdef CONFIG_MMU
  extern int __mm_populate(unsigned long addr, unsigned long len,
diff --git a/io_uring/advise.c b/io_uring/advise.c
index cb7b881665e5..010b55d5a26e 100644
--- a/io_uring/advise.c
+++ b/io_uring/advise.c
@@ -56,7 +56,7 @@ int io_madvise(struct io_kiocb *req, unsigned int issue_flags)
  
  	WARN_ON_ONCE(issue_flags & IO_URING_F_NONBLOCK);
  
-	ret = do_madvise(current->mm, ma->addr, ma->len, ma->advice);
+	ret = do_madvise(current->mm, ma->addr, ma->len, ma->advice, false);

I feel like this doesn't look good in terms of readability. Can we 
introduce an enum for this?

I agree that's not good to read.  Liam alos pointed out a similar issue
Right. I didn't carefully read his comment. Thanks for the info. 
but suggested splitting functions with clear names[1].  I think that also fairly
improves readability, and I slightly prefer that way, since it wouldn't
introduce a new type for only a single use case.  Would that also work for your
concern, or do you have a different opinion?

[1] https://lore.kernel.org/20250115041750.58164-1-sj@kernel.org

I don't have any other concern.

Thanks, Honggyu 

Thanks,
SJ

[...]
--------------bDWX8kwjX3hF0imyFS49bR2E--