Re: [RFC PATCH] mm/madvise: remove redundant mmap_lock operations from process_madvise()

[SeongJae Park <sj@kernel.org>]

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 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


Thanks,
SJ

[...]