Hi Honggyu, On Wed, 15 Jan 2025 13:35:48 +0900 Honggyu Kim <honggyu.kim@xxxxxx> 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@xxxxxxxxxx Thanks, SJ [...]
