[openib-general] IPoIB CM for merge?

[Michael S. Tsirkin]

> Quoting Steve Wise <swise at opengridcomputing.com>:
> Subject: Re: [openib-general] IPoIB CM for merge?
> 
> On Fri, 2007-02-02 at 13:15 +0200, Michael S. Tsirkin wrote:
> > > Quoting Roland Dreier <rdreier at cisco.com>:
> > > Subject: Re: IPoIB CM for merge?
> > > 
> > >  > Could you please spend some time reviewing IPoIB CM code?
> > >  > I am concerned about missing the 2.6.21 merge window.
> > > 
> > > Thanks for the reminder.
> > > 
> > > Can we trade?  Have you looked at the cxgb3 iwarp driver?  Any comments?
> > 
> > OK.
> > I am not sure I have the last version posted so I am going to go by what
> > is there in OFED git tree.
> > 
> > And I also only looked under drivers/infiniband/.
> > 
> > So, here are some questions: I looked in the archives and have not seen
> > these addressed. Maybe these can be answered and then I'll go from there?
> > Does this sound OK?
> > 
> > Files with names like
> > ./core/cxio_hal.c
> > ./core/cxio_hal.h
> > normally generate a fair bit of discussion which wasn't present here,
> > I did not guess everyone was just busy.
> > For example, why is there both struct iwch_cq and struct t3_cq?
> > 
> 
> The cxgb3/core code defines a low level interface to the RDMA bits of
> the T3 device. 
> 
> This code was originally a separate module (named cxio) that allowed
> other RDMA middleware layers to sit on top of the this core rdma module.
> At the time, there was RNIC-PI and OFA being developed.  So that is the
> history of this.  As per the first openib review (about a year ago) of
> this code I merged this core module into the cxgb3 module.  I left the
> file structure and names as-is because it was low priority IMO.
> 
> The t3_cq struct is the low level CQ structure used to manage both a HW
> accessed CQ and a SW CQ (needed to handle error cases and out of order
> completions). The iwch_cq struct contains the stuff needed to integrate
> with the OFA core and uverbs code. It contains a t3_cq inline.

So now that there's a common module, there's no technical reason for
the two-level structure to exist? I would say you want to at least
move the files into a common directory.

I think you will also find that for datapath operations such as poll cq,
converting completion from hardware to struct t3_cqe, and from
that to ib_wc adds an untrivial amount of overhead.


> > File tcb.h comment says:
> > /* This file is automatically generated --- do not edit */
> > This looks like a GPL violation, does it not?
> > 
> 
> I can add the license if that's what you mean.

I mean that this file does not seem to be the source, in the GPL sense.
The following comes from COPYING under linux source directory:

	The source code for a work means the preferred form of the work for
	making modifications to it.  For an executable work, complete source
	code means all the source code for all modules it contains, plus any
	associated interface definition files, plus the scripts used to
	control compilation and installation of the executable.

So I think you must make the actual source available under the terms of GPL.

> > What's the deal with the naming convention?
> > Is there a reason in cxgb3, some files start with iwch and some with cxio?
> > How about using cxgb3 prefix all over?
> 
> The cxio_ prefix is used for the low-level functions/types that talk
> directly with the HW.  iwch_ is the provider driver functions that
> interface with the OFA stack.  I'd rather not change the names.
> Especially since this has already gone through several review cycles.
> I'm hoping we can get this in and improve it with subsequent
> submissions.  Is that reasonable?



-- 
MST