[Openais] Re: just looked at some of the openais code, have some comments

Steven Dake sdake at mvista.com
Tue Jul 6 11:40:20 PDT 2004


On Fri, 2004-07-02 at 12:31, Chris Friesen wrote:
> Steven Dake wrote:
> 
> > 1. I removed the refcount object and added it directly into the saHandle
> > structure.
> 
> Hmm.  This moves more stuff to the handle structure though, which takes more 
> space since we never free up handles once created.  It does let us keep the lock 
> while freeing the instance, but I suggested a way around that earlier.
> 

The extra memory consumption really isn't a big deal, if it makes the
rest of the system cleaner in that a magical offset to a refcount
structure isn't passed around a bunch of places (or assumed to be at the
beginning of the structure).

Even in a heavily loaded system of 1000 API connections, we are talking
about 4k, which is pretty minimal.

> Also, do you ever manipulate the handle mutex when not holding the handle 
> database mutex?  If not, then you could just forgo the handle mutex and use the 
> handle database mutex.
> 

Good idea..  I removed the refcount mutex since the handle mutex is
always held during get/put.

> > 2. now check for fd != 1 in destructors
> 
> Cool.
> 
> > 3. saHandleDestroy added to util and finalize calls
> 
> I'm not sure we should wipe out the "instance" member if we're marking the 
> handle as pending removal.  If we leave it, it allows for cleaner usage later on.
> 

Good idea This allowed me to remove the instance field to put and
instead just use the handle to get at the instance data.

> > 4. handles now have 3 states (active, empty, pending removal)
> 
> > 5. Finalize explictly removes the handle from the database (this can't
> > be done immediately, as someone else could come and use the handle.
> > This is solved with the 3rd pending removal state above)
> 
> The "pending" state doesn't actually protect against handle re-use.  Someone 
> could still use the handle later on, after a new instance has been created with 
> the same handle.  Really its a library usage constraint--see 4 below
> 

A new handle cannot be taken from the handle array if the state is
"pending" it can only be taken if it is "empty".  So this keeps handles
that are still referenced from being reused.

> 
> A few things.
> 
> 1) saClmInitialize()
> --the error path cleanup for saHandleCreate() should be saHandleDestroy(), not 
> saHandleInstancePut()
> 
good call

> 2) saClmFinalize()
> --I think there are too many calls to "put".
> 	--there is a single "get", which needs to be matched with a "put"
> 	--it needs a "destroy" to match the original "create"
> 	--what's the other "put" for?
> 
This was because I left an unbalanced get (ie: no put) in Initialize. 
This seemed a little confusing, so I added the put to the initialize
function.

So now it works like this:

initialize:
create (sets ref count to 1)
get (sets ref count to 2)
put (sets ref count to 1)

finalize:
get (sets ref count to 2)
destroy (sets ref count to 1)
put (sets ref count to 0)

> 
> 3) saHandleInstancePut() isn't very pretty, with some stuff done via handle, and 
> some via instance.  If you left the "instance" field alone in saHandleDestroy(), 
> you wouldn't need the "instance" parameter on this function.
> 

agreed fixed in attached patch

> 4) If we say (as a library usage constraint) that it is illegal to call 
> saHandleInstanceGet() for a particular handle after someone calls 
> saHandleDestroy(), then nothing stops us from doing an immediate removal from 
> the database, as long as the "put" call only takes the instance as a parameter.
> 
We can't make this constraint because of the nature of the AIS
specification, but this case is handled with the 3 states of the handle
if:
1) only empty state handles may be used for new entries
2) only inuse state handles may be used to verify a handle
3) pending removal handles may not be used for new entries or to verify
validity of a handle.  They are dead space until their ref count drops
to zero and their state goes to empty.

> (If we can't say that, then the multiple states doesn't really help us, and we 
> probably need to have increasing handle numbers until we wrap the data type to 
> minimize collision likelihood.)
> 

I see no problems in the above technique with the 3 states.

> It could be done like this:
> 
> struct clmInstance {
> 	int fd;
> 	SaClmCallbacksT callbacks;
> 	int finalize;
> 	int refcount;
> 	pthread_mutex_t mutex;
> };
> 

I'd like to get rid of the refcount and mutex from the instance
structure, since its more of a handle management issue (that the handle
management functions care about), then something the user of the handle
management APIs care about.

> struct saHandle {
> 	void *instance;
> };
> 
> Note: if we used atomic instruction or put lock and refcount always at beginning 
> of instance struct, wouldn't need these parms

yup although there is no need for atomic increment/dec because the
refcount is now manipulated under the handle database lock.

> saInstancePut(instance, lock offset, refcount offset)
> {
> 	//these six lines could be replaced with atomic_dec_and_test
> 	int localref;
> 	lock instance
> 	instance->refcount--;
> 	localref = refcount;
> 	unlock instance
> 	if (localref == 0)
> 		do cleanup stuff
> 		free instance
> }
> 
> 
> saHandleDestroy ( handleDatabase, handle)
> {
> 	struct instance *instance;
> 	lock database mutex
> 	instance = &handleDatabase->handles[handle].instance
> 	handleDatabase->handles[handle].instance = 0;
> 	unlock database mutex
> 	saInstancePut (instance);
> 
> 	return (SA_OK);
> }
> 
> 
> To my view, this is as conceptually simple as it gets:  HandleDestroy 
> immediately destroys the handle, and InstancePut doesn't know about the database 
> at all.
> 

good call

Take a look at the latest patch it should embody most of your ideas.

> 
> 
> Chris
-------------- next part --------------
A non-text attachment was scrubbed...
Name: openais-refcount-4.patch
Type: text/x-patch
Size: 76433 bytes
Desc: not available
Url : http://lists.linux-foundation.org/pipermail/openais/attachments/20040706/e42eb948/openais-refcount-4-0001.bin


More information about the Openais mailing list