On Sun, 2008-08-24 at 22:47 -0400, Oren Laadan wrote:I replaced all of the uses of these with kmalloc()/kfree() and stackI disagree with putting some of the cr_hdr_... on the stack: first, if they
allocations. I'm really not sure what these buy us since our allocators
are already so fast. tbuf, especially, worries me if it gets used in
any kind of nested manner we're going to get some really fun bugs.
grow in size, it's easy to forget to change the allocation to stack.
I can buy that.
Second,
using a standard code/handling for all cr_hdr_... makes the code more readable
and easier for others to extend by simply following existing code.
It actually makes it harder for others to jump into because they see
something which is basically a kmalloc() to the rest of the kernel. We
don't have ext2_alloc() or usb_alloc(), so I'm not sure we should have
cr_alloc(), effectively.
The main argument for ->hbuf is that eventually we would like to buffer
data in the kernel to avoid potentially slow writing/reading when processes
are frozen during checkpoint/restart.
Um, we're writing to a file descriptor and kmap()'ing. Those can both
potentially be very, very slow. I don't think that a few kmalloc()s
thrown in there are going to be noticeable.
By using the simple cr_get_hbuf() and
cr_put_hbuf() we prepare for that: now they just allocate room in ->hbuf,
but later they will provide a pointer directly in the data buffer. Moreover,
it simplifies the error path since cleanup (of ->hbuf) is implicit.
It simplifies *one* error path. But, it complicates the ctx creation
and makes *that* error path more complex. Pick your poison, I guess.
Also,
it is designed to allow checkpoint and restart function to be called in a
nested manner, again simplifying the code. Finally, my experience was that
it can impact performance if you are after very short downtimes, especially
for the checkpoint.
Right, but I'm comparing it to kmalloc() Certainly kmalloc()s can be
used in a nested manner.
The idea with cr_read_obj_type() is that it returns the 'ptag' - parent tag -+/* read the checkpoint header */How about just making cr_read_obj_type() stop returning positive values?
+static int cr_read_hdr(struct cr_ctx *ctx)
+{
+ struct cr_hdr_head *hh = cr_hbuf_get(ctx, sizeof(*hh));
+ int ret;
+
+ ret = cr_read_obj_type(ctx, hh, sizeof(*hh), CR_HDR_HEAD);
+ if (ret < 0)
+ return ret;
+ else if (ret != 0)
+ return -EINVAL;
Is it ever valid for it to return >0?
field of the object that it reads in. The 'ptag' is the tag of the parent
object, and is useful in several places. For instance, the 'ptag' of an MM
header identifies (is equal to) the 'tag' of TASK to which it belongs. In
this case, the 'ptag' should be zero because it has no parent object.
I'll change the variable name from 'ret' to 'ptag' to make it more obvious.
Since this ptag isn't actually used, I can't really offer a suggestion.
I don't see the whole picture.