[openib-general] Re: [PATCH] rdma_lat-09 and results

[Grant Grundler]

On Wed, Jun 15, 2005 at 06:04:40PM +0300, Michael S. Tsirkin wrote:
> OK, I accepted most of it. Thanks!

Welcome! Excellent!

> There were some issues that I fixed, I also made some other small
> cleanups.
> 
> One thing I skipped is the pp_connect_sock consolidation: I like to keep the
> split between client and server at the top level. Connect routines have some
> code duplication but I dont like spreading if (servername) all around them,
> and I dont mind duplication much since this is standard socket stuff.

I moved the servername stuff down one level for several reasons:
1) reduce the size of main() to < 100 lines.
   Why? It's supposed to be an example of how to use verbs.
   The main routine should outline what has to happen, not clutter
   with test setup stuff.

2) I working towards making the server side a daemon.
   ie move stuff out of main:
   o parse commandline parameters
   o loop
	o get parameters via socket
	o set up ctx
	o run test_lat (or test_bw maybe eventually)
	o tear down ctx (e.g release qp, cq, etc)
	o report results
   o clear "global" state (ie free(tstamp), etc)

> Grant, I'm not sure I understand the reason for
> > 	o enumerate return values in main()
> bit but I went along with it. Why do you like it?

In general I like unique return values from programs when they fail.
Makes debugging alot easier. e.g. rdma_lat just spews the usage msg
when a bad parameter is passed to it.  It may not be obvious which
parameter gets rejected depending on how complex the command line is.
rdma_lat is otherwise generally pretty good about printing a
somewhat unique (that's different than "meaningful") error messages.

> The patch also added free(tstamp) in a couple of places, but since
> we dont bother to free resources (qp, cq, ...) and since this was in
> the test part which must be cristal clear, I skipped this bit.

Yeah - this needs to be fixed. I was looking at what needs to happen
for teardown so the test can iterate. But I'm clueless at this
point what needs to happen.

> If we ever do a proper cleanup, its probably a good idea to goto
> outside the loop and handle errors there.

Yes - agree. Can you add that?
It's not obvious to me right now what needs to be cleaned up.
ibv_pingpong wasn't helpful in this case either. :^(

> Here's what I ended up checking in:

looks good - I'll have the next patch ready later today.

thanks!
grant