[ofa-general] Re: [PATCH V2 - libibverbs] Added reference count to completion event channels

[Dotan Barak]

The code has some minor problems:

Roland Dreier wrote:
>  int ibv_destroy_comp_channel(struct ibv_comp_channel *channel)
>  {
> -	if (abi_ver <= 2)
> -		return ibv_destroy_comp_channel_v2(channel);
> +	int ret;
> +
> +	pthread_mutex_lock(&channel->context->mutex);
> +
> +	if (channel->refcnt) {
> +		ret = EBUSY;
> +		goto out;
> +	}
> +
> +	if (abi_ver <= 2) {
> +		ret = ibv_destroy_comp_channel_v2(channel);
> +		goto out;
> +	}
>  
>  	close(channel->fd);
>  	free(channel);
> +	ret = 0;
>  
> -	return 0;
> +out:
> +	pthread_mutex_unlock(&channel->context->mutex);
>   
here you try to unlock the mutex, but the channel is no longer allocated 
...
> +
> +	return ret;
>  }
>  
>  int __ibv_destroy_cq(struct ibv_cq *cq)
>  {
> -	return cq->context->ops.destroy_cq(cq);
> +	int ret;
> +
> +	pthread_mutex_lock(&cq->context->mutex);
> +
> +	ret = cq->context->ops.destroy_cq(cq);
> +	if (cq->channel && !ret)
> +		--cq->channel->refcnt;
> +
> +	pthread_mutex_unlock(&cq->context->mutex);
> +
> +	return ret;
>  }
>  default_symver(__ibv_destroy_cq, ibv_destroy_cq);
>   
1) I believe that the cq->context->ops.destroy_cq(cq) free the memory 
which was allocated to the CQ,
    so the decrease of refcnt and the mutex_unlock causes memory violations
2) optimization suggestion: the mutex should be locked/unlocked only if 
channel is being used


thanks for finding the time for implement the patch
Dotan