[PATCH] locking_selftest: Save/restore migrate_disable_atomic in locking selftest

[PATCH] locking_selftest: Save/restore migrate_disable_atomic in locking selftest

[jianchuan.wang]

From: Jianchuan Wang <jianchuan.wang@windriver.com>

System will hang when enabling the kernel option
CONFIG_DEBUG_LOCKING_API_SELFTESTS and CONFIG_SCHED_CONFIG
in the preempt-rt kernel.

In the preempt-rt kernel, migrate_enable()/migrage_disable() are called
in the spinlock and read/write lock; The lock()/unlock() aren't used
in pairs in the selftest processing changes the migrate_disable_atomic
so that the system will hang in the dotest(), the calltrace is :
printk() ..-> vprintk_emit() -> migrage_disable()
-> WARN_ON_ONCE() ..-> warn_slowpath_common() ..-> printk()

For fixing it, we need save the migrate_disable_atomic before self-testing
and restore migrate_disable_atomic after self-testing.

Signed-off-by: Jianchuan Wang <jianchuan.wang@windriver.com>
---
 lib/locking-selftest.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/lib/locking-selftest.c b/lib/locking-selftest.c
index b93a610..61798da 100644
--- a/lib/locking-selftest.c
+++ b/lib/locking-selftest.c
@@ -997,10 +997,23 @@ static int unexpected_testcase_failures;
 static void dotest(void (*testcase_fn)(void), int expected, int lockclass_mask)
 {
 	unsigned long saved_preempt_count = preempt_count();
+#ifdef CONFIG_SCHED_DEBUG
+	struct task_struct *p = current;
+	int save_migrate_atomic;
+#endif
 
 	WARN_ON(irqs_disabled());
 
+#ifdef CONFIG_SCHED_DEBUG
+	save_migrate_atomic = p->migrate_disable_atomic;
+#endif
+
 	testcase_fn();
+
+#ifdef CONFIG_SCHED_DEBUG
+	p->migrate_disable_atomic = save_migrate_atomic;
+#endif
+
 	/*
 	 * Filter out expected failures:
 	 */
-- 
1.9.1

Re: [PATCH] locking_selftest: Save/restore migrate_disable_atomic in locking selftest

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1292 bytes --]

Hi Jianchuan,

[auto build test ERROR on: v4.4-rc2]
[also build test ERROR on: next-20151120]

url:    https://github.com/0day-ci/linux/commits/jianchuan-wang-windriver-com/locking_selftest-Save-restore-migrate_disable_atomic-in-locking-selftest/20151123-152217
config: x86_64-randconfig-x013-11230343 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   lib/locking-selftest.c: In function 'dotest':
>> lib/locking-selftest.c:981:25: error: 'struct task_struct' has no member named 'migrate_disable_atomic'
     save_migrate_atomic = p->migrate_disable_atomic;
                            ^
   lib/locking-selftest.c:987:3: error: 'struct task_struct' has no member named 'migrate_disable_atomic'
     p->migrate_disable_atomic = save_migrate_atomic;
      ^

vim +981 lib/locking-selftest.c

   975		int save_migrate_atomic;
   976	#endif
   977	
   978		WARN_ON(irqs_disabled());
   979	
   980	#ifdef CONFIG_SCHED_DEBUG
 > 981		save_migrate_atomic = p->migrate_disable_atomic;
   982	#endif
   983	
   984		testcase_fn();

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 20365 bytes --]

Re: [PATCH] locking_selftest: Save/restore migrate_disable_atomic in locking selftest

[Steven Rostedt]

On Mon, 23 Nov 2015 15:30:30 +0800
kbuild test robot <lkp@intel.com> wrote:

> Hi Jianchuan,
> 
> [auto build test ERROR on: v4.4-rc2]
> [also build test ERROR on: next-20151120]
> 
> url:    https://github.com/0day-ci/linux/commits/jianchuan-wang-windriver-com/locking_selftest-Save-restore-migrate_disable_atomic-in-locking-selftest/20151123-152217
> config: x86_64-randconfig-x013-11230343 (attached as .config)
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=x86_64 
> 
> All errors (new ones prefixed by >>):
> 
>    lib/locking-selftest.c: In function 'dotest':
> >> lib/locking-selftest.c:981:25: error: 'struct task_struct' has no member named 'migrate_disable_atomic'  
>      save_migrate_atomic = p->migrate_disable_atomic;
>                             ^
>    lib/locking-selftest.c:987:3: error: 'struct task_struct' has no member named 'migrate_disable_atomic'
>      p->migrate_disable_atomic = save_migrate_atomic;
>       ^


Need to add RT in the subject like "[PATCH RT]". Then perhaps Fengguang
can have his tests either ignore these or test against the -rt trees.

-- Steve

> 
> vim +981 lib/locking-selftest.c
> 
>    975		int save_migrate_atomic;
>    976	#endif
>    977	
>    978		WARN_ON(irqs_disabled());
>    979	
>    980	#ifdef CONFIG_SCHED_DEBUG
>  > 981		save_migrate_atomic = p->migrate_disable_atomic;  
>    982	#endif
>    983	
>    984		testcase_fn();
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [kbuild-all] [PATCH] locking_selftest: Save/restore migrate_disable_atomic in locking selftest

[Fengguang Wu]

On Mon, Nov 23, 2015 at 08:43:25AM -0500, Steven Rostedt wrote:
> On Mon, 23 Nov 2015 15:30:30 +0800
> kbuild test robot <lkp@intel.com> wrote:
> 
> > Hi Jianchuan,
> > 
> > [auto build test ERROR on: v4.4-rc2]
> > [also build test ERROR on: next-20151120]
> > 
> > url:    https://github.com/0day-ci/linux/commits/jianchuan-wang-windriver-com/locking_selftest-Save-restore-migrate_disable_atomic-in-locking-selftest/20151123-152217
> > config: x86_64-randconfig-x013-11230343 (attached as .config)
> > reproduce:
> >         # save the attached .config to linux build tree
> >         make ARCH=x86_64 
> > 
> > All errors (new ones prefixed by >>):
> > 
> >    lib/locking-selftest.c: In function 'dotest':
> > >> lib/locking-selftest.c:981:25: error: 'struct task_struct' has no member named 'migrate_disable_atomic'  
> >      save_migrate_atomic = p->migrate_disable_atomic;
> >                             ^
> >    lib/locking-selftest.c:987:3: error: 'struct task_struct' has no member named 'migrate_disable_atomic'
> >      p->migrate_disable_atomic = save_migrate_atomic;
> >       ^
> 
> 
> Need to add RT in the subject like "[PATCH RT]". Then perhaps Fengguang
> can have his tests either ignore these or test against the -rt trees.

Yes sure. Shall I apply RT patches to this tree/branch?

https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git linux-4.1.y-rt

Thanks,
Fengguang

Re: [kbuild-all] [PATCH] locking_selftest: Save/restore migrate_disable_atomic in locking selftest

[Steven Rostedt]

On Mon, 23 Nov 2015 22:14:08 +0800
Fengguang Wu <lkp@intel.com> wrote:

> > Need to add RT in the subject like "[PATCH RT]". Then perhaps Fengguang
> > can have his tests either ignore these or test against the -rt trees.  
> 
> Yes sure. Shall I apply RT patches to this tree/branch?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git linux-4.1.y-rt

Looks like a good of a branch as any.

-- Steve

Re: [kbuild-all] [PATCH] locking_selftest: Save/restore migrate_disable_atomic in locking selftest

[Sebastian Andrzej Siewior]

On 11/23/2015 03:14 PM, Fengguang Wu wrote:

>> Need to add RT in the subject like "[PATCH RT]". Then perhaps Fengguang
>> can have his tests either ignore these or test against the -rt trees.
> 
> Yes sure. Shall I apply RT patches to this tree/branch?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git linux-4.1.y-rt

I'm going to get you your kbot branch where you run bisect tests and
everything and tests the patches against. The problem with this one is
that we will drop it (or leave it stale) once we move to the next major
kernel release.

> Thanks,
> Fengguang

Sebastian

Re: [kbuild-all] [PATCH] locking_selftest: Save/restore migrate_disable_atomic in locking selftest

[Fengguang Wu]

On Mon, Nov 23, 2015 at 09:17:56AM -0500, Steven Rostedt wrote:
> On Mon, 23 Nov 2015 22:14:08 +0800
> Fengguang Wu <lkp@intel.com> wrote:
> 
> > > Need to add RT in the subject like "[PATCH RT]". Then perhaps Fengguang
> > > can have his tests either ignore these or test against the -rt trees.  
> > 
> > Yes sure. Shall I apply RT patches to this tree/branch?
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git linux-4.1.y-rt
> 
> Looks like a good of a branch as any.

OK. Added the [RT] mapping to that branch.

Thanks,
Fengguang

Re: [kbuild-all] [PATCH] locking_selftest: Save/restore migrate_disable_atomic in locking selftest

[Fengguang Wu]

On Mon, Nov 23, 2015 at 03:35:11PM +0100, Sebastian Andrzej Siewior wrote:
> On 11/23/2015 03:14 PM, Fengguang Wu wrote:
> 
> >> Need to add RT in the subject like "[PATCH RT]". Then perhaps Fengguang
> >> can have his tests either ignore these or test against the -rt trees.
> > 
> > Yes sure. Shall I apply RT patches to this tree/branch?
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git linux-4.1.y-rt
> 
> I'm going to get you your kbot branch where you run bisect tests and
> everything and tests the patches against. The problem with this one is
> that we will drop it (or leave it stale) once we move to the next major
> kernel release.

Thanks! Yes, a stable branch name would be better than "linux-4.1.y-rt"
that's like to become stable over time.

Thanks,
Fengguang

Re: [kbuild-all] [PATCH] locking_selftest: Save/restore migrate_disable_atomic in locking selftest

[Sebastian Andrzej Siewior]

On 11/23/2015 04:03 PM, Fengguang Wu wrote:
> Thanks! Yes, a stable branch name would be better than "linux-4.1.y-rt"
> that's like to become stable over time.

finally. I got to it.
I pushed two branches @ pub/scm/linux/kernel/git/rt/linux-rt-devel.git:

	for-kbuild-bot/current-stable [0]
	for-kbuild-bot/prepare-release [1]

Branch [0] should contain the last -RT release. This could be used for
testing patches against (if you find one with the RT marker in subject).
The tree should start a stable-tree marker (currently it is v4.1.13)
and have -RT tree applied on top. You should be able compile after each
commit (between the stable tag and HEAD) and nothing should introduce
warnings or fail to compile.
The second branch [1] would be similar to the first one except that I
plan to push stuff there before I make a release it. Does this make
sense or do I over think this?
If you do compile tests, it would be nice if you could enable
CONFIG_PREEMPT_RT_FULL. That is where most of the changes start to work.
Nevertheless it should also work without it(i.e. no preemption or
desktop).
If you have slightly different naming scheme or suggestions just tell
me I will adapt to it:)

Thank you for the service.

> Thanks,
> Fengguang

Sebastian

Re: [kbuild-all] [PATCH] locking_selftest: Save/restore migrate_disable_atomic in locking selftest

[Fengguang Wu]

Hi Sebastian,

On Tue, Dec 01, 2015 at 06:59:44PM +0100, Sebastian Andrzej Siewior wrote:
> On 11/23/2015 04:03 PM, Fengguang Wu wrote:
> > Thanks! Yes, a stable branch name would be better than "linux-4.1.y-rt"
> > that's like to become stable over time.
> 
> finally. I got to it.
> I pushed two branches @ pub/scm/linux/kernel/git/rt/linux-rt-devel.git:
> 
> 	for-kbuild-bot/current-stable [0]
> 	for-kbuild-bot/prepare-release [1]
> 
> Branch [0] should contain the last -RT release. This could be used for
> testing patches against (if you find one with the RT marker in subject).
> The tree should start a stable-tree marker (currently it is v4.1.13)
> and have -RT tree applied on top. You should be able compile after each
> commit (between the stable tag and HEAD) and nothing should introduce
> warnings or fail to compile.

Got it, I've updated the RT => for-kbuild-bot/current-stable mapping
accordingly, thanks for the info!

> The second branch [1] would be similar to the first one except that I
> plan to push stuff there before I make a release it. Does this make
> sense or do I over think this?

It's fair enough.

> If you do compile tests, it would be nice if you could enable
> CONFIG_PREEMPT_RT_FULL. That is where most of the changes start to work.
> Nevertheless it should also work without it(i.e. no preemption or
> desktop).

OK, I'll increase testing for CONFIG_PREEMPT_RT_FULL when it's the RT tree.

> If you have slightly different naming scheme or suggestions just tell
> me I will adapt to it:)
> 
> Thank you for the service.

You are welcome!

Thanks,
Fengguang

Re: [PATCH] locking_selftest: Save/restore migrate_disable_atomic in locking selftest

[Sebastian Andrzej Siewior]

* jianchuan.wang@windriver.com | 2015-11-23 15:19:38 [+0800]:

>From: Jianchuan Wang <jianchuan.wang@windriver.com>
>
>System will hang when enabling the kernel option
>CONFIG_DEBUG_LOCKING_API_SELFTESTS and CONFIG_SCHED_CONFIG
>in the preempt-rt kernel.
>
>In the preempt-rt kernel, migrate_enable()/migrage_disable() are called
>in the spinlock and read/write lock; The lock()/unlock() aren't used
>in pairs in the selftest processing changes the migrate_disable_atomic
>so that the system will hang in the dotest(), the calltrace is :
>printk() ..-> vprintk_emit() -> migrage_disable()
>-> WARN_ON_ONCE() ..-> warn_slowpath_common() ..-> printk()
>
>For fixing it, we need save the migrate_disable_atomic before self-testing
>and restore migrate_disable_atomic after self-testing.

So this is similar to saved_preempt_count. This needs to compile with
!CONFIG_PREEMPT_RT_FULL which I believe is not the case.

>Signed-off-by: Jianchuan Wang <jianchuan.wang@windriver.com>
>---
> lib/locking-selftest.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
>diff --git a/lib/locking-selftest.c b/lib/locking-selftest.c
>index b93a610..61798da 100644
>--- a/lib/locking-selftest.c
>+++ b/lib/locking-selftest.c
>@@ -997,10 +997,23 @@ static int unexpected_testcase_failures;
> static void dotest(void (*testcase_fn)(void), int expected, int lockclass_mask)
> {
> 	unsigned long saved_preempt_count = preempt_count();
>+#ifdef CONFIG_SCHED_DEBUG
>+	struct task_struct *p = current;
>+	int save_migrate_atomic;
>+#endif
> 
> 	WARN_ON(irqs_disabled());
> 
>+#ifdef CONFIG_SCHED_DEBUG
>+	save_migrate_atomic = p->migrate_disable_atomic;
>+#endif

Shouldn't we save  p->migrate_disable as well?

Sebastian