[openib-general][PATCH][RFC]: CMA IB implementation

[Sean Hefty]

Guy German wrote:
>>>     memset(&qp_attr, 0, sizeof qp_attr);
>>>     qp_attr.qp_state = qp_state;
>>>
>>>     if (cm_id && !qp_attr_mask) 
>>
>> Or this check...
> 
> 
> This check we do need, because:
> - when we call modify qp state to RTR or RTS cm_id is valid and 
> qp_attr_mask==0, so we need to call ib_cm_init_qp_attr
> - when we call modify qp state to ERROR cm_id==0 and  qp_attr_mask is valid

Why is cm_id 0 when modifying the QP to the error state?  I need to look back 
through the code paths...

>> Is this the outbound private data or inbound?  Why not tie the private 
>> data to an event and avoid storing it with the cma_ctx?
> 
> It is the private data passed by the consumer in the connection request.
> I stored it in cma_ctx to retrieve it back in the cma_path_handler.
> For some reason I saw that the private data was garbaged when I passed 
> the consumer's pointer of private_data (maybe it was my improvised test 
> module)...

With IB it's possible to receive a REQ followed by a REJ, REP followed by a REJ, 
or a REP followed by a DREQ.  (The first two can occur as a result of timeouts.) 
  The private data needs to be handled differently to pass it to the consumer, 
since there can be multiple events outstanding, each with private data, to a 
single cma_id.

>> We can avoid some synchronization by letting it exist until the user 
>> destroys the corresponding cma_id.  Otherwise, there's the potential 
>> of the user trying to destroy it twice.  Once from the 
>> cma_connection_callback reporting an error, and then again here.
> 
> So you suggests this function will always return 0, then ?

I would recommend:  If the connection is reported to the user, return 0.  If a 
new connection request cannot be reported to the user (e.g. unable to allocate 
memory), return a non-zero value to cleanup.

>>> static void cma_path_handler(u64 req_id, void *context, int rec_num)
>>> {
>>>     struct cma_context *cma_ctx = context;
>>>     enum ib_cma_event event;
>>>     int status = 0;
>>>
>>>     if (!cma_ctx) {
>>
>>
>> This check isn't needed.
> 
> 
> What if the consumer destroyed the cma_id, before the path handler cb 
> returned ?

We need to add reference counting to handle this.  Otherwise there's a race 
condition where the cma_ctx could be destroyed immediately after the check that 
will not be detected.

>>> int ib_cma_create_qp(struct ib_pd *pd, u8 port, struct ib_qp **qp_in, 
>>>              struct ib_qp_init_attr *init_attr)
>>> {
>>
>>
>> Why not return struct ib_qp* similar to how the other APIs operate?
> 
> 
> I thought of returning ib_modify_qp status, but I agree that destroying 
> the qp if failed and just returning the qp if not, is better.

 From the user's perspective, he doesn't know if a QP has been returned or not 
on a failure, so I think that it does need to change for that reason.

- Sean