LinuxLists.cc - [PATCH 10/30] cr: core stuff

[permalink] [raw]

Subject: Re: [PATCH 10/30] cr: core stuff

On Fri, Apr 10, 2009 at 11:35:20AM +0200, Ingo Molnar wrote:
>
> * Alexey Dobriyan <[email protected]> wrote:
>
> > +int cr_restore_file(struct cr_context *ctx, loff_t pos)
> > +{
>
> I tried to review this code, but it's almost unreadable to me,

Pity you.

> due to basic code structure mistakes like:

OK, I'll do classic error unwind, not that it was important.

> > + struct cr_image_file *i, *tmp;
> > + struct file *file;
> > + struct cr_object *obj;
> > + char *cr_name;
> > + int rv;
> > +
> > + i = kzalloc(sizeof(*i), GFP_KERNEL);
> > + if (!i)
> > + return -ENOMEM;
> > + rv = cr_pread(ctx, i, sizeof(*i), pos);
> > + if (rv < 0) {
> > + kfree(i);
> > + return rv;
> > + }
> > + if (i->cr_hdr.cr_type != CR_OBJ_FILE) {
> > + kfree(i);
> > + return -EINVAL;
> > + }
> > + /* Image of struct file is variable-sized. */
> > + tmp = i;
> > + i = krealloc(i, i->cr_hdr.cr_len + 1, GFP_KERNEL);
> > + if (!i) {
> > + kfree(tmp);
> > + return -ENOMEM;
> > + }
> > + cr_name = (char *)(i + 1);
> > + rv = cr_pread(ctx, cr_name, i->cr_name_len, pos + sizeof(*i));
> > + if (rv < 0) {
> > + kfree(i);
> > + return -ENOMEM;
> > + }
> > + cr_name[i->cr_name_len] = '\0';
> > +
> > + file = filp_open(cr_name, i->cr_f_flags, 0);
> > + if (IS_ERR(file)) {
> > + kfree(i);
> > + return PTR_ERR(file);
> > + }
> > + if (file->f_dentry->d_inode->i_mode != i->cr_i_mode) {
> > + fput(file);
> > + kfree(i);
> > + return -EINVAL;
> > + }
> > + if (vfs_llseek(file, i->cr_f_pos, SEEK_SET) != i->cr_f_pos) {
> > + fput(file);
> > + kfree(i);
> > + return -EINVAL;
> > + }
> > +
> > + obj = cr_object_create(file);
> > + if (!obj) {
> > + fput(file);
> > + kfree(i);
> > + return -ENOMEM;
> > + }
>
> This contains 7 kfree()s of the same thing (!), 3 fput()s of the
> same thing, replicated all over the place obscuring the real essence
> of the code.
>
> This should be restructured to move all the failure exception cases
> into a clean out of line inverse teardown sequence with proper goto
> labels. That way it will be 70% real code 30% teardown - not 10%
> real code mixed into 90% teardown like above.

OK.

> Also, whoever named a local variable with a type of
> "struct cr_image_file *" as 'i' should be sent back to
> coding primary school.

"i" stands for "image" which is often used in C/R code, because
everything is dumped in image and restored from it, so image itself is
often used.

Because we won't iterate much on C/R, similarity to loop indexes don't matter.

> You really should not write new kernel code until you know, follow
> and respect basic code cleanliness principles. I am not inserting
> any more review feedback value into this code until it does not meet
> _basic_ quality standards that make review efforts smooth and
> efficient.
>
> Oh, and then i saw this sequence:
>
> > + /* Known good and unknown bad flags. */
> > + vm_flags &= ~VM_READ;
> > + vm_flags &= ~VM_WRITE;
> > + vm_flags &= ~VM_EXEC;
> > +// vm_flags &= ~VM_SHARED;
> > + vm_flags &= ~VM_MAYREAD;
> > + vm_flags &= ~VM_MAYWRITE;
> > + vm_flags &= ~VM_MAYEXEC;
> > +// vm_flags &= ~VM_MAYSHARE;
> > + vm_flags &= ~VM_GROWSDOWN;
> > +// vm_flags &= ~VM_GROWSUP;
> > +// vm_flags &= ~VM_PFNMAP;
> > + vm_flags &= ~VM_DENYWRITE;
> > + vm_flags &= ~VM_EXECUTABLE;
> > +// vm_flags &= ~VM_LOCKED;
> > +// vm_flags &= ~VM_IO;
> > +// vm_flags &= ~VM_SEQ_READ;
> > +// vm_flags &= ~VM_RAND_READ;
> > +// vm_flags &= ~VM_DONTCOPY;
> > + vm_flags &= ~VM_DONTEXPAND;
> > +// vm_flags &= ~VM_RESERVED;
> > + vm_flags &= ~VM_ACCOUNT;
> > +// vm_flags &= ~VM_NORESERVE;
> > +// vm_flags &= ~VM_HUGETLB;
> > +// vm_flags &= ~VM_NONLINEAR;
> > +// vm_flags &= ~VM_MAPPED_COPY;
> > +// vm_flags &= ~VM_INSERTPAGE;
> > + vm_flags &= ~VM_ALWAYSDUMP;
> > + vm_flags &= ~VM_CAN_NONLINEAR;
> > +// vm_flags &= ~VM_MIXEDMAP;
> > +// vm_flags &= ~VM_SAO;
> > +// vm_flags &= ~VM_PFN_AT_MMAP;
>
> No comment ...

You have understood what for is it and why it's written in this way?
Really?

Code checks which VMAs are supported to allow checkpointing.

The policy is deny by default.

What was allowed is what is supported (modulo bugs, like VM_ACCOUNT
should be incomplete).

Every flag is mentioned so that grepping will hint someone that C/R code
also cares (not much right now).

This is enough for dynamically-linked busyloop created on Lenny to pass
which is good enough for test program.

Flags will be allowed as C/R progress will go and, e.g, hugetlb and
shared mappings will become supported.

And of course, I don't want to see multiline

vmflags &= ~(VM_READ|VM_WRITE|
[5 lines skipped]

statement and changing it cf 80-column every time someone fixes or adds
VMA flag.

This particular function has more low-level thoughts put it in than some
other core functions and you don't have comments.

2009-04-10 16:19:38

by Brian Haley

[permalink] [raw]

Subject: Re: [PATCH 10/30] cr: core stuff

Alexey Dobriyan wrote:
> And of course, I don't want to see multiline
>
> vmflags &= ~(VM_READ|VM_WRITE|
> [5 lines skipped]

Then why don't you:

#define VM_CR_FOO (VM_READ|VM_WRITE|...)

vmflags &= ~VM_CR_FOO;

-Brian

2009-04-13 08:10:55

[permalink] [raw]

Subject: Re: [PATCH 10/30] cr: core stuff

On Fri, Apr 10, 2009 at 12:19:23PM -0400, Brian Haley wrote:
> Alexey Dobriyan wrote:
> > And of course, I don't want to see multiline
> >
> > vmflags &= ~(VM_READ|VM_WRITE|
> > [5 lines skipped]
>
> Then why don't you:
>
> #define VM_CR_FOO (VM_READ|VM_WRITE|...)
>
> vmflags &= ~VM_CR_FOO;

This won't fix "multiline" part.

2009-04-13 21:47:18

by Serge E. Hallyn

[permalink] [raw]

Subject: Re: [PATCH 10/30] cr: core stuff

Quoting Alexey Dobriyan ([email protected]):

Hi Alexey,

as far as I can see, the main differences between this patch and the
equivalent in Oren's tree are:

1. kernel auto-selects container init to freeze
2. kernel freezes tasks
3. no objhash taking references
4. no hbuf
5. always require CAP_SYS_ADMIN

Are there other differences which you would consider meaningful? Which
do you consider the most important?

Also, since Dave introduced the fops->checkpoint(), we (or at least I)
have been struck by the ugly assymetry with checkpoint() being in fops,
and restart() not. Do you have an idea for fixing that?

thanks,
-serge

2009-04-14 05:24:42

[permalink] [raw]

Subject: Re: [PATCH 10/30] cr: core stuff

Alexey Dobriyan wrote:
> * add struct file_operations::checkpoint
>
> The point of hook is to serialize enough information to allow restoration
> of an opened file.
>
> The idea (good one!) is that the code which supplies struct file_operations
> know better what to do with file.

Actually, credit is due to Dave Hansen (or Christoph Hellwig, or both?).

>
> Hook gets C/R context (a cookie more or less) on which dump code can
> cr_write() and small restrictions on what to write: globally unique object id
> and correct object length to allow jumping through objects.
>
> For usual files on on-disk filesystem add generic_file_checkpoint()
>
> Add ext3 opened regular files and directories for start.
>
> No ->checkpoint, checkpointing is aborted -- deny by default.
>
> FIXME: unlinked, but opened files aren't supported yet.
>
> * C/R image design
>
> The thing should be flexible -- kernel internals changes every day, so we can't
> really afford a format with much enforced structure.
>
> Image consists of header, object images and terminator.
>
> Image header consists of immutable part and mutable part (for future).
>
> Immutable header part is magic and image version: "LinuxC/R" + __le32
>
> Image version determines everything including image header's mutable part.
> Image version is going to be bumped at earliest opportunity following changes
> in kernel internals.
>
> So far image header mutable part consists of arch of the kernel which dumped
> the image (i386, x86_64, ...) and kernel version as found in utsname.
>
> Kernel version as string is for distributions. Distro can support C/R for
> their own kernels, but can't realistically be expected to bump image version --
> this will conflict with mainline kernels having used same version. We also don't
> want requests for private parts of image version space.

So far so good, like in our patch-set.

You also need to address differences in configuration (kernel could
have been recompiled) and runtime environment (boot params, etc).

We deferred this issue to a later time.

>
> Distro expected to keep image version alone and on restart(2) check utsname
> version and compare it against previously release kernel versions and based
> on that turn on compatibility code.

Are you suggesting that conversion of a checkpoint image from an older
version to a newer version be done in the kernel ?

It may work for a few versions, and then you'll get a spaghetti of
#ifdef's in the code, together with a plethora of legacy code.

It is much better/easier to handle checkpoint image transformations
in user space. The kernel will only understand its "current" version
(for some definition of version).

>
> Object image is very flexible, the only required parts are a) object type (u32)
> and b) object total length (u32, [knocks wood]) which must be at the beginning
> of an image. The rest is not generic C/R code problem.
>
> Object images follow one another without holes. Holes are in theory possible but
> unneeded.
>

When would you need holes ?

> Image ends with terminator object. This is mostly to be sure, that, yes, image
> wasn't truncated for some reason.
>
>
> * Objects subject to C/R
>
> The idea is to not be very smart but directly dump core kernel data structures
> related to processes. This includes in this patch:
>
> struct task_struct
> struct mm_struct
> VMAs
> dirty pages
> struct file
>
> Relations between objects (task_struct has pointer to mm_struct) are fullfilled
> by dumping pointed to object first, keeping it's position in dumpfile and saving
> position in a image of pointe? object:

Unless you use the physical position to actually lseek to there to
re-read the data, there is no reason to use the actual position. In
fact it is easier to debug when the shared object identifier is a
simple counter.

If you do use it to lseek, then it's a poor decision -- sounds fragile:
what if we change the file (legitimately) adding data in the middle -
the whole concept breaks.

>
> struct cr_image_task_struct {
> cr_pos_t cr_pos_mm;
> ...
> };
>
> Code so far tries hard to dump objects in certain order so there won't be any loops.
> This property of process that dumpfile can in theory be O_APPEND, will likely be
> sacrifised (read: child can ptrace parent)

The ability to streamline the checkpoint image IMHO is invaluable.
It's the unix way (TM) of doing things; it makes the process pipe-able.

You can do many nice things when the checkpoint can be streamed: you
can compress, sign, encrypt etc on the fly without taking additional
diskspace. You can transfer over the network (e.g. for migration),
or store remotely without explicit file system support. You can easily
transform the stream from one c/r version to another etc.

This should be a design principle. In my experience I never hit a wall
that forced me to "sacrifice" this decision.

> sacrifised (read: child can ptrace parent)

Hmmm... if all tasks are created in user space, then this specific
becomes a no-brainer !

>
> * add struct vm_operations_struct::checkpoint
>
> just like with files, code that creates special VMAs should know what to do with them
> used.
>
> just like with files, deny checkpointing by default
>
> So far used to install vDSO to same place.

VDSO can be a troublemaker; in recent kernels its location in the MM
can be randomized. It is not necessarily immutable - it can reflect
ynamic kernel data. It may contain different code on newer versions,
so must be compared or worked around during restart etc.

>
> * add checkpoint(2)
>
> Done by determining which tasks are subject to checkpointing, freezeing them,
> collecting pointers to necessary kernel internals (task_struct, mm_struct, ...),
> doing that checking supported/unsupported status and aborting if necessary,
> actual dumping, unfreezeing/killing set of tasks.
>
> Also in-checkpoint refcount is maintained to abort on possible invisible changes.
> Now it works:
>
> For every collected object (mm_struct) keep numbers of references from
> other collected objects. It should match object's own refcount.
> If there is a mismatch, something is likely pinning object, which means
> there is "leak" to outside which means checkpoint(2) can't realistically and
> without consequences proceed.
>
> This is in some sense independent check. It's designed to protect from internals
> change when C/R code was forgotten to be updated.
>
> Userpsace supplies pid of root task and opened file descriptor of future dump file.
> Kernel reports 0/-E as usual.
>
> Runtime tracking of "checkpointable" property is explicitly not done.
> This introduces overhead even if checkpoint(2) is not done as shown by proponents.
> Instead any check is done at checkpoint(2) time and -E is returned if something is
> suspicious or known to be unsupported.
>
> FIXME: more checks especially in cr_check_task_struct().
>
> * add restart(2)
>
> Recreate tasks and evething dumped by checkpoint(2) as if nothing happened.
>
> The focus is on correct recreating, checking every possibility that target kernel
> can be on different arch (i386 => x86_64) and target kernel can be very different
> from source kernel by mistake (i386 => x86_64 COMPAT=n) kernel.
>
> restart(2) is done first by creating kernel thread and that demoting it to usual
> process by adding mm_struct, VMAs, et al. This saves time against method when
> userspace does fork(2)+restart(2) -- forked mm_struct will be thrown out anyway
> or at least everything will be unmapped in any case.

Do have figures to support your claims about "saves time" ?

The *largest* component of the restart time, as you probably know,
is the time it takes to restore the memory address space (pages, pages)
of the tasks.

If you do show that this optimization is worth our attention, then it
takes < 10 lines to change current mktree.c to use CLONE_VM ... voila.

I'm interested in hearing more convincing arguments in favor of kernel
creations of restarting tasks (see my other post about it).

>
> Restoration is done in current context except CPU registers at last stage.
> This is because "creation is done by current" is in many, many places,
> e.g. mmap(2) code.
>
> It's expected that filesystem state will be the same. Kernel can't do anything
> about it expect probably virtual filesystems. If a file is not there anymore,
> it's not kernel fault, -E will be returned, restart aborted.
>
> FIXME: errors aren't propagated correctly out of kernel thread context

Heh .. I guess they always propagate correctly out of regular task
context ;)

Oren.

2009-04-14 05:55:17

[permalink] [raw]

Subject: Re: [PATCH 10/30] cr: core stuff

Hi,

Serge E. Hallyn wrote:
> Quoting Alexey Dobriyan ([email protected]):
>
> Hi Alexey,
>
> as far as I can see, the main differences between this patch and the
> equivalent in Oren's tree are:
>
> 1. kernel auto-selects container init to freeze

Actually, this eliminates the possibility to checkpoint a subtree of
tasks, which (under some obvious constraints) can be a handy feature.

> 2. kernel freezes tasks

IMHO better to do it in userspace - that way userspace can accomplish
other tasks while tasks are frozen, such as snapshot the filesystem,
or block/unblock the network.

Is there a good argument to do it kernel ?

> 3. no objhash taking references
> 4. no hbuf
> 5. always require CAP_SYS_ADMIN

I'm now convinced (thanks, Serge!) that it's better not to require
this unless we strictly have to.

Oren.

2009-04-14 15:27:50

[permalink] [raw]

Subject: Re: [PATCH 10/30] cr: core stuff

On Mon, Apr 13, 2009 at 04:47:01PM -0500, Serge E. Hallyn wrote:
> Quoting Alexey Dobriyan ([email protected]):
>
> Hi Alexey,
>
> as far as I can see, the main differences between this patch and the
> equivalent in Oren's tree are:
>
> 1. kernel auto-selects container init to freeze

Note, auto-select part was dropped, userspace is required to pass pid of
container init exactly. This was done to keep semantic of checkpoint(2)
small and extendable.

> 2. kernel freezes tasks
> 3. no objhash taking references

That's because none needed.

> 4. no hbuf

hbuf is an optimization to not allocate/free memory for every image.
For a start it's unnecessary complication, I just kzalloc/dump/kfree.

> 5. always require CAP_SYS_ADMIN
>
> Are there other differences which you would consider meaningful? Which
> do you consider the most important?
>
> Also, since Dave introduced the fops->checkpoint(), we (or at least I)
> have been struck by the ugly assymetry with checkpoint() being in fops,
> and restart() not. Do you have an idea for fixing that?

Module can legally support C/R for its files.

In the end it most certainly will end up with module registering restart
hook for file type N.

Or module registering hook to restart object type N.

This is for discussion.

2009-04-14 15:30:21

by Serge E. Hallyn

[permalink] [raw]

Subject: Re: [PATCH 10/30] cr: core stuff

Quoting Oren Laadan ([email protected]):
>
> Hi,
>
> Serge E. Hallyn wrote:
> > Quoting Alexey Dobriyan ([email protected]):
> >
> > Hi Alexey,
> >
> > as far as I can see, the main differences between this patch and the
> > equivalent in Oren's tree are:
> >
> > 1. kernel auto-selects container init to freeze
>
> Actually, this eliminates the possibility to checkpoint a subtree of
> tasks, which (under some obvious constraints) can be a handy feature.

Yes, I agree. As Dave pointed out on irc yesterday, this patch shows a
very definate whole-container-only point of view which is worth
discussing.

> > 2. kernel freezes tasks
>
> IMHO better to do it in userspace - that way userspace can accomplish
> other tasks while tasks are frozen, such as snapshot the filesystem,
> or block/unblock the network.

That's a good point.

> Is there a good argument to do it kernel ?

Convenience? I guess you don't have to worry about getting your
checkpoint job into a cgroup by itself ahead of time.

> > 3. no objhash taking references
> > 4. no hbuf
> > 5. always require CAP_SYS_ADMIN
>
> I'm now convinced (thanks, Serge!) that it's better not to require
> this unless we strictly have to.

:) Cool.

I think the perceived need for it comes, as above, from the pure
checkpoint-a-whole-container-only view. So long as you will
checkpoint/restore a whole container, then you'll end up doing
something requiring privilege anyway. But that is not all of
the use cases.

-serge

2009-04-14 15:41:48

[permalink] [raw]

Subject: Re: [PATCH 10/30] cr: core stuff

On Tue, 2009-04-14 at 19:27 +0400, Alexey Dobriyan wrote:
> > Also, since Dave introduced the fops->checkpoint(), we (or at least I)
> > have been struck by the ugly assymetry with checkpoint() being in fops,
> > and restart() not. Do you have an idea for fixing that?
>
> Module can legally support C/R for its files.
>
> In the end it most certainly will end up with module registering restart
> hook for file type N.
>
> Or module registering hook to restart object type N.

Yeah, that was my expectation as well. There's a point when we just
have too many kinds of checkpoint objects and the switch statements get
out of hand. Oversimplified, of course, but:

init_restart_handler(CR_FD_GENERIC, restore_generic_fd);
init_restart_handler(CR_FD_SOCKET, restore_socket);
init_restart_handler(CR_FD_PIPE, restore_pipe);

The only question to me is whether we allow the handler functions to do
further reading of the checkpoint image or whether the higher-level code
should be feeding them all the data they'll need in some way.

-- Dave

2009-04-14 15:42:08

by Serge E. Hallyn

[permalink] [raw]

Subject: Re: [PATCH 10/30] cr: core stuff

Quoting Alexey Dobriyan ([email protected]):
> On Mon, Apr 13, 2009 at 04:47:01PM -0500, Serge E. Hallyn wrote:
> > Quoting Alexey Dobriyan ([email protected]):
> >
> > Hi Alexey,
> >
> > as far as I can see, the main differences between this patch and the
> > equivalent in Oren's tree are:
> >
> > 1. kernel auto-selects container init to freeze
>
> Note, auto-select part was dropped, userspace is required to pass pid of
> container init exactly. This was done to keep semantic of checkpoint(2)
> small and extendable.

sys_checkpoint() in this patch still finds the child_reaper of the
passed-in pid, doesn't it? Or are you saying that a later patch in
this set removes that?

> > 2. kernel freezes tasks
> > 3. no objhash taking references
>
> That's because none needed.

Right while I have opinions on some things in this list, I didn't
mean to imply positions on these items. My question was: are
there are differences you want to call out?

> > 4. no hbuf
>
> hbuf is an optimization to not allocate/free memory for every image.
> For a start it's unnecessary complication, I just kzalloc/dump/kfree.
>
> > 5. always require CAP_SYS_ADMIN
> >
> > Are there other differences which you would consider meaningful? Which
> > do you consider the most important?
> >
> > Also, since Dave introduced the fops->checkpoint(), we (or at least I)
> > have been struck by the ugly assymetry with checkpoint() being in fops,
> > and restart() not. Do you have an idea for fixing that?
>
> Module can legally support C/R for its files.
> In the end it most certainly will end up with module registering restart

Which module? The module defining a filesystem?

In that case I'm just not clear on how the restart code will know which
fs's file_operations to use to pick a fops->restart() fn.

> hook for file type N.
>
> Or module registering hook to restart object type N.
>
> This is for discussion.

Ok, it's just something I've wondered (with both patchsets).

thanks,
-serge

2009-04-14 16:00:13

[permalink] [raw]

Subject: Re: [PATCH 10/30] cr: core stuff

On Tue, Apr 14, 2009 at 01:22:03AM -0400, Oren Laadan wrote:
>
> Alexey Dobriyan wrote:
> > * add struct file_operations::checkpoint
> >
> > The point of hook is to serialize enough information to allow restoration
> > of an opened file.
> >
> > The idea (good one!) is that the code which supplies struct file_operations
> > know better what to do with file.
>
> Actually, credit is due to Dave Hansen (or Christoph Hellwig, or both?).
>
> >
> > Hook gets C/R context (a cookie more or less) on which dump code can
> > cr_write() and small restrictions on what to write: globally unique object id
> > and correct object length to allow jumping through objects.
> >
> > For usual files on on-disk filesystem add generic_file_checkpoint()
> >
> > Add ext3 opened regular files and directories for start.
> >
> > No ->checkpoint, checkpointing is aborted -- deny by default.
> >
> > FIXME: unlinked, but opened files aren't supported yet.
> >
> > * C/R image design
> >
> > The thing should be flexible -- kernel internals changes every day, so we can't
> > really afford a format with much enforced structure.
> >
> > Image consists of header, object images and terminator.
> >
> > Image header consists of immutable part and mutable part (for future).
> >
> > Immutable header part is magic and image version: "LinuxC/R" + __le32
> >
> > Image version determines everything including image header's mutable part.
> > Image version is going to be bumped at earliest opportunity following changes
> > in kernel internals.
> >
> > So far image header mutable part consists of arch of the kernel which dumped
> > the image (i386, x86_64, ...) and kernel version as found in utsname.
> >
> > Kernel version as string is for distributions. Distro can support C/R for
> > their own kernels, but can't realistically be expected to bump image version --
> > this will conflict with mainline kernels having used same version. We also don't
> > want requests for private parts of image version space.
>
> So far so good, like in our patch-set.
>
> You also need to address differences in configuration (kernel could
> have been recompiled) and runtime environment (boot params, etc).
>
> We deferred this issue to a later time.
>
> >
> > Distro expected to keep image version alone and on restart(2) check utsname
> > version and compare it against previously release kernel versions and based
> > on that turn on compatibility code.
>
> Are you suggesting that conversion of a checkpoint image from an older
> version to a newer version be done in the kernel ?

For mainline kernel it's completely unrealistic to support all backwards
compatibility code for previous versions. Some mythical userspace
program will convert images.

But it's completely realistic and much easier for distro kernel because
distro kernel doesn't generally include patches with significant in-kernel
internals changes, so they simply can support
'2.6.26-1-amd64' => '2.6.26-2-amd64' situation.

Distros can write conversion program too, but I don't expect they will.

> It may work for a few versions, and then you'll get a spaghetti of
> #ifdef's in the code, together with a plethora of legacy code.

Expectation is for one kernel branch like RHEL5 kernel updates during
RHEL5 lifecycle.

For RHEL5 => RHEL6, it's up to them what to do.

Anyway distro can add compat code _anyway_, for this we help them with
this image format tweak, so they won't bug mainline with "reserve bit 31
for Red Hat".

Image version is kept small (__le32) for this reason too :-)

> It is much better/easier to handle checkpoint image transformations
> in user space. The kernel will only understand its "current" version
> (for some definition of version).
>
> >
> > Object image is very flexible, the only required parts are a) object type (u32)
> > and b) object total length (u32, [knocks wood]) which must be at the beginning
> > of an image. The rest is not generic C/R code problem.
> >
> > Object images follow one another without holes. Holes are in theory possible but
> > unneeded.
> >
>
> When would you need holes ?
>
> > Image ends with terminator object. This is mostly to be sure, that, yes, image
> > wasn't truncated for some reason.
> >
> >
> > * Objects subject to C/R
> >
> > The idea is to not be very smart but directly dump core kernel data structures
> > related to processes. This includes in this patch:
> >
> > struct task_struct
> > struct mm_struct
> > VMAs
> > dirty pages
> > struct file
> >
> > Relations between objects (task_struct has pointer to mm_struct) are fullfilled
> > by dumping pointed to object first, keeping it's position in dumpfile and saving
> > position in a image of pointe? object:
>
> Unless you use the physical position to actually lseek to there to
> re-read the data, there is no reason to use the actual position. In
> fact it is easier to debug when the shared object identifier is a
> simple counter.
>
> If you do use it to lseek, then it's a poor decision -- sounds fragile:
> what if we change the file (legitimately) adding data in the middle -
> the whole concept breaks.

Adder of data is expected to understand image format and update all references
just like surgeon is expected to understand human anatomy.

> > struct cr_image_task_struct {
> > cr_pos_t cr_pos_mm;
> > ...
> > };
> >
> > Code so far tries hard to dump objects in certain order so there won't be any loops.
> > This property of process that dumpfile can in theory be O_APPEND, will likely be
> > sacrifised (read: child can ptrace parent)
>
> The ability to streamline the checkpoint image IMHO is invaluable.
> It's the unix way (TM) of doing things; it makes the process pipe-able.
>
> You can do many nice things when the checkpoint can be streamed: you
> can compress, sign, encrypt etc on the fly without taking additional
> diskspace. You can transfer over the network (e.g. for migration),
> or store remotely without explicit file system support. You can easily
> transform the stream from one c/r version to another etc.
>
> This should be a design principle. In my experience I never hit a wall
> that forced me to "sacrifice" this decision.
>
> > sacrifised (read: child can ptrace parent)
>
> Hmmm... if all tasks are created in user space, then this specific
> becomes a no-brainer !

No!

A ptraces B. Container is checkpointed.

Kernel realizes ptrace is going on. A and B in theory can have any
realitionship.

Consequently, kernel doesn't know in which order to dump A and B.

And there is no such order:
*) A can be parent of B (you dump A, B),
*) A can be child of B (you want to dump B, A, but this conflicts with
->real_parent order)
*) A and B just tasks (any order).

I'm showing that whole issue can be avoided:
*) all tasks are simply created regardless of who is parent of whom
(see kernel_thread())
*) Every task_struct image among other things contains references to
->real_parent and ->parent.
*) After every task is created it's time to change references:
**) lookup who is ->real_parent, change ->real_parent _by hand_
not with some "correct clone(2)" order.
**) lookup who is ->parent, change ->parent.

You're probably escaping all of this with object numbers?

> > * add struct vm_operations_struct::checkpoint
> >
> > just like with files, code that creates special VMAs should know what to do with them
> > used.
> >
> > just like with files, deny checkpointing by default
> >
> > So far used to install vDSO to same place.
>
> VDSO can be a troublemaker; in recent kernels its location in the MM
> can be randomized.

See arch_setup_additional_pages() patch.

> It is not necessarily immutable - it can reflect
> ynamic kernel data. It may contain different code on newer versions,
> so must be compared or worked around during restart etc.

i386 if I'm not mistaken only contain syscall entry code, but, yes,
generally one should check if PC is inside such page.

> > * add checkpoint(2)
> >
> > Done by determining which tasks are subject to checkpointing, freezeing them,
> > collecting pointers to necessary kernel internals (task_struct, mm_struct, ...),
> > doing that checking supported/unsupported status and aborting if necessary,
> > actual dumping, unfreezeing/killing set of tasks.
> >
> > Also in-checkpoint refcount is maintained to abort on possible invisible changes.
> > Now it works:
> >
> > For every collected object (mm_struct) keep numbers of references from
> > other collected objects. It should match object's own refcount.
> > If there is a mismatch, something is likely pinning object, which means
> > there is "leak" to outside which means checkpoint(2) can't realistically and
> > without consequences proceed.
> >
> > This is in some sense independent check. It's designed to protect from internals
> > change when C/R code was forgotten to be updated.
> >
> > Userpsace supplies pid of root task and opened file descriptor of future dump file.
> > Kernel reports 0/-E as usual.
> >
> > Runtime tracking of "checkpointable" property is explicitly not done.
> > This introduces overhead even if checkpoint(2) is not done as shown by proponents.
> > Instead any check is done at checkpoint(2) time and -E is returned if something is
> > suspicious or known to be unsupported.
> >
> > FIXME: more checks especially in cr_check_task_struct().
> >
> > * add restart(2)
> >
> > Recreate tasks and evething dumped by checkpoint(2) as if nothing happened.
> >
> > The focus is on correct recreating, checking every possibility that target kernel
> > can be on different arch (i386 => x86_64) and target kernel can be very different
> > from source kernel by mistake (i386 => x86_64 COMPAT=n) kernel.
> >
> > restart(2) is done first by creating kernel thread and that demoting it to usual
> > process by adding mm_struct, VMAs, et al. This saves time against method when
> > userspace does fork(2)+restart(2) -- forked mm_struct will be thrown out anyway
> > or at least everything will be unmapped in any case.
>
> Do have figures to support your claims about "saves time" ?
>
> The *largest* component of the restart time, as you probably know,
> is the time it takes to restore the memory address space (pages, pages)
> of the tasks.
>
> If you do show that this optimization is worth our attention, then it
> takes < 10 lines to change current mktree.c to use CLONE_VM ... voila.
>
> I'm interested in hearing more convincing arguments in favor of kernel
> creations of restarting tasks (see my other post about it).

OK, in another post.

> > Restoration is done in current context except CPU registers at last stage.
> > This is because "creation is done by current" is in many, many places,
> > e.g. mmap(2) code.
> >
> > It's expected that filesystem state will be the same. Kernel can't do anything
> > about it expect probably virtual filesystems. If a file is not there anymore,
> > it's not kernel fault, -E will be returned, restart aborted.
> >
> > FIXME: errors aren't propagated correctly out of kernel thread context
>
> Heh .. I guess they always propagate correctly out of regular task
> context ;)

:-)

2009-04-14 16:37:29

[permalink] [raw]

Subject: "partial" container checkpoint

On Tue, 2009-04-14 at 10:29 -0500, Serge E. Hallyn wrote:
> I think the perceived need for it comes, as above, from the pure
> checkpoint-a-whole-container-only view. So long as you will
> checkpoint/restore a whole container, then you'll end up doing
> something requiring privilege anyway. But that is not all of
> the use cases.

Yeah, there are certainly a lot of shades of gray here. I've been
talking to some HPC guys in the last couple of days. They certainly
have a need for checkpoint/restart, but much less of a need for doing
entire containers.

It also occurs to me that we have the potential to pull some
long-out-of-tree users back in. VMADump users, for instance:

http://bproc.sourceforge.net/c268.html

If we could do *just* a selective checkpoint of a single process's VMAs,
the bproc users could probably use sys_checkpoint() in some way. That's
*way* less than an entire container, but it would be really useful to
some people.

-- Dave

2009-04-14 16:40:10

[permalink] [raw]

Subject: Re: [PATCH 10/30] cr: core stuff

On Tue, 2009-04-14 at 20:00 +0400, Alexey Dobriyan wrote:
> > Are you suggesting that conversion of a checkpoint image from an older
> > version to a newer version be done in the kernel ?
>
> For mainline kernel it's completely unrealistic to support all backwards
> compatibility code for previous versions. Some mythical userspace
> program will convert images.
>
> But it's completely realistic and much easier for distro kernel because
> distro kernel doesn't generally include patches with significant in-kernel
> internals changes, so they simply can support
> '2.6.26-1-amd64' => '2.6.26-2-amd64' situation.
>
> Distros can write conversion program too, but I don't expect they will.

Yeah, I'm with you on this. If distros ever start to care about c/r
*that* much, they'll start making this part of their testing process.
Personally, I think just giving a kernel version is pretty worthless
these days. People do tons of stuff to the kernel without bumping it at
all.

-- Dave

2009-04-14 16:48:54

[permalink] [raw]

Subject: Re: [PATCH 10/30] cr: core stuff

On Tue, 2009-04-14 at 10:41 -0500, Serge E. Hallyn wrote:
> > Module can legally support C/R for its files.
> > In the end it most certainly will end up with module registering restart
>
> Which module? The module defining a filesystem?
>
> In that case I'm just not clear on how the restart code will know which
> fs's file_operations to use to pick a fops->restart() fn.

There's not an f_op on the restart side -- there can't be. The problem
is that we get a CR_FD_FOO object and need to call off into the "foo"
code to recreate the 'struct file'. To me, that screams of a nice list
of function handlers indexed be CR_FD_FOO.

So, we have a list of these sitting around somewhere:

int restore_fd_func(struct cr_ctx *ctx, struct cr_fd_hdr *fd, void *private)

and when we see a CR_FD_HDR object, we look up its type and call the
respective handler. The handler will get enough data to go and restore
the fd. The fd number and other things common to all fds should be
present in the cr_fd_hdr.

-- Dave

2009-04-14 16:58:22

[permalink] [raw]

Subject: Re: [PATCH 10/30] cr: core stuff

On Tue, Apr 14, 2009 at 08:41:34AM -0700, Dave Hansen wrote:
> On Tue, 2009-04-14 at 19:27 +0400, Alexey Dobriyan wrote:
> > > Also, since Dave introduced the fops->checkpoint(), we (or at least I)
> > > have been struck by the ugly assymetry with checkpoint() being in fops,
> > > and restart() not. Do you have an idea for fixing that?
> >
> > Module can legally support C/R for its files.
> >
> > In the end it most certainly will end up with module registering restart
> > hook for file type N.
> >
> > Or module registering hook to restart object type N.
>
> Yeah, that was my expectation as well. There's a point when we just
> have too many kinds of checkpoint objects and the switch statements get
> out of hand. Oversimplified, of course, but:
>
> init_restart_handler(CR_FD_GENERIC, restore_generic_fd);
> init_restart_handler(CR_FD_SOCKET, restore_socket);
> init_restart_handler(CR_FD_PIPE, restore_pipe);
>
> The only question to me is whether we allow the handler functions to do
> further reading of the checkpoint image or whether the higher-level code
> should be feeding them all the data they'll need in some way.

It depends, but since you don't know what it's in dumped state, it's
better to leave freedom for restart hook.

It gets restart context as cookie, position of start of an object, and
exported functions cr_read/cr_pread/whatever which accept cooked
context.

2009-04-14 17:00:06

[permalink] [raw]

Subject: Re: [PATCH 10/30] cr: core stuff

On Tue, Apr 14, 2009 at 10:41:39AM -0500, Serge E. Hallyn wrote:
> Quoting Alexey Dobriyan ([email protected]):
> > On Mon, Apr 13, 2009 at 04:47:01PM -0500, Serge E. Hallyn wrote:
> > > Quoting Alexey Dobriyan ([email protected]):
> > >
> > > Hi Alexey,
> > >
> > > as far as I can see, the main differences between this patch and the
> > > equivalent in Oren's tree are:
> > >
> > > 1. kernel auto-selects container init to freeze
> >
> > Note, auto-select part was dropped, userspace is required to pass pid of
> > container init exactly. This was done to keep semantic of checkpoint(2)
> > small and extendable.
>
> sys_checkpoint() in this patch still finds the child_reaper of the
> passed-in pid, doesn't it? Or are you saying that a later patch in
> this set removes that?

I posted with auto-selecting?

Code now looks like this:

rcu_read_lock();
tsk = find_task_by_vpid(pid);
if (tsk) {
struct nsproxy *nsproxy;

nsproxy = task_nsproxy(tsk);
if (nsproxy) {
init_tsk = nsproxy->pid_ns->child_reaper;
if (init_tsk != tsk)
init_tsk = NULL;
} else
init_tsk = NULL;
if (init_tsk)
get_task_struct(init_tsk);
}
rcu_read_unlock();

This is to buy as little semantics on checkpoint(2) as possible.

If users later will complain that it's for some reason hard to see who is
root of container, it could be added.

2009-04-14 17:04:20

[permalink] [raw]

Subject: Re: [PATCH 10/30] cr: core stuff

On Tue, Apr 14, 2009 at 10:41:39AM -0500, Serge E. Hallyn wrote:
> Quoting Alexey Dobriyan ([email protected]):
> > On Mon, Apr 13, 2009 at 04:47:01PM -0500, Serge E. Hallyn wrote:
> > > Quoting Alexey Dobriyan ([email protected]):
> > >
> > > Hi Alexey,
> > >
> > > as far as I can see, the main differences between this patch and the
> > > equivalent in Oren's tree are:
> > >
> > > 1. kernel auto-selects container init to freeze
> >
> > Note, auto-select part was dropped, userspace is required to pass pid of
> > container init exactly. This was done to keep semantic of checkpoint(2)
> > small and extendable.
>
> sys_checkpoint() in this patch still finds the child_reaper of the
> passed-in pid, doesn't it? Or are you saying that a later patch in
> this set removes that?
>
> > > 2. kernel freezes tasks
> > > 3. no objhash taking references
> >
> > That's because none needed.
>
> Right while I have opinions on some things in this list, I didn't
> mean to imply positions on these items. My question was: are
> there are differences you want to call out?

Sorry? "none needed" is relevant to only item 3. If tasks don't
dissapear during checkpoint, why would netns dissapear.
Taking refcount on checkpoint(2) is likely unneeded.

But it's low-level detail anyway.

2009-04-14 17:23:41

[permalink] [raw]

Subject: checkpoint/restart: taking refcounts on kernel objects

On Tue, 2009-04-14 at 21:04 +0400, Alexey Dobriyan wrote:
> > Right while I have opinions on some things in this list, I didn't
> > mean to imply positions on these items. My question was: are
> > there are differences you want to call out?
>
> Sorry? "none needed" is relevant to only item 3. If tasks don't
> dissapear during checkpoint, why would netns dissapear.
> Taking refcount on checkpoint(2) is likely unneeded.
>
> But it's low-level detail anyway.

I guess it is a matter of whether we consider a task that gets unfrozen
a kernel bug or not. If we don't take refcounts and we do reference an
object that disappears, then we *certainly* have a kernel bug that can
crash the kernel. If we take refcounts, we at least limit the ways in
which the kernel can crash when something screwy happens.

On the other hand, the objhash is a kinda weird way to do it. Taking
and releasing arbitrary refcounts on arbitrary kernel objects one level
too much of abstraction for me.

Come to think of it... In the pipe case, we're *guaranteed* to have
someone hold an extra refcount for us after we encounter the first side
of the pipe: the other side of the pipe. If the other side isn't there,
then we didn't need to save the reference. If it is there, it was
holding a refcount and we didn't need an extra one.

-- Dave

2009-04-14 17:30:43

[permalink] [raw]

Subject: Re: [PATCH 10/30] cr: core stuff

On Tue, Apr 14, 2009 at 09:39:50AM -0700, Dave Hansen wrote:
> On Tue, 2009-04-14 at 20:00 +0400, Alexey Dobriyan wrote:
> > > Are you suggesting that conversion of a checkpoint image from an older
> > > version to a newer version be done in the kernel ?
> >
> > For mainline kernel it's completely unrealistic to support all backwards
> > compatibility code for previous versions. Some mythical userspace
> > program will convert images.
> >
> > But it's completely realistic and much easier for distro kernel because
> > distro kernel doesn't generally include patches with significant in-kernel
> > internals changes, so they simply can support
> > '2.6.26-1-amd64' => '2.6.26-2-amd64' situation.
> >
> > Distros can write conversion program too, but I don't expect they will.
>
> Yeah, I'm with you on this. If distros ever start to care about c/r
> *that* much, they'll start making this part of their testing process.
> Personally, I think just giving a kernel version is pretty worthless
> these days. People do tons of stuff to the kernel without bumping it at
> all.

Well, to some extent this is cop-out.

It allows to easily see (hexdump(1) :-) what kernel dumped image.
And it allows for distro to easily check if it's restart on same version
or from previous version with high degree of confidentness.

Distro kernels have very specific unames if looking for kernels and
kernel updates they officially ship, but yes, this is not 100% reliable.

2009-04-14 17:46:30

[permalink] [raw]

Subject: Re: [PATCH 10/30] cr: core stuff

Alexey Dobriyan wrote:
> On Tue, Apr 14, 2009 at 10:41:39AM -0500, Serge E. Hallyn wrote:
>> Quoting Alexey Dobriyan ([email protected]):
>>> On Mon, Apr 13, 2009 at 04:47:01PM -0500, Serge E. Hallyn wrote:
>>>> Quoting Alexey Dobriyan ([email protected]):
>>>>
>>>> Hi Alexey,
>>>>
>>>> as far as I can see, the main differences between this patch and the
>>>> equivalent in Oren's tree are:
>>>>
>>>> 1. kernel auto-selects container init to freeze
>>> Note, auto-select part was dropped, userspace is required to pass pid of
>>> container init exactly. This was done to keep semantic of checkpoint(2)
>>> small and extendable.
>> sys_checkpoint() in this patch still finds the child_reaper of the
>> passed-in pid, doesn't it? Or are you saying that a later patch in
>> this set removes that?
>>
>>>> 2. kernel freezes tasks
>>>> 3. no objhash taking references
>>> That's because none needed.
>> Right while I have opinions on some things in this list, I didn't
>> mean to imply positions on these items. My question was: are
>> there are differences you want to call out?
>
> Sorry? "none needed" is relevant to only item 3. If tasks don't
> dissapear during checkpoint, why would netns dissapear.
> Taking refcount on checkpoint(2) is likely unneeded.
>
> But it's low-level detail anyway.

Then you need to prevent anyone from thawing the tasks while you're
checkpointing.

An alternative would be to still grab references, just in case, and
ask to be notified if cgroup was thawed - and abort operation (safely).

Oren.

2009-04-14 18:22:23

[permalink] [raw]

Subject: Re: [PATCH 10/30] cr: core stuff

Alexey Dobriyan wrote:
> On Tue, Apr 14, 2009 at 01:22:03AM -0400, Oren Laadan wrote:
>> Alexey Dobriyan wrote:
>>> * add struct file_operations::checkpoint
>>>
>>> The point of hook is to serialize enough information to allow restoration
>>> of an opened file.
>>>
>>> The idea (good one!) is that the code which supplies struct file_operations
>>> know better what to do with file.
>> Actually, credit is due to Dave Hansen (or Christoph Hellwig, or both?).
>>
>>> Hook gets C/R context (a cookie more or less) on which dump code can
>>> cr_write() and small restrictions on what to write: globally unique object id
>>> and correct object length to allow jumping through objects.
>>>
>>> For usual files on on-disk filesystem add generic_file_checkpoint()
>>>
>>> Add ext3 opened regular files and directories for start.
>>>
>>> No ->checkpoint, checkpointing is aborted -- deny by default.
>>>
>>> FIXME: unlinked, but opened files aren't supported yet.
>>>
>>> * C/R image design
>>>
>>> The thing should be flexible -- kernel internals changes every day, so we can't
>>> really afford a format with much enforced structure.
>>>
>>> Image consists of header, object images and terminator.
>>>
>>> Image header consists of immutable part and mutable part (for future).
>>>
>>> Immutable header part is magic and image version: "LinuxC/R" + __le32
>>>
>>> Image version determines everything including image header's mutable part.
>>> Image version is going to be bumped at earliest opportunity following changes
>>> in kernel internals.
>>>
>>> So far image header mutable part consists of arch of the kernel which dumped
>>> the image (i386, x86_64, ...) and kernel version as found in utsname.
>>>
>>> Kernel version as string is for distributions. Distro can support C/R for
>>> their own kernels, but can't realistically be expected to bump image version --
>>> this will conflict with mainline kernels having used same version. We also don't
>>> want requests for private parts of image version space.
>> So far so good, like in our patch-set.
>>
>> You also need to address differences in configuration (kernel could
>> have been recompiled) and runtime environment (boot params, etc).
>>
>> We deferred this issue to a later time.
>>
>>> Distro expected to keep image version alone and on restart(2) check utsname
>>> version and compare it against previously release kernel versions and based
>>> on that turn on compatibility code.
>> Are you suggesting that conversion of a checkpoint image from an older
>> version to a newer version be done in the kernel ?
>
> For mainline kernel it's completely unrealistic to support all backwards
> compatibility code for previous versions. Some mythical userspace
> program will convert images.
>
> But it's completely realistic and much easier for distro kernel because
> distro kernel doesn't generally include patches with significant in-kernel
> internals changes, so they simply can support
> '2.6.26-1-amd64' => '2.6.26-2-amd64' situation.
>
> Distros can write conversion program too, but I don't expect they will.
>
>> It may work for a few versions, and then you'll get a spaghetti of
>> #ifdef's in the code, together with a plethora of legacy code.
>
> Expectation is for one kernel branch like RHEL5 kernel updates during
> RHEL5 lifecycle.
>
> For RHEL5 => RHEL6, it's up to them what to do.
>
> Anyway distro can add compat code _anyway_, for this we help them with
> this image format tweak, so they won't bug mainline with "reserve bit 31
> for Red Hat".
>
> Image version is kept small (__le32) for this reason too :-)
>
>> It is much better/easier to handle checkpoint image transformations
>> in user space. The kernel will only understand its "current" version
>> (for some definition of version).
>>
>>> Object image is very flexible, the only required parts are a) object type (u32)
>>> and b) object total length (u32, [knocks wood]) which must be at the beginning
>>> of an image. The rest is not generic C/R code problem.
>>>
>>> Object images follow one another without holes. Holes are in theory possible but
>>> unneeded.
>>>
>> When would you need holes ?
>>
>>> Image ends with terminator object. This is mostly to be sure, that, yes, image
>>> wasn't truncated for some reason.
>>>
>>>
>>> * Objects subject to C/R
>>>
>>> The idea is to not be very smart but directly dump core kernel data structures
>>> related to processes. This includes in this patch:
>>>
>>> struct task_struct
>>> struct mm_struct
>>> VMAs
>>> dirty pages
>>> struct file
>>>
>>> Relations between objects (task_struct has pointer to mm_struct) are fullfilled
>>> by dumping pointed to object first, keeping it's position in dumpfile and saving
>>> position in a image of pointe? object:
>> Unless you use the physical position to actually lseek to there to
>> re-read the data, there is no reason to use the actual position. In
>> fact it is easier to debug when the shared object identifier is a
>> simple counter.
>>
>> If you do use it to lseek, then it's a poor decision -- sounds fragile:
>> what if we change the file (legitimately) adding data in the middle -
>> the whole concept breaks.
>
> Adder of data is expected to understand image format and update all references
> just like surgeon is expected to understand human anatomy.
>
>>> struct cr_image_task_struct {
>>> cr_pos_t cr_pos_mm;
>>> ...
>>> };
>>>
>>> Code so far tries hard to dump objects in certain order so there won't be any loops.
>>> This property of process that dumpfile can in theory be O_APPEND, will likely be
>>> sacrifised (read: child can ptrace parent)
>> The ability to streamline the checkpoint image IMHO is invaluable.
>> It's the unix way (TM) of doing things; it makes the process pipe-able.
>>
>> You can do many nice things when the checkpoint can be streamed: you
>> can compress, sign, encrypt etc on the fly without taking additional
>> diskspace. You can transfer over the network (e.g. for migration),
>> or store remotely without explicit file system support. You can easily
>> transform the stream from one c/r version to another etc.
>>
>> This should be a design principle. In my experience I never hit a wall
>> that forced me to "sacrifice" this decision.
>>
>>> sacrifised (read: child can ptrace parent)
>> Hmmm... if all tasks are created in user space, then this specific
>> becomes a no-brainer !
>
> No!

Actually yes :)

>
> A ptraces B. Container is checkpointed.
>
> Kernel realizes ptrace is going on. A and B in theory can have any
> realitionship.
>
> Consequently, kernel doesn't know in which order to dump A and B.
>
> And there is no such order:
> *) A can be parent of B (you dump A, B),
> *) A can be child of B (you want to dump B, A, but this conflicts with
> ->real_parent order)
> *) A and B just tasks (any order).

Current code does not support ptrace() - which has a multitude
if tidy-bits issues to solve during restart regardless.

However, creating tasks in userspace uses (and will uses) only
"real" process relationships, not ptrace-relationships, when it
comes to decide on the fork/clone order.

Technically, that can be done in checkpoint (dumping the task tree)
or in restart-user-space (rearranging the data before fork/clone).

>
> I'm showing that whole issue can be avoided:

If the issue can be avoided, then why would you need to sacrifice
the stream-ability of the checkpoint image ?

> *) all tasks are simply created regardless of who is parent of whom
> (see kernel_thread())
> *) Every task_struct image among other things contains references to
> ->real_parent and ->parent.
> *) After every task is created it's time to change references:
> **) lookup who is ->real_parent, change ->real_parent _by hand_
> not with some "correct clone(2)" order.
> **) lookup who is ->parent, change ->parent.
>
> You're probably escaping all of this with object numbers?

(Will be) escaping this by arranging to fork/clone in the proper order.

>
>>> * add struct vm_operations_struct::checkpoint
>>>
>>> just like with files, code that creates special VMAs should know what to do with them
>>> used.
>>>
>>> just like with files, deny checkpointing by default
>>>
>>> So far used to install vDSO to same place.
>> VDSO can be a troublemaker; in recent kernels its location in the MM
>> can be randomized.
>
> See arch_setup_additional_pages() patch.
>
>> It is not necessarily immutable - it can reflect
>> ynamic kernel data. It may contain different code on newer versions,
>> so must be compared or worked around during restart etc.
>
> i386 if I'm not mistaken only contain syscall entry code, but, yes,
> generally one should check if PC is inside such page.

If you up restart on a different kernel that has a different VDSO,
then you need to bring the old VDSO with you, and tweak it so it
pulls the dynamic kernel data from the right place. Ugh ... :(

Oren.

>
>>> * add checkpoint(2)
>>>
>>> Done by determining which tasks are subject to checkpointing, freezeing them,
>>> collecting pointers to necessary kernel internals (task_struct, mm_struct, ...),
>>> doing that checking supported/unsupported status and aborting if necessary,
>>> actual dumping, unfreezeing/killing set of tasks.
>>>
>>> Also in-checkpoint refcount is maintained to abort on possible invisible changes.
>>> Now it works:
>>>
>>> For every collected object (mm_struct) keep numbers of references from
>>> other collected objects. It should match object's own refcount.
>>> If there is a mismatch, something is likely pinning object, which means
>>> there is "leak" to outside which means checkpoint(2) can't realistically and
>>> without consequences proceed.
>>>
>>> This is in some sense independent check. It's designed to protect from internals
>>> change when C/R code was forgotten to be updated.
>>>
>>> Userpsace supplies pid of root task and opened file descriptor of future dump file.
>>> Kernel reports 0/-E as usual.
>>>
>>> Runtime tracking of "checkpointable" property is explicitly not done.
>>> This introduces overhead even if checkpoint(2) is not done as shown by proponents.
>>> Instead any check is done at checkpoint(2) time and -E is returned if something is
>>> suspicious or known to be unsupported.
>>>
>>> FIXME: more checks especially in cr_check_task_struct().
>>>
>>> * add restart(2)
>>>
>>> Recreate tasks and evething dumped by checkpoint(2) as if nothing happened.
>>>
>>> The focus is on correct recreating, checking every possibility that target kernel
>>> can be on different arch (i386 => x86_64) and target kernel can be very different
>>> from source kernel by mistake (i386 => x86_64 COMPAT=n) kernel.
>>>
>>> restart(2) is done first by creating kernel thread and that demoting it to usual
>>> process by adding mm_struct, VMAs, et al. This saves time against method when
>>> userspace does fork(2)+restart(2) -- forked mm_struct will be thrown out anyway
>>> or at least everything will be unmapped in any case.
>> Do have figures to support your claims about "saves time" ?
>>
>> The *largest* component of the restart time, as you probably know,
>> is the time it takes to restore the memory address space (pages, pages)
>> of the tasks.
>>
>> If you do show that this optimization is worth our attention, then it
>> takes < 10 lines to change current mktree.c to use CLONE_VM ... voila.
>>
>> I'm interested in hearing more convincing arguments in favor of kernel
>> creations of restarting tasks (see my other post about it).
>
> OK, in another post.
>
>>> Restoration is done in current context except CPU registers at last stage.
>>> This is because "creation is done by current" is in many, many places,
>>> e.g. mmap(2) code.
>>>
>>> It's expected that filesystem state will be the same. Kernel can't do anything
>>> about it expect probably virtual filesystems. If a file is not there anymore,
>>> it's not kernel fault, -E will be returned, restart aborted.
>>>
>>> FIXME: errors aren't propagated correctly out of kernel thread context
>> Heh .. I guess they always propagate correctly out of regular task
>> context ;)
>
> :-)
>

2009-04-14 18:35:42

by Kevin Fox

[permalink] [raw]

Subject: Re: "partial" container checkpoint

On Tue, 2009-04-14 at 09:37 -0700, Dave Hansen wrote:
> On Tue, 2009-04-14 at 10:29 -0500, Serge E. Hallyn wrote:
> > I think the perceived need for it comes, as above, from the pure
> > checkpoint-a-whole-container-only view. So long as you will
> > checkpoint/restore a whole container, then you'll end up doing
> > something requiring privilege anyway. But that is not all of
> > the use cases.
>
> Yeah, there are certainly a lot of shades of gray here. I've been
> talking to some HPC guys in the last couple of days. They certainly
> have a need for checkpoint/restart, but much less of a need for doing
> entire containers.

We'd be uncomfortable running partial checkpoints. We'd much rather have
slurm spawn off a container and just checkpoint that. Who knows what
users code spawns off other processes...

Kevin

>
> It also occurs to me that we have the potential to pull some
> long-out-of-tree users back in. VMADump users, for instance:
>
> http://bproc.sourceforge.net/c268.html
>
> If we could do *just* a selective checkpoint of a single process's VMAs,
> the bproc users could probably use sys_checkpoint() in some way. That's
> *way* less than an entire container, but it would be really useful to
> some people.
>
> -- Dave
>
> _______________________________________________
> Containers mailing list
> [email protected]
> https://lists.linux-foundation.org/mailman/listinfo/containers

2009-04-14 19:00:33

[permalink] [raw]

Subject: Re: [PATCH 10/30] cr: core stuff

> >> The ability to streamline the checkpoint image IMHO is invaluable.
> >> It's the unix way (TM) of doing things; it makes the process pipe-able.
> >>
> >> You can do many nice things when the checkpoint can be streamed: you
> >> can compress, sign, encrypt etc on the fly without taking additional
> >> diskspace. You can transfer over the network (e.g. for migration),
> >> or store remotely without explicit file system support. You can easily
> >> transform the stream from one c/r version to another etc.
> >>
> >> This should be a design principle. In my experience I never hit a wall
> >> that forced me to "sacrifice" this decision.
> >>
> >>> sacrifised (read: child can ptrace parent)
> >> Hmmm... if all tasks are created in user space, then this specific
> >> becomes a no-brainer !
> >
> > No!
>
> Actually yes :)
>
> >
> > A ptraces B. Container is checkpointed.
> >
> > Kernel realizes ptrace is going on. A and B in theory can have any
> > realitionship.
> >
> > Consequently, kernel doesn't know in which order to dump A and B.
> >
> > And there is no such order:
> > *) A can be parent of B (you dump A, B),
> > *) A can be child of B (you want to dump B, A, but this conflicts with
> > ->real_parent order)
> > *) A and B just tasks (any order).
>
> Current code does not support ptrace() - which has a multitude
> if tidy-bits issues to solve during restart regardless.
>
> However, creating tasks in userspace uses (and will uses) only
> "real" process relationships, not ptrace-relationships, when it
> comes to decide on the fork/clone order.
>
> Technically, that can be done in checkpoint (dumping the task tree)
> or in restart-user-space (rearranging the data before fork/clone).
>
> >
> > I'm showing that whole issue can be avoided:
>
> If the issue can be avoided, then why would you need to sacrifice
> the stream-ability of the checkpoint image ?
>
> > *) all tasks are simply created regardless of who is parent of whom
> > (see kernel_thread())
> > *) Every task_struct image among other things contains references to
> > ->real_parent and ->parent.
> > *) After every task is created it's time to change references:
> > **) lookup who is ->real_parent, change ->real_parent _by hand_
> > not with some "correct clone(2)" order.
> > **) lookup who is ->parent, change ->parent.
> >
> > You're probably escaping all of this with object numbers?
>
> (Will be) escaping this by arranging to fork/clone in the proper order.

task_struct and reparenting is just an example.

There is another loop:

struct user_struct => struct user_namespace => struct user_namespace::creator

Before actual dump each struct user_struct gets unique id (objref, whatever)
and simply dumped regardless of order.

Image of struct user_namespace contains id of creator user and dumped.

On restart:
restart user_ns
restart user
lookup object by creator id
if found, rewrite ->creator
if not found, restore creator user, and rewrite ->creator.

So, yes, if object number is dumped on disk, you get streamability in
presence of loops.

Clever. Just needs a way to quickly lookup file position by object id.

BTW, this is why OpenVZ code have "section concept.
I hoped it won't be needed.

2009-04-14 19:29:17