[ofw] patch 2/2 Add support for RDMAoEth to the low level driver

[Tzachi Dar]

Thanks for your comments.

Please see below.

Thanks
Tzachi 

> -----Original Message-----
> From: Sean Hefty [mailto:sean.hefty at intel.com] 
> Sent: Wednesday, December 02, 2009 6:58 PM
> To: Tzachi Dar; ofw at lists.openfabrics.org
> 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
> 
>