Messages in this thread | | | Subject | Re: [RFC v2][PATCH 2/9] General infrastructure for checkpoint restart | From | Dave Hansen <> | Date | Tue, 26 Aug 2008 09:42:29 -0700 |
| |
On Sun, 2008-08-24 at 22:47 -0400, Oren Laadan wrote: > > I replaced all of the uses of these with kmalloc()/kfree() and stack > > 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. > > I disagree with putting some of the cr_hdr_... on the stack: first, if they > 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.
> >> +/* read the checkpoint header */ > >> +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; > > > > How about just making cr_read_obj_type() stop returning positive values? > > Is it ever valid for it to return >0? > > The idea with cr_read_obj_type() is that it returns the 'ptag' - parent tag - > 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.
-- Dave
| |