On Fri 14-09-18 22:54:45, Tetsuo Handa wrote: > On 2018/09/13 22:40, Michal Hocko wrote: > > On Thu 13-09-18 20:53:24, Tetsuo Handa wrote: > >> On 2018/09/13 20:35, Michal Hocko wrote: > >>>> Next question. > >>>> > >>>> /* Use -1 here to ensure all VMAs in the mm are unmapped */ > >>>> unmap_vmas(&tlb, vma, 0, -1); > >>>> > >>>> in exit_mmap() will now race with the OOM reaper. And unmap_vmas() will handle > >>>> VM_HUGETLB or VM_PFNMAP or VM_SHARED or !vma_is_anonymous() vmas, won't it? > >>>> Then, is it guaranteed to be safe if the OOM reaper raced with unmap_vmas() ? > >>> > >>> I do not understand the question. unmap_vmas is basically MADV_DONTNEED > >>> and that doesn't require the exclusive mmap_sem lock so yes it should be > >>> safe those two to race (modulo bugs of course but I am not aware of any > >>> there). > >>> > >> > >> You need to verify that races we observed with VM_LOCKED can't happen > >> for VM_HUGETLB / VM_PFNMAP / VM_SHARED / !vma_is_anonymous() cases. > > > > Well, VM_LOCKED is kind of special because that is not a permanent state > > which might change. VM_HUGETLB, VM_PFNMAP resp VM_SHARED are not changed > > throughout the vma lifetime. > > > OK, next question. > Is it guaranteed that arch_exit_mmap(mm) is safe with the OOM reaper? I do not see any obvious problem and we used to allow to race unmaping in exit and oom_reaper paths before we had to handle mlocked vmas specially. > Well, anyway, diffstat of your proposal would be > > include/linux/oom.h | 2 -- > mm/internal.h | 3 +++ > mm/memory.c | 28 ++++++++++++-------- > mm/mmap.c | 73 +++++++++++++++++++++++++++++++---------------------- > mm/oom_kill.c | 46 ++++++++++++++++++++++++--------- > 5 files changed, 98 insertions(+), 54 deletions(-) > > trying to hand over only __free_pgtables() part at the risk of > setting MMF_OOM_SKIP without reclaiming any memory due to dropping > __oom_reap_task_mm() and scattering down_write()/up_write() inside > exit_mmap(), while diffstat of my proposal (not tested yet) would be > > include/linux/mm_types.h | 2 + > include/linux/oom.h | 3 +- > include/linux/sched.h | 2 +- > kernel/fork.c | 11 +++ > mm/mmap.c | 42 ++++------- > mm/oom_kill.c | 182 ++++++++++++++++++++++------------------------- > 6 files changed, 117 insertions(+), 125 deletions(-) > > trying to wait until __mmput() completes and also trying to handle > multiple OOM victims in parallel. > > You are refusing timeout based approach but I don't think this is > something we have to be frayed around the edge about possibility of > overlooking races/bugs because you don't want to use timeout. And you > have never showed that timeout based approach cannot work well enough. I have tried to explain why I do not like the timeout based approach several times alreay and I am getting fed up repeating it over and over again. The main point though is that we know _what_ we are waiting for and _how_ we are synchronizing different parts rather than let's wait some time and hopefully something happens. Moreover, we have a backoff mechanism. The new class of oom victims with a large amount of memory in page tables can fit into that model. The new model adds few more branches to the exit path so if this is acceptable for other mm developers then I think this is much more preferrable to add a diffrent retry mechanism. -- Michal Hocko SUSE Labs
