Message-ID: <20080916220912.GA21663@us.ibm.com>
Date:	Tue, 16 Sep 2008 17:09:12 -0500
From:	"Serge E. Hallyn" <serue@...ibm.com>
To:	Oren Laadan <orenl@...columbia.edu>
Cc:	dave@...ux.vnet.ibm.com, containers@...ts.linux-foundation.org,
	jeremy@...p.org, linux-kernel@...r.kernel.org, arnd@...db.de
Subject: Re: [RFC v5][PATCH 7/8] Infrastructure for shared objects

Quoting Oren Laadan (orenl@...columbia.edu):
> 
> 
> Serge E. Hallyn wrote:
> > Quoting Oren Laadan (orenl@...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@...columbia.edu>
> > 
> > Acked-by: Serge Hallyn <serue@...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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists