From: Robert R. <rob...@am...> - 2011-05-31 17:29:30
|
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(-) |
From: Robert R. <rob...@am...> - 2011-05-31 17:29:28
|
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: <st...@ke...> # .27+ Signed-off-by: Robert Richter <rob...@am...> --- 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 |
From: Robert R. <rob...@am...> - 2011-05-31 17:29:31
|
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 <mar...@gm...> Cc: Carl Love <ca...@us...> Cc: <st...@ke...> # .36+ Signed-off-by: Robert Richter <rob...@am...> --- 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 |
From: Robert R. <rob...@am...> - 2011-05-31 17:31:18
|
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: <st...@ke...> # .36+ Signed-off-by: Robert Richter <rob...@am...> --- 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 |
From: Robert R. <rob...@am...> - 2011-06-08 12:54:05
|
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 |
From: Ingo M. <mi...@el...> - 2011-06-08 14:06:47
|
* Robert Richter <rob...@am...> 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 |
From: Robert R. <rob...@am...> - 2011-06-09 07:04:20
|
On 08.06.11 10:06:31, Ingo Molnar wrote: > > * Robert Richter <rob...@am...> 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 |
From: Ingo M. <mi...@el...> - 2011-06-09 07:15:42
|
* Robert Richter <rob...@am...> wrote: > On 08.06.11 10:06:31, Ingo Molnar wrote: > > > > * Robert Richter <rob...@am...> 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 |