Re: [PATCH 2/8] IB/rdmavt: Add post send to rdmavt

[Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Tue, Jan 19, 2016 at 06:23:14PM +0200, Moni Shoua wrote:
>> @@ -730,20 +863,46 @@ int rvt_post_recv(struct ib_qp *ibqp, struct 
>> ib_recv_wr *wr,
>>  int rvt_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
>>                   struct ib_send_wr **bad_wr)
>>  {
>> +       struct rvt_qp *qp = ibqp_to_rvtqp(ibqp);
>> +       struct rvt_dev_info *rdi = ib_to_rvt(ibqp->device);
>> +       unsigned long flags = 0;
>> +       int call_send;
>> +       unsigned nreq = 0;
>> +       int err = 0;
>> +
>> +       spin_lock_irqsave(&qp->s_lock, flags);
>> +
>>         /*
>> -        * VT-DRIVER-API: do_send()
>> -        * Driver needs to have a do_send() call which is a single entry point
>> -        * to take an already formed packet and throw it out on the wire. Once
>> -        * the packet is sent the driver needs to make an upcall to rvt so the
>> -        * completion queue can be notified and/or any other outstanding
>> -        * work/book keeping can be finished.
>Why was this design for do_send() removed?
>This requires that each low level drivers do the packet formatting by
>themselves --> code duplication.

The packet building is still being done in the drivers right now. While the 
code is similar between the existing drivers there are hardware specific 
aspects which are critical to performance which will need untangled very 
carefully.  We do plan to move this stuff into rdmavt as well. However, I 
think it makes sense to let this initial cut of rdmavt get some soak time 
given the scope of the changes. So this would be a next step in the 
iterative development.

>> +bail:
>> +       if (nreq && !call_send)
>> +               rdi->driver_f.schedule_send(qp);
>> +       spin_unlock_irqrestore(&qp->s_lock, flags);
>> +       if (nreq && call_send)
>> +               rdi->driver_f.do_send(qp);
>> +       return err;
>>  }
>Wouldn't it be better if scheduling for sending packets be done in RVT?

The progress mechanism is probably a device specific thing. One 
implementation may use work queues, another, something else. For this reason 
we leave it down in the driver. Rdmavt has the two driver calls shown here, 
one to tell the driver to schedule a packet for sending based on whatever 
it's policy is. The other is to kick the driver to do a send right away, for 
performance reasons.

>>  /**
>> diff --git a/include/rdma/rdma_vt.h b/include/rdma/rdma_vt.h
>> index f2d50b9..3d92239 100644
>> --- a/include/rdma/rdma_vt.h
>> +++ b/include/rdma/rdma_vt.h
>> @@ -230,6 +230,8 @@ struct rvt_driver_provided {
>>         void * (*qp_priv_alloc)(struct rvt_dev_info *rdi, struct rvt_qp *qp);
>>         void (*qp_priv_free)(struct rvt_dev_info *rdi, struct rvt_qp *qp);
>>         void (*notify_qp_reset)(struct rvt_qp *qp);
>> +       void (*schedule_send)(struct rvt_qp *qp);
>> +       void (*do_send)(struct rvt_qp *qp);
>A proper explanation about the meaning for each driver provided
>function is required.
>This is true for the 2 new added function and for any other that was
>already there

Agree. I'll perhaps add another patch which cleans up these and other 
comments. Make sure it all makes sense as a number of the original comments 
were put in place to get the ball rolling and stimulate discussion.

-Denny
--
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