[openib-general] [PATCH 1/6] libibverbs include files changes.

[Krishna Kumar2]

Hi Sean & Roland,

Thanks for your review comments. I will respond to all comments in
this mail to make it easy :)

A.
-------
>  > Why not just do:
>  > 
>  > #define rdma_structure_name ibv_structure_name
>  > 
>  > Then we simply remove the #define and replace ibv_structure_name with 

>  > rdma_structure_name when ready.  This better shows the changes, 
prevents 
>  > duplicating every structure, and should avoid any compile warnings.
> 
> I agree, except let's make rdma_structure_name the real name, and do
> the define the opposite way.  That way it's easier to remove the
> compatibility stuff later.

I had tried this some time back (with amso driver actually) and found it 
had some
issues. Eg, some data structures have the same name as API routines. I did 
:

"cscope -1<fn_names> -L" on all function names and found 27 functions that
also clashed with structure names. The full list is :

********* Problem with ibv_query_device **********
include/infiniband/kern-abi.h ibv_query_device 146 struct ibv_query_device 
{
src/verbs.c ibv_query_device 81 int ibv_query_device(struct ibv_context 
*context
********* Problem with ibv_query_port **********
include/infiniband/kern-abi.h ibv_query_port 198 struct ibv_query_port {
src/verbs.c ibv_query_port 87 int ibv_query_port(struct ibv_context 
*context
********* Problem with ibv_alloc_pd **********
include/infiniband/kern-abi.h ibv_alloc_pd 231 struct ibv_alloc_pd {
src/verbs.c ibv_alloc_pd 137 struct ibv_pd *ibv_alloc_pd(struct 
ibv_context *
********* Problem with ibv_dealloc_pd **********
include/infiniband/kern-abi.h ibv_dealloc_pd 243 struct ibv_dealloc_pd {
src/verbs.c ibv_dealloc_pd 148 int ibv_dealloc_pd(struct ibv_pd *pd)
********* Problem with ibv_reg_mr **********
include/infiniband/kern-abi.h ibv_reg_mr 250 struct ibv_reg_mr {
src/verbs.c ibv_reg_mr 153 struct ibv_mr *ibv_reg_mr(struct ibv_pd *pd,
********* Problem with ibv_dereg_mr **********
include/infiniband/kern-abi.h ibv_dereg_mr 269 struct ibv_dereg_mr {
src/verbs.c ibv_dereg_mr 167 int ibv_dereg_mr(struct ibv_mr *mr)
********* Problem with ibv_create_comp_channel **********
include/infiniband/kern-abi.h ibv_create_comp_channel 276 struct 
ibv_create_comp_channel {
src/verbs.c ibv_create_comp_channel 190 struct ibv_comp_channel 
*ibv_create_comp_channel(struct ibv_context *context)
********* Problem with ibv_create_cq **********
include/infiniband/kern-abi.h ibv_create_cq 287 struct ibv_create_cq {
src/verbs.c ibv_create_cq 232 struct ibv_cq *ibv_create_cq(struct 
ibv_context *context, int cqe, void *cq_context,
********* Problem with ibv_resize_cq **********
include/infiniband/kern-abi.h ibv_resize_cq 346 struct ibv_resize_cq {
src/verbs.c ibv_resize_cq 250 int ibv_resize_cq(struct ibv_cq *cq, int 
cqe)
********* Problem with ibv_destroy_cq **********
include/infiniband/kern-abi.h ibv_destroy_cq 360 struct ibv_destroy_cq {
src/verbs.c ibv_destroy_cq 258 int ibv_destroy_cq(struct ibv_cq *cq)
********* Problem with ibv_poll_cq **********
include/infiniband/kern-abi.h ibv_poll_cq 323 struct ibv_poll_cq {
include/infiniband/verbs.h ibv_poll_cq 831 static inline int 
ibv_poll_cq(struct ibv_cq *cq, int num_entries, struct ibv_wc *wc)
********* Problem with ibv_req_notify_cq **********
include/infiniband/kern-abi.h ibv_req_notify_cq 338 struct 
ibv_req_notify_cq {
include/infiniband/verbs.h ibv_req_notify_cq 845 static inline int 
ibv_req_notify_cq(struct ibv_cq *cq, int solicited_only)
********* Problem with ibv_create_srq **********
include/infiniband/kern-abi.h ibv_create_srq 696 struct ibv_create_srq {
src/verbs.c ibv_create_srq 289 struct ibv_srq *ibv_create_srq(struct 
ibv_pd *pd,
********* Problem with ibv_modify_srq **********
include/infiniband/kern-abi.h ibv_modify_srq 716 struct ibv_modify_srq {
src/verbs.c ibv_modify_srq 310 int ibv_modify_srq(struct ibv_srq *srq,
********* Problem with ibv_query_srq **********
include/infiniband/kern-abi.h ibv_query_srq 727 struct ibv_query_srq {
src/verbs.c ibv_query_srq 317 int ibv_query_srq(struct ibv_srq *srq, 
struct ibv_srq_attr *srq_attr)
********* Problem with ibv_destroy_srq **********
include/infiniband/kern-abi.h ibv_destroy_srq 744 struct ibv_destroy_srq {
src/verbs.c ibv_destroy_srq 322 int ibv_destroy_srq(struct ibv_srq *srq)
********* Problem with ibv_post_srq_recv **********
include/infiniband/kern-abi.h ibv_post_srq_recv 636 struct 
ibv_post_srq_recv {
include/infiniband/verbs.h ibv_post_srq_recv 901 static inline int 
ibv_post_srq_recv(struct ibv_srq *srq,
********* Problem with ibv_create_qp **********
include/infiniband/kern-abi.h ibv_create_qp 432 struct ibv_create_qp {
src/verbs.c ibv_create_qp 327 struct ibv_qp *ibv_create_qp(struct ibv_pd 
*pd,
********* Problem with ibv_modify_qp **********
include/infiniband/kern-abi.h ibv_modify_qp 524 struct ibv_modify_qp {
src/verbs.c ibv_modify_qp 364 int ibv_modify_qp(struct ibv_qp *qp, struct 
ibv_qp_attr *attr,
********* Problem with ibv_query_qp **********
include/infiniband/kern-abi.h ibv_query_qp 480 struct ibv_query_qp {
src/verbs.c ibv_query_qp 348 int ibv_query_qp(struct ibv_qp *qp, struct 
ibv_qp_attr *attr,
********* Problem with ibv_destroy_qp **********
include/infiniband/kern-abi.h ibv_destroy_qp 557 struct ibv_destroy_qp {
src/verbs.c ibv_destroy_qp 379 int ibv_destroy_qp(struct ibv_qp *qp)
********* Problem with ibv_post_send **********
include/infiniband/kern-abi.h ibv_post_send 598 struct ibv_post_send {
include/infiniband/verbs.h ibv_post_send 943 static inline int 
ibv_post_send(struct ibv_qp *qp, struct ibv_send_wr *wr,
********* Problem with ibv_post_recv **********
include/infiniband/kern-abi.h ibv_post_recv 620 struct ibv_post_recv {
include/infiniband/verbs.h ibv_post_recv 952 static inline int 
ibv_post_recv(struct ibv_qp *qp, struct ibv_recv_wr *wr,
********* Problem with ibv_create_ah **********
include/infiniband/kern-abi.h ibv_create_ah 652 struct ibv_create_ah {
src/verbs.c ibv_create_ah 384 struct ibv_ah *ibv_create_ah(struct ibv_pd 
*pd, struct ibv_ah_attr *attr)
********* Problem with ibv_destroy_ah **********
include/infiniband/kern-abi.h ibv_destroy_ah 667 struct ibv_destroy_ah {
src/verbs.c ibv_destroy_ah 452 int ibv_destroy_ah(struct ibv_ah *ah)
********* Problem with ibv_attach_mcast **********
include/infiniband/kern-abi.h ibv_attach_mcast 674 struct ibv_attach_mcast 
{
src/verbs.c ibv_attach_mcast 457 int ibv_attach_mcast(struct ibv_qp *qp, 
union ibv_gid *gid, uint16_t lid)
********* Problem with ibv_detach_mcast **********
include/infiniband/kern-abi.h ibv_detach_mcast 685 struct ibv_detach_mcast 
{
src/verbs.c ibv_detach_mcast 462 int ibv_detach_mcast(struct ibv_qp *qp, 
union ibv_gid *gid, uint16_t lid)

That was the reason I wrote separate routines and data structures so that
#defines don't do wrong things. Does that sound reasonable ?

B.
-----

- "Maybe the best approach is to use rdmav_ to replace ibv_" :

OK, that sounds consistent. Is "rdv_" better than the longer "rdmav_" ?

C.
--------

- "Path records are IB specific.  Not sure we need to rename them" and 
"These
   changes look fine.  We just need to decide if we want to change 
everything 
   that's ibv_* to rdma_*, or keep IB specific names (path records, GIDs, 
PKeys, 
   etc.) the same."

I had indicated this in my "Information notes" in the [PATCH 0/6] :
        "IB specific routines are also converted to use RDMA generic API's 
for sake
        of uniformness (knowing that transport dependent names will be 
removed
        once all apps are converted)."

The issue is between deciding to have either rd(ma)_v or ibv_ for IB 
specific
structures. Currently there is no other transport other than IB that has 
these
specific structures, but if that changes it might be better to keep the 
name
transport agnostic. Another reason that I see at this time is to have 
uniform
names which means that this library exports names using one prefix - this 
means
that I do not have to care about the underlying transport type and I also 
do not
have to remember that ibv_ is for [a, b, c, d] operations and rdma_ is for 
[e, f, g, h]
operations. What do you feel ?

D.
--------
> > We named ourselves OpenFabrics instead of OpenRDMA for a reason

> Wasn't OpenRDMA already taken?

Correct. Also "RDMA" name is a generic enough name and has nothing to do 
with
openRDMA. Roland had rightly pointed out in my first attempt that usage of 
"#ifdef
OPENRDMA_xxx" was inappropirate, so I corrected that in this patchset (I 
had
changed OPENIB to OPENRDMA blindly :)).

Thanks,

- KK