On 3/2/22 09:24, Michal Hocko wrote: > Sorry, this has slipped through cracks. > > On Mon 14-02-22 15:39:31, Nico Pache wrote: > [...] >> We've recently been discussing the following if statement in __oom_reap_task_mm: >> if (vma_is_anonymous(vma) || !(vma->vm_flags & VM_SHARED)) >> >> Given the comment above it, and some of the upstream discussion the original >> RFC, we are struggling to see why this should be a `||` and not an `&&`. If we >> only want to reap anon memory and reaping shared memory can be dangerous is this >> statement incorrect? >> >> We have a patch queued up to make this change, but wanted to get your opinion on >> why this was originally designed this way in case we are missing something. > > I do not really see why this would be wrong. Private file backed > mappings can contain a reapable memory as well. I do not see how this > would solve the futex issue. We were basing our discussion around the following comment: /* * Only anonymous pages have a good chance to be dropped * without additional steps which we cannot afford as we * are OOM already. * * We do not even care about fs backed pages because all * which are reclaimable have already been reclaimed and * we do not want to block exit_mmap by keeping mm ref * count elevated without a good reason. */ So changing to an && would align the functionality with this comment by ignoring fs backed pages, and additionally it prevents shared mappings from being reaped. We have tested this change and found we can no longer reproduce the issue. In our case we allocate the mutex on a MAP_SHARED|MAP_ANONYMOUS mmap so the if- statement in question would no longer return true after the && change. If it is the case that private fs backed pages matter perhaps we want something like this: if ((vma_is_anonymous(vma) && !(vma->vm_flags & VM_SHARED)) ||(!vma_is_anonymous(vma) && !(vma->vm_flags & VM_SHARED))) or more simply: if(!(vma->vm_flags & VM_SHARED)) to exclude all VM_SHARED mappings. -- Nico
