On Fri, 31 Jan 2025 16:53:17 +0000 Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> 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@xxxxxxxxxx>
[...]
> > --- 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
