Messages in this thread | | | From | Linus Torvalds <> | Date | Fri, 30 Jun 2023 09:19:29 -0700 | Subject | Re: [PATCH] mm: Update do_vmi_align_munmap() return semantics |
| |
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
| |