lkml.org 
[lkml]   [2017]   [Dec]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [patch v2 1/2] mm, mmu_notifier: annotate mmu notifiers with blockable invalidate callbacks
    On Thu 14-12-17 13:30:56, David Rientjes wrote:
    > Commit 4d4bbd8526a8 ("mm, oom_reaper: skip mm structs with mmu notifiers")
    > prevented the oom reaper from unmapping private anonymous memory with the
    > oom reaper when the oom victim mm had mmu notifiers registered.
    >
    > The rationale is that doing mmu_notifier_invalidate_range_{start,end}()
    > around the unmap_page_range(), which is needed, can block and the oom
    > killer will stall forever waiting for the victim to exit, which may not
    > be possible without reaping.
    >
    > That concern is real, but only true for mmu notifiers that have blockable
    > invalidate_range_{start,end}() callbacks. This patch adds a "flags" field
    > to mmu notifier ops that can set a bit to indicate that these callbacks do
    > not block.
    >
    > The implementation is steered toward an expensive slowpath, such as after
    > the oom reaper has grabbed mm->mmap_sem of a still alive oom victim.
    >
    > Signed-off-by: David Rientjes <rientjes@google.com>

    Yes, this make sense. I haven't checked all the existing mmu notifiers
    but those that you have marked seem to be OK.

    I just think that the semantic of the flag should be describe more. See
    below

    Acked-by: Michal Hocko <mhocko@suse.com>

    > ---
    > v2:
    > - specifically exclude mmu_notifiers without invalidate callbacks
    > - move flags to mmu_notifier_ops per Paolo
    > - reverse flag from blockable -> not blockable per Christian
    >
    > drivers/infiniband/hw/hfi1/mmu_rb.c | 1 +
    > drivers/iommu/amd_iommu_v2.c | 1 +
    > drivers/iommu/intel-svm.c | 1 +
    > drivers/misc/sgi-gru/grutlbpurge.c | 1 +
    > include/linux/mmu_notifier.h | 21 +++++++++++++++++++++
    > mm/mmu_notifier.c | 31 +++++++++++++++++++++++++++++++
    > virt/kvm/kvm_main.c | 1 +
    > 7 files changed, 57 insertions(+)
    >
    [...]
    > diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h
    > --- a/include/linux/mmu_notifier.h
    > +++ b/include/linux/mmu_notifier.h
    > @@ -10,6 +10,9 @@
    > struct mmu_notifier;
    > struct mmu_notifier_ops;
    >
    > +/* mmu_notifier_ops flags */
    > +#define MMU_INVALIDATE_DOES_NOT_BLOCK (0x01)
    > +
    > #ifdef CONFIG_MMU_NOTIFIER
    >
    > /*
    > @@ -26,6 +29,15 @@ struct mmu_notifier_mm {
    > };
    >
    > struct mmu_notifier_ops {
    > + /*
    > + * Flags to specify behavior of callbacks for this MMU notifier.
    > + * Used to determine which context an operation may be called.
    > + *
    > + * MMU_INVALIDATE_DOES_NOT_BLOCK: invalidate_{start,end} does not
    > + * block
    > + */
    > + int flags;

    This should be more specific IMHO. What do you think about the following
    wording?

    invalidate_{start,end,range} doesn't block on any locks which depend
    directly or indirectly (via lock chain or resources e.g. worker context)
    on a memory allocation.

    > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
    > --- a/virt/kvm/kvm_main.c
    > +++ b/virt/kvm/kvm_main.c
    > @@ -476,6 +476,7 @@ static void kvm_mmu_notifier_release(struct mmu_notifier *mn,
    > }
    >
    > static const struct mmu_notifier_ops kvm_mmu_notifier_ops = {
    > + .flags = MMU_INVALIDATE_DOES_NOT_BLOCK,
    > .invalidate_range_start = kvm_mmu_notifier_invalidate_range_start,
    > .invalidate_range_end = kvm_mmu_notifier_invalidate_range_end,
    > .clear_flush_young = kvm_mmu_notifier_clear_flush_young,

    --
    Michal Hocko
    SUSE Labs

    \
     
     \ /
      Last update: 2017-12-15 17:26    [W:5.264 / U:0.400 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site