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 742C1C02180 for ; Wed, 15 Jan 2025 04:35:55 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E5A726B0082; Tue, 14 Jan 2025 23:35:54 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id E0ACA6B0083; Tue, 14 Jan 2025 23:35:54 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id D02F76B0089; Tue, 14 Jan 2025 23:35:54 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id ACE666B0082 for ; Tue, 14 Jan 2025 23:35:54 -0500 (EST) Received: from smtpin01.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 1B78C1C80F7 for ; Wed, 15 Jan 2025 04:35:54 +0000 (UTC) X-FDA: 83008423428.01.647FD0D Received: from invmail4.hynix.com (exvmail4.skhynix.com [166.125.252.92]) by imf17.hostedemail.com (Postfix) with ESMTP id 5A98840008 for ; Wed, 15 Jan 2025 04:35:51 +0000 (UTC) Authentication-Results: imf17.hostedemail.com; dkim=none; spf=pass (imf17.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=1736915752; 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; bh=LDBwzK0pzXS1CervcwLonfAkAJEGLGWHiq1P7V+4XWY=; b=LysxFm3waaW7qDXvHc/K9qXKGWkmlN98U+MBaKBMY/bKVJ1PBEyPR8KdiGYrYLUBaLW3FF lCXHL8TIODWao7c7cPlLJMrJQKBuQQb0kfeaD6bdrZGS6YAUm0BA+qcfIzMsLUtDd23tBO xkadm4vs61l3P5EEkXxO66A0vlwt6Wk= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1736915752; a=rsa-sha256; cv=none; b=qTySEeeCRX3s060c3JlizTXf58nyKsm1EOHk1hmF5ImavwplF4ggABp5k6ZXhhJLHOptDL E2aJWU7NYamOt+4rElRsKc5oSJbsw4Z9zA4bVbHoZ4i6115YcKojT4SorQzPPLfIxU8xfK rDlPzCxwPurlj+/K4agkJ12YzjKfEbA= ARC-Authentication-Results: i=1; imf17.hostedemail.com; dkim=none; spf=pass (imf17.hostedemail.com: domain of honggyu.kim@sk.com designates 166.125.252.92 as permitted sender) smtp.mailfrom=honggyu.kim@sk.com; dmarc=none X-AuditID: a67dfc5b-3c9ff7000001d7ae-ca-67873b240812 Message-ID: <00242654-5e98-4489-ae6d-f4dd01bfdaa7@sk.com> Date: Wed, 15 Jan 2025 13:35:48 +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: <20250111004618.1566-1-sj@kernel.org> From: Honggyu Kim In-Reply-To: <20250111004618.1566-1-sj@kernel.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFupkkeLIzCtJLcpLzFFi42LhesuzSFfFuj3dYFabhcWc9WvYLOas2sZo sfpuP5vFk/+/WS3etZ5jsbi8aw6bxb01/1ktTs5ayWJx+OsbJgdOj52z7rJ7XD5b6rFpVSeb x6ZPk9g9Tsz4zeLxYvNMRo+PT2+xeHzeJBfAEcVlk5Kak1mWWqRvl8CVsX5fK1PBXbuKrr9v WBoYVxl0MXJySAiYSCzpWMcGY18+1c4KYvMKWEpMODEBzGYRUJU4seELO0RcUOLkzCcsILao gLzE/VszgOJcHMwCk5gkXm2YCVYkLJAsMf3OQbChzAIiErM725hBbBEBRYlzjy+CDRUSMJRY eXw3WA2bgJrElZeTmEBsTgEjiVtnWxkhes0kurZ2QdnyEtvfzmEGWSYhcJ9NYtm8T+wQV0tK HFxxg2UCo+AsJAfOQrJ7FpJZs5DMWsDIsopRKDOvLDcxM8dEL6MyL7NCLzk/dxMjMIKW1f6J 3sH46ULwIUYBDkYlHt4L8W3pQqyJZcWVuYcYJTiYlUR4l7C1pgvxpiRWVqUW5ccXleakFh9i lOZgURLnNfpWniIkkJ5YkpqdmlqQWgSTZeLglGpgXKFxXq7B+FqO7W+mOYlhxjcvWod/V+uR /ORb8nJjbcf7rBPeVi/4I80lvpRmTPzcdM6J6d6OvzqhZlNZrO+7vvuYlLSpRO9TYsbfc/d9 BRWUoxsl/J5efsNxqSr/S+KbG/1vbUI00y5tyvz4/Pz0Hfxvl1RweC3ep+8p8EZzTbmIRu/W SUzaSizFGYmGWsxFxYkAoQqHyJwCAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrALMWRmVeSWpSXmKPExsXCNUNLT1fFuj3d4NFvLYs569ewWcxZtY3R YvXdfjaLJ/9/s1q8az3HYnF47klWi8u75rBZ3Fvzn9Xi5KyVQLGvb5gcuDx2zrrL7nH5bKnH plWdbB6bPk1i9zgx4zeLx4vNMxk9Pj69xeKx+MUHJo/Pm+QCOKO4bFJSczLLUov07RK4Mtbv a2UquGtX0fX3DUsD4yqDLkZODgkBE4nLp9pZQWxeAUuJCScmgNksAqoSJzZ8YYeIC0qcnPmE BcQWFZCXuH9rBlCci4NZYBKTxKsNM8GKhAWSJabfOcgGYjMLiEjM7mxjBrFFBBQlzj2+CDZU SMBQYuXx3WA1bAJqEldeTmICsTkFjCRunW1lhOg1k+ja2gVly0tsfzuHeQIj3ywkd8xCsmIW kpZZSFoWMLKsYhTJzCvLTczMMdUrzs6ozMus0EvOz93ECIyHZbV/Ju5g/HLZ/RCjAAejEg/v iYi2dCHWxLLiytxDjBIczEoivEvYWtOFeFMSK6tSi/Lji0pzUosPMUpzsCiJ83qFpyYICaQn lqRmp6YWpBbBZJk4OKUaGM+Umb48XRommrZ/7267wyFGTTwKr/KPORXGW2XOzWxLDV9UZZOv ItTVOVfK6+TT881bGOoqjXoiV80zYVjxJclETWDHkS7b8MN6E9wFt5663PrErmfnzunzghq2 LL/75mbJftZTxt+23hVimOH36v9Bmx8pMX+un358L8TqF8OZLyUFclsnPFRiKc5INNRiLipO BACBhKntgwIAAA== X-CFilter-Loop: Reflected X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: 5A98840008 X-Stat-Signature: gi88fpe9ejrrxqzoecng6e7yghcowf7d X-Rspam-User: X-HE-Tag: 1736915751-213312 X-HE-Meta: U2FsdGVkX18hLE86bYFAen17x4+R+5aP3hoIQZy/qXLD/q4v5SzqpPBsPCokUCUFh6222HwrXf4CIRJz4mctEzpL76gp2Olb227ZigXNRpI/vKlRS2kSTvWYNGOnDSehkm6crXZL4/futK+o4lZTmU7NSq+ywjR0PE9alCYrcL96KWeat2DSjUfsSdYGEMGo7vTZhyjZ5MKExFObo0hvTJenbC7EN5/P4YD7GBPpcTHcypg3ZtkvdcmMIZc2WVg2cgYe4Kb2sOPBHUhfOtG5+HLGVJeM9icrDEP7nhe+RYkE5mn/CnZzuNpHdKK5/W0xrCqLKvIVxNF2q4XYrShi+vInmx2djCUx3Mpwrq0ly7B4rdkdma7xTDvmT9h0gG/RGNI7bk70E21W30brvgIqAt8wKyZ+QYcYBSWHzx1oWp2Rvz5M+11F/PjOlkeoZp5VQ2SFJ2f4SLOK2Lfw5B2mQrhPKUeHuDYwiya+yelhs53uXu6X8JAhDsdGhnRG59Hj4Exc2zJxfJDh9Npm27zxemZ6gQK75twacl2KPkm4naH5rALEBQMxZ3VZnc9d6LGRNXS6C5Y/iguFnM+9uRuoR5wXe/x46gZSu+NNyMq3GiOi+7iRqFnA6NzAthcU8yViYZwvcVuBFtSjB6HGZI+FeF2vnTkFaW6SOePE3ePBq8rsqI7kLDUvNex48s2iO/Yq+RVk257Qb2WbjJrkxvZK49KZO3RhIQQh+AlahehjabZAGOoxtLxR4t0Ztn3SdogySyp51uyl63KZv7AHzEpe881wyC2+bCTr1Jr7U9UKF/m/3FIEM0/RNDcGoNlfP1qkJDEh8pd5PX8oNkKBc3bjC/BOntXQBFetBxNCH1VsaOSGelko/VMqLDHTB/vUngKT3NwX+i2TczJFnHfZlhdwcLfO9To5uk4R3/19Jbd/Myd3UQxGpLyP90tOIR4YG1bfpImRi1Oyedyyq/+suHv XARXalom zwSHBqwgagdYsyOAdwTSojiXExMgb7ONVxyoaS3wYeyRoPFOkHRSpWBe54O2lR3eLgVMVHSYvyYRP2bQNOCdGunSkCB733k+BuIAMyUmTaPKtICdcyazdlmDCA6CrkJjSfBGoFoDD9x2Gn32lmoV+gC75uSrYjZfWvLMJlW7n5I7joULFEZaS+FkV4A== 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: 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. > > Evaluation > ========== > > I measured the time to apply MADV_DONTNEED advice to 256 MiB memory > using multiple madvise() calls, 4 KiB per each call. I also do the same > with process_madvise(), but with varying iovec size from 1 to 1024. > The source code for the measurement is available at GitHub[1]. > > The measurement results are as below. 'sz_batches' column shows the > iovec size of process_madvise() calls. '0' is for madvise() calls case. > 'before' and 'after' columns are the measured time to apply > MADV_DONTNEED to the 256 MiB memory buffer in nanoseconds, on kernels > that built without and with this patch, respectively. So lower value > means better efficiency. 'after/before' column is the ratio of 'after' > to 'before'. > > sz_batches before after after/before > 0 124062365 96670188 0.779206393494111 > 1 136341258 113915688 0.835518827323714 > 2 105314942 78898211 0.749164453796119 > 4 82012858 59778998 0.728897875989153 > 8 82562651 51003069 0.617749895167489 > 16 71474930 47575960 0.665631431888076 > 32 71391211 42902076 0.600943385033768 > 64 68225932 41337835 0.605896230190011 > 128 71053578 42467240 0.597679120395598 > 256 85094126 41630463 0.489228398679364 > 512 68531628 44049763 0.6427654542221 > 1024 79338892 43370866 0.546653285755491 > > The measurement shows this patch reduces the process_madvise() latency, > proportional to the batching size, from about 25% with the batch size 2 > to about 55% with the batch size 1,024. The trend is somewhat we can > expect. > > Interestingly, this patch has also optimize madvise() and single batch > size process_madvise(), though. I ran this test multiple times, but the > results are consistent. I'm still investigating if there are something > I'm missing. But I believe the investigation may not necessarily be a > blocker of this RFC, so just posting this. I will add updates of the > madvise() and single batch size process_madvise() investigation later. > > [1] https://github.com/sjp38/eval_proc_madvise > > Signed-off-by: SeongJae Park > --- > 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? > io_req_set_res(req, ret, 0); > return IOU_OK; > #else > diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c > index a6174f725bd7..30b5a251d73e 100644 > --- a/mm/damon/vaddr.c > +++ b/mm/damon/vaddr.c > @@ -646,7 +646,7 @@ static unsigned long damos_madvise(struct damon_target *target, > if (!mm) > return 0; > > - applied = do_madvise(mm, start, len, behavior) ? 0 : len; > + applied = do_madvise(mm, start, len, behavior, false) ? 0 : len; > mmput(mm); > > return applied; > diff --git a/mm/madvise.c b/mm/madvise.c > index 49f3a75046f6..c107376db9d5 100644 > --- a/mm/madvise.c > +++ b/mm/madvise.c > @@ -1637,7 +1637,8 @@ int madvise_set_anon_name(struct mm_struct *mm, unsigned long start, > * -EAGAIN - a kernel resource was temporarily unavailable. > * -EPERM - memory is sealed. > */ > -int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int behavior) > +int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, > + int behavior, bool lock_held) > { > unsigned long end; > int error; > @@ -1668,12 +1669,14 @@ int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int beh > return madvise_inject_error(behavior, start, start + len_in); > #endif > > - write = madvise_need_mmap_write(behavior); > - if (write) { > - if (mmap_write_lock_killable(mm)) > - return -EINTR; > - } else { > - mmap_read_lock(mm); > + if (!lock_held) { > + write = madvise_need_mmap_write(behavior); > + if (write) { > + if (mmap_write_lock_killable(mm)) > + return -EINTR; > + } else { > + mmap_read_lock(mm); > + } > } > > start = untagged_addr_remote(mm, start); > @@ -1692,17 +1695,19 @@ int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int beh > } > blk_finish_plug(&plug); > > - if (write) > - mmap_write_unlock(mm); > - else > - mmap_read_unlock(mm); > + if (!lock_held) { > + if (write) > + mmap_write_unlock(mm); > + else > + mmap_read_unlock(mm); > + } > > return error; > } > > SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior) > { > - return do_madvise(current->mm, start, len_in, behavior); > + return do_madvise(current->mm, start, len_in, behavior, false); > } > > /* Perform an madvise operation over a vector of addresses and lengths. */ > @@ -1711,12 +1716,28 @@ static ssize_t vector_madvise(struct mm_struct *mm, struct iov_iter *iter, > { > ssize_t ret = 0; > size_t total_len; > + bool hold_lock = true; > + int write; > > total_len = iov_iter_count(iter); > > +#ifdef CONFIG_MEMORY_FAILURE > + if (behavior == MADV_HWPOISON || behavior == MADV_SOFT_OFFLINE) > + hold_lock = false; > +#endif > + if (hold_lock) { > + write = madvise_need_mmap_write(behavior); > + if (write) { > + if (mmap_write_lock_killable(mm)) > + return -EINTR; > + } else { > + mmap_read_lock(mm); > + } > + } > + > while (iov_iter_count(iter)) { > ret = do_madvise(mm, (unsigned long)iter_iov_addr(iter), > - iter_iov_len(iter), behavior); > + iter_iov_len(iter), behavior, hold_lock); > /* > * An madvise operation is attempting to restart the syscall, > * but we cannot proceed as it would not be correct to repeat > @@ -1739,6 +1760,13 @@ static ssize_t vector_madvise(struct mm_struct *mm, struct iov_iter *iter, > iov_iter_advance(iter, iter_iov_len(iter)); > } > > + if (hold_lock) { > + if (write) > + mmap_write_unlock(mm); > + else > + mmap_read_unlock(mm); > + } > + > ret = (total_len - iov_iter_count(iter)) ? : ret; > > return ret;