diff mbox series

[rdma-next,v3,9/9] RDMA/restrack: Drop valid restrack field as source of ambiguity

Message ID 20200926101938.2964394-10-leon@kernel.org (mailing list archive)
State Changes Requested
Delegated to: Jason Gunthorpe
Headers show
Series Track memory allocation with restrack DB help | expand

Commit Message

Leon Romanovsky Sept. 26, 2020, 10:19 a.m. UTC 
From: Leon Romanovsky <leonro@mellanox.com>

The valid field was needed to distinguish between supported/not
supported QPs, after the create_qp was changed to support all types,
that field can be dropped in a favor of no_track field.

Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/infiniband/core/restrack.c | 29 ++++++++---------------------
 include/rdma/restrack.h            |  9 ---------
 2 files changed, 8 insertions(+), 30 deletions(-)

Comments

Jason Gunthorpe Oct. 2, 2020, 12:55 p.m. UTC | #1 
On Sat, Sep 26, 2020 at 01:19:38PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
> 
> The valid field was needed to distinguish between supported/not
> supported QPs, after the create_qp was changed to support all types,
> that field can be dropped in a favor of no_track field.
> 
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
>  drivers/infiniband/core/restrack.c | 29 ++++++++---------------------
>  include/rdma/restrack.h            |  9 ---------
>  2 files changed, 8 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
> index 593af32d86a0..6ca3e6f3adb5 100644
> +++ b/drivers/infiniband/core/restrack.c
> @@ -143,7 +143,7 @@ static struct ib_device *res_to_dev(struct rdma_restrack_entry *res)
>  		return container_of(res, struct rdma_counter, res)->device;
>  	default:
>  		WARN_ONCE(true, "Wrong resource tracking type %u\n", res->type);
> -		return NULL;
> +		return ERR_PTR(-EINVAL);
>  	}
>  }
>  
> @@ -223,7 +223,7 @@ int __must_check rdma_restrack_add(struct rdma_restrack_entry *res)
>  	struct rdma_restrack_root *rt;
>  	int ret = 0;
>  
> -	if (!dev)
> +	if (IS_ERR_OR_NULL(dev))
>  		return -ENODEV;

dev can't be NULL

Not sure why this was changed? The error code is always thrown away,
what was wrong with keeping it as NULL? 

Now that all callers check the return code this should be a WARN_ON as
calling restrack_add in a way that is guarenteed to fail us a ULP
error.

> +	WARN_ONCE(!dev && res->type != RDMA_RESTRACK_CM_ID,
> +		  "IB device should be set for restrack type %s",
> +		  type2str(res->type));
> +	if (res->no_track || IS_ERR_OR_NULL(dev))
>  		goto out;

dev is never NULL so that WARN_ONCE doesn't work

Why does this exclude CM_ID? I thought all the fixing in the cm was so
restrack_add and _del were prefectly paired and a device must be
present?

Jason
Leon Romanovsky Oct. 2, 2020, 1:05 p.m. UTC | #2 
On Fri, Oct 02, 2020 at 09:55:35AM -0300, Jason Gunthorpe wrote:
> On Sat, Sep 26, 2020 at 01:19:38PM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@mellanox.com>
> >
> > The valid field was needed to distinguish between supported/not
> > supported QPs, after the create_qp was changed to support all types,
> > that field can be dropped in a favor of no_track field.
> >
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> >  drivers/infiniband/core/restrack.c | 29 ++++++++---------------------
> >  include/rdma/restrack.h            |  9 ---------
> >  2 files changed, 8 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
> > index 593af32d86a0..6ca3e6f3adb5 100644
> > +++ b/drivers/infiniband/core/restrack.c
> > @@ -143,7 +143,7 @@ static struct ib_device *res_to_dev(struct rdma_restrack_entry *res)
> >  		return container_of(res, struct rdma_counter, res)->device;
> >  	default:
> >  		WARN_ONCE(true, "Wrong resource tracking type %u\n", res->type);
> > -		return NULL;
> > +		return ERR_PTR(-EINVAL);
> >  	}
> >  }
> >
> > @@ -223,7 +223,7 @@ int __must_check rdma_restrack_add(struct rdma_restrack_entry *res)
> >  	struct rdma_restrack_root *rt;
> >  	int ret = 0;
> >
> > -	if (!dev)
> > +	if (IS_ERR_OR_NULL(dev))
> >  		return -ENODEV;
>
> dev can't be NULL
>
> Not sure why this was changed? The error code is always thrown away,
> what was wrong with keeping it as NULL?
>
> Now that all callers check the return code this should be a WARN_ON as
> calling restrack_add in a way that is guarenteed to fail us a ULP
> error.
>
> > +	WARN_ONCE(!dev && res->type != RDMA_RESTRACK_CM_ID,
> > +		  "IB device should be set for restrack type %s",
> > +		  type2str(res->type));
> > +	if (res->no_track || IS_ERR_OR_NULL(dev))
> >  		goto out;
>
> dev is never NULL so that WARN_ONCE doesn't work
>
> Why does this exclude CM_ID? I thought all the fixing in the cm was so
> restrack_add and _del were prefectly paired and a device must be
> present?

Both NULL and RDMA_RESTRACK_CM_ID are not needed, this is how I
discovered the need to clean the cma.c, but left it here.

Thanks

>
> Jason
diff mbox series

Patch

diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
index 593af32d86a0..6ca3e6f3adb5 100644
--- a/drivers/infiniband/core/restrack.c
+++ b/drivers/infiniband/core/restrack.c
@@ -143,7 +143,7 @@  static struct ib_device *res_to_dev(struct rdma_restrack_entry *res)
 		return container_of(res, struct rdma_counter, res)->device;
 	default:
 		WARN_ONCE(true, "Wrong resource tracking type %u\n", res->type);
-		return NULL;
+		return ERR_PTR(-EINVAL);
 	}
 }
 
@@ -223,7 +223,7 @@  int __must_check rdma_restrack_add(struct rdma_restrack_entry *res)
 	struct rdma_restrack_root *rt;
 	int ret = 0;
 
-	if (!dev)
+	if (IS_ERR_OR_NULL(dev))
 		return -ENODEV;
 
 	if (res->no_track)
@@ -261,10 +261,7 @@  int __must_check rdma_restrack_add(struct rdma_restrack_entry *res)
 	}
 
 out:
-	if (ret)
-		return ret;
-	res->valid = true;
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL(rdma_restrack_add);
 
@@ -323,25 +320,16 @@  EXPORT_SYMBOL(rdma_restrack_put);
  */
 void rdma_restrack_del(struct rdma_restrack_entry *res)
 {
+	struct ib_device *dev = res_to_dev(res);
 	struct rdma_restrack_entry *old;
 	struct rdma_restrack_root *rt;
-	struct ib_device *dev;
 
-	if (!res->valid) {
-		if (res->task) {
-			put_task_struct(res->task);
-			res->task = NULL;
-		}
-		return;
-	}
-
-	if (res->no_track)
+	WARN_ONCE(!dev && res->type != RDMA_RESTRACK_CM_ID,
+		  "IB device should be set for restrack type %s",
+		  type2str(res->type));
+	if (res->no_track || IS_ERR_OR_NULL(dev))
 		goto out;
 
-	dev = res_to_dev(res);
-	if (WARN_ON(!dev))
-		return;
-
 	rt = &dev->res[res->type];
 
 	old = xa_erase(&rt->xa, res->id);
@@ -350,7 +338,6 @@  void rdma_restrack_del(struct rdma_restrack_entry *res)
 	WARN_ON(old != res);
 
 out:
-	res->valid = false;
 	rdma_restrack_put(res);
 	wait_for_completion(&res->comp);
 }
diff --git a/include/rdma/restrack.h b/include/rdma/restrack.h
index 05e18839eaff..a4140c6e2c81 100644
--- a/include/rdma/restrack.h
+++ b/include/rdma/restrack.h
@@ -59,15 +59,6 @@  enum rdma_restrack_type {
  * struct rdma_restrack_entry - metadata per-entry
  */
 struct rdma_restrack_entry {
-	/**
-	 * @valid: validity indicator
-	 *
-	 * The entries are filled during rdma_restrack_add,
-	 * can be attempted to be free during rdma_restrack_del.
-	 *
-	 * As an example for that, see mlx5 QPs with type MLX5_IB_QPT_HW_GSI
-	 */
-	bool			valid;
 	/**
 	 * @no_track: don't add this entry to restrack DB
 	 *