On 09/09, Mircea CIRJALIU - MELIU wrote: > > > From: Oleg Nesterov <oleg@xxxxxxxxxx> > > > > But why is it safe to drop ->mmap_sem without checking > > FAULT_FLAG_ALLOW_RETRY/RETRY_NOWAIT ? > > > Dropping mmap_sem will have the same effects regardless of FAULT_FLAG_ALLOW_RETRY/RETRY_NOWAIT. > Another thread can unmap the VMA from underneath us, or remap another one in its place. > In the end, the VMA has to be revalidated when re-acquiring the mmap_sem. > Or am I wrong?! To simplify, lets forget about RETRY_NOWAIT/TRIED. Again, I can be easily wrong. But iiuc, you simply can't drop mmap_sem if FAULT_FLAG_ALLOW_RETRY is not set, the caller doesn't expect mmap_sem can be unlocked. OTOH, if FAULT_FLAG_ALLOW_RETRY is set and you drop mmap_sem, you can only return VM_FAULT_RETRY to let the caller know it was dropped. > > > + * If FAULT_FLAG_ALLOW_RETRY is set, the mmap_sem must be > > released > > > + * before returning VM_FAULT_RETRY only if > > FAULT_FLAG_RETRY_NOWAIT is > > > + * not set. > > > > Well, iiuc FAULT_FLAG_ALLOW_RETRY means that ->fault() _can_ drop > > mmap_sem and return VM_FAULT_RETRY (unless NOWAIT). > > That comment is just copied from elsewhere in the code. > My interpretation was that the fault handler _should_ return with mmap_sem > held or not depending on FAULT_FLAG_RETRY_NOWAIT. Yes, this depends on FAULT_FLAG_RETRY_NOWAIT. But your comment above looks as if he mmap_sem must be _always_ released if FAULT_FLAG_ALLOW_RETRY && !FAULT_FLAG_RETRY_NOWAIT. This is not true. Nevermind. If you ever resend this patch, please CC mm/ experts. I tried to look at this code again and I feel it has much more problems, but as I said this is not my area. And I think you should split this patch, mirror_vm_fault() should come in a separate patch to simplify the review. Oleg.
