On Wed, Jun 12, 2019 at 12:01:23PM +0000, Mark Zhang wrote: > On 6/12/2019 1:54 AM, Jason Gunthorpe wrote: > > On Thu, Jun 06, 2019 at 01:53:37PM +0300, Leon Romanovsky wrote: > >> From: Mark Zhang <markz@xxxxxxxxxxxx> > >> > >> Add support for ib callbacks counter_bind_qp() and counter_unbind_qp(). > >> > >> Signed-off-by: Mark Zhang <markz@xxxxxxxxxxxx> > >> Reviewed-by: Majd Dibbiny <majd@xxxxxxxxxxxx> > >> Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx> > >> drivers/infiniband/hw/mlx5/main.c | 53 +++++++++++++++++++++++++++++++ > >> 1 file changed, 53 insertions(+) > >> > >> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c > >> index 8b7bcf8f68fb..66c94a060718 100644 > >> +++ b/drivers/infiniband/hw/mlx5/main.c > >> @@ -5533,6 +5533,57 @@ static int mlx5_ib_get_hw_stats(struct ib_device *ibdev, > >> return num_counters; > >> } > >> > >> +static int mlx5_ib_counter_bind_qp(struct rdma_counter *counter, > >> + struct ib_qp *qp) > >> +{ > >> + struct mlx5_ib_dev *dev = to_mdev(qp->device); > >> + u16 cnt_set_id = 0; > >> + int err; > >> + > >> + if (!counter->id) { > >> + err = mlx5_cmd_alloc_q_counter(dev->mdev, > >> + &cnt_set_id, > >> + MLX5_SHARED_RESOURCE_UID); > >> + if (err) > >> + return err; > >> + counter->id = cnt_set_id; > >> + } > >> + > >> + err = mlx5_ib_qp_set_counter(qp, counter); > >> + if (err) > >> + goto fail_set_counter; > >> + > >> + return 0; > >> + > >> +fail_set_counter: > >> + mlx5_core_dealloc_q_counter(dev->mdev, cnt_set_id); > >> + counter->id = 0; > >> + > >> + return err; > >> +} > >> + > >> +static int mlx5_ib_counter_unbind_qp(struct ib_qp *qp, bool force) > >> +{ > >> + struct mlx5_ib_dev *dev = to_mdev(qp->device); > >> + struct rdma_counter *counter = qp->counter; > >> + int err; > >> + > >> + err = mlx5_ib_qp_set_counter(qp, NULL); > >> + if (err && !force) > >> + return err; > >> + > >> + /* > >> + * Deallocate the counter if this is the last QP bound on it; > >> + * If @force is set then we still deallocate the q counter > >> + * no matter if there's any error in previous. used for cases > >> + * like qp destroy. > >> + */ > >> + if (atomic_read(&counter->usecnt) == 1) > >> + return mlx5_core_dealloc_q_counter(dev->mdev, counter->id); > > > > This looks like a nonsense thing to write, what it is trying to do > > with that atomic? > > > > I still can't see why this isn't a normal kref. > > > > Hi Jason, > > Have discussed with Leon, unlike other resources, counter alloc/dealloc > isn't called explicitly. So we need a refcount to record how many QPs > are bound on this counter, when it comes to 0 then the counter can be > deallocated. Whether to use atomic or kref the code is similar, it is > not able to take advantage of kref/completion. That doesn't explain the nonsense "atomic_read(&counter->usecnt) == 1" test Jason