Some fixes for 3.0 for review. Will apply them later and send a pull
request.
Thanks
-Robert
The following changes since commit cbf74cea070fa1f705de4712e25d9e56ae6543c7:
oprofile, x86: Add comments to IBS LVT offset initialization (2011-05-30 16:36:54 +0200)
Robert Richter (3):
oprofile: Free potentially owned tasks in case of errors
oprofile: Fix locking dependency in sync_start()
oprofile, dcookies: Fix possible circular locking dependency
drivers/oprofile/buffer_sync.c | 21 +++++++++++----------
fs/dcookies.c | 3 +++
2 files changed, 14 insertions(+), 10 deletions(-)
After registering the task free notifier we possibly have tasks in our
dying_tasks list. Free them after unregistering the notifier in case
of an error.
Cc: <[email protected]> # .36+
Signed-off-by: Robert Richter <[email protected]>
---
drivers/oprofile/buffer_sync.c | 13 +++++++++----
1 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/oprofile/buffer_sync.c b/drivers/oprofile/buffer_sync.c
index a3984f4..04250aa 100644
--- a/drivers/oprofile/buffer_sync.c
+++ b/drivers/oprofile/buffer_sync.c
@@ -141,6 +141,13 @@ static struct notifier_block module_load_nb = {
.notifier_call = module_load_notify,
};
+static void free_all_tasks(void)
+{
+ /* make sure we don't leak task structs */
+ process_task_mortuary();
+ process_task_mortuary();
+}
+
int sync_start(void)
{
int err;
@@ -174,6 +181,7 @@ out3:
profile_event_unregister(PROFILE_TASK_EXIT, &task_exit_nb);
out2:
task_handoff_unregister(&task_free_nb);
+ free_all_tasks();
out1:
free_cpumask_var(marked_cpus);
goto out;
@@ -192,10 +200,7 @@ void sync_stop(void)
mutex_unlock(&buffer_mutex);
flush_cpu_work();
- /* make sure we don't leak task structs */
- process_task_mortuary();
- process_task_mortuary();
-
+ free_all_tasks();
free_cpumask_var(marked_cpus);
}
--
1.7.5.rc3
This fixes the A->B/B->A locking dependency, see the warning below.
The function task_exit_notify() is called with (task_exit_notifier)
.rwsem set and then calls sync_buffer() which locks buffer_mutex. In
sync_start() the buffer_mutex was set to prevent notifier functions to
be started before sync_start() is finished. But when registering the
notifier, (task_exit_notifier).rwsem is locked too, but now in
different order than in sync_buffer(). In theory this causes a locking
dependency, what does not occur in practice since task_exit_notify()
is always called after the notifier is registered which means the lock
is already released.
However, after checking the notifier functions it turned out the
buffer_mutex in sync_start() is unnecessary. This is because
sync_buffer() may be called from the notifiers even if sync_start()
did not finish yet, the buffers are already allocated but empty. No
need to protect this with the mutex.
So we fix this theoretical locking dependency by removing buffer_mutex
in sync_start(). This is similar to the implementation before commit:
750d857 oprofile: fix crash when accessing freed task structs
which introduced the locking dependency.
Lockdep warning:
oprofiled/4447 is trying to acquire lock:
(buffer_mutex){+.+...}, at: [<ffffffffa0000e55>] sync_buffer+0x31/0x3ec [oprofile]
but task is already holding lock:
((task_exit_notifier).rwsem){++++..}, at: [<ffffffff81058026>] __blocking_notifier_call_chain+0x39/0x67
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 ((task_exit_notifier).rwsem){++++..}:
[<ffffffff8106557f>] lock_acquire+0xf8/0x11e
[<ffffffff81463a2b>] down_write+0x44/0x67
[<ffffffff810581c0>] blocking_notifier_chain_register+0x52/0x8b
[<ffffffff8105a6ac>] profile_event_register+0x2d/0x2f
[<ffffffffa00013c1>] sync_start+0x47/0xc6 [oprofile]
[<ffffffffa00001bb>] oprofile_setup+0x60/0xa5 [oprofile]
[<ffffffffa00014e3>] event_buffer_open+0x59/0x8c [oprofile]
[<ffffffff810cd3b9>] __dentry_open+0x1eb/0x308
[<ffffffff810cd59d>] nameidata_to_filp+0x60/0x67
[<ffffffff810daad6>] do_last+0x5be/0x6b2
[<ffffffff810dbc33>] path_openat+0xc7/0x360
[<ffffffff810dbfc5>] do_filp_open+0x3d/0x8c
[<ffffffff810ccfd2>] do_sys_open+0x110/0x1a9
[<ffffffff810cd09e>] sys_open+0x20/0x22
[<ffffffff8146ad4b>] system_call_fastpath+0x16/0x1b
-> #0 (buffer_mutex){+.+...}:
[<ffffffff81064dfb>] __lock_acquire+0x1085/0x1711
[<ffffffff8106557f>] lock_acquire+0xf8/0x11e
[<ffffffff814634f0>] mutex_lock_nested+0x63/0x309
[<ffffffffa0000e55>] sync_buffer+0x31/0x3ec [oprofile]
[<ffffffffa0001226>] task_exit_notify+0x16/0x1a [oprofile]
[<ffffffff81467b96>] notifier_call_chain+0x37/0x63
[<ffffffff8105803d>] __blocking_notifier_call_chain+0x50/0x67
[<ffffffff81058068>] blocking_notifier_call_chain+0x14/0x16
[<ffffffff8105a718>] profile_task_exit+0x1a/0x1c
[<ffffffff81039e8f>] do_exit+0x2a/0x6fc
[<ffffffff8103a5e4>] do_group_exit+0x83/0xae
[<ffffffff8103a626>] sys_exit_group+0x17/0x1b
[<ffffffff8146ad4b>] system_call_fastpath+0x16/0x1b
other info that might help us debug this:
1 lock held by oprofiled/4447:
#0: ((task_exit_notifier).rwsem){++++..}, at: [<ffffffff81058026>] __blocking_notifier_call_chain+0x39/0x67
stack backtrace:
Pid: 4447, comm: oprofiled Not tainted 2.6.39-00007-gcf4d8d4 #10
Call Trace:
[<ffffffff81063193>] print_circular_bug+0xae/0xbc
[<ffffffff81064dfb>] __lock_acquire+0x1085/0x1711
[<ffffffffa0000e55>] ? sync_buffer+0x31/0x3ec [oprofile]
[<ffffffff8106557f>] lock_acquire+0xf8/0x11e
[<ffffffffa0000e55>] ? sync_buffer+0x31/0x3ec [oprofile]
[<ffffffff81062627>] ? mark_lock+0x42f/0x552
[<ffffffffa0000e55>] ? sync_buffer+0x31/0x3ec [oprofile]
[<ffffffff814634f0>] mutex_lock_nested+0x63/0x309
[<ffffffffa0000e55>] ? sync_buffer+0x31/0x3ec [oprofile]
[<ffffffffa0000e55>] sync_buffer+0x31/0x3ec [oprofile]
[<ffffffff81058026>] ? __blocking_notifier_call_chain+0x39/0x67
[<ffffffff81058026>] ? __blocking_notifier_call_chain+0x39/0x67
[<ffffffffa0001226>] task_exit_notify+0x16/0x1a [oprofile]
[<ffffffff81467b96>] notifier_call_chain+0x37/0x63
[<ffffffff8105803d>] __blocking_notifier_call_chain+0x50/0x67
[<ffffffff81058068>] blocking_notifier_call_chain+0x14/0x16
[<ffffffff8105a718>] profile_task_exit+0x1a/0x1c
[<ffffffff81039e8f>] do_exit+0x2a/0x6fc
[<ffffffff81465031>] ? retint_swapgs+0xe/0x13
[<ffffffff8103a5e4>] do_group_exit+0x83/0xae
[<ffffffff8103a626>] sys_exit_group+0x17/0x1b
[<ffffffff8146ad4b>] system_call_fastpath+0x16/0x1b
Reported-by: Marcin Slusarz <[email protected]>
Cc: Carl Love <[email protected]>
Cc: <[email protected]> # .36+
Signed-off-by: Robert Richter <[email protected]>
---
drivers/oprofile/buffer_sync.c | 8 ++------
1 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/oprofile/buffer_sync.c b/drivers/oprofile/buffer_sync.c
index 04250aa..f34b5b2 100644
--- a/drivers/oprofile/buffer_sync.c
+++ b/drivers/oprofile/buffer_sync.c
@@ -155,8 +155,6 @@ int sync_start(void)
if (!zalloc_cpumask_var(&marked_cpus, GFP_KERNEL))
return -ENOMEM;
- mutex_lock(&buffer_mutex);
-
err = task_handoff_register(&task_free_nb);
if (err)
goto out1;
@@ -173,7 +171,6 @@ int sync_start(void)
start_cpu_work();
out:
- mutex_unlock(&buffer_mutex);
return err;
out4:
profile_event_unregister(PROFILE_MUNMAP, &munmap_nb);
@@ -190,14 +187,13 @@ out1:
void sync_stop(void)
{
- /* flush buffers */
- mutex_lock(&buffer_mutex);
end_cpu_work();
unregister_module_notifier(&module_load_nb);
profile_event_unregister(PROFILE_MUNMAP, &munmap_nb);
profile_event_unregister(PROFILE_TASK_EXIT, &task_exit_nb);
task_handoff_unregister(&task_free_nb);
- mutex_unlock(&buffer_mutex);
+ barrier(); /* do all of the above first */
+
flush_cpu_work();
free_all_tasks();
--
1.7.5.rc3
The lockdep warning below detects a possible A->B/B->A locking
dependency of mm->mmap_sem and dcookie_mutex. The order in
sync_buffer() is mm->mmap_sem/dcookie_mutex, while in
sys_lookup_dcookie() it is vice versa.
Fixing it in sys_lookup_dcookie() by unlocking dcookie_mutex before
copy_to_user().
oprofiled/4432 is trying to acquire lock:
(&mm->mmap_sem){++++++}, at: [<ffffffff810b444b>] might_fault+0x53/0xa3
but task is already holding lock:
(dcookie_mutex){+.+.+.}, at: [<ffffffff81124d28>] sys_lookup_dcookie+0x45/0x149
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (dcookie_mutex){+.+.+.}:
[<ffffffff8106557f>] lock_acquire+0xf8/0x11e
[<ffffffff814634f0>] mutex_lock_nested+0x63/0x309
[<ffffffff81124e5c>] get_dcookie+0x30/0x144
[<ffffffffa0000fba>] sync_buffer+0x196/0x3ec [oprofile]
[<ffffffffa0001226>] task_exit_notify+0x16/0x1a [oprofile]
[<ffffffff81467b96>] notifier_call_chain+0x37/0x63
[<ffffffff8105803d>] __blocking_notifier_call_chain+0x50/0x67
[<ffffffff81058068>] blocking_notifier_call_chain+0x14/0x16
[<ffffffff8105a718>] profile_task_exit+0x1a/0x1c
[<ffffffff81039e8f>] do_exit+0x2a/0x6fc
[<ffffffff8103a5e4>] do_group_exit+0x83/0xae
[<ffffffff8103a626>] sys_exit_group+0x17/0x1b
[<ffffffff8146ad4b>] system_call_fastpath+0x16/0x1b
-> #0 (&mm->mmap_sem){++++++}:
[<ffffffff81064dfb>] __lock_acquire+0x1085/0x1711
[<ffffffff8106557f>] lock_acquire+0xf8/0x11e
[<ffffffff810b4478>] might_fault+0x80/0xa3
[<ffffffff81124de7>] sys_lookup_dcookie+0x104/0x149
[<ffffffff8146ad4b>] system_call_fastpath+0x16/0x1b
other info that might help us debug this:
1 lock held by oprofiled/4432:
#0: (dcookie_mutex){+.+.+.}, at: [<ffffffff81124d28>] sys_lookup_dcookie+0x45/0x149
stack backtrace:
Pid: 4432, comm: oprofiled Not tainted 2.6.39-00008-ge5a450d #9
Call Trace:
[<ffffffff81063193>] print_circular_bug+0xae/0xbc
[<ffffffff81064dfb>] __lock_acquire+0x1085/0x1711
[<ffffffff8102ef13>] ? get_parent_ip+0x11/0x42
[<ffffffff810b444b>] ? might_fault+0x53/0xa3
[<ffffffff8106557f>] lock_acquire+0xf8/0x11e
[<ffffffff810b444b>] ? might_fault+0x53/0xa3
[<ffffffff810d7d54>] ? path_put+0x22/0x27
[<ffffffff810b4478>] might_fault+0x80/0xa3
[<ffffffff810b444b>] ? might_fault+0x53/0xa3
[<ffffffff81124de7>] sys_lookup_dcookie+0x104/0x149
[<ffffffff8146ad4b>] system_call_fastpath+0x16/0x1b
References: https://bugzilla.kernel.org/show_bug.cgi?id=13809
Cc: <[email protected]> # .27+
Signed-off-by: Robert Richter <[email protected]>
---
fs/dcookies.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/fs/dcookies.c b/fs/dcookies.c
index a21cabd..dda0dc7 100644
--- a/fs/dcookies.c
+++ b/fs/dcookies.c
@@ -178,6 +178,8 @@ SYSCALL_DEFINE(lookup_dcookie)(u64 cookie64, char __user * buf, size_t len)
/* FIXME: (deleted) ? */
path = d_path(&dcs->path, kbuf, PAGE_SIZE);
+ mutex_unlock(&dcookie_mutex);
+
if (IS_ERR(path)) {
err = PTR_ERR(path);
goto out_free;
@@ -194,6 +196,7 @@ SYSCALL_DEFINE(lookup_dcookie)(u64 cookie64, char __user * buf, size_t len)
out_free:
kfree(kbuf);
+ return err;
out:
mutex_unlock(&dcookie_mutex);
return err;
--
1.7.5.rc3
Ingo,
please pull oprofile fixes for v3.0 (tip/perf/urgent):
git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent
Thanks,
-Robert
The following changes since commit d7ebe75b065a7c2d58ffc12f9d2e00d5ea4e71eb:
perf: Fix comments in include/linux/perf_event.h (2011-06-04 12:31:14 +0200)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent
Robert Richter (4):
oprofile, x86: Add comments to IBS LVT offset initialization
oprofile: Free potentially owned tasks in case of errors
oprofile: Fix locking dependency in sync_start()
oprofile, dcookies: Fix possible circular locking dependency
arch/x86/kernel/apic/apic.c | 3 ++-
arch/x86/oprofile/op_model_amd.c | 13 +++++++++----
drivers/oprofile/buffer_sync.c | 21 +++++++++++----------
fs/dcookies.c | 3 +++
4 files changed, 25 insertions(+), 15 deletions(-)
--
Advanced Micro Devices, Inc.
Operating System Research Center
* Robert Richter <[email protected]> wrote:
> Ingo,
>
> please pull oprofile fixes for v3.0 (tip/perf/urgent):
>
> git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent
>
> Thanks,
>
> -Robert
>
>
> The following changes since commit d7ebe75b065a7c2d58ffc12f9d2e00d5ea4e71eb:
>
> perf: Fix comments in include/linux/perf_event.h (2011-06-04 12:31:14 +0200)
>
> are available in the git repository at:
> git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent
>
> Robert Richter (4):
> oprofile, x86: Add comments to IBS LVT offset initialization
> oprofile: Free potentially owned tasks in case of errors
> oprofile: Fix locking dependency in sync_start()
> oprofile, dcookies: Fix possible circular locking dependency
>
> arch/x86/kernel/apic/apic.c | 3 ++-
> arch/x86/oprofile/op_model_amd.c | 13 +++++++++----
> drivers/oprofile/buffer_sync.c | 21 +++++++++++----------
> fs/dcookies.c | 3 +++
> 4 files changed, 25 insertions(+), 15 deletions(-)
Pulled, thanks a lot Robert!
Ingo
On 08.06.11 10:06:31, Ingo Molnar wrote:
>
> * Robert Richter <[email protected]> wrote:
>
> > Ingo,
> >
> > please pull oprofile fixes for v3.0 (tip/perf/urgent):
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent
> >
> > Thanks,
> >
> > -Robert
> >
> >
> > The following changes since commit d7ebe75b065a7c2d58ffc12f9d2e00d5ea4e71eb:
> >
> > perf: Fix comments in include/linux/perf_event.h (2011-06-04 12:31:14 +0200)
> >
> > are available in the git repository at:
> > git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent
> >
> > Robert Richter (4):
> > oprofile, x86: Add comments to IBS LVT offset initialization
Ingo,
it seems you pulled (86dd7909c2c4ae3f219a9233bf0f095b05632ecf) from
cbf74cea070fa1f705de4712e25d9e56ae6543c7 (oprofile, x86: Add comments to IBS LVT offset initialization)
These ones are still missing:
> > oprofile: Free potentially owned tasks in case of errors
> > oprofile: Fix locking dependency in sync_start()
> > oprofile, dcookies: Fix possible circular locking dependency
But oprofile/urgent is
fe47ae7f53e179d2ef6771024feb000cbb86640f (oprofile, dcookies: Fix possible circular locking dependency)
Is this intended? Maybe a korg git sync issue?
Thanks,
-Robert
> >
> > arch/x86/kernel/apic/apic.c | 3 ++-
> > arch/x86/oprofile/op_model_amd.c | 13 +++++++++----
> > drivers/oprofile/buffer_sync.c | 21 +++++++++++----------
> > fs/dcookies.c | 3 +++
> > 4 files changed, 25 insertions(+), 15 deletions(-)
>
> Pulled, thanks a lot Robert!
>
> Ingo
>
--
Advanced Micro Devices, Inc.
Operating System Research Center
* Robert Richter <[email protected]> wrote:
> On 08.06.11 10:06:31, Ingo Molnar wrote:
> >
> > * Robert Richter <[email protected]> wrote:
> >
> > > Ingo,
> > >
> > > please pull oprofile fixes for v3.0 (tip/perf/urgent):
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent
> > >
> > > Thanks,
> > >
> > > -Robert
> > >
> > >
> > > The following changes since commit d7ebe75b065a7c2d58ffc12f9d2e00d5ea4e71eb:
> > >
> > > perf: Fix comments in include/linux/perf_event.h (2011-06-04 12:31:14 +0200)
> > >
> > > are available in the git repository at:
> > > git://git.kernel.org/pub/scm/linux/kernel/git/rric/oprofile.git urgent
> > >
> > > Robert Richter (4):
> > > oprofile, x86: Add comments to IBS LVT offset initialization
>
> Ingo,
>
> it seems you pulled (86dd7909c2c4ae3f219a9233bf0f095b05632ecf) from
>
> cbf74cea070fa1f705de4712e25d9e56ae6543c7 (oprofile, x86: Add comments to IBS LVT offset initialization)
>
> These ones are still missing:
>
> > > oprofile: Free potentially owned tasks in case of errors
> > > oprofile: Fix locking dependency in sync_start()
> > > oprofile, dcookies: Fix possible circular locking dependency
>
> But oprofile/urgent is
>
> fe47ae7f53e179d2ef6771024feb000cbb86640f (oprofile, dcookies: Fix possible circular locking dependency)
>
> Is this intended? Maybe a korg git sync issue?
Yeah, probably - and i reviewed the on-lkml patches you sent earlier
and didnt notice that these three are missing from the pull request.
I pulled again and the bits are there now.
Thanks,
Ingo