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 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
