[systemd-devel] [PATCH 2/2] domain: grab the domain's parent lock only when needed

[Djalal Harouni]

On Thu, Mar 20, 2014 at 10:58:21AM +0100, Daniel Mack wrote:
> On 03/19/2014 09:24 PM, Djalal Harouni wrote:
> > Signed-off-by: Djalal Harouni <tixxdz at opendz.org>
> > ---
> >  domain.c | 13 +++++--------
> >  1 file changed, 5 insertions(+), 8 deletions(-)
> > 
> > diff --git a/domain.c b/domain.c
> > index d27cad2..554b4fe 100644
> > --- a/domain.c
> > +++ b/domain.c
> > @@ -183,12 +183,13 @@ struct kdbus_domain *kdbus_domain_unref(struct kdbus_domain *domain)
> >  	return NULL;
> >  }
> >  
> > -static struct kdbus_domain *kdbus_domain_find(struct kdbus_domain const *parent,
> > +static struct kdbus_domain *kdbus_domain_find(struct kdbus_domain *parent,
> >  				      const char *name)
> >  {
> >  	struct kdbus_domain *domain = NULL;
> >  	struct kdbus_domain *n;
> >  
> > +	mutex_lock(&parent->lock);
> >  	list_for_each_entry(n, &parent->domain_list, domain_entry) {
> >  		if (strcmp(n->name, name))
> >  			continue;
> > @@ -196,6 +197,7 @@ static struct kdbus_domain *kdbus_domain_find(struct kdbus_domain const *parent,
> >  		domain = kdbus_domain_ref(n);
> >  		break;
> >  	}
> > +	mutex_unlock(&parent->lock);
> >  
> >  	return domain;
> >  }
> > @@ -288,9 +290,6 @@ int kdbus_domain_new(struct kdbus_domain *parent, const char *name,
> >  	atomic64_set(&d->msg_seq_last, 0);
> >  	idr_init(&d->user_idr);
> >  
> > -	if (parent)
> > -		mutex_lock(&parent->lock);
> > -
> >  	mutex_lock(&kdbus_subsys_lock);
> >  
> >  	/* compose name and path of base directory in /dev */
> > @@ -340,21 +339,19 @@ int kdbus_domain_new(struct kdbus_domain *parent, const char *name,
> >  
> >  	/* link into parent domain */
> >  	if (parent) {
> 
> What if 'parent' was already unrefed at this point? That can happen any
> time as soon we give up the lock.
Ok yes, but in that case what about the first 'parent' check at line
259 ? we can also race against it? I thought that since we are at this
point, we are sure that the parent won't be unrefed!


> kdbus_domain_unref() calls kdbus_domain_disconnect(), wihch grabs
> domain->lock, so we're only safe as long as we hold the lock across all
> operations on the object, right?
Yes, you are right.

I've two questions:

1) Can we improve it in ordre to reduce the lock hold time ?
currently creating a domain will make create/disconnect/open... buses
and endpoints on the parent of that domain block, these are separated
operations on different domains.

Also it seems that now there is only support for one level of nested
domains? will this be increased?


2) What about creating custom endpoints on the bus that was already
unrefed ? IMHO this is the same scenario!


Hmm perhaps this can be improved by taking a ref ASAP and revalidate
objects by checking the "*->disconnected" ?


Thanks Daniel!

> 
> Daniel
> 

-- 
Djalal Harouni
http://opendz.org