Re: [PATCH v3 05/11] iommu/sva: Assign a PASID to mm on PASID allocation and free it on mm exit

[Fenghua Yu]

On Sat, Feb 05, 2022 at 12:56:00AM +0100, Thomas Gleixner wrote:
> On Fri, Jan 28 2022 at 12:28, Fenghua Yu wrote:
> > To avoid complexity of updating each thread's PASID status (e.g. sending
> > IPI to update IA32_PASID MSR) on allocating and freeing PASID, once
> > allocated and assigned to an mm, the PASID stays with the mm for the
> > rest of the mm's lifetime. A reference to the PASID is taken on
> > allocating the PASID. Binding/unbinding the PASID won't change refcount.
> > The reference is dropped on mm exit and thus the PASID is freed.
> >
> > Two helpers mm_pasid_set() and mm_pasid_drop() are defined in mm because
> > the PASID operations handle the pasid member in mm_struct and should be
> > part of mm operations. Because IOASID's reference count is not used any
> > more and removed, unused ioasid_get() and iommu_sva_free_pasid()
> > are deleted and ioasid_put() is renamed to ioasid_free().
> >
> > 20-bit PASID allows up to 1M processes bound to PASIDs at the same time.
> > With cgroups and other controls that might limit the number of process
> > creation, the limited number of PASIDs is not a realistic issue for
> > lazy PASID free.
> 
> Please take a step back and think hard about it whether that changelog
> makes sense to you a year from now.
> 
> Let me walk you through:
> 
> > To avoid complexity of updating each thread's PASID status (e.g. sending
> > IPI to update IA32_PASID MSR) on allocating and freeing PASID, once
> > allocated and assigned to an mm, the PASID stays with the mm for the
> > rest of the mm's lifetime.
> 
> You are missing the oportunity to tell a story about the history of this
> decision here:
> 
>   PASIDs are process wide. It was attempted to use refcounted PASIDs to
>   free them when the last thread drops the refcount. This turned out to
>   be complex and error prone. Given the fact that the PASID space is 20
>   bits, which allows up to 1M processes to have a PASID associated
>   concurrently, PASID resource exhaustion is not a realistic concern.
> 
>   Therefore it was decided to simplify the approach and stick with lazy
>   on demand PASID allocation, but drop the eager free approach and make
>   a allocated PASID lifetime bound to the life time of the process.
> 
> > A reference to the PASID is taken on allocating the
> > PASID. Binding/unbinding the PASID won't change refcount.  The
> > reference is dropped on mm exit and thus the PASID is freed.
> 
> There is no refcount in play anymore, right? So how does this part of
> the changelog make any sense?
> 
> This is followed by:
> 
> > Two helpers mm_pasid_set() and mm_pasid_drop() are defined in mm because
> > the PASID operations handle the pasid member in mm_struct and should be
> > part of mm operations. Because IOASID's reference count is not used any
> > more and removed, unused ioasid_get() and iommu_sva_free_pasid()
> > are deleted and ioasid_put() is renamed to ioasid_free().
> 
> which does not provide much rationale and just blurbs about _what_ the
> patch is doing and not about the why and the connection to the
> above. And the refcount removal section contradicts the stale text
> above.
> 
> So this paragraph should be something like this:
> 
>   Get rid of the refcounting mechanisms and replace/rename the
>   interfaces to reflect this new approach.
> 
> Hmm?

Sure. I will update the commit message with your comments.

Thanks.

-Fenghua