[PATCH] migrate_disable pushd down in rt_read_trylock

[PATCH] migrate_disable pushd down in rt_read_trylock

[Nicholas Mc Guire]

>From 5c9a0c1510ec29c1e148f66f3c111f52f7565df1 Mon Sep 17 00:00:00 2001
From: Nicholas Mc Guire <der.herr@hofr.at>
Date: Fri, 22 Nov 2013 02:41:48 -0500
Subject: [PATCH] migrate_disable pushd down in rt_read_trylock

 No need to migrate_disable before requesting the lock and no need to 
 speculatively disable/enable on every recursive call. migration_disable 
 can be done at the latest point in the code before returning an acquired 
 ``lock.

 patch is on top of 3.12-rt2

 No change of functionality

Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
---
 kernel/rt.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/kernel/rt.c b/kernel/rt.c
index 71d26a4..54211f0 100644
--- a/kernel/rt.c
+++ b/kernel/rt.c
@@ -212,19 +212,18 @@ int __lockfunc rt_read_trylock(rwlock_t *rwlock)
 	 * but not when read_depth == 0 which means that the lock is
 	 * write locked.
 	 */
-	migrate_disable();
 	if (rt_mutex_owner(lock) != current) {
 		ret = rt_mutex_trylock(lock);
-		if (ret)
+		if (ret) {
 			rwlock_acquire(&rwlock->dep_map, 0, 1, _RET_IP_);
+			migrate_disable();
+		}
 	} else if (!rwlock->read_depth) {
 		ret = 0;
 	}
 
 	if (ret)
 		rwlock->read_depth++;
-	else
-		migrate_enable();
 
 	return ret;
 }
-- 
1.7.2.5

Re: [PATCH] migrate_disable pushd down in rt_read_trylock

[Sebastian Andrzej Siewior]

* Nicholas Mc Guire | 2013-11-23 01:51:58 [+0100]:

>>From 5c9a0c1510ec29c1e148f66f3c111f52f7565df1 Mon Sep 17 00:00:00 2001
>From: Nicholas Mc Guire <der.herr@hofr.at>
>Date: Fri, 22 Nov 2013 02:41:48 -0500
>Subject: [PATCH] migrate_disable pushd down in rt_read_trylock
>
> No need to migrate_disable before requesting the lock and no need to 
> speculatively disable/enable on every recursive call. migration_disable 
> can be done at the latest point in the code before returning an acquired 
> ``lock.
>
> patch is on top of 3.12-rt2
>
> No change of functionality
Applied without this line.

>Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>

Sebastian

Re: [PATCH] migrate_disable pushd down in rt_read_trylock

[Sebastian Andrzej Siewior]

* Sebastian Andrzej Siewior | 2013-11-29 16:14:01 [+0100]:

>* Nicholas Mc Guire | 2013-11-23 01:51:58 [+0100]:
>
>>>From 5c9a0c1510ec29c1e148f66f3c111f52f7565df1 Mon Sep 17 00:00:00 2001
>>From: Nicholas Mc Guire <der.herr@hofr.at>
>>Date: Fri, 22 Nov 2013 02:41:48 -0500
>>Subject: [PATCH] migrate_disable pushd down in rt_read_trylock
>>
>> No need to migrate_disable before requesting the lock and no need to 
>> speculatively disable/enable on every recursive call. migration_disable 
>> can be done at the latest point in the code before returning an acquired 
>> ``lock.
>>
>> patch is on top of 3.12-rt2
>>
>> No change of functionality
>Applied without this line.

and dropped because there is a problem with this:
- Now 
  if you read_lock() and then read_try_lock() then migrate_disable() is
  called by each caller. Also on read_unlock() migrate_enable() is called
  by each caller.

- with patch
  read_lock() calls migrate_disable() and read_try_lock() does not. Both
  get the lock. So on read_unlock(), the read_try_lock() owner remains
  unbalanced.

disabling migration prior incrementing read_depth should fix this.

>>Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>

Sebastian

Re: [PATCH] migrate_disable pushd down in rt_read_trylock

[Nicholas Mc Guire]

On Fri, 29 Nov 2013, Sebastian Andrzej Siewior wrote:

> * Sebastian Andrzej Siewior | 2013-11-29 16:14:01 [+0100]:
> 
> >* Nicholas Mc Guire | 2013-11-23 01:51:58 [+0100]:
> >
> >>>From 5c9a0c1510ec29c1e148f66f3c111f52f7565df1 Mon Sep 17 00:00:00 2001
> >>From: Nicholas Mc Guire <der.herr@hofr.at>
> >>Date: Fri, 22 Nov 2013 02:41:48 -0500
> >>Subject: [PATCH] migrate_disable pushd down in rt_read_trylock
> >>
> >> No need to migrate_disable before requesting the lock and no need to 
> >> speculatively disable/enable on every recursive call. migration_disable 
> >> can be done at the latest point in the code before returning an acquired 
> >> ``lock.
> >>
> >> patch is on top of 3.12-rt2
> >>
> >> No change of functionality
> >Applied without this line.
> 
> and dropped because there is a problem with this:
> - Now 
>   if you read_lock() and then read_try_lock() then migrate_disable() is
>   called by each caller. Also on read_unlock() migrate_enable() is called
>   by each caller.
> 
> - with patch
>   read_lock() calls migrate_disable() and read_try_lock() does not. Both
>   get the lock. So on read_unlock(), the read_try_lock() owner remains
>   unbalanced.
> 
> disabling migration prior incrementing read_depth should fix this.
>
yup - that one is broken - interesting that the boxes run happily for 
days now with this bug applied :)
4core i3 and a 4core i7

So the fix would be to do the push_down into rt_read_lock and then 
balance it in read_unlock conditioned on read_depth reaching 0.

have a few more cleanups - will give them another scan if I missed 
this in any of the others.

thx!
hofrat 

Re: [PATCH] migrate_disable pushd down in rt_read_trylock

[Nicholas Mc Guire]

On Sat, 30 Nov 2013, Nicholas Mc Guire wrote:

> On Fri, 29 Nov 2013, Sebastian Andrzej Siewior wrote:
> 
> > * Sebastian Andrzej Siewior | 2013-11-29 16:14:01 [+0100]:
> > 
> > >* Nicholas Mc Guire | 2013-11-23 01:51:58 [+0100]:
> > >
> > >>>From 5c9a0c1510ec29c1e148f66f3c111f52f7565df1 Mon Sep 17 00:00:00 2001
> > >>From: Nicholas Mc Guire <der.herr@hofr.at>
> > >>Date: Fri, 22 Nov 2013 02:41:48 -0500
> > >>Subject: [PATCH] migrate_disable pushd down in rt_read_trylock
> > >>
> > >> No need to migrate_disable before requesting the lock and no need to 
> > >> speculatively disable/enable on every recursive call. migration_disable 
> > >> can be done at the latest point in the code before returning an acquired 
> > >> ``lock.
> > >>
> > >> patch is on top of 3.12-rt2
> > >>
> > >> No change of functionality
> > >Applied without this line.
> > 
> > and dropped because there is a problem with this:
> > - Now 
> >   if you read_lock() and then read_try_lock() then migrate_disable() is
> >   called by each caller. Also on read_unlock() migrate_enable() is called
> >   by each caller.
> > 
> > - with patch
> >   read_lock() calls migrate_disable() and read_try_lock() does not. Both
> >   get the lock. So on read_unlock(), the read_try_lock() owner remains
> >   unbalanced.
> > 
> > disabling migration prior incrementing read_depth should fix this.
> >
> yup - that one is broken - interesting that the boxes run happily for 
> days now with this bug applied :)
> 4core i3 and a 4core i7
>
Maybe not so suprising after all read_lock is only in use in three places
the lockdep cases definitely were not on in my configuration
and the mca.c case seemes not to have been hit ither (or there simply was
no recursive call in that path).

call sites:
0 mca.c     default_monarch_init_proc 1634 if (read_trylock(&tasklist_lock)) {
1 lockdep.c debug_show_all_locks      4139 if (!read_trylock(&tasklist_lock)) {
2 lockdep.c debug_show_all_locks      4166 if (read_trylock(&tasklist_lock))

given that the broken patch was disabling once and enabling potentially a
number of times it should have triggert the
WARN_ON_ONCE(p->migrate_disable <= 0); in migrate_enable() if the recursive
case would have ever bin hit... so much to testing and locking...

thx!
hofrat

Re: [PATCH] migrate_disable pushd down in rt_read_trylock

[Sebastian Andrzej Siewior]

* Nicholas Mc Guire | 2013-11-30 07:47:55 [+0100]:

>given that the broken patch was disabling once and enabling potentially a
>number of times it should have triggert the
>WARN_ON_ONCE(p->migrate_disable <= 0); in migrate_enable() if the recursive
>case would have ever bin hit... so much to testing and locking...

And is why I removed the line
> No change of functionality

from the change log because it was not obvious to me that is a
zero-change patch :)

Are you going to redo this one?

>thx!
>hofrat

Sebastian

Re: [PATCH] migrate_disable pushd down in rt_read_trylock

[Nicholas Mc Guire]

On Sun, 15 Dec 2013, Sebastian Andrzej Siewior wrote:

> * Nicholas Mc Guire | 2013-11-30 07:47:55 [+0100]:
> 
> >given that the broken patch was disabling once and enabling potentially a
> >number of times it should have triggert the
> >WARN_ON_ONCE(p->migrate_disable <= 0); in migrate_enable() if the recursive
> >case would have ever bin hit... so much to testing and locking...
> 
> And is why I removed the line
> > No change of functionality
> 
> from the change log because it was not obvious to me that is a
> zero-change patch :)

well that was the intent and obviously
it was not the case - so it served its purpose
to document the intent.

> 
> Are you going to redo this one?
>
no - as David Miller stated clearly that he sees these
split-api locks as a valid idiom and there were clear
worries that this would break in the future I see no point
in pushing this. The alternative of removing the recursive
migrate_disable/enable in local_bh_* directly is good
enough I guess - see: 
0001-make-migrate-disable-enable-conditioned-on-softirq_n.patch

thx!
hofrat

Re: [PATCH] migrate_disable pushd down in rt_read_trylock

[Sebastian Andrzej Siewior]

On 12/15/2013 03:15 PM, Nicholas Mc Guire wrote:
> no - as David Miller stated clearly that he sees these
> split-api locks as a valid idiom and there were clear
> worries that this would break in the future I see no point
> in pushing this. The alternative of removing the recursive
> migrate_disable/enable in local_bh_* directly is good
> enough I guess - see: 
> 0001-make-migrate-disable-enable-conditioned-on-softirq_n.patch

okay.

> 
> thx!
> hofrat

Sebastian