On Tue, Jul 18, 2023 at 05:56:15PM +1000, Alistair Popple wrote:
> 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?
> 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?
> @@ -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
