Thanks for your comments. Please see below.
Thanks Tzachi > -----Original Message----- > From: Sean Hefty [mailto:[email protected]] > Sent: Wednesday, December 02, 2009 6:58 PM > To: Tzachi Dar; [email protected] > Subject: RE: [ofw] patch 2/2 Add support for RDMAoEth to the > low level driver > > >Index: core/al/kernel/al_cm_cep.c > >=================================================================== > >--- core/al/kernel/al_cm_cep.c (revision 2617) > >+++ core/al/kernel/al_cm_cep.c (working copy) > >@@ -4390,6 +4390,10 @@ > > > > p_av->attr.grh_valid = !ib_gid_is_link_local( &p_path->dgid ); > > > >+ // make this same as Linux code: p_path->hop_flow_raw >= 2 > means grh present. > >+ if(cl_hton32(p_path->hop_flow_raw) >= 2) p_av->attr.grh_valid = 1; > > Please submit this as a separate patch, and it would be best > to decide on a single way to set grh_valid. > > I guess that it might be even better not to commit this part at all. This should probably go with other changes in the ibal library. > >Index: ulp/libibverbs/examples/pingpong.c > >=================================================================== > >--- ulp/libibverbs/examples/pingpong.c (revision 2617) > >+++ ulp/libibverbs/examples/pingpong.c (working copy) > >@@ -50,3 +50,25 @@ > > > > return attr.lid; > > } > >+ > >+void str2gid(char *grh, union ibv_gid *gid) { char tmp; > >+ > >+ tmp = grh[8]; > >+ grh[8] = 0; > >+ (*((ULONG*)&gid->raw[0])) = htonl(strtoul(grh, NULL, 16)); > grh[8] = > >+ tmp; > >+ > >+ tmp = grh[16]; > >+ grh[16] = 0; > >+ (*((ULONG*)&gid->raw[4])) = htonl(strtoul(grh + 8, NULL, 16)); > >+ grh[16] = tmp; > >+ > >+ tmp = grh[24]; > >+ grh[24] = 0; > >+ (*((ULONG*)&gid->raw[8])) = htonl(strtoul(grh + 16, NULL, 16)); > >+ grh[24] = tmp; > >+ > >+ (*((ULONG*)&gid->raw[12])) = htonl(strtoul(grh + 24, NULL, 16)); } > > This should just use inet_pton, and please submit libibverb > changes as a separate patch. > I guess that inet_pton can be used, I'll change that. > Other QP connection parameters (QPN, LID) are automatically > discovered and exchanged by the libibverbs sample programs. > Why have the user specify this one value, rather than letting > each side discover the GIDs using a local query? > I'll also do this change, it will indeed make the life of the user more easily. Actually, I believe that in this case the program will only be changed to accept -g (without parametes) and in that case the grh will be discovered automaticly. > > #endif /* IBV_PINGPONG_H */ > >Index: ulp/libibverbs/examples/rc_pingpong/rc_pingpong.c > >=================================================================== > >--- ulp/libibverbs/examples/rc_pingpong/rc_pingpong.c (revision 2617) > >+++ ulp/libibverbs/examples/rc_pingpong/rc_pingpong.c (working copy) > >@@ -44,6 +44,8 @@ > > PINGPONG_SEND_WRID = 2, > > }; > > > >+char *grh = NULL; > >+ > > struct pingpong_context { > > struct ibv_context *context; > > struct ibv_comp_channel *channel; > >@@ -55,6 +57,8 @@ > > int size; > > int rx_depth; > > int pending; > >+ struct ibv_port_attr portinfo; > > I don't see where portinfo gets used. Will be removed. > > >+ union ibv_gid dgid; > > dgid makes more sense in struct pingpong_dest. > Agreed, I'll change that. > > }; > > > > struct pingpong_dest { > >@@ -69,6 +73,7 @@ > > { > > struct ibv_qp_attr attr; > > > >+ attr.port_num = (u8)port; > > attr.qp_state = IBV_QPS_RTR; > > attr.path_mtu = mtu; > > attr.dest_qp_num = dest->qpn; > >@@ -81,6 +86,15 @@ > > attr.ah_attr.src_path_bits = 0; > > attr.ah_attr.port_num = (uint8_t) port; > > Why isn't this sufficient to specify the port? > Can you please explain what you mean? > >+ if(grh) > >+ { > >+ str2gid(grh, &ctx->dgid); > >+ attr.ah_attr.is_global = 1; > >+ attr.ah_attr.grh.hop_limit = 1; > >+ attr.ah_attr.grh.dgid = ctx->dgid; > > Might as well save the dgid to grh.dgid directly, rather than > using ctx->dgid as some temporary space. I'll change that. > > >+ attr.ah_attr.grh.sgid_index = 0; > >+ } > >+ > > if (ibv_modify_qp(ctx->qp, &attr, > > IBV_QP_STATE | > > IBV_QP_AV | > >@@ -499,7 +513,7 @@ > > while (1) { > > int c; > > > >- c = getopt(argc, argv, "h:p:d:i:s:m:r:n:l:e"); > >+ c = getopt(argc, argv, "h:p:d:i:s:m:r:n:l:g:e"); > > usage() must be updated appropriately Agreed. > > > if (c == -1) > > break; > > > >@@ -555,6 +569,10 @@ > > servername = _strdup(optarg); > > break; > > > >+ case 'g': > >+ grh = _strdup(optarg); > >+ break; > >+ > > default: > > usage(argv[0]); > > return 1; > >@@ -606,7 +624,7 @@ > > my_dest.psn = rand() & 0xffffff; > > if (!my_dest.lid) { > > fprintf(stderr, "Couldn't get local LID\n"); > >- return 1; > >+ //return 1; > > This is an unusual change. The real issue is that if you are running on native IB than you should have a lid. On the other hand if you are running on LLE, than you don't have a lid (and this is not an error). As a result I'll change the test to realy exit if this is running over ib only. I'm thinking of something like: "if I have a lid but you don't then exit". > > - Sean > > _______________________________________________ ofw mailing list [email protected] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
