lkml.org 
[lkml]   [2008]   [Aug]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC v2][PATCH 2/9] General infrastructure for checkpoint restart
    From
    Date
    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



    \
     
     \ /
      Last update: 2008-08-26 18:45    [W:4.080 / U:0.048 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site