Re: [PATCH] cld: use XDR for all messages
On Wed, Jan 13, 2010 at 4:10 PM, Jeff Garzik wrote: > OK, final review. Please fix these problems and resend (some of these > repeated from other emails; putting here to consolidate): > > * needs to build patch builds and runs against a fresh git repo clone + > ./autogen.sh + ./configure + make distcheck... Arg! Looks like it needs more work in the automake department. FWIW (which isn't much), ./autogen + ./configure + make did work for me. I probably had some previous build products hanging around in the system include paths. Also, your point about "make clean" is well taken. > * you increased the size of struct cldc_call_opts to over 256k! That is > unworkable in multi-thread programs, where struct cldc_call_opts may be > allocated on the stack -- as it is in most of the tool and test code using > libcldc. another solution, acceptable to usage within a thread (often > limited to 8k stacks) must be found. Good point. It could be solved by dynamically allocating the buffer, or by moving the statically allocated temporary buffer into cldc_session. Dynamic allocation is probably better, but it will require API users to free the buffer after use. > * it would be nice if this patch were separated out a bit, as there are a > lot of semi-related changes and cleanups mixed in amongst the XDR > conversion. I'm not 100% sure how much work this would be. I can take a look, at least... > * do we really need separate pkt_is_first() and pkt_is_last() ? I tend to > prefer a flags-based approach, where the code tests bits in a 'flags' > variable. I'll make it flag-based. > > * the stuff in cld_fmt.h is essentially global application identifier > namespace. as such, exporting functions like 'dump_buf' or 'opstr' may run > into trouble. suggest '__cld_' prefix, perhaps. Yeah, cld_fmt.c functions get linked into libcldc, so it's best to prefix them with "cldc_" sincerely, Colin -- To unsubscribe from this list: send the line "unsubscribe hail-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] cld: use XDR for all messages
On 01/10/2010 10:00 AM, Colin McCabe wrote: This patch moves CLD from using manual data serialization to using XDR (via rpcgen). Both the packet header and the message body are now serialized and deserialized using XDR. This makes it easy to have a variable-length packet header, as well as a variable-length message body. Message definitions are now in cld_msg.x. Helper functions for using messages are in cld_fmt.c. The new client and server-side functions for sending messages don't require the caller to precompute the size of the message. Instead, you simply tell it what type of message you have, and give it the data. It handles determining message size and encoding. The only fields that I did not choose to deal with using XDR are the packet sequence ID and the packet SHA checksum. The seqid would have been inconvenient and inefficient to store in the XDR header, because we have to check a lot of packets' seqids against the ACKs we receive very frequently. The SHA checksum can't be computed until the rest of the packet has been serialized, which is counter to the way XDR works (it is a single-pass serialization system.) So these fields are in the packet footer, which comes at the end of every packet. This patch introduces a minor libcldc API change in struct cldc_call_opts. Signed-off-by: Colin McCabe --- .gitignore |4 + include/Makefile.am|2 +- include/cld_common.h | 10 + include/cld_fmt.h | 78 include/cld_msg.h | 252 - include/cldc.h | 37 +- lib/Makefile.am| 14 +- lib/cld_fmt.c | 189 ++ lib/cld_msg_rpc.x | 212 +++ lib/cldc.c | 960 +--- lib/common.c |6 +- server/Makefile.am |9 +- server/cld.h | 97 -- server/cldb.h |2 +- server/msg.c | 304 ++-- server/server.c| 743 - server/session.c | 396 ++--- test/load-file-event.c | 14 +- test/save-file-event.c |2 - tools/cldcli.c | 22 +- 20 files changed, 1744 insertions(+), 1609 deletions(-) create mode 100644 include/cld_fmt.h delete mode 100644 include/cld_msg.h create mode 100644 lib/cld_fmt.c create mode 100644 lib/cld_msg_rpc.x OK, final review. Please fix these problems and resend (some of these repeated from other emails; putting here to consolidate): * needs to build patch builds and runs against a fresh git repo clone + ./autogen.sh + ./configure + make distcheck * one build fix I applied was adding the generated header to BUILT_SOURCES in lib/Makefile.am, referring to http://www.gnu.org/software/automake/manual/automake.html#Sources * make sure the generated header can be found for distcheck builds. For me, I needed to add lib/ to the INCLUDE dirs. YMMV. As Pete says, better solutions could be found. You could always put cld_msg_rpc.x in include/, generate the XDR header first thing (reorder SUBDIRS in ./Makefile.am), and then generate the xdr.c file by referencing source file ../include/cld_msg_rpc.x. It's ugly, but cld_msg_rpc.h is definitely an exported header that needs special treatment. This solution, as a side effect, would work within existing INCLUDES Makefile.am setups. * include/Makefile.am needs cld_fmt.h added to include_HEADERS * verify that 'make clean' and 'make distclean' work as expected, removing the built objects (clean) and generated source files (distclean). distclean requires a ./configure step before the tree is buildable... ie. it takes the working tree back to just-unpacked-from-tarball state. * you increased the size of struct cldc_call_opts to over 256k! That is unworkable in multi-thread programs, where struct cldc_call_opts may be allocated on the stack -- as it is in most of the tool and test code using libcldc. another solution, acceptable to usage within a thread (often limited to 8k stacks) must be found. * it would be nice if this patch were separated out a bit, as there are a lot of semi-related changes and cleanups mixed in amongst the XDR conversion. These changes are technically correct, such as - HAIL_DEBUG(&sess->log, "rx_gen: comparing req->xid (%llu) " - "with resp->xid_in (%llu)", - (unsigned long long) le64_to_cpu(req->xid), - (unsigned long long) le64_to_cpu(resp->xid_in)); + HAIL_DEBUG(&sess->log, "%s: comparing req->xid (%llu) " + "with resp.xid_in (%llu)", + __func__, + (unsigned long long) req->xid, + (unsigned long long) resp.xid_in); but adding __func__ to HAIL_DEBUG() statement is clearly not something strictly related to XDR conversion. Pulling out these cleanups
Re: [PATCH] cld: use XDR for all messages
On 01/13/2010 04:38 PM, Pete Zaitcev wrote: On Wed, 13 Jan 2010 16:03:45 -0500 Jeff Garzik wrote: Well, this definitely does not build as-is. lib/Makefile.am needs BUILT_SOURCES = cld_msg_rpc.h otherwise nothing builds at all, because cld_msg_rpc.h does not exist. test/Makefile.am and tools/Makefile.am both need -I$(top_srcdir)/lib added to their INCLUDES. [] Kernel places generated files into the top include directory, e.g. autoconf.h and asm-offsets.h for asm/. It's probably easier than piling up include paths. I dunno. Anything other than $(top_srcdir)/include requires an addition to INCLUDES. (remember, build-dir != src-dir always, especially with 'make distcheck') Speaking of 'make distcheck', that is broken in the XDR patch, too. include/Makefile.am needs cld_fmt.h added to include_HEADERS. Jeff -- To unsubscribe from this list: send the line "unsubscribe hail-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] cld: use XDR for all messages
On Wed, 13 Jan 2010 16:03:45 -0500 Jeff Garzik wrote: > Well, this definitely does not build as-is. lib/Makefile.am needs > > BUILT_SOURCES = cld_msg_rpc.h > > otherwise nothing builds at all, because cld_msg_rpc.h does not exist. > test/Makefile.am and tools/Makefile.am both need > > -I$(top_srcdir)/lib > > added to their INCLUDES. [] Kernel places generated files into the top include directory, e.g. autoconf.h and asm-offsets.h for asm/. It's probably easier than piling up include paths. I dunno. -- Pete -- To unsubscribe from this list: send the line "unsubscribe hail-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] cld: use XDR for all messages
On 01/10/2010 10:00 AM, Colin McCabe wrote: This patch moves CLD from using manual data serialization to using XDR (via rpcgen). Both the packet header and the message body are now serialized and deserialized using XDR. This makes it easy to have a variable-length packet header, as well as a variable-length message body. Message definitions are now in cld_msg.x. Helper functions for using messages are in cld_fmt.c. The new client and server-side functions for sending messages don't require the caller to precompute the size of the message. Instead, you simply tell it what type of message you have, and give it the data. It handles determining message size and encoding. The only fields that I did not choose to deal with using XDR are the packet sequence ID and the packet SHA checksum. The seqid would have been inconvenient and inefficient to store in the XDR header, because we have to check a lot of packets' seqids against the ACKs we receive very frequently. The SHA checksum can't be computed until the rest of the packet has been serialized, which is counter to the way XDR works (it is a single-pass serialization system.) So these fields are in the packet footer, which comes at the end of every packet. This patch introduces a minor libcldc API change in struct cldc_call_opts. Signed-off-by: Colin McCabe --- .gitignore |4 + include/Makefile.am|2 +- include/cld_common.h | 10 + include/cld_fmt.h | 78 include/cld_msg.h | 252 - include/cldc.h | 37 +- lib/Makefile.am| 14 +- lib/cld_fmt.c | 189 ++ lib/cld_msg_rpc.x | 212 +++ lib/cldc.c | 960 +--- lib/common.c |6 +- server/Makefile.am |9 +- server/cld.h | 97 -- server/cldb.h |2 +- server/msg.c | 304 ++-- server/server.c| 743 - server/session.c | 396 ++--- test/load-file-event.c | 14 +- test/save-file-event.c |2 - tools/cldcli.c | 22 +- 20 files changed, 1744 insertions(+), 1609 deletions(-) create mode 100644 include/cld_fmt.h delete mode 100644 include/cld_msg.h create mode 100644 lib/cld_fmt.c create mode 100644 lib/cld_msg_rpc.x Well, this definitely does not build as-is. lib/Makefile.am needs BUILT_SOURCES = cld_msg_rpc.h otherwise nothing builds at all, because cld_msg_rpc.h does not exist. test/Makefile.am and tools/Makefile.am both need -I$(top_srcdir)/lib added to their INCLUDES. And you should probably examine http://www.gnu.org/software/automake/manual/automake.html#Clean to make sure that "make distclean" and "make clean" work. Still playing with it. You definitely need to make sure your patch builds and runs against a fresh git repo check + autogen.sh + ./configure run. Jeff -- To unsubscribe from this list: send the line "unsubscribe hail-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] cld: use XDR for all messages
On 01/10/2010 10:00 AM, Colin McCabe wrote: This patch moves CLD from using manual data serialization to using XDR (via rpcgen). Both the packet header and the message body are now serialized and deserialized using XDR. This makes it easy to have a variable-length packet header, as well as a variable-length message body. Message definitions are now in cld_msg.x. Helper functions for using messages are in cld_fmt.c. The new client and server-side functions for sending messages don't require the caller to precompute the size of the message. Instead, you simply tell it what type of message you have, and give it the data. It handles determining message size and encoding. The only fields that I did not choose to deal with using XDR are the packet sequence ID and the packet SHA checksum. The seqid would have been inconvenient and inefficient to store in the XDR header, because we have to check a lot of packets' seqids against the ACKs we receive very frequently. The SHA checksum can't be computed until the rest of the packet has been serialized, which is counter to the way XDR works (it is a single-pass serialization system.) So these fields are in the packet footer, which comes at the end of every packet. This patch introduces a minor libcldc API change in struct cldc_call_opts. Signed-off-by: Colin McCabe --- .gitignore |4 + include/Makefile.am|2 +- include/cld_common.h | 10 + include/cld_fmt.h | 78 include/cld_msg.h | 252 - include/cldc.h | 37 +- lib/Makefile.am| 14 +- lib/cld_fmt.c | 189 ++ lib/cld_msg_rpc.x | 212 +++ lib/cldc.c | 960 +--- lib/common.c |6 +- server/Makefile.am |9 +- server/cld.h | 97 -- server/cldb.h |2 +- server/msg.c | 304 ++-- server/server.c| 743 - server/session.c | 396 ++--- test/load-file-event.c | 14 +- test/save-file-event.c |2 - tools/cldcli.c | 22 +- 20 files changed, 1744 insertions(+), 1609 deletions(-) create mode 100644 include/cld_fmt.h delete mode 100644 include/cld_msg.h create mode 100644 lib/cld_fmt.c create mode 100644 lib/cld_msg_rpc.x Fantastic! This will take a day or two to review, so please be a little bit patient, waiting for the patch to be applied. At first glance, it looks pretty good. Thanks, this will really help with future protocol changes, most notably (a) fleshing out multi-master support and (b) introducing cache coherency. Jeff -- To unsubscribe from this list: send the line "unsubscribe hail-devel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
