On Thu, Jan 16, 2025 at 05:30:58PM -0800, SeongJae Park wrote: > Optimize redundant mmap lock operations from process_madvise() by > directly doing the mmap locking first, and then the remaining works for > all ranges in the loop. > > Signed-off-by: SeongJae Park <sj@xxxxxxxxxx> I wonder if this might increase lock contention because now all of the vector operations will hold the relevant mm lock without releasing after each operation? Probably it's ok given limited size of iov, but maybe in future we'd want to set a limit on the ranges before we drop/reacquire lock? I've tested this against my PIDFD_SELF changes [0] and the process_madvise() invocation in the guard-pages tests which utilises process_madvise() in a perhaps 'unusual' way so is a good test bed for this, and all working fine. Amazingly, this appears to be the only place (afaict) where process_madivse() is actually tested... Buuuut, I think this change is incorrect, I comment on this below. Should be an easy fix though. [0]:https://lore.kernel.org/all/cover.1738268370.git.lorenzo.stoakes@xxxxxxxxxx/ > --- > mm/madvise.c | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/mm/madvise.c b/mm/madvise.c > index 913936a5c353..1a796a03a4fb 100644 > --- a/mm/madvise.c > +++ b/mm/madvise.c > @@ -1752,9 +1752,26 @@ static ssize_t vector_madvise(struct mm_struct *mm, struct iov_iter *iter, > > total_len = iov_iter_count(iter); > > + ret = madvise_lock(mm, behavior); > + if (ret) > + return ret; > + > while (iov_iter_count(iter)) { > - ret = do_madvise(mm, (unsigned long)iter_iov_addr(iter), > - iter_iov_len(iter), behavior); > + unsigned long start = (unsigned long)iter_iov_addr(iter); This is nicer than just passing in. > + size_t len_in = iter_iov_len(iter); Equally so here... > + size_t len; > + > + if (!is_valid_madvise(start, len_in, behavior)) { > + ret = -EINVAL; > + break; > + } > + > + len = PAGE_ALIGN(len_in); > + if (start + len == start) > + ret = 0; > + else > + ret = madvise_do_behavior(mm, start, len_in, len, > + behavior); Very slight duplication here (I wonder if we can somehow wrap the 'if zero length return 0' thing?). But it doesn't really matter, probably better to spell it out, actually. > /* > * An madvise operation is attempting to restart the syscall, > * but we cannot proceed as it would not be correct to repeat > @@ -1776,6 +1793,7 @@ static ssize_t vector_madvise(struct mm_struct *mm, struct iov_iter *iter, > break; > iov_iter_advance(iter, iter_iov_len(iter)); > } > + madvise_unlock(mm, behavior); > > ret = (total_len - iov_iter_count(iter)) ? : ret; So I think this is now wrong because of the work I did recently. In this code: /* * An madvise operation is attempting to restart the syscall, * but we cannot proceed as it would not be correct to repeat * the operation in aggregate, and would be surprising to the * user. * * As we have already dropped locks, it is safe to just loop and * try again. We check for fatal signals in case we need exit * early anyway. */ if (ret == -ERESTARTNOINTR) { if (fatal_signal_pending(current)) { ret = -EINTR; break; } continue; } Note that it assumes the locks have been dropped before simply trying again, as the only way this would happen is because of a race, and we may end up stuck in a loop if we just hold on to the lock. So I'd suggest updating this comment and changing the code like this: if (ret == -ERESTARTNOINTR) { if (fatal_signal_pending(current)) { ret = -EINTR; break; } + /* Drop and reacquire lock to unwind race. */ + madvise_unlock(mm, behaviour); + madvise_lock(mm, behaviour); continue; } Which brings back the existing behaviour. By the way I hate that this function swallows error codes. But that's not your fault, and is now established user-facing behaviour so yeah. Big sigh. > > -- > 2.39.5
