Re: [PATCH v2 1/3] mm/mmu_notifier: use structure for invalidate_range_start/end callback
From: Kuehling, Felix
Date: Wed Dec 05 2018 - 16:42:52 EST
The amdgpu part looks good to me.
A minor nit-pick in mmu_notifier.c (inline).
Either way, the series is Acked-by: Felix Kuehling <Felix.Kuehling@xxxxxxx>
On 2018-12-05 12:36 a.m., jglisse@xxxxxxxxxx wrote:
> From: JÃrÃme Glisse <jglisse@xxxxxxxxxx>
>
> To avoid having to change many callback definition everytime we want
> to add a parameter use a structure to group all parameters for the
> mmu_notifier invalidate_range_start/end callback. No functional changes
> with this patch.
>
> Signed-off-by: JÃrÃme Glisse <jglisse@xxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Matthew Wilcox <mawilcox@xxxxxxxxxxxxx>
> Cc: Ross Zwisler <zwisler@xxxxxxxxxx>
> Cc: Jan Kara <jack@xxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Cc: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
> Cc: Michal Hocko <mhocko@xxxxxxxxxx>
> Cc: Christian Koenig <christian.koenig@xxxxxxx>
> Cc: Felix Kuehling <felix.kuehling@xxxxxxx>
> Cc: Ralph Campbell <rcampbell@xxxxxxxxxx>
> Cc: John Hubbard <jhubbard@xxxxxxxxxx>
> Cc: kvm@xxxxxxxxxxxxxxx
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Cc: linux-rdma@xxxxxxxxxxxxxxx
> Cc: linux-fsdevel@xxxxxxxxxxxxxxx
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c | 43 +++++++++++--------------
> drivers/gpu/drm/i915/i915_gem_userptr.c | 14 ++++----
> drivers/gpu/drm/radeon/radeon_mn.c | 16 ++++-----
> drivers/infiniband/core/umem_odp.c | 20 +++++-------
> drivers/infiniband/hw/hfi1/mmu_rb.c | 13 +++-----
> drivers/misc/mic/scif/scif_dma.c | 11 ++-----
> drivers/misc/sgi-gru/grutlbpurge.c | 14 ++++----
> drivers/xen/gntdev.c | 12 +++----
> include/linux/mmu_notifier.h | 14 +++++---
> mm/hmm.c | 23 ++++++-------
> mm/mmu_notifier.c | 21 ++++++++++--
> virt/kvm/kvm_main.c | 14 +++-----
> 12 files changed, 102 insertions(+), 113 deletions(-)
>
[snip]
> diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c
> index 5119ff846769..5f6665ae3ee2 100644
> --- a/mm/mmu_notifier.c
> +++ b/mm/mmu_notifier.c
> @@ -178,14 +178,20 @@ int __mmu_notifier_invalidate_range_start(struct mm_struct *mm,
> unsigned long start, unsigned long end,
> bool blockable)
> {
> + struct mmu_notifier_range _range, *range = &_range;
I'm not sure why you need to access _range indirectly through a pointer.
See below.
> struct mmu_notifier *mn;
> int ret = 0;
> int id;
>
> + range->blockable = blockable;
> + range->start = start;
> + range->end = end;
> + range->mm = mm;
This could just assign _range.blockable, _range.start, etc. without the
indirection. Or you could even use an initializer instead:
struct mmu_notifier_range range = {
ÂÂÂ .blockable = blockable,
ÂÂÂ .start = start,
ÂÂÂ ...
};
> +
> id = srcu_read_lock(&srcu);
> hlist_for_each_entry_rcu(mn, &mm->mmu_notifier_mm->list, hlist) {
> if (mn->ops->invalidate_range_start) {
> - int _ret = mn->ops->invalidate_range_start(mn, mm, start, end, blockable);
> + int _ret = mn->ops->invalidate_range_start(mn, range);
This could just use &_range without the indirection.
Same in ..._invalidate_range_end below.
Regards,
 Felix
> if (_ret) {
> pr_info("%pS callback failed with %d in %sblockable context.\n",
> mn->ops->invalidate_range_start, _ret,
> @@ -205,9 +211,20 @@ void __mmu_notifier_invalidate_range_end(struct mm_struct *mm,
> unsigned long end,
> bool only_end)
> {
> + struct mmu_notifier_range _range, *range = &_range;
> struct mmu_notifier *mn;
> int id;
>
> + /*
> + * The end call back will never be call if the start refused to go
> + * through because of blockable was false so here assume that we
> + * can block.
> + */
> + range->blockable = true;
> + range->start = start;
> + range->end = end;
> + range->mm = mm;
> +
> id = srcu_read_lock(&srcu);
> hlist_for_each_entry_rcu(mn, &mm->mmu_notifier_mm->list, hlist) {
> /*
> @@ -226,7 +243,7 @@ void __mmu_notifier_invalidate_range_end(struct mm_struct *mm,
> if (!only_end && mn->ops->invalidate_range)
> mn->ops->invalidate_range(mn, mm, start, end);
> if (mn->ops->invalidate_range_end)
> - mn->ops->invalidate_range_end(mn, mm, start, end);
> + mn->ops->invalidate_range_end(mn, range);
> }
> srcu_read_unlock(&srcu, id);
> }
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 2679e476b6c3..f829f63f2b16 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -360,10 +360,7 @@ static void kvm_mmu_notifier_change_pte(struct mmu_notifier *mn,
> }
>
> static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
> - struct mm_struct *mm,
> - unsigned long start,
> - unsigned long end,
> - bool blockable)
> + const struct mmu_notifier_range *range)
> {
> struct kvm *kvm = mmu_notifier_to_kvm(mn);
> int need_tlb_flush = 0, idx;
> @@ -377,7 +374,7 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
> * count is also read inside the mmu_lock critical section.
> */
> kvm->mmu_notifier_count++;
> - need_tlb_flush = kvm_unmap_hva_range(kvm, start, end);
> + need_tlb_flush = kvm_unmap_hva_range(kvm, range->start, range->end);
> need_tlb_flush |= kvm->tlbs_dirty;
> /* we've to flush the tlb before the pages can be freed */
> if (need_tlb_flush)
> @@ -385,7 +382,8 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
>
> spin_unlock(&kvm->mmu_lock);
>
> - ret = kvm_arch_mmu_notifier_invalidate_range(kvm, start, end, blockable);
> + ret = kvm_arch_mmu_notifier_invalidate_range(kvm, range->start,
> + range->end, range->blockable);
>
> srcu_read_unlock(&kvm->srcu, idx);
>
> @@ -393,9 +391,7 @@ static int kvm_mmu_notifier_invalidate_range_start(struct mmu_notifier *mn,
> }
>
> static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn,
> - struct mm_struct *mm,
> - unsigned long start,
> - unsigned long end)
> + const struct mmu_notifier_range *range)
> {
> struct kvm *kvm = mmu_notifier_to_kvm(mn);
>