Re: [SSI-devel] R1.9.3 Infiniband ICS patches
Brought to you by:
brucewalker,
rogertsang
From: Smith, S. <sta...@in...> - 2006-06-23 17:32:20
|
Aneesh Kumar wrote: > On 6/21/06, Smith, Stan <sta...@in...> wrote: >> Hi John, >> Could you please review, or delegate a review, of these minor >> patches to support ICS over Infiniband. These patches build an ICS >> device infrastructure in support of 3 ICS device flavors: >> Ethernet (default). >> Infiniband IPoIB similar to ICS Ethernet in that they both use IPv4 >> transport protocols. Infiniband reliable connections with RDMA read >> support.=20 >>=20 >> This patch does not contain IB stack code, only the infrastructure to >> support ICS device selection; see Cluster Kconfig selection. >>=20 >=20 > I have looked at the changes. I am attaching the comments below. >=20 > +ifeq ($(CONFIG_ICS_ETHERNET),y) > obj-$(CONFIG_ICS) :=3D ics_tcp/ > +endif >=20 > This can be written as >=20 > obj-$(CONFIG_ICS_ETHERNET) :=3D ics_tcp/ >=20 >=20 > True with rest of Makefile changes Will do. >=20 > typedef union { > +#if defined(CONFIG_ICS_IP) > tcp_cli_llhandle_t tcpcli_llhandle; > +#elif defined(CONFIG_ICS_IB_VERBS) > + ib_cli_llhandle_t ibcli_llhandle; > +#else > +#error "No define for cli_ll_handle" > +#endif > + > } cli_llhandle_t; >=20 I suspect the union is historical due to previous ICS devices which are no longer supported?=20 My goal was to make minimal src tree impact. Removing the union sounds good although there will be work cleaning up #define's in tcp_sock.h. Please advise when you push these changes as I will need to do similar cleanup in ics/ics_ib_private.h as it was based on tcp_sock.h. >=20 > I guess the purpose of making it a union is to avoid these type of > typedefs. But then since we conditionally build the infiniband and IP > transport that would be possible. >=20 > Something i noticed in the file system with respect to inode is the > generic inode is a part of each file system inode and each file system > have macros to get the file system specific macro from generic one > Take a look at PROC_I. I guess for the cvs code we can push the > #define as you kept here. I will clean it up in my git repository > later. >=20 >=20 > -aneesh Thanks for the feedback. Stan. |