Re: [PATCH 2/3] read_lock migrate_disable pushdown to rt_read_lock

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 6 Dec 2013 03:33:34 +0100
Nicholas Mc Guire <der.herr@hofr.at> wrote:

> > > -	migrate_disable();
> > >  	if (rt_mutex_owner(lock) != current) {
> > >  		ret = rt_mutex_trylock(lock);
> > > -		if (ret)
> > > +		if (ret) {
> > > +			migrate_disable();
> > >  			rwlock_acquire(&rwlock->dep_map, 0, 1, _RET_IP_);
> > > +		}
> > 
> > I like this patch in general, but I'm nervous about this part.
> > 
> > What was the original reason to disable migration before checking the
> > mutex owner? Seems like we should have only disabled it on success. I'm
> > assuming there was some subtle reason not to.
> >
> 
> I think that it was simply being treated semantically as a lock - but it
> is actually not.
>                        hold a
>  migrate_disable/lock  per_cpu  unlock/migrate_enable
>  lock/migrate_disable  per_cpu  migrate_enable/unlcok
>  lock/migrate_disable  per_cpu  unlock/migrate_enable
>  migrate_disable/lock  per_cpu  migrate_enable/unlcok
>                        reference
> 
> are all equivalent - the only thing you need to ensure that the per cpu 
> object will not be accessed before both lock and migration_disable have
> been sucessful. So from my understanding this is safe.

I think you may be right, but I'm still a little nervous about this
code. But that's good. We all should be nervous about any locking
code ;-)

> 
> if we get migrated after a succesful trylock what would go wrong ?
> the protected object will not be accessed until after the spin_trylock
> returned so migration is disabled

I agree.

> 
> > If there was some strange reason, I'm not convinced that your change
> > makes that reason go away.
> >
> IF there is a reason then this patch is broken - my conclusion up to now
> is that there is no such reason.
> 

Let me analyze the original code first. I'll poke peterz and tglx too
to make sure this modification is OK.

Thanks,

-- Steve