* Shakeel Butt <shakeel.butt@xxxxxxxxx> [250114 13:14]: > Ccing relevant folks. Thanks Shakeel. > > On Fri, Jan 10, 2025 at 04:46:18PM -0800, 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 <sj@xxxxxxxxxx> > > --- > > 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); We are dropping externs when it is not needed as things are changed. Also, please don't use a flags for this. It will have a single user of true, probably ever. It might be better to break do_madvise up into more parts: 1. is_madvise_valid(), which does the checking. 2. madivse_do_behavior() The locking type is already extracted to madivse_need_mmap_write(). What do you think? > > > > #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); > > 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; > > -- > > 2.39.5 >
