Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <wiuXi4KmjgTocOZMpHTZuZP+y8r5wYynvvT_ZPXyG+TYA@mail.gmail.com> <20230630160519.3869505-1-Liam.Howlett@oracle.com>
In-Reply-To: <20230630160519.3869505-1-Liam.Howlett@oracle.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Fri, 30 Jun 2023 09:19:29 -0700
Message-ID: <CAHk-=wg=DGSsA+=rr3bMDKrGNgy4C+PGM_w2PtpK4+Sx9qFF8w@mail.gmail.com>
Subject: Re: [PATCH] mm: Update do_vmi_align_munmap() return semantics
To:     "Liam R. Howlett" <Liam.Howlett@oracle.com>
Cc:     Matthew Wilcox <willy@infradead.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, 30 Jun 2023 at 09:06, Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> Update do_vmi_align_munmap() to return 0 for success.  Clean up the
> callers and comments to always expect the lock downgrade to be honored
> on the success path.  The error path will always leave the lock
> untouched.

Thanks for doing this, but with this cleanup, it becomes clear that
some of the callers that asked for a downgrade didn't actually want
that at all...

For example:

> +               if (do_vma_munmap(&vmi, brkvma, newbrk, oldbrk, &uf, true))
> +                       goto out;
> +
> +               mmap_read_unlock(mm);
> +               goto success_unlocked;

this clearly wanted the lock to be dropped entirely.

As did this one:

>         ret = do_vmi_munmap(&vmi, mm, start, len, &uf, downgrade);
>         /*
> -        * Returning 1 indicates mmap_lock is downgraded.
> -        * But 1 is not legal return value of vm_munmap() and munmap(), reset
> -        * it to 0 before return.
> +        * Returning 0 is successful, but the lock status depends what was
> +        * passed in.
>          */
> -       if (ret == 1) {
> +       if (!ret && downgrade)
>                 mmap_read_unlock(mm);
> -               ret = 0;
> -       } else
> +       else
>                 mmap_write_unlock(mm);

And this one:

> +               ret = do_vmi_munmap(&vmi, mm, addr + new_len, old_len - new_len,
> +                                   &uf_unmap, true);
> +               if (ret)
> +                       goto out;
> +
> +               mmap_read_unlock(current->mm);

I didn't look at what all the indirect callers here were doing, but it
really looked to me like *most* callers wanted the lock dropped
entirely at the end.

In fact, looking at that patch, it looks like *all* of the callers
that asked for downgrading actually really wanted the lock dropped
entirely.

But I may well be missing some context.  So take this not as a NAK,
but as a "you looked at all this code, could it perhaps be simplified
a bit more still?"

               Linus