Re: [PATCH 1/4] mm_notifiers: Rename invalidate_range notifier
From: Jason Gunthorpe <jgg@ziepe.ca>
Date: 2023-07-18 17:57:19
Also in:
kvm, linux-arm-kernel, linux-iommu, linux-mm, lkml
On Tue, Jul 18, 2023 at 05:56:15PM +1000, Alistair Popple wrote:
quoted hunk ↗ jump to hunk
diff --git a/include/asm-generic/tlb.h b/include/asm-generic/tlb.h index b466172..48c81b9 100644 --- a/include/asm-generic/tlb.h +++ b/include/asm-generic/tlb.h@@ -456,7 +456,7 @@ static inline void tlb_flush_mmu_tlbonly(struct mmu_gather *tlb) return; tlb_flush(tlb); - mmu_notifier_invalidate_range(tlb->mm, tlb->start, tlb->end); + mmu_notifier_invalidate_secondary_tlbs(tlb->mm, tlb->start, tlb->end); __tlb_reset_range(tlb);
Does this compile? I don't see "mmu_notifier_invalidate_secondary_tlbs" ? Maybe we don't need to rename this function since you pretty much remove it in the next patches?
quoted hunk ↗ jump to hunk
diff --git a/mm/mmu_notifier.c b/mm/mmu_notifier.c index 50c0dde..34c5a84 100644 --- a/mm/mmu_notifier.c +++ b/mm/mmu_notifier.c@@ -207,7 +207,7 @@ mmu_interval_read_begin(struct mmu_interval_notifier *interval_sub) * spin_lock * seq = ++subscriptions->invalidate_seq * spin_unlock - * op->invalidate_range(): + * op->invalidate_secondary_tlbs():
The later patch should delete this stuff from the comment too, we no longer guarantee this relationship?
quoted hunk ↗ jump to hunk
@@ -560,23 +560,23 @@ mn_hlist_invalidate_end(struct mmu_notifier_subscriptions *subscriptions, hlist_for_each_entry_rcu(subscription, &subscriptions->list, hlist, srcu_read_lock_held(&srcu)) { /* - * Call invalidate_range here too to avoid the need for the - * subsystem of having to register an invalidate_range_end - * call-back when there is invalidate_range already. Usually a - * subsystem registers either invalidate_range_start()/end() or - * invalidate_range(), so this will be no additional overhead - * (besides the pointer check). + * Subsystems should register either invalidate_secondary_tlbs() + * or invalidate_range_start()/end() callbacks. * - * We skip call to invalidate_range() if we know it is safe ie - * call site use mmu_notifier_invalidate_range_only_end() which - * is safe to do when we know that a call to invalidate_range() - * already happen under page table lock. + * We call invalidate_secondary_tlbs() here so that subsystems + * can use larger range based invalidations. In some cases + * though invalidate_secondary_tlbs() needs to be called while + * holding the page table lock. In that case call sites use + * mmu_notifier_invalidate_range_only_end() and we know it is + * safe to skip secondary TLB invalidation as it will have + * already been done. */ - if (!only_end && subscription->ops->invalidate_range) - subscription->ops->invalidate_range(subscription, - range->mm, - range->start, - range->end); + if (!only_end && subscription->ops->invalidate_secondary_tlbs) + subscription->ops->invalidate_secondary_tlbs(
More doesn't compile, and the comment has the same issue.. But I think the approach in this series looks fine, it is so much cleaner after we remove all the cruft in patch 4, just look at the diffstat.. Jason