On Wed, May 10, 2023 at 04:44:59PM -0500, Linus Torvalds wrote: > On Wed, May 10, 2023 at 4:33 PM Linus Torvalds > <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > > > > We'd still keep the RETRY bit as a "this did not complete, you need to > > retry", but at least the whole secondary meaning of "oh, and if it > > isn't set, you need to release the lock you took" would go away. > > "unless VM_FAULT_COMPLETED is set, in which case everything was fine, > and you shouldn't release the lock because we already released it". > > I completely forgot about that wart that came in last year. > > I think that if we made handle_mm_fault() always unlock, that thing > would go away entirely, since "0" would now just mean the same thing. > > Is there really any case that *wants* to keep the mmap lock held, and > couldn't just always re-take it if it needs to do another page > (possibly retry, but the retry case obviously already has that issue)? FAULT_FLAG_RETRY_NOWAIT? > Certainly nothing wants the vma lock, so it's only the "mmap_sem" case > that would be an issue. You're definitely right that the gup path is broken which I didn't notice when reading... I know I shouldn't review such a still slightly involved patch during travel. :( I still think maybe we have chance to generalize at least the fault path, I'd still start with something like having just 2-3 archs having a shared routine handle only some part of the fault path (I remember there was a previous discussion previously, but I didn't follow up much from there..). So even if we still need duplicates over many archs, we'll start to have something we can use as a baseline in fault path. Does it sound a sane thing to consider as a start, or maybe not? The other question - considering RETRY_NOWAIT being there - do we still want to have something like what Johannes proposed first to fix the problem (with all arch and gup fixed)? I'd think yes, but I could missed something. Thanks, -- Peter Xu
