Re: [RFC v5][PATCH 7/8] Infrastructure for shared objects

Serge E. Hallyn
2008-09-16T22:10:08Z

Quoting Oren Laadan (orenl@cs.columbia.edu):
>
>
> Serge E. Hallyn wrote:
> > Quoting Oren Laadan (orenl@cs.columbia.edu):
> >> Infrastructure to handle objects that may be shared and referenced by
> >> multiple tasks or other objects, e..g open files, memory address space
> >> etc.
> >>
> >> The state of shared objects is saved once. On the first encounter, the
> >> state is dumped and the object is assigned a unique identifier (objref)
> >> and also stored in a hash table (indexed by its physical kenrel address).
> >> >From then on the object will be found in the hash and only its identifier
> >> is saved.
> >>
> >> On restart the identifier is looked up in the hash table; if not found
> >> then the state is read, the object is created, and added to the hash
> >> table (this time indexed by its identifier). Otherwise, the object in
> >> the hash table is used.
> >>
> >> Signed-off-by: Oren Laadan <orenl@cs.columbia.edu>
> >
> > Acked-by: Serge Hallyn <serue@us.ibm.com>
> >
> > Thanks, Oren, I actually think this is quite nice and readable.
> >
> > Though three questions below. First one is, since you've mentioned
> > having multiple threads doing checkpoint, won't you need some locking?
>
> yes.
>
> > I assume that's coming in later patches if/when needed?
>
> yes.
>
> [...]
>
> >> +++ b/checkpoint/objhash.c
> >> @@ -0,0 +1,237 @@
> >> +/*
> >> + * Checkpoint-restart - object hash infrastructure to manage shared objects
> >> + *
> >> + * Copyright (C) 2008 Oren Laadan
> >> + *
> >> + * This file is subject to the terms and conditions of the GNU General Public
> >> + * License. See the file COPYING in the main directory of the Linux
> >> + * distribution for more details.
> >> + */
> >> +
> >> +#include <linux/kernel.h>
> >> +#include <linux/file.h>
> >> +#include <linux/hash.h>
> >> +#include <linux/checkpoint.h>
> >> +
> >> +struct cr_objref {
> >> + int objref;
> >> + void *ptr;
> >> + unsigned short type;
> >
> > What is the point of the 'type'?
> >
> > By that I mean: is it meant to catch bugs in the implementation, or bad
> > checkpoint images?
>

...

> There are different functions to inc/dec the reference count of objects
> of different types. '->type' keeps track of the type of the object, so
> we know which function to call. (At this point, we only track shared
> 'struct file' so it isn't that clear from the code).

Doh, right.

>
> >
> >> + unsigned short flags;
> >> + struct hlist_node hash;
> >> +};
> >> +
> >> +struct cr_objhash {
> >> + struct hlist_head *head;
> >> + int objref_index;
> >
> > What exactly will objref_index be used for? I don't see any real
> > usage here or in your later patches.
> >
>
> 'objref_index' is a counter used to assign unique identifiers (objref)
> to objects as they are added to the hash table. It is incremented with
> every object that joins the hash table (and the old value is used as
> the objref of that object). It is used in cr_obj_new().

If you were to rename objref_index to next_free_ref, I think that'd
be helpful. (I also don't think 'obj' should in the name at all, and
you should just s/objref/id/, but that's really too much pain)

thanks,
-serge