lkml.org 
[lkml]   [2008]   [Nov]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2.6.28-rc4]lock_stat: Add "con-hungry" to show that how many person-time fight for the ticket spinlock
From
Date
On Tue, 2008-11-18 at 19:00 +0800, Yang Xi wrote:
> Because the implementation of X86 spinlock adopts ticket lock, we can
> know how many threads are waiting for the lock when there is a
> contention on the spinlock. We add the number of these threads to the
> bounces[bounce_hungry] and name "bounces[bounce_hungry]" as
> "con-hungry". Finally, we can use this number to show that "the number
> of threads waiting for a spinlock/ a contention". From it, we can know
> that whether it is valuable to replace the spinlock with more
> scalable spinlock since the design principle of scalability spinlock
> is to decrease the cache invalid cost, if the "con-hungry/contentions"
> is not high, we cannot benefit from more scalable spinlocks.
>
>
> Signed-off-by: Yang Xi <hiyangxi@gmail.com>

While I like the idea, the code is horrid, please clean this up and try
again.

The lockstat code should not ever need to know about lock implementation
details like which you #ifdeffed in.

> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index 331e5f1..a71398e 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -136,6 +136,7 @@ enum bounce_type {
> bounce_acquired_read,
> bounce_contended_write,
> bounce_contended_read,
> + bounce_hungry,
> nr_bounce_types,
>
> bounce_acquired = bounce_acquired_write,
> @@ -165,6 +166,7 @@ struct lockdep_map {
> const char *name;
> #ifdef CONFIG_LOCK_STAT
> int cpu;
> + int isspinlock;
> #endif

perhaps a bitfield?

> };
>
> diff --git a/kernel/lockdep.c b/kernel/lockdep.c
> index 06e1571..b3314ed 100644
> --- a/kernel/lockdep.c
> +++ b/kernel/lockdep.c
> @@ -3000,6 +3000,20 @@ __lock_contended(struct lockdep_map *lock,
> unsigned long ip)
> struct lock_class_stats *stats;
> unsigned int depth;
> int i, point;
> + spinlock_t * lock_ptr;
> + unsigned long hungry;
> +
> + if (lock->isspinlock){
> + lock_ptr = container_of(lock,spinlock_t,dep_map);
> +
> +#if (NR_CPUS < 256)
> + hungry=(unsigned
> char)((lock_ptr->raw_lock.slock>>8&0xff)-(lock_ptr->raw_lock.slock&0xff));
> +
> +#else
> + hungry=(unsigned
> short)((lock_ptr->raw_lock.slock>>16&0xffff)-(lock_ptr->raw_lock.slock&0xffff));
> +
> +#endif

Prime example of uglyness that is unacceptable.

> + }
>
> depth = curr->lockdep_depth;
> if (DEBUG_LOCKS_WARN_ON(!depth))
> @@ -3030,9 +3044,13 @@ found_it:
> stats->contention_point[point]++;
> if (lock->cpu != smp_processor_id())
> stats->bounces[bounce_contended + !!hlock->read]++;
> + if (lock->isspinlock){
> + stats->bounces[bounce_hungry]+=hungry+1;
> + }
> put

superfluous braces.


> diff --git a/lib/spinlock_debug.c b/lib/spinlock_debug.c
> index 9c4b025..2ec34ec 100644
> --- a/lib/spinlock_debug.c
> +++ b/lib/spinlock_debug.c
> @@ -27,6 +27,10 @@ void __spin_lock_init(spinlock_t *lock, const char *name,
> lock->magic = SPINLOCK_MAGIC;
> lock->owner = SPINLOCK_OWNER_INIT;
> lock->owner_cpu = -1;
> + lock->dep_map.isspinlock = 0;
> +#if ((defined(CONFIG_X86) || defined(CONFIG_X86_64))&&
> defined(CONFIG_LOCK_STAT))
> + lock->dep_map.isspinlock = 1;
> +#endif

sorry, no, imagine the utter mess this will become when some other arch
adds ticket locks too...



\
 
 \ /
  Last update: 2008-11-18 17:25    [W:0.057 / U:1.268 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site