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

[SeongJae Park <sj@kernel.org>]

On Fri, 31 Jan 2025 16:53:17 +0000 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:

> 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@kernel.org>
[...]
> > --- 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,
[...]
> >  		/*
> >  		 * 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.

Nice catch!

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

Thank you for this kind suggestion.  I will update next version of this patch
in this way.

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


Thanks,
SJ