Re: [RFC] [Patch 4/4] lock contention tracking slimmed down

Peter Zijlstra
2007-06-07T07:44:32Z

On Wed, 2007-06-06 at 23:34 +0200, Martin Peschke wrote:

>
> Signed-off-by: Martin Peschke <mp3@de.ibm.com>
> ---
>
> include/linux/lockdep.h | 35 ++---
> kernel/lockdep.c | 122 ++-----------------
> kernel/lockdep_proc.c | 294 +++++++-----------------------------------------
> 3 files changed, 76 insertions(+), 375 deletions(-)
>
> Index: linux/kernel/lockdep.c
> ===================================================================
> --- linux.orig/kernel/lockdep.c
> +++ linux/kernel/lockdep.c
> @@ -37,6 +37,7 @@
> #include <linux/debug_locks.h>
> #include <linux/irqflags.h>
> #include <linux/utsname.h>
> +#include <linux/statistic.h>
>
> #include <asm/sections.h>
>
> @@ -119,93 +120,9 @@ unsigned long nr_lock_classes;
> static struct lock_class lock_classes[MAX_LOCKDEP_KEYS];
>
> #ifdef CONFIG_LOCK_STAT
> static void lock_release_holdtime(struct held_lock *hlock)
> {
> + struct statistic *stat = hlock->class->stat;
> s64 holdtime;
>
> if (!lock_stat)
> @@ -213,12 +130,10 @@ static void lock_release_holdtime(struct
>
> holdtime = sched_clock() - hlock->holdtime_stamp;
>
> if (hlock->read)
> + statistic_inc(stat, LOCK_STAT_HOLD_READ, holdtime);
> else
> + statistic_inc(stat, LOCK_STAT_HOLD_WRITE, holdtime);
> }
> #else
> static inline void lock_release_holdtime(struct held_lock *hlock)
> @@ -2745,9 +2660,8 @@ __lock_contended(struct lockdep_map *loc
> {
> struct task_struct *curr = current;
> struct held_lock *hlock, *prev_hlock;
> unsigned int depth;
> + int i;
>
> depth = curr->lockdep_depth;
> if (DEBUG_LOCKS_WARN_ON(!depth))
> @@ -2761,22 +2675,15 @@ __lock_contended(struct lockdep_map *loc
> */
> if (prev_hlock && prev_hlock->irq_context != hlock->irq_context)
> break;
> + if (hlock->instance == lock) {
> + hlock->waittime_stamp = sched_clock();
> + _statistic_inc(hlock->class->stat, LOCK_STAT_CONT, ip);
> + return;
> + }
> prev_hlock = hlock;
> }
> print_lock_contention_bug(curr, lock, ip);
> return;
> }
>
> static void
> @@ -2784,7 +2691,7 @@ __lock_acquired(struct lockdep_map *lock
> {
> struct task_struct *curr = current;
> struct held_lock *hlock, *prev_hlock;
> + struct statistic *stat;
> unsigned int depth;
> u64 now;
> s64 waittime;
> @@ -2817,12 +2724,11 @@ found_it:
> waittime = now - hlock->waittime_stamp;
> hlock->holdtime_stamp = now;
>
> + stat = hlock->class->stat;
> if (hlock->read)
> + _statistic_inc(stat, LOCK_STAT_WAIT_READ, waittime);
> else
> + _statistic_inc(stat, LOCK_STAT_WAIT_WRITE, waittime);
> }
>
> void lock_contended(struct lockdep_map *lock, unsigned long ip)
> Index: linux/include/linux/lockdep.h
> ===================================================================
> --- linux.orig/include/linux/lockdep.h
> +++ linux/include/linux/lockdep.h
> @@ -17,6 +17,7 @@ struct lockdep_map;
> #include <linux/list.h>
> #include <linux/debug_locks.h>
> #include <linux/stacktrace.h>
> +#include <linux/statistic.h>
>
> /*
> * Lock-class usage-state bits:
> @@ -72,6 +73,17 @@ struct lock_class_key {
> struct lockdep_subclass_key subkeys[MAX_LOCKDEP_SUBCLASSES];
> };
>
> +#ifdef CONFIG_LOCK_STAT
> +enum lock_stat_enum {
> + LOCK_STAT_CONT,
> + LOCK_STAT_WAIT_READ,
> + LOCK_STAT_WAIT_WRITE,
> + LOCK_STAT_HOLD_READ,
> + LOCK_STAT_HOLD_WRITE,
> + _LOCK_STAT_NUMBER
> +};
> +#endif
> +
> /*
> * The lock-class itself:
> */
> @@ -117,30 +129,11 @@ struct lock_class {
> int name_version;
>
> #ifdef CONFIG_LOCK_STAT
> + struct statistic stat[_LOCK_STAT_NUMBER];
> + struct statistic_coll stat_coll;
> #endif
> };
>
> /*
> * Map the lock object (the lock instance) to the lock-class object.
> * This is embedded into specific lock instances:
> Index: linux/kernel/lockdep_proc.c
> ===================================================================
> --- linux.orig/kernel/lockdep_proc.c
> +++ linux/kernel/lockdep_proc.c
> @@ -349,260 +349,64 @@ static const struct file_operations proc
> };
>
> #ifdef CONFIG_LOCK_STAT
> +struct statistic_info lock_stat_info[_LOCK_STAT_NUMBER] = {
> + [LOCK_STAT_CONT] = {
> + .name = "contentions",
> + .x_unit = "instruction_pointer",
> + .y_unit = "occurrence",
> + .defaults = "type=sparse entries=4",
> + .flags = STATISTIC_FLAGS_LABEL,
> + },
> + [LOCK_STAT_WAIT_READ] = {
> + .name = "wait_read",
> + .x_unit = "nanoseconds",
> + .y_unit = "occurrence",
> + .defaults = "type=utilisation",
> + },
> + [LOCK_STAT_WAIT_WRITE] = {
> + .name = "wait_write",
> + .x_unit = "nanoseconds",
> + .y_unit = "occurrence",
> + .defaults = "type=utilisation",
> + },
> + [LOCK_STAT_HOLD_READ] = {
> + .name = "hold_read",
> + .x_unit = "nanoseconds",
> + .y_unit = "occurrence",
> + .defaults = "type=utilisation",
> + },
> + [LOCK_STAT_HOLD_WRITE] = {
> + .name = "hold_write",
> + .x_unit = "nanoseconds",
> + .y_unit = "occurrence",
> + .defaults = "type=utilisation",
> }
> };
>
> +struct statistic_ui lock_stat_ui;
>
> +static void lock_stat_label(struct statistic_coll *coll, int i,
> + void *key, char *buf, int size)
> +{
> + sprint_symbol(buf, *(unsigned long *)key);
> }
>
> +static void lock_stat_init(void)
> {
> struct lock_class *class;
> + statistic_create(&lock_stat_ui, "lockstat");
> + list_for_each_entry(class, &all_lock_classes, lock_entry) {
> + class->stat_coll.stat = class->stat;
> + class->stat_coll.info = lock_stat_info;
> + class->stat_coll.number = _LOCK_STAT_NUMBER;
> + class->stat_coll.label = lock_stat_label;
> + strncpy(class->stat_coll.name, class->name,
> + STATISTIC_COLL_NAME_SIZE - 1);
> + statistic_attach(&class->stat_coll, &lock_stat_ui);
> }
> }
> +#endif
>
> static int __init lockdep_proc_init(void)
> {
> @@ -617,9 +421,7 @@ static int __init lockdep_proc_init(void
> entry->proc_fops = &proc_lockdep_stats_operations;
>
> #ifdef CONFIG_LOCK_STAT
> + lock_stat_init();
> #endif
>
> return 0;

I'm confused as to where the class->stat objects are initialised? Is
that done in lock_stat_init()? If so, then you have a bug.