Re: [PATCH 31/37] librdmacm: provide abstracted verb calls

[Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>]

Sean Hefty wrote:
>>> +static inline int
>>> +rdma_get_send_comp(struct rdma_cm_id *id, struct ibv_wc *wc)
>>> +{
>>> +	struct ibv_cq *cq;
>>> +	void *context;
>>> +	int ret;
>>> +
>>> +	ret = ibv_poll_cq(id->send_cq, 1, wc);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = ibv_req_notify_cq(id->send_cq, 0);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = ibv_poll_cq(id->send_cq, 1, wc);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = ibv_get_cq_event(id->send_cq_channel, &cq, &context);
>>> +	if (ret)
>>> +		return ret;
>>>
>>>       
>> This doesn't look correct.  If the send isn't complete by the time the
>> 2nd ibv_poll_cq() completes, then this function will return without
>> having filled in the wc.  Or am I missing something?  Shouldn't the
>> ibv_get_cq_event() be the first thing this function does?  The same
>> issue/question exists for rdma_get_recv_comp().
>>     
>
> I think it's possible for the function to return without having filled in a wc.
>   


So its busted?  Or is this intended behavior?


> If the 2nd poll removes a completion, it can leave a cq event on the channel,
> which a subsequent call could retrieve, but then find the cq empty.
>
> The idea for this call is to abstract poll, notify_cq, and get_cq_event, but
> still provide decent performance.  (Scalability is a separate matter.  I
> couldn't find a decent way to abstract a CQ shared across QPs or between the
> receive and send queues.)
>
> To avoid returning from the call without a completion, I think the following
> structure works:
>
> poll()
> notify_cq()
> poll()
> while (no completion) {
> 	get_cq_event()
> 	poll()
> }
>   

Is rdma_get_send_completion() supposed to return exactly one wc?  If so 
then the 2 polls can cause a wc to get silently discarded.   I must 
still not be understanding the intended use?

I would think this should just be:

get_cq_event()
notify_cq()
poll()


> The only drawback I see is that it's theoretically possible to build up a queue
> of cq events in the kernel.  Not sure how to fix that.  Any ideas?
>
>   

That can always happen, yes?


Steve.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html