RE: [PATCH v2 4/9] iommu/ioasid: Add reference couting functions
Hi Jacob,
> -Original Message-
> From: iommu [mailto:iommu-boun...@lists.linux-foundation.org] On Behalf Of
> Jacob Pan
> Sent: 22 August 2020 05:35
> To: iommu@lists.linux-foundation.org; LKML ;
> Jean-Philippe Brucker ; Lu Baolu
> ; Joerg Roedel ; David
> Woodhouse
> Cc: Tian, Kevin ; Raj Ashok ; Wu
> Hao
> Subject: [PATCH v2 4/9] iommu/ioasid: Add reference couting functions
>
> There can be multiple users of an IOASID, each user could have hardware
> contexts associated with the IOASID. In order to align lifecycles,
> reference counting is introduced in this patch. It is expected that when
> an IOASID is being freed, each user will drop a reference only after its
> context is cleared.
>
> Signed-off-by: Jacob Pan
> ---
> drivers/iommu/ioasid.c | 113
> +
> include/linux/ioasid.h | 4 ++
> 2 files changed, 117 insertions(+)
>
> diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> index f73b3dbfc37a..5f31d63c75b1 100644
> --- a/drivers/iommu/ioasid.c
> +++ b/drivers/iommu/ioasid.c
> @@ -717,6 +717,119 @@ int ioasid_set_for_each_ioasid(struct ioasid_set
> *set,
> EXPORT_SYMBOL_GPL(ioasid_set_for_each_ioasid);
>
> /**
> + * IOASID refcounting rules
> + * - ioasid_alloc() set initial refcount to 1
> + *
> + * - ioasid_free() decrement and test refcount.
> + * If refcount is 0, ioasid will be freed. Deleted from the system-wide
> + * xarray as well as per set xarray. The IOASID will be returned to the
> + * pool and available for new allocations.
> + *
> + * If recount is non-zero, mark IOASID as
> IOASID_STATE_FREE_PENDING.
> + * No new reference can be added. The IOASID is not returned to the
> pool
> + * for reuse.
> + * After free, ioasid_get() will return error but ioasid_find() and other
> + * non refcount adding APIs will continue to work until the last
> reference
> + * is dropped
> + *
> + * - ioasid_get() get a reference on an active IOASID
> + *
> + * - ioasid_put() decrement and test refcount of the IOASID.
> + * If refcount is 0, ioasid will be freed. Deleted from the system-wide
> + * xarray as well as per set xarray. The IOASID will be returned to the
> + * pool and available for new allocations.
> + * Do nothing if refcount is non-zero.
> + *
Is it better to have a return for this based on whether ioasid is freed or not?
I was going through Jean's SMMUv3 SVA patches[1] and that one returns true
if ioasid was freed. And that info is subsequently used to reset the pasid
associated
with a mm. Though, not sure that is still relevant or not.
Thanks,
Shameer
1.
https://lore.kernel.org/linux-iommu/20200918101852.582559-3-jean-phili...@linaro.org/
> + * - ioasid_find() does not take reference, caller must hold reference
> + *
> + * ioasid_free() can be called multiple times without error until all refs
> are
> + * dropped.
> + */
> +
> +int ioasid_get_locked(struct ioasid_set *set, ioasid_t ioasid)
> +{
> + struct ioasid_data *data;
> +
> + data = xa_load(_allocator->xa, ioasid);
> + if (!data) {
> + pr_err("Trying to get unknown IOASID %u\n", ioasid);
> + return -EINVAL;
> + }
> + if (data->state == IOASID_STATE_FREE_PENDING) {
> + pr_err("Trying to get IOASID being freed%u\n", ioasid);
> + return -EBUSY;
> + }
> +
> + if (set && data->set != set) {
> + pr_err("Trying to get IOASID not in set%u\n", ioasid);
> + /* data found but does not belong to the set */
> + return -EACCES;
> + }
> + refcount_inc(>users);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ioasid_get_locked);
> +
> +/**
> + * ioasid_get - Obtain a reference of an ioasid
> + * @set
> + * @ioasid
> + *
> + * Check set ownership if @set is non-null.
> + */
> +int ioasid_get(struct ioasid_set *set, ioasid_t ioasid)
> +{
> + int ret = 0;
> +
> + spin_lock(_allocator_lock);
> + ret = ioasid_get_locked(set, ioasid);
> + spin_unlock(_allocator_lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(ioasid_get);
> +
> +void ioasid_put_locked(struct ioasid_set *set, ioasid_t ioasid)
> +{
> + struct ioasid_data *data;
> +
> + data = xa_load(_allocator->xa, ioasid);
> + if (!data) {
> + pr_err("Trying to put unknown IOASID %u\n", ioasid);
> + return;
> + }
> +
> + if (set && data->set != set) {
> + pr_err("Trying to drop IOASID not in the set %u\n", ioasid);
> +
Re: [PATCH v2 4/9] iommu/ioasid: Add reference couting functions
On Tue, 1 Sep 2020 14:13:00 +0200
Auger Eric wrote:
> Hi Jacob,
>
> On 8/22/20 6:35 AM, Jacob Pan wrote:
> > There can be multiple users of an IOASID, each user could have
> > hardware contexts associated with the IOASID. In order to align
> > lifecycles, reference counting is introduced in this patch. It is
> > expected that when an IOASID is being freed, each user will drop a
> > reference only after its context is cleared.
> >
> > Signed-off-by: Jacob Pan
> > ---
> > drivers/iommu/ioasid.c | 113
> > +
> > include/linux/ioasid.h | 4 ++ 2 files changed, 117 insertions(+)
> >
> > diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> > index f73b3dbfc37a..5f31d63c75b1 100644
> > --- a/drivers/iommu/ioasid.c
> > +++ b/drivers/iommu/ioasid.c
> > @@ -717,6 +717,119 @@ int ioasid_set_for_each_ioasid(struct
> > ioasid_set *set, EXPORT_SYMBOL_GPL(ioasid_set_for_each_ioasid);
> >
> > /**
> > + * IOASID refcounting rules
> > + * - ioasid_alloc() set initial refcount to 1
> > + *
> > + * - ioasid_free() decrement and test refcount.
> > + * If refcount is 0, ioasid will be freed. Deleted from the
> > system-wide
> > + * xarray as well as per set xarray. The IOASID will be
> > returned to the
> > + * pool and available for new allocations.
> > + *
> > + * If recount is non-zero, mark IOASID as
> > IOASID_STATE_FREE_PENDING.
> s/recount/refcount
> > + * No new reference can be added. The IOASID is not returned
> > to the pool
> can be taken
will do. and move to doc as Jean suggested.
> > + * for reuse.
> > + * After free, ioasid_get() will return error but
> > ioasid_find() and other
> > + * non refcount adding APIs will continue to work until the
> > last reference
> > + * is dropped
> > + *
> > + * - ioasid_get() get a reference on an active IOASID
> > + *
> > + * - ioasid_put() decrement and test refcount of the IOASID.
> > + * If refcount is 0, ioasid will be freed. Deleted from the
> > system-wide
> > + * xarray as well as per set xarray. The IOASID will be
> > returned to the
> > + * pool and available for new allocations.
> > + * Do nothing if refcount is non-zero.
> I would drop this last sentence
will do.
> > + *
> > + * - ioasid_find() does not take reference, caller must hold
> > reference
> So can you use ioasid_find() once the state is
> IOASID_STATE_FREE_PENDING? According to Jean's reply, the "IOASID may
> be freed once ioasid_find() returns but not the returned data." So
> holding a ref is not mandated right?
right, you can still find the private data in FREE_PENDING state. I
will drop the comment.
> > + *
> > + * ioasid_free() can be called multiple times without error until
> > all refs are
> > + * dropped.
> > + */
> > +
> > +int ioasid_get_locked(struct ioasid_set *set, ioasid_t ioasid)
> > +{
> > + struct ioasid_data *data;
> > +
> > + data = xa_load(_allocator->xa, ioasid);
> > + if (!data) {
> > + pr_err("Trying to get unknown IOASID %u\n",
> > ioasid);
> > + return -EINVAL;
> > + }
> > + if (data->state == IOASID_STATE_FREE_PENDING) {
> > + pr_err("Trying to get IOASID being freed%u\n",
> > ioasid);
> > + return -EBUSY;
> > + }
> > +
> > + if (set && data->set != set) {
> > + pr_err("Trying to get IOASID not in set%u\n",
> > ioasid);
> maybe try to normalize your traces using always the same formatting
> for ioasids and ioasid sets. Also we can understand %u is the id of
> the set.
it is confusing, this is not the set ID. I will change to:
pr_err("Trying to get IOASID %u outside the set\n", ioasid);
> > + /* data found but does not belong to the set */
> you can get rid of this comment
> > + return -EACCES;
> > + }
> > + refcount_inc(>users);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(ioasid_get_locked);
> > +
> > +/**
> > + * ioasid_get - Obtain a reference of an ioasid
> > + * @set
> > + * @ioasid
> > + *
> > + * Check set ownership if @set is non-null.
> > + */
> > +int ioasid_get(struct ioasid_set *set, ioasid_t ioasid)
> > +{
> > + int ret = 0;
> > +
> > + spin_lock(_allocator_lock);
> > + ret = ioasid_get_locked(set, ioasid);
> > + spin_unlock(_allocator_lock);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(ioasid_get);
> > +
> > +void ioasid_put_locked(struct ioasid_set *set, ioasid_t ioasid)
> > +{
> > + struct ioasid_data *data;
> > +
> > + data = xa_load(_allocator->xa, ioasid);
> > + if (!data) {
> > + pr_err("Trying to put unknown IOASID %u\n",
> > ioasid);
> > + return;
> > + }
> > +
> > + if (set && data->set != set) {
> > + pr_err("Trying to drop IOASID not in the set
> > %u\n", ioasid);
> was set%u above
yes, same change below?
pr_err("Trying to drop IOASID %u outside the set\n", ioasid);
> > + return;
> > + }
> > +
> > + if (!refcount_dec_and_test(>users)) {
> > +
Re: [PATCH v2 4/9] iommu/ioasid: Add reference couting functions
On Tue, 25 Aug 2020 12:19:37 +0200
Jean-Philippe Brucker wrote:
> On Fri, Aug 21, 2020 at 09:35:13PM -0700, Jacob Pan wrote:
> > There can be multiple users of an IOASID, each user could have
> > hardware contexts associated with the IOASID. In order to align
> > lifecycles, reference counting is introduced in this patch. It is
> > expected that when an IOASID is being freed, each user will drop a
> > reference only after its context is cleared.
> >
> > Signed-off-by: Jacob Pan
> > ---
> > drivers/iommu/ioasid.c | 113
> > +
> > include/linux/ioasid.h | 4 ++ 2 files changed, 117 insertions(+)
> >
> > diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> > index f73b3dbfc37a..5f31d63c75b1 100644
> > --- a/drivers/iommu/ioasid.c
> > +++ b/drivers/iommu/ioasid.c
> > @@ -717,6 +717,119 @@ int ioasid_set_for_each_ioasid(struct
> > ioasid_set *set, EXPORT_SYMBOL_GPL(ioasid_set_for_each_ioasid);
> >
> > /**
> > + * IOASID refcounting rules
> > + * - ioasid_alloc() set initial refcount to 1
> > + *
> > + * - ioasid_free() decrement and test refcount.
> > + * If refcount is 0, ioasid will be freed. Deleted from the
> > system-wide
> > + * xarray as well as per set xarray. The IOASID will be
> > returned to the
> > + * pool and available for new allocations.
> > + *
> > + * If recount is non-zero, mark IOASID as
> > IOASID_STATE_FREE_PENDING.
> > + * No new reference can be added. The IOASID is not returned
> > to the pool
> > + * for reuse.
> > + * After free, ioasid_get() will return error but
> > ioasid_find() and other
> > + * non refcount adding APIs will continue to work until the
> > last reference
> > + * is dropped
> > + *
> > + * - ioasid_get() get a reference on an active IOASID
> > + *
> > + * - ioasid_put() decrement and test refcount of the IOASID.
> > + * If refcount is 0, ioasid will be freed. Deleted from the
> > system-wide
> > + * xarray as well as per set xarray. The IOASID will be
> > returned to the
> > + * pool and available for new allocations.
> > + * Do nothing if refcount is non-zero.
> > + *
> > + * - ioasid_find() does not take reference, caller must hold
> > reference
> > + *
> > + * ioasid_free() can be called multiple times without error until
> > all refs are
> > + * dropped.
> > + */
>
> Since you already document this in ioasid.rst, I'm not sure the
> comment is necessary. Maybe the doc for _free/_put would be better in
> the function's documentation.
>
good point. will move the free/put to doc.
> > +
> > +int ioasid_get_locked(struct ioasid_set *set, ioasid_t ioasid)
> > +{
> > + struct ioasid_data *data;
> > +
> > + data = xa_load(_allocator->xa, ioasid);
> > + if (!data) {
> > + pr_err("Trying to get unknown IOASID %u\n",
> > ioasid);
> > + return -EINVAL;
> > + }
> > + if (data->state == IOASID_STATE_FREE_PENDING) {
> > + pr_err("Trying to get IOASID being freed%u\n",
> > ioasid);
> > + return -EBUSY;
> > + }
> > +
> > + if (set && data->set != set) {
> > + pr_err("Trying to get IOASID not in set%u\n",
> > ioasid);
> > + /* data found but does not belong to the set */
> > + return -EACCES;
> > + }
> > + refcount_inc(>users);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(ioasid_get_locked);
>
> Is it necessary to export the *_locked variant? Who'd call them and
> how would they acquire the lock?
>
It is used by KVM inside the atomic notifier handler. Notifier chain is
invoked under the lock.
> > +
> > +/**
> > + * ioasid_get - Obtain a reference of an ioasid
> > + * @set
> > + * @ioasid
>
> Can be dropped. The doc checker will throw a warning, though.
>
yes, will do
> > + *
> > + * Check set ownership if @set is non-null.
> > + */
> > +int ioasid_get(struct ioasid_set *set, ioasid_t ioasid)
> > +{
> > + int ret = 0;
>
> No need to initialize ret
>
right,
> > +
> > + spin_lock(_allocator_lock);
> > + ret = ioasid_get_locked(set, ioasid);
> > + spin_unlock(_allocator_lock);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(ioasid_get);
> > +
> > +void ioasid_put_locked(struct ioasid_set *set, ioasid_t ioasid)
> > +{
> > + struct ioasid_data *data;
> > +
> > + data = xa_load(_allocator->xa, ioasid);
> > + if (!data) {
> > + pr_err("Trying to put unknown IOASID %u\n",
> > ioasid);
> > + return;
> > + }
> > +
> > + if (set && data->set != set) {
> > + pr_err("Trying to drop IOASID not in the set
> > %u\n", ioasid);
> > + return;
> > + }
> > +
> > + if (!refcount_dec_and_test(>users)) {
> > + pr_debug("%s: IOASID %d has %d remainning users\n",
> > + __func__, ioasid,
> > refcount_read(>users));
> > + return;
> > + }
> > + ioasid_do_free(data);
> > +}
> > +EXPORT_SYMBOL_GPL(ioasid_put_locked);
> > +
> > +/**
> > + * ioasid_put - Drop a reference
Re: [PATCH v2 4/9] iommu/ioasid: Add reference couting functions
Hi Jacob,
On 8/22/20 6:35 AM, Jacob Pan wrote:
> There can be multiple users of an IOASID, each user could have hardware
> contexts associated with the IOASID. In order to align lifecycles,
> reference counting is introduced in this patch. It is expected that when
> an IOASID is being freed, each user will drop a reference only after its
> context is cleared.
>
> Signed-off-by: Jacob Pan
> ---
> drivers/iommu/ioasid.c | 113
> +
> include/linux/ioasid.h | 4 ++
> 2 files changed, 117 insertions(+)
>
> diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> index f73b3dbfc37a..5f31d63c75b1 100644
> --- a/drivers/iommu/ioasid.c
> +++ b/drivers/iommu/ioasid.c
> @@ -717,6 +717,119 @@ int ioasid_set_for_each_ioasid(struct ioasid_set *set,
> EXPORT_SYMBOL_GPL(ioasid_set_for_each_ioasid);
>
> /**
> + * IOASID refcounting rules
> + * - ioasid_alloc() set initial refcount to 1
> + *
> + * - ioasid_free() decrement and test refcount.
> + * If refcount is 0, ioasid will be freed. Deleted from the system-wide
> + * xarray as well as per set xarray. The IOASID will be returned to the
> + * pool and available for new allocations.
> + *
> + * If recount is non-zero, mark IOASID as IOASID_STATE_FREE_PENDING.
s/recount/refcount
> + * No new reference can be added. The IOASID is not returned to the pool
can be taken
> + * for reuse.
> + * After free, ioasid_get() will return error but ioasid_find() and other
> + * non refcount adding APIs will continue to work until the last
> reference
> + * is dropped
> + *
> + * - ioasid_get() get a reference on an active IOASID
> + *
> + * - ioasid_put() decrement and test refcount of the IOASID.
> + * If refcount is 0, ioasid will be freed. Deleted from the system-wide
> + * xarray as well as per set xarray. The IOASID will be returned to the
> + * pool and available for new allocations.
> + * Do nothing if refcount is non-zero.
I would drop this last sentence
> + *
> + * - ioasid_find() does not take reference, caller must hold reference
So can you use ioasid_find() once the state is
IOASID_STATE_FREE_PENDING? According to Jean's reply, the "IOASID may be
freed once ioasid_find() returns but not the returned data." So holding
a ref is not mandated right?
> + *
> + * ioasid_free() can be called multiple times without error until all refs
> are
> + * dropped.
> + */
> +
> +int ioasid_get_locked(struct ioasid_set *set, ioasid_t ioasid)
> +{
> + struct ioasid_data *data;
> +
> + data = xa_load(_allocator->xa, ioasid);
> + if (!data) {
> + pr_err("Trying to get unknown IOASID %u\n", ioasid);
> + return -EINVAL;
> + }
> + if (data->state == IOASID_STATE_FREE_PENDING) {
> + pr_err("Trying to get IOASID being freed%u\n", ioasid);
> + return -EBUSY;
> + }
> +
> + if (set && data->set != set) {
> + pr_err("Trying to get IOASID not in set%u\n", ioasid);
maybe try to normalize your traces using always the same formatting for
ioasids and ioasid sets. Also we can understand %u is the id of the set.
> + /* data found but does not belong to the set */
you can get rid of this comment
> + return -EACCES;
> + }
> + refcount_inc(>users);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ioasid_get_locked);
> +
> +/**
> + * ioasid_get - Obtain a reference of an ioasid
> + * @set
> + * @ioasid
> + *
> + * Check set ownership if @set is non-null.
> + */
> +int ioasid_get(struct ioasid_set *set, ioasid_t ioasid)
> +{
> + int ret = 0;
> +
> + spin_lock(_allocator_lock);
> + ret = ioasid_get_locked(set, ioasid);
> + spin_unlock(_allocator_lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(ioasid_get);
> +
> +void ioasid_put_locked(struct ioasid_set *set, ioasid_t ioasid)
> +{
> + struct ioasid_data *data;
> +
> + data = xa_load(_allocator->xa, ioasid);
> + if (!data) {
> + pr_err("Trying to put unknown IOASID %u\n", ioasid);
> + return;
> + }
> +
> + if (set && data->set != set) {
> + pr_err("Trying to drop IOASID not in the set %u\n", ioasid);
was set%u above
> + return;
> + }
> +
> + if (!refcount_dec_and_test(>users)) {
> + pr_debug("%s: IOASID %d has %d remainning users\n",
> + __func__, ioasid, refcount_read(>users));
> + return;
> + }
> + ioasid_do_free(data);
> +}
> +EXPORT_SYMBOL_GPL(ioasid_put_locked);
> +
> +/**
> + * ioasid_put - Drop a reference of an ioasid
> + * @set
> + * @ioasid
> + *
> + * Check set ownership if @set is non-null.
> + */
> +void ioasid_put(struct ioasid_set *set, ioasid_t ioasid)
> +{
> + spin_lock(_allocator_lock);
> + ioasid_put_locked(set, ioasid);
> + spin_unlock(_allocator_lock);
> +}
> +EXPORT_SYMBOL_GPL(ioasid_put);
> +
> +/**
> * ioasid_find - Find IOASID data
> * @set:
Re: [PATCH v2 4/9] iommu/ioasid: Add reference couting functions
On Mon, Aug 24, 2020 at 10:26:55AM +0800, Lu Baolu wrote: > Hi Jacob, > > On 8/22/20 12:35 PM, Jacob Pan wrote: > > There can be multiple users of an IOASID, each user could have hardware > > contexts associated with the IOASID. In order to align lifecycles, > > reference counting is introduced in this patch. It is expected that when > > an IOASID is being freed, each user will drop a reference only after its > > context is cleared. > > > > Signed-off-by: Jacob Pan [...] > > +/** > >* ioasid_find - Find IOASID data > >* @set: the IOASID set > >* @ioasid: the IOASID to find > > Do you need to increase the refcount of the found ioasid and ask the > caller to drop it after use? Otherwise, the ioasid might be freed > elsewhere. ioasid_find() takes a getter function as parameter, which ensures that the returned data is valid. It fetches the IOASID data under rcu_read_lock() and calls the getter on the private data (for example mmget_not_zero() for bare-metal SVA). Given that, I don't think returning with a reference to the IOASID is necessary. The IOASID may be freed once ioasid_find() returns but not the returned data. Thanks, Jean ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 4/9] iommu/ioasid: Add reference couting functions
On Fri, Aug 21, 2020 at 09:35:13PM -0700, Jacob Pan wrote:
> There can be multiple users of an IOASID, each user could have hardware
> contexts associated with the IOASID. In order to align lifecycles,
> reference counting is introduced in this patch. It is expected that when
> an IOASID is being freed, each user will drop a reference only after its
> context is cleared.
>
> Signed-off-by: Jacob Pan
> ---
> drivers/iommu/ioasid.c | 113
> +
> include/linux/ioasid.h | 4 ++
> 2 files changed, 117 insertions(+)
>
> diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
> index f73b3dbfc37a..5f31d63c75b1 100644
> --- a/drivers/iommu/ioasid.c
> +++ b/drivers/iommu/ioasid.c
> @@ -717,6 +717,119 @@ int ioasid_set_for_each_ioasid(struct ioasid_set *set,
> EXPORT_SYMBOL_GPL(ioasid_set_for_each_ioasid);
>
> /**
> + * IOASID refcounting rules
> + * - ioasid_alloc() set initial refcount to 1
> + *
> + * - ioasid_free() decrement and test refcount.
> + * If refcount is 0, ioasid will be freed. Deleted from the system-wide
> + * xarray as well as per set xarray. The IOASID will be returned to the
> + * pool and available for new allocations.
> + *
> + * If recount is non-zero, mark IOASID as IOASID_STATE_FREE_PENDING.
> + * No new reference can be added. The IOASID is not returned to the pool
> + * for reuse.
> + * After free, ioasid_get() will return error but ioasid_find() and other
> + * non refcount adding APIs will continue to work until the last
> reference
> + * is dropped
> + *
> + * - ioasid_get() get a reference on an active IOASID
> + *
> + * - ioasid_put() decrement and test refcount of the IOASID.
> + * If refcount is 0, ioasid will be freed. Deleted from the system-wide
> + * xarray as well as per set xarray. The IOASID will be returned to the
> + * pool and available for new allocations.
> + * Do nothing if refcount is non-zero.
> + *
> + * - ioasid_find() does not take reference, caller must hold reference
> + *
> + * ioasid_free() can be called multiple times without error until all refs
> are
> + * dropped.
> + */
Since you already document this in ioasid.rst, I'm not sure the comment
is necessary. Maybe the doc for _free/_put would be better in the
function's documentation.
> +
> +int ioasid_get_locked(struct ioasid_set *set, ioasid_t ioasid)
> +{
> + struct ioasid_data *data;
> +
> + data = xa_load(_allocator->xa, ioasid);
> + if (!data) {
> + pr_err("Trying to get unknown IOASID %u\n", ioasid);
> + return -EINVAL;
> + }
> + if (data->state == IOASID_STATE_FREE_PENDING) {
> + pr_err("Trying to get IOASID being freed%u\n", ioasid);
> + return -EBUSY;
> + }
> +
> + if (set && data->set != set) {
> + pr_err("Trying to get IOASID not in set%u\n", ioasid);
> + /* data found but does not belong to the set */
> + return -EACCES;
> + }
> + refcount_inc(>users);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ioasid_get_locked);
Is it necessary to export the *_locked variant? Who'd call them and how
would they acquire the lock?
> +
> +/**
> + * ioasid_get - Obtain a reference of an ioasid
> + * @set
> + * @ioasid
Can be dropped. The doc checker will throw a warning, though.
> + *
> + * Check set ownership if @set is non-null.
> + */
> +int ioasid_get(struct ioasid_set *set, ioasid_t ioasid)
> +{
> + int ret = 0;
No need to initialize ret
> +
> + spin_lock(_allocator_lock);
> + ret = ioasid_get_locked(set, ioasid);
> + spin_unlock(_allocator_lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(ioasid_get);
> +
> +void ioasid_put_locked(struct ioasid_set *set, ioasid_t ioasid)
> +{
> + struct ioasid_data *data;
> +
> + data = xa_load(_allocator->xa, ioasid);
> + if (!data) {
> + pr_err("Trying to put unknown IOASID %u\n", ioasid);
> + return;
> + }
> +
> + if (set && data->set != set) {
> + pr_err("Trying to drop IOASID not in the set %u\n", ioasid);
> + return;
> + }
> +
> + if (!refcount_dec_and_test(>users)) {
> + pr_debug("%s: IOASID %d has %d remainning users\n",
> + __func__, ioasid, refcount_read(>users));
> + return;
> + }
> + ioasid_do_free(data);
> +}
> +EXPORT_SYMBOL_GPL(ioasid_put_locked);
> +
> +/**
> + * ioasid_put - Drop a reference of an ioasid
> + * @set
> + * @ioasid
> + *
> + * Check set ownership if @set is non-null.
> + */
> +void ioasid_put(struct ioasid_set *set, ioasid_t ioasid)
> +{
> + spin_lock(_allocator_lock);
> + ioasid_put_locked(set, ioasid);
> + spin_unlock(_allocator_lock);
> +}
> +EXPORT_SYMBOL_GPL(ioasid_put);
> +
> +/**
> * ioasid_find - Find IOASID data
> * @set: the IOASID set
> * @ioasid: the IOASID to find
> diff --git a/include/linux/ioasid.h
Re: [PATCH v2 4/9] iommu/ioasid: Add reference couting functions
Hi Jacob,
On 8/22/20 12:35 PM, Jacob Pan wrote:
There can be multiple users of an IOASID, each user could have hardware
contexts associated with the IOASID. In order to align lifecycles,
reference counting is introduced in this patch. It is expected that when
an IOASID is being freed, each user will drop a reference only after its
context is cleared.
Signed-off-by: Jacob Pan
---
drivers/iommu/ioasid.c | 113 +
include/linux/ioasid.h | 4 ++
2 files changed, 117 insertions(+)
diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
index f73b3dbfc37a..5f31d63c75b1 100644
--- a/drivers/iommu/ioasid.c
+++ b/drivers/iommu/ioasid.c
@@ -717,6 +717,119 @@ int ioasid_set_for_each_ioasid(struct ioasid_set *set,
EXPORT_SYMBOL_GPL(ioasid_set_for_each_ioasid);
/**
+ * IOASID refcounting rules
+ * - ioasid_alloc() set initial refcount to 1
+ *
+ * - ioasid_free() decrement and test refcount.
+ * If refcount is 0, ioasid will be freed. Deleted from the system-wide
+ * xarray as well as per set xarray. The IOASID will be returned to the
+ * pool and available for new allocations.
+ *
+ * If recount is non-zero, mark IOASID as IOASID_STATE_FREE_PENDING.
+ * No new reference can be added. The IOASID is not returned to the pool
+ * for reuse.
+ * After free, ioasid_get() will return error but ioasid_find() and other
+ * non refcount adding APIs will continue to work until the last reference
+ * is dropped
+ *
+ * - ioasid_get() get a reference on an active IOASID
+ *
+ * - ioasid_put() decrement and test refcount of the IOASID.
+ * If refcount is 0, ioasid will be freed. Deleted from the system-wide
+ * xarray as well as per set xarray. The IOASID will be returned to the
+ * pool and available for new allocations.
+ * Do nothing if refcount is non-zero.
+ *
+ * - ioasid_find() does not take reference, caller must hold reference
+ *
+ * ioasid_free() can be called multiple times without error until all refs are
+ * dropped.
+ */
+
+int ioasid_get_locked(struct ioasid_set *set, ioasid_t ioasid)
+{
+ struct ioasid_data *data;
+
+ data = xa_load(_allocator->xa, ioasid);
+ if (!data) {
+ pr_err("Trying to get unknown IOASID %u\n", ioasid);
+ return -EINVAL;
+ }
+ if (data->state == IOASID_STATE_FREE_PENDING) {
+ pr_err("Trying to get IOASID being freed%u\n", ioasid);
+ return -EBUSY;
+ }
+
+ if (set && data->set != set) {
+ pr_err("Trying to get IOASID not in set%u\n", ioasid);
+ /* data found but does not belong to the set */
+ return -EACCES;
+ }
+ refcount_inc(>users);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(ioasid_get_locked);
+
+/**
+ * ioasid_get - Obtain a reference of an ioasid
+ * @set
+ * @ioasid
+ *
+ * Check set ownership if @set is non-null.
+ */
+int ioasid_get(struct ioasid_set *set, ioasid_t ioasid)
+{
+ int ret = 0;
+
+ spin_lock(_allocator_lock);
+ ret = ioasid_get_locked(set, ioasid);
+ spin_unlock(_allocator_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(ioasid_get);
+
+void ioasid_put_locked(struct ioasid_set *set, ioasid_t ioasid)
+{
+ struct ioasid_data *data;
+
+ data = xa_load(_allocator->xa, ioasid);
+ if (!data) {
+ pr_err("Trying to put unknown IOASID %u\n", ioasid);
+ return;
+ }
+
+ if (set && data->set != set) {
+ pr_err("Trying to drop IOASID not in the set %u\n", ioasid);
+ return;
+ }
+
+ if (!refcount_dec_and_test(>users)) {
+ pr_debug("%s: IOASID %d has %d remainning users\n",
+ __func__, ioasid, refcount_read(>users));
+ return;
+ }
+ ioasid_do_free(data);
+}
+EXPORT_SYMBOL_GPL(ioasid_put_locked);
+
+/**
+ * ioasid_put - Drop a reference of an ioasid
+ * @set
+ * @ioasid
+ *
+ * Check set ownership if @set is non-null.
+ */
+void ioasid_put(struct ioasid_set *set, ioasid_t ioasid)
+{
+ spin_lock(_allocator_lock);
+ ioasid_put_locked(set, ioasid);
+ spin_unlock(_allocator_lock);
+}
+EXPORT_SYMBOL_GPL(ioasid_put);
+
+/**
* ioasid_find - Find IOASID data
* @set: the IOASID set
* @ioasid: the IOASID to find
Do you need to increase the refcount of the found ioasid and ask the
caller to drop it after use? Otherwise, the ioasid might be freed
elsewhere.
Best regards,
baolu
diff --git a/include/linux/ioasid.h b/include/linux/ioasid.h
index 412d025d440e..310abe4187a3 100644
--- a/include/linux/ioasid.h
+++ b/include/linux/ioasid.h
@@ -76,6 +76,10 @@ int ioasid_attach_data(ioasid_t ioasid, void *data);
int ioasid_register_allocator(struct ioasid_allocator_ops *allocator);
void ioasid_unregister_allocator(struct ioasid_allocator_ops *allocator);
void ioasid_is_in_set(struct ioasid_set *set, ioasid_t ioasid);
[PATCH v2 4/9] iommu/ioasid: Add reference couting functions
There can be multiple users of an IOASID, each user could have hardware
contexts associated with the IOASID. In order to align lifecycles,
reference counting is introduced in this patch. It is expected that when
an IOASID is being freed, each user will drop a reference only after its
context is cleared.
Signed-off-by: Jacob Pan
---
drivers/iommu/ioasid.c | 113 +
include/linux/ioasid.h | 4 ++
2 files changed, 117 insertions(+)
diff --git a/drivers/iommu/ioasid.c b/drivers/iommu/ioasid.c
index f73b3dbfc37a..5f31d63c75b1 100644
--- a/drivers/iommu/ioasid.c
+++ b/drivers/iommu/ioasid.c
@@ -717,6 +717,119 @@ int ioasid_set_for_each_ioasid(struct ioasid_set *set,
EXPORT_SYMBOL_GPL(ioasid_set_for_each_ioasid);
/**
+ * IOASID refcounting rules
+ * - ioasid_alloc() set initial refcount to 1
+ *
+ * - ioasid_free() decrement and test refcount.
+ * If refcount is 0, ioasid will be freed. Deleted from the system-wide
+ * xarray as well as per set xarray. The IOASID will be returned to the
+ * pool and available for new allocations.
+ *
+ * If recount is non-zero, mark IOASID as IOASID_STATE_FREE_PENDING.
+ * No new reference can be added. The IOASID is not returned to the pool
+ * for reuse.
+ * After free, ioasid_get() will return error but ioasid_find() and other
+ * non refcount adding APIs will continue to work until the last reference
+ * is dropped
+ *
+ * - ioasid_get() get a reference on an active IOASID
+ *
+ * - ioasid_put() decrement and test refcount of the IOASID.
+ * If refcount is 0, ioasid will be freed. Deleted from the system-wide
+ * xarray as well as per set xarray. The IOASID will be returned to the
+ * pool and available for new allocations.
+ * Do nothing if refcount is non-zero.
+ *
+ * - ioasid_find() does not take reference, caller must hold reference
+ *
+ * ioasid_free() can be called multiple times without error until all refs are
+ * dropped.
+ */
+
+int ioasid_get_locked(struct ioasid_set *set, ioasid_t ioasid)
+{
+ struct ioasid_data *data;
+
+ data = xa_load(_allocator->xa, ioasid);
+ if (!data) {
+ pr_err("Trying to get unknown IOASID %u\n", ioasid);
+ return -EINVAL;
+ }
+ if (data->state == IOASID_STATE_FREE_PENDING) {
+ pr_err("Trying to get IOASID being freed%u\n", ioasid);
+ return -EBUSY;
+ }
+
+ if (set && data->set != set) {
+ pr_err("Trying to get IOASID not in set%u\n", ioasid);
+ /* data found but does not belong to the set */
+ return -EACCES;
+ }
+ refcount_inc(>users);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(ioasid_get_locked);
+
+/**
+ * ioasid_get - Obtain a reference of an ioasid
+ * @set
+ * @ioasid
+ *
+ * Check set ownership if @set is non-null.
+ */
+int ioasid_get(struct ioasid_set *set, ioasid_t ioasid)
+{
+ int ret = 0;
+
+ spin_lock(_allocator_lock);
+ ret = ioasid_get_locked(set, ioasid);
+ spin_unlock(_allocator_lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(ioasid_get);
+
+void ioasid_put_locked(struct ioasid_set *set, ioasid_t ioasid)
+{
+ struct ioasid_data *data;
+
+ data = xa_load(_allocator->xa, ioasid);
+ if (!data) {
+ pr_err("Trying to put unknown IOASID %u\n", ioasid);
+ return;
+ }
+
+ if (set && data->set != set) {
+ pr_err("Trying to drop IOASID not in the set %u\n", ioasid);
+ return;
+ }
+
+ if (!refcount_dec_and_test(>users)) {
+ pr_debug("%s: IOASID %d has %d remainning users\n",
+ __func__, ioasid, refcount_read(>users));
+ return;
+ }
+ ioasid_do_free(data);
+}
+EXPORT_SYMBOL_GPL(ioasid_put_locked);
+
+/**
+ * ioasid_put - Drop a reference of an ioasid
+ * @set
+ * @ioasid
+ *
+ * Check set ownership if @set is non-null.
+ */
+void ioasid_put(struct ioasid_set *set, ioasid_t ioasid)
+{
+ spin_lock(_allocator_lock);
+ ioasid_put_locked(set, ioasid);
+ spin_unlock(_allocator_lock);
+}
+EXPORT_SYMBOL_GPL(ioasid_put);
+
+/**
* ioasid_find - Find IOASID data
* @set: the IOASID set
* @ioasid: the IOASID to find
diff --git a/include/linux/ioasid.h b/include/linux/ioasid.h
index 412d025d440e..310abe4187a3 100644
--- a/include/linux/ioasid.h
+++ b/include/linux/ioasid.h
@@ -76,6 +76,10 @@ int ioasid_attach_data(ioasid_t ioasid, void *data);
int ioasid_register_allocator(struct ioasid_allocator_ops *allocator);
void ioasid_unregister_allocator(struct ioasid_allocator_ops *allocator);
void ioasid_is_in_set(struct ioasid_set *set, ioasid_t ioasid);
+int ioasid_get(struct ioasid_set *set, ioasid_t ioasid);
+int ioasid_get_locked(struct ioasid_set *set, ioasid_t ioasid);
+void ioasid_put(struct ioasid_set *set, ioasid_t ioasid);
+void ioasid_put_locked(struct ioasid_set *set,
