linux-kernel - Re: [RFC PATCH] mm/madvise: remove redundant mmap_lock operations from process

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <c210f152-5eb7-456a-8cd3-ec75e1a5b266@sk.com>
Date: Wed, 15 Jan 2025 16:15:34 +0900
From: Honggyu Kim <honggyu.kim@...com>
To: SeongJae Park <sj@...nel.org>
Cc: kernel_team@...ynix.com, Andrew Morton <akpm@...ux-foundation.org>,
 Jens Axboe <axboe@...nel.dk>, Pavel Begunkov <asml.silence@...il.com>,
 damon@...ts.linux.dev, io-uring@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-mm@...ck.org,
 lorenzo.stoakes@...cle.com, Liam.Howlett@...cle.com
Subject: Re: [RFC PATCH] mm/madvise: remove redundant mmap_lock operations
 from process_madvise()

Hi SeongJae,

I'm resending this because my new mail client mistakenly set the mail
format to HTML. Sorry for the noise.

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@...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 but

Right. I didn't carefully read his comment. Thanks for the info.

> 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
> 
> [...]