Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Fenghua Yu <fenghua.yu@intel.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Andy Lutomirski <luto@kernel.org>,
        Tony Luck <tony.luck@intel.com>,
        Lu Baolu <baolu.lu@linux.intel.com>,
        Joerg Roedel <joro@8bytes.org>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Jacob Pan <jacob.jun.pan@linux.intel.com>,
        Ashok Raj <ashok.raj@intel.com>,
        Ravi V Shankar <ravi.v.shankar@intel.com>
Cc:     iommu@lists.linux-foundation.org, x86 <x86@kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Fenghua Yu <fenghua.yu@intel.com>
Subject: Re: [PATCH v3 05/11] iommu/sva: Assign a PASID to mm on PASID
 allocation and free it on mm exit
In-Reply-To: <20220128202905.2274672-6-fenghua.yu@intel.com>
References: <20220128202905.2274672-1-fenghua.yu@intel.com>
 <20220128202905.2274672-6-fenghua.yu@intel.com>
Date:   Sat, 05 Feb 2022 00:56:00 +0100
Message-ID: <87r18ib2zz.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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?

Thanks,

        tglx