Re: [PATCH 2/2] iommu/amd: Remove clear_flush_young notifier

[Jean-Philippe Brucker]

Hi Peter,

On 30/01/2019 05:57, Peter Xu wrote:
> AMD IOMMU driver is using the clear_flush_young() to do cache flushing
> but that's actually already covered by invalidate_range().  Remove the
> extra notifier and the chunks.

Aren't uses of clear_flush_young() and invalidate_range() orthogonal? If
I understood correctly, when doing reclaim the kernel clears the young
bit from the PTE. This requires flushing secondary TLBs (ATCs), so that
new accesses from devices will go through the IOMMU, set the young bit
again and the kernel can accurately track page use. I didn't see
invalidate_range() being called by rmap or vmscan in this case, but
might just be missing it.

Two MMU notifiers are used for the young bit, clear_flush_young() and
clear_young(). I believe the former should invalidate ATCs so that DMA
accesses participate in PTE aging. Otherwise the kernel can't know that
the device is still using entries marked 'old'. The latter,
clear_young() is exempted from invalidating the secondary TLBs by
mmu_notifier.h and the IOMMU driver doesn't need to implement it.

Thanks,
Jean

> 
> Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
> ---
>  drivers/iommu/amd_iommu_v2.c | 24 ------------------------
>  1 file changed, 24 deletions(-)
> 
> diff --git a/drivers/iommu/amd_iommu_v2.c b/drivers/iommu/amd_iommu_v2.c
> index 23dae9348ace..5d7ef750e4a0 100644
> --- a/drivers/iommu/amd_iommu_v2.c
> +++ b/drivers/iommu/amd_iommu_v2.c
> @@ -370,29 +370,6 @@ static struct pasid_state *mn_to_state(struct mmu_notifier *mn)
>  	return container_of(mn, struct pasid_state, mn);
>  }
>  
> -static void __mn_flush_page(struct mmu_notifier *mn,
> -			    unsigned long address)
> -{
> -	struct pasid_state *pasid_state;
> -	struct device_state *dev_state;
> -
> -	pasid_state = mn_to_state(mn);
> -	dev_state   = pasid_state->device_state;
> -
> -	amd_iommu_flush_page(dev_state->domain, pasid_state->pasid, address);
> -}
> -
> -static int mn_clear_flush_young(struct mmu_notifier *mn,
> -				struct mm_struct *mm,
> -				unsigned long start,
> -				unsigned long end)
> -{
> -	for (; start < end; start += PAGE_SIZE)
> -		__mn_flush_page(mn, start);
> -
> -	return 0;
> -}
> -
>  static void mn_invalidate_range(struct mmu_notifier *mn,
>  				struct mm_struct *mm,
>  				unsigned long start, unsigned long end)
> @@ -430,7 +407,6 @@ static void mn_release(struct mmu_notifier *mn, struct mm_struct *mm)
>  
>  static const struct mmu_notifier_ops iommu_mn = {
>  	.release		= mn_release,
> -	.clear_flush_young      = mn_clear_flush_young,
>  	.invalidate_range       = mn_invalidate_range,
>  };
>  
>