Sean, (sorry for the repeat attempts here)
Just touching base to revisit some of the items that we agreed no several
months ago with respect to the ibv_send_wr struct. As you probably recall, we
discussed how to extend this structure, with the question coming in the context
of extending XRC, and given the fact that we would like to push upstream our
new transport type (Dynamically Connected Transport) and the Cross Channel
synchronization capabilities. You had expressed the desire to keep the struct
size small. Here is what we had agreed on:
struct ibv_send_wr {
uint64_t wr_id;
struct ibv_send_wr *next;
struct ibv_sge *sg_list;
int num_sge;
enum ibv_wr_opcode opcode;
int send_flags;
uint32_t imm_data; /* in
network byte order */
union {
struct {
uint64_t
remote_addr;
uint32_t rkey;
} rdma;
struct {
uint64_t
remote_addr;
uint64_t
compare_add;
uint64_t swap;
uint32_t rkey;
} atomic;
struct {
struct ibv_ah *ah;
uint32_t
remote_qpn;
uint32_t
remote_qkey;
} ud;
+ struct {
+ struct ibv_send_wr_ext
*ext;
+ } ext;
} wr;
};
struct ibv_send_wr_ext {
uint32_t mask;
union{
struct {
uint64_t
remote_addr;
uint32_t rkey;
} rdma;
struct {
uint64_t
remote_addr;
uint64_t
compare_add;
uint64_t swap;
uint32_t rkey;
} atomic;
struct {
struct ibv_cq *cq;
int32_t cq_count;
} cqe_wait;
struct {
struct ibv_qp *qp;
int32_t wqe_count;
enum comm_type;
} wqe_enable;
} task;
union {
struct {
struct ibv_ah *ah;
uint32_t
remote_qpn;
uint32_t
remote_qkey;
} ud;
struct {
struct ibv_ah *ah;
uint64_t remote dct_access_key;
uint32_t dct_number;
}dc;
struct {
uint32_t
remote_srqn;
} xrc;
} qp;
union{
struct{
enum verbs_core_direct_ops data_op;
enum verbs_wr_calc_op calc_op;
enum verbs_wr_data_type data_type;
} calc;
}op;
};
Tzahi, Yishai, and myself talked about this today, and would like to revisit
this, before we push XRC upstream. The main concern with this struct is that
it requires an additional indirection in the critical path, when one wants to
access the extended capabilities. There are two alternatives we would like to
suggest:
- Remove the pointer to the ibv_send_wr_ext pointer from the wr union in
ibv_send_wr, and put the ibv_send_wr (less the duplicated data structures)
after the union, with the usual comp_mask flag to indicate what is supported.
This would be our #1 preference, because of the performance implications.
- Remove the pointer to the ibv_send_wr_ext pointer from the wr union in
ibv_send_wr, and put pointers to relevant structs (task, qp (maybe should be
named transport), and op, in the current example) after the union, with the
usual comp_mask flag to indicate what is supported. This would be our second
preference.
What do you think here ?
Also, one other thing that Yishai noticed that you added a struct in support of
XRC in ibv_kern_send_wr, which is not used. We would be inclined not to add
this at the current stage, but leave that to the time that someone adds a post
send that goes through the kernel. What do you think ?
Thanks,
Rich
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
