On 01-Apr-19 16:47, Leon Romanovsky wrote:
> On Mon, Apr 01, 2019 at 03:11:26PM +0300, Gal Pressman wrote:
>> On 01-Apr-19 11:47, Leon Romanovsky wrote:
>>> From: Mark Zhang <markz@xxxxxxxxxxxx>
>>>
>>> In manual mode a QP is bound to a counter manually. If counter is not
>>> specified then a new one will be allocated.
>>> Manually mode is enabled when user binds a QP, and disabled when the
>>> last manually bound QP is unbound.
>>> When auto-mode is turned off and there are counters left, manual mode
>>> is enabled so that the user is able to access these counters.
>>
>> IMO, an API to allocate a counter makes more sense than allocating it on the
>> first bind when a counter is not specified.
>
> This suggestion was raised during internal code review and we decided to
> make users life simple and don't require from them to manage counter
> lifetime. Allocation by users will immediately means that they need
> to deallocate explicitly too. Otherwise we will find ourselves with
> asymmetrical API which is not good. It is unclear who and when will
> call to deallocate.
>
> The decision to make bind/unbind internally allocate and deallocate
> allowed us to ensure that counter lifetime is strict and managed by the
> kernel.
>
>> The part about turning off auto mode with counters left seems like a mess, do we
>> really want all these hidden "rules" instead of keeping it simple? Don't let the
>> user change modes when a counter still has resources bind to it.
>
> There are no any hidden rules, once you disable auto mode, it will
> applicable for new objects only. There are no changes in old and
> already allocated objects.
I think it's better to be explicit than deciding for the user when to
allocate/deallocate his resources, but that's just my opinion..
>
>>
>>>
>>> Signed-off-by: Mark Zhang <markz@xxxxxxxxxxxx>
>>> Reviewed-by: Majd Dibbiny <majd@xxxxxxxxxxxx>
>>> Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
>>> ---
>>> +static int rdma_counter_bind_qp_manual(struct rdma_counter *counter,
>>> + struct ib_qp *qp)
>>> +{
>>> + int ret = 0;
>
> We will fix this and everything below.
>
> Thanks
>
