On 6/12/2019 9:44 PM, Jason Gunthorpe wrote:
> On Wed, Jun 12, 2019 at 03:12:27PM +0300, Leon Romanovsky wrote:
>> On Wed, Jun 12, 2019 at 09:08:02AM -0300, Jason Gunthorpe wrote:
>>> 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
>>
>> It means that all QPs "returned" this counter.
>
> It doesn't make sense to do something like that with an atomic, either
> you know there is no possible atomic_inc/dec at this point (which begs the
> question why test it), or it is racy
>
How about add a new parameter "last_qp", if it is true then deallocate
the counter? So that the "atomic_read()" check can be performed in core
layer.
> Jason
>
