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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html