mm: drop oom code from exit_mmap

With the last usage of MMF_OOM_VICTIM in exit_mmap gone, this flag is now unused and can be removed.

...

include/linux/oom.h +0 -0

@@ -77,15 +77,6 @@ static inline bool tsk_is_oom_victim(struct task_struct * tsk)
 	return tsk->signal->oom_mm;
 }

Quoted 10 lines · On Tue, 31 May 2022 15:31:00 -0700 Suren Baghdasaryan <[email protected]> wrote:

With the last usage of MMF_OOM_VICTIM in exit_mmap gone, this flag is now unused and can be removed.

...

include/linux/oom.h +0 -0

@@ -77,15 +77,6 @@ static inline bool tsk_is_oom_victim(struct task_struct * tsk)
      return tsk->signal->oom_mm;
 }

include/linux/oom.h static inline bool tsk_is_oom_victim(struct task_struct * tsk) · line 88

Quoted 10 lines

include/linux/oom.h +0 -9

 
-/*
- * Use this helper if tsk->mm != mm and the victim mm needs a special
- * handling. This is guaranteed to stay true after once set.
- */
-static inline bool mm_is_oom_victim(struct mm_struct *mm)
-{
-     return test_bit(MMF_OOM_VICTIM, &mm->flags);
-}
-

The patch "mm: multi-gen LRU: support page table walks" from the MGLRU series
(https://lkml.kernel.org/r/[email protected])
adds two calls to mm_is_oom_victim(), so my build broke.

I assume the fix is simply

mm/vmscan.c renamed from mm/vmscan.c~mm-delete-unused-mmf_oom_victim-flag-fix +0 -6

@@ -3429,9 +3429,6 @@ static bool should_skip_mm(struct mm_str
        if (size < MIN_LRU_BATCH)
                return true;
 
-       if (mm_is_oom_victim(mm))
-               return true;
-
        return !mmget_not_zero(mm);
 }
 

@@ -4127,9 +4124,6 @@ restart:
 
                walk_pmd_range(&val, addr, next, args);
 
-               if (mm_is_oom_victim(args->mm))
-                       return 1;
-
                /* a racy check to curtail the waiting time */
                if (wq_has_sleeper(&walk->lruvec->mm_state.wait))
                        return 1;

_

Please confirm?

Quoted 24 lines

mm/vmscan.c renamed from mm/vmscan.c~mm-delete-unused-mmf_oom_victim-flag-fix +0 -6

@@ -3429,9 +3429,6 @@ static bool should_skip_mm(struct mm_str
        if (size < MIN_LRU_BATCH)
                return true;
 
-       if (mm_is_oom_victim(mm))
-               return true;
-
        return !mmget_not_zero(mm);
 }
 

@@ -4127,9 +4124,6 @@ restart:
 
                walk_pmd_range(&val, addr, next, args);
 
-               if (mm_is_oom_victim(args->mm))
-                       return 1;
-
                /* a racy check to curtail the waiting time */
                if (wq_has_sleeper(&walk->lruvec->mm_state.wait))
                        return 1;

_

Quoted 1 line

Please confirm?

LGTM. The deleted checks are not about correctness.

I've queued

mm/vmscan.c +1 -1

@@ -3402,7 +3402,7 @@ static bool should_skip_mm(struct mm_struct *mm,
 struct lru_gen_mm_walk *walk)
        if (size < MIN_LRU_BATCH)
                return true;
 
-       if (mm_is_oom_victim(mm))
+       if (test_bit(MMF_OOM_REAP_QUEUED, &mm->flags))
                return true;
 

        return !mmget_not_zero(mm);

mm/vmscan.c +1 -1

@@ -4109,7 +4109,7 @@ static int walk_pud_range(p4d_t *p4d, unsigned
 long start, unsigned long end,
 
                walk_pmd_range(&val, addr, next, args);
 
-               if (mm_is_oom_victim(args->mm))
+               if (test_bit(MMF_OOM_REAP_QUEUED, &args->mm->flags))
                        return 1;
 

                /* a racy check to curtail the waiting time */

Quoted 24 lines · On Mon, 22 Aug 2022 16:33:51 -0600 Yu Zhao <[email protected]> wrote:

Quoted 24 lines

mm/vmscan.c renamed from mm/vmscan.c~mm-delete-unused-mmf_oom_victim-flag-fix +0 -6

@@ -3429,9 +3429,6 @@ static bool should_skip_mm(struct mm_str
        if (size < MIN_LRU_BATCH)
                return true;
 
-       if (mm_is_oom_victim(mm))
-               return true;
-
        return !mmget_not_zero(mm);
 }
 

@@ -4127,9 +4124,6 @@ restart:
 
                walk_pmd_range(&val, addr, next, args);
 
-               if (mm_is_oom_victim(args->mm))
-                       return 1;
-
                /* a racy check to curtail the waiting time */
                if (wq_has_sleeper(&walk->lruvec->mm_state.wait))
                        return 1;

_

Quoted 3 lines

Quoted 1 line

Please confirm?

LGTM. The deleted checks are not about correctness.

OK, for now.

Quoted 19 lines

I've queued

mm/vmscan.c +1 -1

@@ -3402,7 +3402,7 @@ static bool should_skip_mm(struct mm_struct *mm,
 struct lru_gen_mm_walk *walk)
        if (size < MIN_LRU_BATCH)
                return true;
 
-       if (mm_is_oom_victim(mm))
+       if (test_bit(MMF_OOM_REAP_QUEUED, &mm->flags))
                return true;
 

        return !mmget_not_zero(mm);

mm/vmscan.c +0 -0

@@ -4109,7 +4109,7 @@ static int walk_pud_range(p4d_t *p4d, unsigned
 long start, unsigned long end,
 
                walk_pmd_range(&val, addr, next, args);
 

Quoted 5 lines

mm/vmscan.c +1 -1

-               if (mm_is_oom_victim(args->mm))
+               if (test_bit(MMF_OOM_REAP_QUEUED, &args->mm->flags))
                        return 1;
 

                /* a racy check to curtail the waiting time */

Oh. Why? What does this change do?

Quoted 5 lines

Quoted 5 lines

(quoted diff) +0 -0

@@ -4109,7 +4109,7 @@ static int walk_pud_range(p4d_t *p4d, unsigned
 long start, unsigned long end,
 
                walk_pmd_range(&val, addr, next, args);
 

Quoted 7 lines

Quoted 5 lines

(quoted diff) +1 -1

-               if (mm_is_oom_victim(args->mm))
+               if (test_bit(MMF_OOM_REAP_QUEUED, &args->mm->flags))
                        return 1;
 

                /* a racy check to curtail the waiting time */

Oh. Why? What does this change do?

The MMF_OOM_REAP_QUEUED flag is similar to the deleted MMF_OOM_VICTIM flag, but it's set at a later stage during an OOM kill.

When either is set, the OOM reaper is probably already freeing the memory of this mm_struct, or at least it's going to. So there is no need to dwell on it in the reclaim path, hence not about correctness.

Quoted 13 lines · On Mon, 22 Aug 2022 16:59:29 -0600 Yu Zhao <[email protected]> wrote:

Quoted 12 lines

Quoted 10 lines

(quoted diff) +1 -1

@@ -4109,7 +4109,7 @@ static int walk_pud_range(p4d_t *p4d, unsigned
 long start, unsigned long end,
 
                walk_pmd_range(&val, addr, next, args);
 
-               if (mm_is_oom_victim(args->mm))
+               if (test_bit(MMF_OOM_REAP_QUEUED, &args->mm->flags))
                        return 1;
 

                /* a racy check to curtail the waiting time */

Oh. Why? What does this change do?

Quoted 6 lines

The MMF_OOM_REAP_QUEUED flag is similar to the deleted MMF_OOM_VICTIM flag, but it's set at a later stage during an OOM kill.

When either is set, the OOM reaper is probably already freeing the memory of this mm_struct, or at least it's going to. So there is no need to dwell on it in the reclaim path, hence not about correctness.

Thanks. That sounds worthy of some code comments?

Quoted 19 lines · Andrew Morton

Quoted 16 lines · On Mon, 22 Aug 2022 16:59:29 -0600 Yu Zhao <[email protected]> wrote:

Quoted 5 lines

Quoted 5 lines

(quoted diff) +0 -0

@@ -4109,7 +4109,7 @@ static int walk_pud_range(p4d_t *p4d, unsigned
 long start, unsigned long end,
 
                walk_pmd_range(&val, addr, next, args);
 

Quoted 7 lines

Quoted 5 lines

(quoted diff) +1 -1

-               if (mm_is_oom_victim(args->mm))
+               if (test_bit(MMF_OOM_REAP_QUEUED, &args->mm->flags))
                        return 1;
 

                /* a racy check to curtail the waiting time */

Oh. Why? What does this change do?

The MMF_OOM_REAP_QUEUED flag is similar to the deleted MMF_OOM_VICTIM flag, but it's set at a later stage during an OOM kill.

Quoted 5 lines

Quoted 3 lines

When either is set, the OOM reaper is probably already freeing the memory of this mm_struct, or at least it's going to. So there is no need to dwell on it in the reclaim path, hence not about correctness.

Thanks. That sounds worthy of some code comments?

Will do. Thanks.

Quoted 24 lines · Yu Zhao

Quoted 23 lines · Andrew Morton

Quoted 19 lines · On Mon, 22 Aug 2022 16:59:29 -0600 Yu Zhao <[email protected]> wrote:

Quoted 5 lines

Quoted 5 lines

(quoted diff) +0 -0

@@ -4109,7 +4109,7 @@ static int walk_pud_range(p4d_t *p4d, unsigned
 long start, unsigned long end,
 
                walk_pmd_range(&val, addr, next, args);
 

Quoted 7 lines

Quoted 5 lines

(quoted diff) +1 -1

-               if (mm_is_oom_victim(args->mm))
+               if (test_bit(MMF_OOM_REAP_QUEUED, &args->mm->flags))
                        return 1;
 

                /* a racy check to curtail the waiting time */

Oh. Why? What does this change do?

The MMF_OOM_REAP_QUEUED flag is similar to the deleted MMF_OOM_VICTIM flag, but it's set at a later stage during an OOM kill.

When either is set, the OOM reaper is probably already freeing the memory of this mm_struct, or at least it's going to. So there is no need to dwell on it in the reclaim path, hence not about correctness.

Quoted 3 lines

Quoted 1 line

Thanks. That sounds worthy of some code comments?

Will do. Thanks.

I would rather not see this abuse.

You cannot really make any
assumptions about oom_reaper and how quickly it is going to free the memory.

If this is really worth it (and I have to say I doubt it) then it should be a separate patch with numbers justifying it.

On Tue, Aug 23, 2022 at 2:36 AM Michal Hocko <[email protected]> wrote:

Quoted 3 lines

You cannot really make any
assumptions about oom_reaper and how quickly it is going to free the memory.

Agreed. But here we are talking about heuristics, not dependencies on certain behaviors. Assume we are playing a guessing game: there are multiple mm_structs available for reclaim, would the oom-killed ones be more profitable on average? I'd say no, because I assume it's more likely than unlikely that the oom reaper is doing/to do its work. Note that the assumption is about likelihood, hence arguably valid.

Quoted 1 line · Yu Zhao

On Tue, Aug 23, 2022 at 2:36 AM Michal Hocko <[email protected]> wrote:

[...]

Quoted 10 lines

Quoted 3 lines

You cannot really make any
assumptions about oom_reaper and how quickly it is going to free the memory.

Agreed. But here we are talking about heuristics, not dependencies on certain behaviors. Assume we are playing a guessing game: there are multiple mm_structs available for reclaim, would the oom-killed ones be more profitable on average? I'd say no, because I assume it's more likely than unlikely that the oom reaper is doing/to do its work. Note that the assumption is about likelihood, hence arguably valid.

Well, my main counter argument would be that we do not really want to carve last resort mechanism (which the oom reaper is) into any heuristic because any future changes into that mechanism will be much harder to justify and change. There is a cost of the maintenance that should be considered. While you might be right that this change would be beneficial, there is no actual proof of that. Historically we've had several examples of such a behavior which was really hard to change later on because the effect would be really hard to evaluate.

The primary reason to invoke the oom reaper from the exit_mmap path used to be a prevention of an excessive oom killing if the oom victim exit races with the oom reaper (see [1] for more details). The invocation has moved around since then because of the interaction with the munlock logic but the underlying reason has remained the same (see [2]).

Munlock code is no longer a problem since [3] and there shouldn't be any blocking operation before the memory is unmapped by exit_mmap so the oom reaper invocation can be dropped. The unmapping part can be done with the non-exclusive mmap_sem and the exclusive one is only required when page tables are freed.

Remove the oom_reaper from exit_mmap which will make the code easier to read. This is really unlikely to make any observable difference although some microbenchmarks could benefit from one less branch that needs to be evaluated even though it almost never is true.

[1] 212925802454 ("mm: oom: let oom_reap_task and exit_mmap run concurrently")
[2] 27ae357fa82b ("mm, oom: fix concurrent munlock and oom reaper unmap, v3")
[3] a213e5cf71cb ("mm/munlock: delete munlock_vma_pages_all(), allow oomreap")

include/linux/oom.h +0 -2

@@ -106,8 +106,6 @@ static inline vm_fault_t check_stable_address_space(struct mm_struct *mm)
 	return 0;
 }
 
-bool __oom_reap_task_mm(struct mm_struct *mm);
-
 long oom_badness(struct task_struct *p,
 		unsigned long totalpages);
 

mm/mmap.c +0 -9

index 2b9305ed0dda..b7918e6bb0db 100644
@@ -3110,30 +3110,13 @@ void exit_mmap(struct mm_struct *mm)
 	/* mm's last user has gone, and its about to be pulled down */
 	mmu_notifier_release(mm);
 
-	if (unlikely(mm_is_oom_victim(mm))) {
-		/*
-		 * Manually reap the mm to free as much memory as possible.
-		 * Then, as the oom reaper does, set MMF_OOM_SKIP to disregard
-		 * this mm from further consideration.  Taking mm->mmap_lock for
-		 * write after setting MMF_OOM_SKIP will guarantee that the oom
-		 * reaper will not run on this mm again after mmap_lock is
-		 * dropped.
-		 *

Quoted 12 lines · On Tue, 31 May 2022 15:30:59 -0700 Suren Baghdasaryan <[email protected]> wrote:

The primary reason to invoke the oom reaper from the exit_mmap path used to be a prevention of an excessive oom killing if the oom victim exit races with the oom reaper (see [1] for more details). The invocation has moved around since then because of the interaction with the munlock logic but the underlying reason has remained the same (see [2]).

Munlock code is no longer a problem since [3] and there shouldn't be any blocking operation before the memory is unmapped by exit_mmap so the oom reaper invocation can be dropped. The unmapping part can be done with the non-exclusive mmap_sem and the exclusive one is only required when page tables are freed.

Quoted 9 lines

Remove the oom_reaper from exit_mmap which will make the code easier to read. This is really unlikely to make any observable difference although some microbenchmarks could benefit from one less branch that needs to be evaluated even though it almost never is true.

[1] 212925802454 ("mm: oom: let oom_reap_task and exit_mmap run concurrently")
[2] 27ae357fa82b ("mm, oom: fix concurrent munlock and oom reaper unmap, v3")
[3] a213e5cf71cb ("mm/munlock: delete munlock_vma_pages_all(), allow oomreap")

I've just reinstated the mapletree patchset so there are some conflicting changes.

Quoted 33 lines

include/linux/oom.h +0 -2

@@ -106,8 +106,6 @@ static inline vm_fault_t check_stable_address_space(struct mm_struct *mm)
      return 0;
 }
 
-bool __oom_reap_task_mm(struct mm_struct *mm);
-
 long oom_badness(struct task_struct *p,
              unsigned long totalpages);
 

mm/mmap.c +0 -14

index 2b9305ed0dda..b7918e6bb0db 100644
@@ -3110,30 +3110,13 @@ void exit_mmap(struct mm_struct *mm)
      /* mm's last user has gone, and its about to be pulled down */
      mmu_notifier_release(mm);
 
-     if (unlikely(mm_is_oom_victim(mm))) {
-             /*
-              * Manually reap the mm to free as much memory as possible.
-              * Then, as the oom reaper does, set MMF_OOM_SKIP to disregard
-              * this mm from further consideration.  Taking mm->mmap_lock for
-              * write after setting MMF_OOM_SKIP will guarantee that the oom
-              * reaper will not run on this mm again after mmap_lock is
-              * dropped.
-              *
-              * Nothing can be holding mm->mmap_lock here and the above call
-              * to mmu_notifier_release(mm) ensures mmu notifier callbacks in
-              * __oom_reap_task_mm() will not block.
-              */
-             (void)__oom_reap_task_mm(mm);

mm/mmap.c void exit_mmap(struct mm_struct *mm) · line 3113

Quoted 5 lines

mm/mmap.c +1 -4

-             set_bit(MMF_OOM_SKIP, &mm->flags);
-     }
-
-     mmap_write_lock(mm);
+     mmap_read_lock(mm);

Unclear why this patch fiddles with the mm_struct locking in this fashion - changelogging that would have been helpful.

But iirc mapletree wants to retain a write_lock here, so I ended up with

void exit_mmap(struct mm_struct *mm)

{
        struct mmu_gather tlb;
        struct vm_area_struct *vma;
        unsigned long nr_accounted = 0;
        MA_STATE(mas, &mm->mm_mt, 0, 0);
        int count = 0;

        /* mm's last user has gone, and its about to be pulled down */
        mmu_notifier_release(mm);

        mmap_write_lock(mm);
        arch_exit_mmap(mm);

        vma = mas_find(&mas, ULONG_MAX);
        if (!vma) {
                /* Can happen if dup_mmap() received an OOM */
                mmap_write_unlock(mm);
                return;
        }

        lru_add_drain();
        flush_cache_mm(mm);
        tlb_gather_mmu_fullmm(&tlb, mm);
        /* update_hiwater_rss(mm) here? but nobody should be looking */
        /* Use ULONG_MAX here to ensure all VMAs in the mm are unmapped */
        unmap_vmas(&tlb, &mm->mm_mt, vma, 0, ULONG_MAX);

        /*
         * Set MMF_OOM_SKIP to hide this task from the oom killer/reaper
         * because the memory has been already freed. Do not bother checking
         * mm_is_oom_victim because setting a bit unconditionally is cheaper.
         */
        set_bit(MMF_OOM_SKIP, &mm->flags);
        free_pgtables(&tlb, &mm->mm_mt, vma, FIRST_USER_ADDRESS,
                      USER_PGTABLES_CEILING);
        tlb_finish_mmu(&tlb);

        /*
         * Walk the list again, actually closing and freeing it, with preemption
         * enabled, without holding any MM locks besides the unreachable
         * mmap_write_lock.
         */
        do {
                if (vma->vm_flags & VM_ACCOUNT)
                        nr_accounted += vma_pages(vma);
                remove_vma(vma);
                count++;

                cond_resched();
        } while ((vma = mas_find(&mas, ULONG_MAX)) != NULL);

        BUG_ON(count != mm->map_count);

        trace_exit_mmap(mm);
        __mt_destroy(&mm->mm_mt);
        mm->mmap = NULL;

        mmap_write_unlock(mm);
        vm_unacct_memory(nr_accounted);
}

Quoted 61 lines · Andrew Morton

Quoted 12 lines · On Tue, 31 May 2022 15:30:59 -0700 Suren Baghdasaryan <[email protected]> wrote:

The primary reason to invoke the oom reaper from the exit_mmap path used to be a prevention of an excessive oom killing if the oom victim exit races with the oom reaper (see [1] for more details). The invocation has moved around since then because of the interaction with the munlock logic but the underlying reason has remained the same (see [2]).

Munlock code is no longer a problem since [3] and there shouldn't be any blocking operation before the memory is unmapped by exit_mmap so the oom reaper invocation can be dropped. The unmapping part can be done with the non-exclusive mmap_sem and the exclusive one is only required when page tables are freed.

Quoted 9 lines

Remove the oom_reaper from exit_mmap which will make the code easier to read. This is really unlikely to make any observable difference although some microbenchmarks could benefit from one less branch that needs to be evaluated even though it almost never is true.

[1] 212925802454 ("mm: oom: let oom_reap_task and exit_mmap run concurrently")
[2] 27ae357fa82b ("mm, oom: fix concurrent munlock and oom reaper unmap, v3")
[3] a213e5cf71cb ("mm/munlock: delete munlock_vma_pages_all(), allow oomreap")

I've just reinstated the mapletree patchset so there are some conflicting changes.

Quoted 33 lines

include/linux/oom.h +0 -2

@@ -106,8 +106,6 @@ static inline vm_fault_t check_stable_address_space(struct mm_struct *mm)
      return 0;
 }
 
-bool __oom_reap_task_mm(struct mm_struct *mm);
-
 long oom_badness(struct task_struct *p,
              unsigned long totalpages);
 

mm/mmap.c +0 -14

index 2b9305ed0dda..b7918e6bb0db 100644
@@ -3110,30 +3110,13 @@ void exit_mmap(struct mm_struct *mm)
      /* mm's last user has gone, and its about to be pulled down */
      mmu_notifier_release(mm);
 
-     if (unlikely(mm_is_oom_victim(mm))) {
-             /*
-              * Manually reap the mm to free as much memory as possible.
-              * Then, as the oom reaper does, set MMF_OOM_SKIP to disregard
-              * this mm from further consideration.  Taking mm->mmap_lock for
-              * write after setting MMF_OOM_SKIP will guarantee that the oom
-              * reaper will not run on this mm again after mmap_lock is
-              * dropped.
-              *
-              * Nothing can be holding mm->mmap_lock here and the above call
-              * to mmu_notifier_release(mm) ensures mmu notifier callbacks in
-              * __oom_reap_task_mm() will not block.
-              */
-             (void)__oom_reap_task_mm(mm);

Quoted 8 lines

mm/mmap.c void exit_mmap(struct mm_struct *mm) · line 3113

Quoted 5 lines

mm/mmap.c +1 -4

-             set_bit(MMF_OOM_SKIP, &mm->flags);
-     }
-
-     mmap_write_lock(mm);
+     mmap_read_lock(mm);

Unclear why this patch fiddles with the mm_struct locking in this fashion - changelogging that would have been helpful.

Yeah, I should have clarified this in the description. Everything up to unmap_vmas() can be done under mmap_read_lock and that way oom-reaper and process_mrelease can do the unmapping in parallel with exit_mmap. That's the reason we take mmap_read_lock, unmap the vmas, mark the mm with MMF_OOM_SKIP and take the mmap_write_lock to execute free_pgtables. I think maple trees do not change that except there is no mm->mmap anymore, so the line at the end of exit_mmap where we reset mm->mmap to NULL can be removed (I show that line below).

Quoted 52 lines

But iirc mapletree wants to retain a write_lock here, so I ended up with

void exit_mmap(struct mm_struct *mm)

{
        struct mmu_gather tlb;
        struct vm_area_struct *vma;
        unsigned long nr_accounted = 0;
        MA_STATE(mas, &mm->mm_mt, 0, 0);
        int count = 0;

        /* mm's last user has gone, and its about to be pulled down */
        mmu_notifier_release(mm);

        mmap_write_lock(mm);
        arch_exit_mmap(mm);

        vma = mas_find(&mas, ULONG_MAX);
        if (!vma) {
                /* Can happen if dup_mmap() received an OOM */
                mmap_write_unlock(mm);
                return;
        }

        lru_add_drain();
        flush_cache_mm(mm);
        tlb_gather_mmu_fullmm(&tlb, mm);
        /* update_hiwater_rss(mm) here? but nobody should be looking */
        /* Use ULONG_MAX here to ensure all VMAs in the mm are unmapped */
        unmap_vmas(&tlb, &mm->mm_mt, vma, 0, ULONG_MAX);

        /*
         * Set MMF_OOM_SKIP to hide this task from the oom killer/reaper
         * because the memory has been already freed. Do not bother checking
         * mm_is_oom_victim because setting a bit unconditionally is cheaper.
         */
        set_bit(MMF_OOM_SKIP, &mm->flags);
        free_pgtables(&tlb, &mm->mm_mt, vma, FIRST_USER_ADDRESS,
                      USER_PGTABLES_CEILING);
        tlb_finish_mmu(&tlb);

        /*
         * Walk the list again, actually closing and freeing it, with preemption
         * enabled, without holding any MM locks besides the unreachable
         * mmap_write_lock.
         */
        do {
                if (vma->vm_flags & VM_ACCOUNT)
                        nr_accounted += vma_pages(vma);
                remove_vma(vma);
                count++;

Quoted 8 lines

                cond_resched();
        } while ((vma = mas_find(&mas, ULONG_MAX)) != NULL);

        BUG_ON(count != mm->map_count);

        trace_exit_mmap(mm);
        __mt_destroy(&mm->mm_mt);
        mm->mmap = NULL;

^^^ this line above needs to be removed when the patch is applied over the maple tree patchset.

Quoted 4 lines

        mmap_write_unlock(mm);
        vm_unacct_memory(nr_accounted);
}

On Wed, Jun 1, 2022 at 2:36 PM Andrew Morton <[email protected]> wrote:

Quoted 53 lines

But iirc mapletree wants to retain a write_lock here, so I ended up with

void exit_mmap(struct mm_struct *mm)

{
        struct mmu_gather tlb;
        struct vm_area_struct *vma;
        unsigned long nr_accounted = 0;
        MA_STATE(mas, &mm->mm_mt, 0, 0);
        int count = 0;

        /* mm's last user has gone, and its about to be pulled down */
        mmu_notifier_release(mm);

        mmap_write_lock(mm);
        arch_exit_mmap(mm);

        vma = mas_find(&mas, ULONG_MAX);
        if (!vma) {
                /* Can happen if dup_mmap() received an OOM */
                mmap_write_unlock(mm);
                return;
        }

        lru_add_drain();
        flush_cache_mm(mm);
        tlb_gather_mmu_fullmm(&tlb, mm);
        /* update_hiwater_rss(mm) here? but nobody should be looking */
        /* Use ULONG_MAX here to ensure all VMAs in the mm are unmapped */
        unmap_vmas(&tlb, &mm->mm_mt, vma, 0, ULONG_MAX);

        /*
         * Set MMF_OOM_SKIP to hide this task from the oom killer/reaper
         * because the memory has been already freed. Do not bother checking
         * mm_is_oom_victim because setting a bit unconditionally is cheaper.
         */
        set_bit(MMF_OOM_SKIP, &mm->flags);
        free_pgtables(&tlb, &mm->mm_mt, vma, FIRST_USER_ADDRESS,
                      USER_PGTABLES_CEILING);
        tlb_finish_mmu(&tlb);

        /*
         * Walk the list again, actually closing and freeing it, with preemption
         * enabled, without holding any MM locks besides the unreachable
         * mmap_write_lock.
         */
        do {
                if (vma->vm_flags & VM_ACCOUNT)
                        nr_accounted += vma_pages(vma);
                remove_vma(vma);
                count++;
                cond_resched();
        } while ((vma = mas_find(&mas, ULONG_MAX)) != NULL);

Quoted 5 lines

        BUG_ON(count != mm->map_count);

        trace_exit_mmap(mm);
        __mt_destroy(&mm->mm_mt);
        mm->mmap = NULL;

^^^ this line above needs to be removed when the patch is applied over the maple tree patchset.

Quoted 1 line · Suren Baghdasaryan

On Wed, Jun 1, 2022 at 2:36 PM Andrew Morton <[email protected]> wrote:

[...]

Quoted 59 lines

Quoted 58 lines

But iirc mapletree wants to retain a write_lock here, so I ended up with

void exit_mmap(struct mm_struct *mm)

{
        struct mmu_gather tlb;
        struct vm_area_struct *vma;
        unsigned long nr_accounted = 0;
        MA_STATE(mas, &mm->mm_mt, 0, 0);
        int count = 0;

        /* mm's last user has gone, and its about to be pulled down */
        mmu_notifier_release(mm);

        mmap_write_lock(mm);
        arch_exit_mmap(mm);

        vma = mas_find(&mas, ULONG_MAX);
        if (!vma) {
                /* Can happen if dup_mmap() received an OOM */
                mmap_write_unlock(mm);
                return;
        }

        lru_add_drain();
        flush_cache_mm(mm);
        tlb_gather_mmu_fullmm(&tlb, mm);
        /* update_hiwater_rss(mm) here? but nobody should be looking */
        /* Use ULONG_MAX here to ensure all VMAs in the mm are unmapped */
        unmap_vmas(&tlb, &mm->mm_mt, vma, 0, ULONG_MAX);

        /*
         * Set MMF_OOM_SKIP to hide this task from the oom killer/reaper
         * because the memory has been already freed. Do not bother checking
         * mm_is_oom_victim because setting a bit unconditionally is cheaper.
         */
        set_bit(MMF_OOM_SKIP, &mm->flags);
        free_pgtables(&tlb, &mm->mm_mt, vma, FIRST_USER_ADDRESS,
                      USER_PGTABLES_CEILING);
        tlb_finish_mmu(&tlb);

        /*
         * Walk the list again, actually closing and freeing it, with preemption
         * enabled, without holding any MM locks besides the unreachable
         * mmap_write_lock.
         */
        do {
                if (vma->vm_flags & VM_ACCOUNT)
                        nr_accounted += vma_pages(vma);
                remove_vma(vma);
                count++;
                cond_resched();
        } while ((vma = mas_find(&mas, ULONG_MAX)) != NULL);

        BUG_ON(count != mm->map_count);

        trace_exit_mmap(mm);
        __mt_destroy(&mm->mm_mt);
        mm->mmap = NULL;

Quoted 2 lines

^^^ this line above needs to be removed when the patch is applied over the maple tree patchset.

I am not fully up to date on the maple tree changes. Could you explain why resetting mm->mmap is not needed anymore please?

Quoted 65 lines · Michal Hocko

Quoted 1 line · Suren Baghdasaryan

On Wed, Jun 1, 2022 at 2:36 PM Andrew Morton <[email protected]> wrote:

[...]

Quoted 61 lines

Quoted 50 lines

But iirc mapletree wants to retain a write_lock here, so I ended up with

void exit_mmap(struct mm_struct *mm)

{
        struct mmu_gather tlb;
        struct vm_area_struct *vma;
        unsigned long nr_accounted = 0;
        MA_STATE(mas, &mm->mm_mt, 0, 0);
        int count = 0;

        /* mm's last user has gone, and its about to be pulled down */
        mmu_notifier_release(mm);

        mmap_write_lock(mm);
        arch_exit_mmap(mm);

        vma = mas_find(&mas, ULONG_MAX);
        if (!vma) {
                /* Can happen if dup_mmap() received an OOM */
                mmap_write_unlock(mm);
                return;
        }

        lru_add_drain();
        flush_cache_mm(mm);
        tlb_gather_mmu_fullmm(&tlb, mm);
        /* update_hiwater_rss(mm) here? but nobody should be looking */
        /* Use ULONG_MAX here to ensure all VMAs in the mm are unmapped */
        unmap_vmas(&tlb, &mm->mm_mt, vma, 0, ULONG_MAX);

        /*
         * Set MMF_OOM_SKIP to hide this task from the oom killer/reaper
         * because the memory has been already freed. Do not bother checking
         * mm_is_oom_victim because setting a bit unconditionally is cheaper.
         */
        set_bit(MMF_OOM_SKIP, &mm->flags);
        free_pgtables(&tlb, &mm->mm_mt, vma, FIRST_USER_ADDRESS,
                      USER_PGTABLES_CEILING);
        tlb_finish_mmu(&tlb);

        /*
         * Walk the list again, actually closing and freeing it, with preemption
         * enabled, without holding any MM locks besides the unreachable
         * mmap_write_lock.
         */
        do {
                if (vma->vm_flags & VM_ACCOUNT)
                        nr_accounted += vma_pages(vma);
                remove_vma(vma);
                count++;

Quoted 8 lines

                cond_resched();
        } while ((vma = mas_find(&mas, ULONG_MAX)) != NULL);

        BUG_ON(count != mm->map_count);

        trace_exit_mmap(mm);
        __mt_destroy(&mm->mm_mt);
        mm->mmap = NULL;

^^^ this line above needs to be removed when the patch is applied over the maple tree patchset.

Quoted 2 lines

I am not fully up to date on the maple tree changes. Could you explain why resetting mm->mmap is not needed anymore please?

The maple tree patch set removes the linked list, including mm->mmap. The call to __mt_destroy() means none of the old VMAs can be found in the race condition that mm->mmap = NULL was solving.

Quoted 2 lines

Unclear why this patch fiddles with the mm_struct locking in this fashion - changelogging that would have been helpful.

Yeah, I should have clarified this in the description. Everything up to unmap_vmas() can be done under mmap_read_lock and that way oom-reaper and process_mrelease can do the unmapping in parallel with exit_mmap. That's the reason we take mmap_read_lock, unmap the vmas, mark the mm with MMF_OOM_SKIP and take the mmap_write_lock to execute free_pgtables. I think maple trees do not change that except there is no mm->mmap anymore, so the line at the end of exit_mmap where we reset mm->mmap to NULL can be removed (I show that line below).

Quoted 11 lines · Suren Baghdasaryan

Quoted 2 lines

Unclear why this patch fiddles with the mm_struct locking in this fashion - changelogging that would have been helpful.

Yeah, I should have clarified this in the description. Everything up to unmap_vmas() can be done under mmap_read_lock and that way oom-reaper and process_mrelease can do the unmapping in parallel with exit_mmap. That's the reason we take mmap_read_lock, unmap the vmas, mark the mm with MMF_OOM_SKIP and take the mmap_write_lock to execute free_pgtables. I think maple trees do not change that except there is no mm->mmap anymore, so the line at the end of exit_mmap where we reset mm->mmap to NULL can be removed (I show that line below).

I don't understand why we _want_ unmapping to proceed in parallel? Is it so urgent to unmap these page tables that we need two processes doing it at the same time? And doesn't that just change the contention from visible (contention on a lock) to invisible (contention on cachelines)?