[openib-general] ISER cleanup

[Dan Bar Dov]

> -----Original Message-----
> On Mon, Aug 22, 2005 at 01:22:55PM +0300, Dan Bar Dov wrote:
> > We have begun a cleanup of ISER based on the inputs we received.
> > Mostly cosmetic cleanups were already commited.
> 
> yup - good progress and some more cosmetic stuff noted below.
> Then need to start looking at addressing Christoph's (hch) comments.

Some of the comments were taken care of in today's commits
iovecs removed
procfs removed
Function entry/exit traces removed
Unnecessary files removed:
kernel_dep.h
iser_bhs.h
iser_trace.c

> 
> Still need to remove kernel_dep.h and probably most of the 
> files in iser/include/.
> 
> Those also all have a trailing "/* DAT 1.2 */" 
> that might mislead in the future.
> Maybe a comment in the header about "Based on DAT 1.2" release.

All DAT 1.2 comments removed. Actually the current code is not DAT 1.2 compatible, but the openIB flavor compatible.
Since work started on a CM abstraction, I expect ISER to get off of kdapl and onto ib-verbs + CM abstraction.
> 
> 
> iser_api.h
> 	Should iSCSI be providiing the jump table definitions?
> 	struct iser_api_t
> 	struct iser_api_cb_t
> 
> iser_ext_api.h
> 	typedef void * iser_conn_request_t;
> 	Delete stuff like this - it just obscures what is going on.
OK

> 
> 	I'm not sure what this file is doing.
> 	I was expecting iSCSI framework to define the data structures
> 	it needs to talk to a service provider.
This is an "extended API". The ISER spec defines an ISER API, but it does not consider implementation. 
We chose to implement the extra api out of the iser_api structute and in the iser_ext_api struct. 
iSCSI is still not part of the kernel so we had first modified and added the datamover framework to 
linux-iscsi and now to open-iscsi. Once open-iscsi is in the kernel we'll use it as the framework.

> 
> iser_pdu.h
> 	sorry - Didn't have time to understand what this is about.
Most definitions are duplicates from the iscsi and will disappear. 
The struct iser_send_pdu defines the ISER extensions to the iscsi pdu.

> 
> iser_types.h
> 	delete typdef void * iser_api_handle_t.
> 	replace usage of iser_api_handle_t with "void *".
> 	Ditto for all "void *" typedefs in that file.
OK

> 
> 	Kernel already defines scatter-gather lists type.
The iser_data_buf struct can point to a scatterlist array but can also be used to point at a single buffer. 
It does not replicate scatterlist but allows us to deal with two types of registrations - single buffer and scatter lists.

> 
> kernel_dep.h
> 	Delete this file.
> 	This content belongs in a seperate patch that people can grab
> 	and apply when they want to build iSER on an older kernel.
> 	See src/linux/kernel/patches
Gone.

> 
> 
> > Removed vi comments
> 
> yup - mostly. Some are still present in iser/include/*.h.
Gone as well.

> 
> > Removed CONFIG_INFINIBAND refrences
> > Reorganized module
> > Rewritten Makefile to new style
> > Added Kconfig file
> > Using kernel min/max
> 
> all very good.
> 
> 
> > There are many other things to be done, including both coding style 
> > and substance, we'll proceed addressing all the technical 
> issues that 
> > were commented on.
> 
> great!
We are going to simplify the local memory registrations by registering all memory like in the SRP driver.
We do not understand some of the substance issues - for example, dma related comments - are taken care of by iscsi, 
not the transport. The io_mmu comment, we completely do not understand - there was some platform specific code, but its all gone now.

BTW the code is now down to 7K LOC + 2K LOC of heavily commented header files.

> 
> thanks,
> grant