Prev: [PATCH 08/20] epca: Kill the big kernel lock
Next: [PATCHv4 0/8] The FunctionFS composite function
From: Arnd Bergmann on 17 Jun 2010 15:50
On Thursday 17 June 2010 21:13:52 Tony Luck wrote:

> These changes showed up in linux-next (tag: next-20100617) ... and I'm seeing
> a few WARN_ON messages on ia64 while booting:
>
> WARNING: at include/linux/tty.h:589 tty_open+0x160/0xc60()
> ...
> Stack trace for the first of these looks like:
> Call Trace:
> ...
> [<a0000001001c5270>] do_filp_open+0x2f0/0xb40
> [<a0000001001a5290>] do_sys_open+0x90/0x200
> [<a0000001001a54d0>] sys_open+0x50/0x80
> [<a000000100b907e0>] kernel_init+0x340/0x420
> [<a000000100013c10>] kernel_thread_helper+0x30/0x60
> [<a00000010000a0c0>] start_kernel_thread+0x20/0x40
>
> Does anyone see anything similar on other architectures? Or is ia64 doing
> something "special" here?

Ah, I forgot to test the tty series without the other patches I have
in my bkl removal repository and without CONFIG_TTY_MUTEX.

Does the patch below make this go away? We should probably include
the 'misc' branch of my BKL repository in -next to fix this.

Arnd

--
From 99b699e56b23775c0c9a131208e1a5e13f6cfad3 Mon Sep 17 00:00:00 2001
From: Arnd Bergmann <arnd(a)arndb.de>
Date: Mon, 15 Mar 2010 19:00:34 +0100
Subject: [PATCH] init: remove the BKL from startup code

I have shown by code review that no driver takes
the BKL at init time any more, so whatever the
init code was locking against is no longer there
and it is now safe to remove the BKL there.

Signed-off-by: Arnd Bergmann <arnd(a)arndb.de>

diff --git a/init/main.c b/init/main.c
index 3bdb152..81821e1 100644
--- a/init/main.c
+++ b/init/main.c
@@ -434,7 +434,6 @@ static noinline void __init_refok rest_init(void)
rcu_read_lock();
kthreadd_task = find_task_by_pid_ns(pid, &init_pid_ns);
rcu_read_unlock();
- unlock_kernel();

/*
* The boot idle thread must execute schedule()
@@ -555,7 +554,6 @@ asmlinkage void __init start_kernel(void)
* Interrupts are still disabled. Do necessary setups, then
* enable them
*/
- lock_kernel();
tick_init();
boot_cpu_init();
page_address_init();
@@ -819,7 +817,6 @@ static noinline int init_post(void)
/* need to finish all async __init code before freeing the memory */
async_synchronize_full();
free_initmem();
- unlock_kernel();
mark_rodata_ro();
system_state = SYSTEM_RUNNING;
numa_default_policy();
@@ -855,8 +852,6 @@ static noinline int init_post(void)

static int __init kernel_init(void * unused)
{
- lock_kernel();
-
/*
* init can allocate pages on any node
*/
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 086d363..8047ca5 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -734,13 +734,6 @@ __acquires(kernel_lock)
return -1;
}

- /*
- * When this gets called we hold the BKL which means that
- * preemption is disabled. Various trace selftests however
- * need to disable and enable preemption for successful tests.
- * So we drop the BKL here and grab it after the tests again.
- */
- unlock_kernel();
mutex_lock(&trace_types_lock);

tracing_selftest_running = true;
@@ -822,7 +815,6 @@ __acquires(kernel_lock)
#endif

out_unlock:
- lock_kernel();
return ret;
}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Tony Luck on 17 Jun 2010 16:20
> Does the patch below make this go away? We should probably include
> the 'misc' branch of my BKL repository in -next to fix this.

That fixes a couple of instances (the tty_open() cases). I still have:

WARNING: at include/linux/tty.h:589 tty_release+0x90/0xbc0()
WARNING: at include/linux/tty.h:589 tty_release+0x630/0xbc0()
WARNING: at include/linux/tty.h:589 tty_release+0x90/0xbc0()
WARNING: at include/linux/tty.h:589 tty_release+0x630/0xbc0()
WARNING: at include/linux/tty.h:589 tty_release+0x90/0xbc0()
WARNING: at include/linux/tty.h:589 tty_release+0x630/0xbc0()

Stack traces for these all look the same:

Call Trace:
[<a000000100015990>] show_stack+0x50/0xa0
[<a00000010090f1f0>] dump_stack+0x30/0x50
[<a00000010008e280>] warn_slowpath_common+0xc0/0x120
[<a00000010008e320>] warn_slowpath_null+0x40/0x60
[<a00000010053d910>] tty_release+0x90/0xbc0
[<a0000001001ab200>] __fput+0x260/0x420
[<a0000001001ab400>] fput+0x40/0x60
[<a00000010053b3b0>] tty_vhangup_locked+0x870/0x8a0
[<a00000010054f3f0>] pty_close+0x350/0x3a0
[<a00000010053ddd0>] tty_release+0x550/0xbc0
[<a0000001001ab200>] __fput+0x260/0x420
[<a0000001001ab400>] fput+0x40/0x60
[<a0000001001a4dc0>] filp_close+0x120/0x140
[<a0000001001a4f90>] sys_close+0x1b0/0x2c0
[<a00000010000b940>] ia64_ret_from_syscall+0x0/0x20

-Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 17 Jun 2010 17:50
On Thursday 17 June 2010 22:15:32 Tony Luck wrote:
> Call Trace:
> [<a000000100015990>] show_stack+0x50/0xa0
> [<a00000010090f1f0>] dump_stack+0x30/0x50
> [<a00000010008e280>] warn_slowpath_common+0xc0/0x120
> [<a00000010008e320>] warn_slowpath_null+0x40/0x60
> [<a00000010053d910>] tty_release+0x90/0xbc0
> [<a0000001001ab200>] __fput+0x260/0x420
> [<a0000001001ab400>] fput+0x40/0x60
> [<a00000010053b3b0>] tty_vhangup_locked+0x870/0x8a0
> [<a00000010054f3f0>] pty_close+0x350/0x3a0
> [<a00000010053ddd0>] tty_release+0x550/0xbc0
> [<a0000001001ab200>] __fput+0x260/0x420
> [<a0000001001ab400>] fput+0x40/0x60
> [<a0000001001a4dc0>] filp_close+0x120/0x140
> [<a0000001001a4f90>] sys_close+0x1b0/0x2c0
> [<a00000010000b940>] ia64_ret_from_syscall+0x0/0x20
>

Ah, this sucks. I think Alan actually tried to warn me of this problem and I
thought I had it right, but obviously I got it wrong in the end. I really
should have run into this during testing though, not sure why I didn't.

The good news is that the warning message is harmless for the normal
case where CONFIG_TTY_MUTEX remains disabled, it's only debugging code
to warn that there is a bug once the option gets turned on.

Unfortunately however the only fix I see is to push the BTM further down
into the hangup function, which makes the pty code slightly more complex
and which I'm sure is an equivalent transformation.

I'll try doing some more tests with this patch and CONFIG_TTY_LOCK disabled.

Signed-off-by: Arnd Bergmann <arnd(a)arndb.de>

diff --git a/drivers/char/pty.c b/drivers/char/pty.c
index c9af9ff..0902127 100644
--- a/drivers/char/pty.c
+++ b/drivers/char/pty.c
@@ -62,7 +62,9 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
if (tty->driver == ptm_driver)
devpts_pty_kill(tty->link);
#endif
- tty_vhangup_locked(tty->link);
+ unlock_kernel();
+ tty_vhangup(tty->link);
+ lock_kernel();
}
}

diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index 5db354d..852ccb4 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -471,7 +471,7 @@ void tty_wakeup(struct tty_struct *tty)
EXPORT_SYMBOL_GPL(tty_wakeup);

/**
- * do_tty_hangup - actual handler for hangup events
+ * __tty_hangup - actual handler for hangup events
* @work: tty device
*
* This can be called by the "eventd" kernel thread. That is process
@@ -492,7 +492,7 @@ EXPORT_SYMBOL_GPL(tty_wakeup);
* tasklist_lock to walk task list for hangup event
* ->siglock to protect ->signal/->sighand
*/
-void tty_vhangup_locked(struct tty_struct *tty)
+void __tty_hangup(struct tty_struct *tty)
{
struct file *cons_filp = NULL;
struct file *filp, *f = NULL;
@@ -512,10 +512,12 @@ void tty_vhangup_locked(struct tty_struct *tty)
}
spin_unlock(&redirect_lock);

+ tty_lock();
+
/* inuse_filps is protected by the single tty lock,
this really needs to change if we want to flush the
workqueue with the lock held */
- check_tty_count(tty, "do_tty_hangup");
+ check_tty_count(tty, "tty_hangup");

file_list_lock();
/* This breaks for file handles being sent over AF_UNIX sockets ? */
@@ -594,6 +596,9 @@ void tty_vhangup_locked(struct tty_struct *tty)
*/
set_bit(TTY_HUPPED, &tty->flags);
tty_ldisc_enable(tty);
+
+ tty_unlock();
+
if (f)
fput(f);
}
@@ -603,9 +608,7 @@ static void do_tty_hangup(struct work_struct *work)
struct tty_struct *tty =
container_of(work, struct tty_struct, hangup_work);

- tty_lock();
- tty_vhangup_locked(tty);
- tty_unlock();
+ __tty_hangup(tty);
}

/**
@@ -643,13 +646,12 @@ void tty_vhangup(struct tty_struct *tty)

printk(KERN_DEBUG "%s vhangup...\n", tty_name(tty, buf));
#endif
- tty_lock();
- tty_vhangup_locked(tty);
- tty_unlock();
+ __tty_hangup(tty);
}

EXPORT_SYMBOL(tty_vhangup);

+
/**
* tty_vhangup_self - process vhangup for own ctty
*
@@ -727,10 +729,8 @@ void disassociate_ctty(int on_exit)
if (tty) {
tty_pgrp = get_pid(tty->pgrp);
if (on_exit) {
- tty_lock();
if (tty->driver->type != TTY_DRIVER_TYPE_PTY)
- tty_vhangup_locked(tty);
- tty_unlock();
+ tty_vhangup(tty);
}
tty_kref_put(tty);
} else if (on_exit) {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Frederic Weisbecker on 17 Jun 2010 18:10
On Thu, Jun 17, 2010 at 11:48:07PM +0200, Arnd Bergmann wrote:
> On Thursday 17 June 2010 22:15:32 Tony Luck wrote:
> > Call Trace:
> > [<a000000100015990>] show_stack+0x50/0xa0
> > [<a00000010090f1f0>] dump_stack+0x30/0x50
> > [<a00000010008e280>] warn_slowpath_common+0xc0/0x120
> > [<a00000010008e320>] warn_slowpath_null+0x40/0x60
> > [<a00000010053d910>] tty_release+0x90/0xbc0
> > [<a0000001001ab200>] __fput+0x260/0x420
> > [<a0000001001ab400>] fput+0x40/0x60
> > [<a00000010053b3b0>] tty_vhangup_locked+0x870/0x8a0
> > [<a00000010054f3f0>] pty_close+0x350/0x3a0
> > [<a00000010053ddd0>] tty_release+0x550/0xbc0
> > [<a0000001001ab200>] __fput+0x260/0x420
> > [<a0000001001ab400>] fput+0x40/0x60
> > [<a0000001001a4dc0>] filp_close+0x120/0x140
> > [<a0000001001a4f90>] sys_close+0x1b0/0x2c0
> > [<a00000010000b940>] ia64_ret_from_syscall+0x0/0x20
> >
>
> Ah, this sucks. I think Alan actually tried to warn me of this problem and I
> thought I had it right, but obviously I got it wrong in the end. I really
> should have run into this during testing though, not sure why I didn't.
>
> The good news is that the warning message is harmless for the normal
> case where CONFIG_TTY_MUTEX remains disabled, it's only debugging code
> to warn that there is a bug once the option gets turned on.
>
> Unfortunately however the only fix I see is to push the BTM further down
> into the hangup function, which makes the pty code slightly more complex
> and which I'm sure is an equivalent transformation.
>
> I'll try doing some more tests with this patch and CONFIG_TTY_LOCK disabled.
>
> Signed-off-by: Arnd Bergmann <arnd(a)arndb.de>
>
> diff --git a/drivers/char/pty.c b/drivers/char/pty.c
> index c9af9ff..0902127 100644
> --- a/drivers/char/pty.c
> +++ b/drivers/char/pty.c
> @@ -62,7 +62,9 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
> if (tty->driver == ptm_driver)
> devpts_pty_kill(tty->link);
> #endif
> - tty_vhangup_locked(tty->link);
> + unlock_kernel();
> + tty_vhangup(tty->link);
> + lock_kernel();



Don't you mean tty_unlock / tty_lock there?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  | 
Pages: 1 2 3
Prev: [PATCH 08/20] epca: Kill the big kernel lock
Next: [PATCHv4 0/8] The FunctionFS composite function