Thanks for the detailed review! Some replies below. I left the IETF list out of this reply since it's basically porting, not protocol.
At 07:01 AM 2/19/2006, Christoph Hellwig wrote: >On Wed, Feb 08, 2006 at 03:58:56PM -0500, Talpey, Thomas wrote: >> We have released an updated NFS/RDMA client for Linux at >> the project's Sourceforge site: > >Thanks, this looks much better than the previous patch. > >Comments: > > - please don't build the rdma transport unconditional, but make it > a user-visible config option It's an option, but it's located in fs/Kconfig not net/. This is the way SUNRPC is selected, so we simply followed that. BTW, Chuck's transport switch doesn't support dynamically loading modules yet so there is a dependency to work out until that's in place. > - please use the kernel u*/s* types instead of (u)int*_t We use uint*_t for the user-visible protocol definitions (on the wire) and u32 etc for kernel stuff. I'll recheck if we got something wrong. > - please include your local headers after the <linux/*.h> headers, There are a couple of issues with header include ordering that seem to change pretty often. In a couple of cases we had to rearrange things to avoid forward declarations, I'll recheck this. > and keep all the includes at the beginning of the files, just after > the licence comment block > - chunktype shouldn't be a typedef but a pure enum, and the > names look a bit too generic, please add an rdma_ prefix Ok on both. > - please kill the XDR_TARGET and pos0 macros, maybe RPC_SEND_SEG0 > and RPC_SEND_LEN0, too > - RPC_SEND_VECS should become an inline functions and be spelled > lowercase > - RPC_SEND_COPY is probably too large to be inlined and should be > spelled lowercase > - RPC_RECV_VECS should be an inline and spelled lowercase > - RPC_RECV_SEG0 and PC_RECV_LEN0 should probably go away. > - RPC_RECV_COPY is probably too large to be inlined and should be > spelled lowercase > - RPC_RECV_COPY same comment about highmem and kmap as in > RPC_SEND_COPY These are killable. They were there to support code sharing for 2.4 kernels and are easy to eliminate now. > - the CONFIG_HIGHMEM ifdef block in RPC_SEND_COPY is wrong. Please > always use kmap, it does the right thing for non-highmem aswell. > The PageHighMem check and using kmap_high directly is always > wrong, they are internal implementation details. I'd also suggest > evaluating kmap_atomic because it scales much better on SMP systems. Yes, there are some issues here which we're still working out. In fact, we can't use kunmap() in the context you mention because in 2.6.14 (or is it .15) it started to check for being invoked in interrupt context. There is one configuration in which we do call it in bh context. The call won't block but the kernel BUG_ON's. This is something on our list to address. > - please try to avoid file-scope forward-prototypes but try to order the > code in the natural flow where they aren't required Good point. Will recheck for these. > - structures like rpcrdma_msg that are on the wire should use __be* > for endianess annotations, and the cpu_to_be*/be*_to_cpu accessor > functions instead of hton?/ntoh?. Please verify that these annotations > are correct using sparse -D__CHECK_ENDIAN__=1 Hmm, okay but existing RPC and NFS code don't do this. I'm reluctant to differ from the style of the governing subsystem. I'll check w/Trond. > - rdma_convert_physiov/rdma_convert_phys are completely broken. > page_to_phys can't be used by driver/fs code. RDMA only deals with bus > addresses, not physical addresses. You must use the dma mapping API > instead. Also coalescing decisions are made by the dma layer, because > they are platform dependent and much more complex then what the code > in this patch does. Now that we are moving to OpenIB api's this is needed. There is some thought necessary w.r.t. our max-performance mode of preregistering memory in DMA mode. That's on our list of course. > - transport.c is missing a GPL license statement Oops. > - in transport.c please don't use CamelCase variable names. This is just for module parameters? These are going away but we don't have the new NFS mount API yet. There is a comment to that effect but maybe it doesn't mention the module stuff. > - MODULE_PARM shouldn't be used in new code, but module_param instead. Ditto. > - please don't use the (void) function() style, it just obsfucates the > code without benefit. Ok. > - try_module_get(THIS_MODULE) is always wrong. Reference counting > should happen from the calling module. This is the same convention used by the other RPC transports. I will pass the comment along. > - please initialize global or file-scope spinlocks with > DEFINE_SPINLOCK(). Ok. > - the traditional name for the second argument to spin_lock_irqsave is > just flags, not lock_flags. This doesn't really matter, but > following such conventions makes it easier to understand the code > for kernel hackers that just occasionally drop into your code. Ok. > - no need to case the return value from kmalloc/kzalloc/etc. They > return void * which can be directly assigned to any pointer type. Ok. Did *I* write that code? Very unlike me. :-) > - please avoid typedes for structure types, like struct rdma_ia, > struct rdma_ep, etc.. I thought we got rid of those. Will recheck. Tom. _______________________________________________ openib-general mailing list [email protected] http://openib.org/mailman/listinfo/openib-general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
