From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6EA3FC433F5 for ; Thu, 19 May 2022 21:33:17 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id DE5176B0071; Thu, 19 May 2022 17:33:16 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D6F0C6B0072; Thu, 19 May 2022 17:33:16 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id BE6DF6B0073; Thu, 19 May 2022 17:33:16 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id A91046B0071 for ; Thu, 19 May 2022 17:33:16 -0400 (EDT) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 778B3110F for ; Thu, 19 May 2022 21:33:16 +0000 (UTC) X-FDA: 79483793592.17.F76F287 Received: from mail-yb1-f170.google.com (mail-yb1-f170.google.com [209.85.219.170]) by imf19.hostedemail.com (Postfix) with ESMTP id 172D51A0014 for ; Thu, 19 May 2022 21:33:04 +0000 (UTC) Received: by mail-yb1-f170.google.com with SMTP id x2so10871706ybi.8 for ; Thu, 19 May 2022 14:33:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=KJgyhCLbfkLUlSVWc6gxLNMA5weA8FPJHsM5kmfSHZA=; b=LXpKiUAOH0sNgfz3jtpzeUN1PQlPI5k7fHa4FUYrTAjjYbGUn5zPg7ABBB5TN2PhzU XleloqOyz5XDe0KK9pUnddYi/32YW83PGL6ZvaNvwrQt9L6fft/dyVwgi9o/msGKI/Ts dEQPfFMiyTQ/XNYcd41ZijdK5DQ6icZaKaEb+Wa3LbSLJI5203kaPzyVJSzGWXAJqu5J l7ZdrLqGlBhJGkX7A8nxf4sRwdq8DlWAAzmzlRyHKregcB9UVMf4FdbmXuMbN4QBkreF Y86MMjihwgkJyQHUBpiToc2FBkOUOuiphgdRt7hn86E6vuvrjn5ohsEmZnyctpoeK2EA HIQw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=KJgyhCLbfkLUlSVWc6gxLNMA5weA8FPJHsM5kmfSHZA=; b=SVoW7E5WAHVm4bQC+9lPa3PdmZil/x3DOMfkBaKBJdBKMHs98AfZz/vV3TUiSqJJA9 rNSIz2qK4MQzwuQGxo4s6GFSTdkyLiwI1vPTYHOgGvR+WXto8EowTvOtbEh21mW646pF EHDd1KZvk3K8bivocJKJNwc0bjQrI8ubtBhR4XV4idALY8ziJ6eM+08wUGiF/WfO5nZH zGZfO3V6JFDLrJ+LiUftmvExzzEtkchZkqlXIpI8CM6FX/K/0YzatVCZ17+Ssq9Ukp/h HoEq7gfLtDSNVk8xLB9PrE+iYVupzN3293sGXUnKme/atvE7BHK67CZCDZM0iISnc9L0 RbIg== X-Gm-Message-State: AOAM5311vXXNvn0FwlzBjuVyeu+o58nsGZ/rtzvIGs0Vi+KNohmTln8y N/5X1FgkKHt4cOw+QWjN0g1vu86EIuAfzVHLxiCIjQ== X-Google-Smtp-Source: ABdhPJyXhxGaWmOzAjD8Cj5tgCTdYVhrMShQ1nFkxKlnJuE+Usj/HWxjoHYX3vaGQfevsqcKg5rliqkUr6ccovN6Ugk= X-Received: by 2002:a5b:30d:0:b0:64b:4193:7911 with SMTP id j13-20020a5b030d000000b0064b41937911mr6150984ybp.509.1652995994845; Thu, 19 May 2022 14:33:14 -0700 (PDT) MIME-Version: 1.0 References: <20220516075619.1277152-1-surenb@google.com> <20220519202149.3ywynqhbxlzp6uyn@revolver> In-Reply-To: <20220519202149.3ywynqhbxlzp6uyn@revolver> From: Suren Baghdasaryan Date: Thu, 19 May 2022 14:33:03 -0700 Message-ID: Subject: Re: [PATCH v2 1/2] mm: drop oom code from exit_mmap To: Liam Howlett Cc: "akpm@linux-foundation.org" , "mhocko@suse.com" , "rientjes@google.com" , "willy@infradead.org" , "hannes@cmpxchg.org" , "guro@fb.com" , "minchan@kernel.org" , "kirill@shutemov.name" , "aarcange@redhat.com" , "brauner@kernel.org" , "hch@infradead.org" , "oleg@redhat.com" , "david@redhat.com" , "jannh@google.com" , "shakeelb@google.com" , "peterx@redhat.com" , "jhubbard@nvidia.com" , "shuah@kernel.org" , "linux-kernel@vger.kernel.org" , "linux-mm@kvack.org" , "kernel-team@android.com" Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 172D51A0014 X-Stat-Signature: uawfx9kom8phjntcmghkmzxhyfncwes6 X-Rspam-User: Authentication-Results: imf19.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=LXpKiUAO; spf=pass (imf19.hostedemail.com: domain of surenb@google.com designates 209.85.219.170 as permitted sender) smtp.mailfrom=surenb@google.com; dmarc=pass (policy=reject) header.from=google.com X-Rspamd-Server: rspam09 X-HE-Tag: 1652995984-483062 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, May 19, 2022 at 1:22 PM Liam Howlett wrote: > > * Suren Baghdasaryan [220516 03:56]: > > 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") > > > > Signed-off-by: Suren Baghdasaryan > > Acked-by: Michal Hocko > > --- > > include/linux/oom.h | 2 -- > > mm/mmap.c | 31 ++++++++++++------------------- > > mm/oom_kill.c | 2 +- > > 3 files changed, 13 insertions(+), 22 deletions(-) > > > > diff --git a/include/linux/oom.h b/include/linux/oom.h > > index 2db9a1432511..6cdf0772dbae 100644 > > --- a/include/linux/oom.h > > +++ b/include/linux/oom.h > > @@ -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); > > > > diff --git a/mm/mmap.c b/mm/mmap.c > > index 313b57d55a63..ded42150e706 100644 > > --- a/mm/mmap.c > > +++ b/mm/mmap.c > > @@ -3105,30 +3105,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); > > - set_bit(MMF_OOM_SKIP, &mm->flags); > > - } > > - > > - mmap_write_lock(mm); > > + mmap_read_lock(mm); > > arch_exit_mmap(mm); > > arch_exit_mmap() was called under the write lock before, is it safe to > call it under the read lock? Ah, good catch. I missed at least one call chain which I believe would require arch_exit_mmap() to be called under write lock: arch_exit_mmap ldt_arch_exit_mmap free_ldt_pgtables free_pgd_range I'll need to check whether arch_exit_mmap() has to be called before unmap_vmas(). If not, we could move it further down when we hold the write lock. Andrew, please remove this patchset from your tree for now until I fix this. > > > > > vma = mm->mmap; > > if (!vma) { > > /* Can happen if dup_mmap() received an OOM */ > > - mmap_write_unlock(mm); > > + mmap_read_unlock(mm); > > return; > > } > > > > @@ -3138,6 +3121,16 @@ void exit_mmap(struct mm_struct *mm) > > /* update_hiwater_rss(mm) here? but nobody should be looking */ > > /* Use -1 here to ensure all VMAs in the mm are unmapped */ > > unmap_vmas(&tlb, vma, 0, -1); > > + mmap_read_unlock(mm); > > + > > + /* > > + * 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); > > + > > + mmap_write_lock(mm); > > Is there a race here? We had a VMA but after the read lock was dropped, > could the oom killer cause the VMA to be invalidated? I don't think so > but the comment above about dup_mmap() receiving an OOM makes me > question it. The code before kept the write lock from when the VMA was > found until the end of the mm edits - and it had the check for !vma > within the block itself. We are also hiding it from the oom killer > outside the read lock so it is possible for oom to find it in that > window, right? When I was trying to understand that comment and looked into dup_mmap() code, my conclusion was that this check was there to protect us from the case when dup_mmap() gets interrupted and leaves mm->mmap=NULL. So, in a sense it was not really a race with OOM killer but an interrupted dup_mmap() case. So, once we checked it above we don't need to recheck again under write lock. When I asked Michal about this he was in agreement but it's possible we overlooked some corner case. If so, please let me know and I can add this check here. > > Could we just unconditionally set the skip bit before taking a write > lock for the duration of the exit? I'm probably missing your reason for > doing it this way. That's what I'm doing - unconditionally setting MMF_OOM_SKIP before taking the write lock. Did I miss something? > > > free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING); > > tlb_finish_mmu(&tlb); > > > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > > index 49d7df39b02d..36355b162727 100644 > > --- a/mm/oom_kill.c > > +++ b/mm/oom_kill.c > > @@ -509,7 +509,7 @@ static DECLARE_WAIT_QUEUE_HEAD(oom_reaper_wait); > > static struct task_struct *oom_reaper_list; > > static DEFINE_SPINLOCK(oom_reaper_lock); > > > > -bool __oom_reap_task_mm(struct mm_struct *mm) > > +static bool __oom_reap_task_mm(struct mm_struct *mm) > > { > > struct vm_area_struct *vma; > > bool ret = true; > > -- > > 2.36.0.550.gb090851708-goog > > > > > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. >