Re: [PATCH 2/3] lockmeter: lockmeter fix for generic_read_trylock

Re: [PATCH 2/3] lockmeter: lockmeter fix for generic_read_trylock

[Zwane Mwaikambo]

On Thu, 16 Sep 2004, Ray Bryant wrote:

> Update lockmeter.c with generic_raw_read_trylock fix.
>
> + * Generic declaration of the raw read_trylock() function,
> + * architectures are supposed to optimize this:
> + */
> +int __lockfunc generic_raw_read_trylock(rwlock_t *lock)
> +{
> +	_metered_read_lock(lock, __builtin_return_address(0));
> +	return 1;
> +}

What's really going on here? I'm slightly confused by the 
_metered_read_lock usage.

Thanks,
	Zwane

[PATCH 0/3] lockmeter: fixes

[Ray Bryant]

Andrew,

The first patch in this series is a replacement patch for the prempt-fix
patch I sent earlier this morning.  There was a missing paren in that
previous version.

Adding this to 2.6.9-rc2-mm1 (just after lockmeter-2.patch) fixes the
preempt compile problem on ia64, at least.  However, I then started 
getting a missing symbol for generic_raw_read_trylock.  Hence the second
patch of this series.

(It is the nature of this lockmeter fix for the COOL bits that whenever
a change is made to kernel/spinlock.c, you need to make a correspoding
change to the kernel/lockmeter.c code that parallels spinlock.c. I 
don't see any good way around this.)

Finally, Zwane has suggested making in_lock_functions() an inline.
I sent him and you a simple patch to do that, and if that is applied,
you then need the third patch of this series.

All three of these patches are intended to follow lockmeter-2.patch in
the series.

I compiled lockmeter on/off, preempt on/off (all 4) kernels on ia64.
I'm working on doing that for i386, but my i386 machines are much slower
than an 8-way Altix for doing compiles. :-)  I'll test some of these
as well.

Best Regards,

Ray

raybry@sgi.com

-- 
Best Regards,
Ray
-----------------------------------------------
Ray Bryant                       raybry@sgi.com
The box said: "Requires Windows 98 or better",
           so I installed Linux.
-----------------------------------------------

[PATCH 1/3] lockmeter: fixed-up: lockmeter fixes for preempt case

[Ray Bryant]

Updated lockmeter-preempt-fix.patch.

Signed-off-by: Ray Bryant <raybry@sgi.com>

Index: linux-2.6.9-rc2-mm1/kernel/lockmeter.c
===================================================================
--- linux-2.6.9-rc2-mm1.orig/kernel/lockmeter.c	2004-09-16 11:03:05.000000000 -0700
+++ linux-2.6.9-rc2-mm1/kernel/lockmeter.c	2004-09-16 12:03:20.000000000 -0700
@@ -1236,18 +1236,58 @@
 EXPORT_SYMBOL(_write_trylock);
 
 #if defined(CONFIG_SMP) && defined(CONFIG_PREEMPT)
+/*
+ * This could be a long-held lock.  If another CPU holds it for a long time,
+ * and that CPU is not asked to reschedule then *this* CPU will spin on the
+ * lock for a long time, even if *this* CPU is asked to reschedule.
+ *
+ * So what we do here, in the slow (contended) path is to spin on the lock by
+ * hand while permitting preemption.
+ *
+ * Called inside preempt_disable().
+ */
+static inline void __preempt_spin_lock(spinlock_t *lock, void *caller_pc)
+{
+	if (preempt_count() > 1) {
+		_metered_spin_lock(lock, caller_pc);
+		return;
+	}
+
+	do {
+		preempt_enable();
+		while (spin_is_locked(lock))
+			cpu_relax();
+		preempt_disable();
+	} while (!_metered_spin_trylock(lock, caller_pc));
+}
+
 void __lockfunc _spin_lock(spinlock_t *lock)
 {
 	preempt_disable();
 	if (unlikely(!_metered_spin_trylock(lock, __builtin_return_address(0))))
-		__preempt_spin_lock(lock);
+		__preempt_spin_lock(lock, __builtin_return_address(0));
+}
+
+static inline void __preempt_write_lock(rwlock_t *lock, void *caller_pc)
+{
+	if (preempt_count() > 1) {
+		_metered_write_lock(lock, caller_pc);
+		return;
+	}
+
+	do {
+		preempt_enable();
+		while (rwlock_is_locked(lock))
+			cpu_relax();
+		preempt_disable();
+	} while (!_metered_write_trylock(lock,caller_pc));
 }
 
 void __lockfunc _write_lock(rwlock_t *lock)
 {
 	preempt_disable();
 	if (unlikely(!_metered_write_trylock(lock, __builtin_return_address(0))))
-		__preempt_write_lock(lock);
+		__preempt_write_lock(lock, __builtin_return_address(0));
 }
 #else
 void __lockfunc _spin_lock(spinlock_t *lock)
@@ -1458,3 +1498,13 @@
 	return 0;
 }
 EXPORT_SYMBOL(_spin_trylock_bh);
+
+int in_lock_functions(unsigned long addr)
+{
+	/* Linker adds these: start and end of __lockfunc functions */
+	extern char __lock_text_start[], __lock_text_end[];
+
+	return addr >= (unsigned long)__lock_text_start
+	&& addr < (unsigned long)__lock_text_end;
+}
+EXPORT_SYMBOL(in_lock_functions);

-- 
Best Regards,
Ray
-----------------------------------------------
Ray Bryant                       raybry@sgi.com
The box said: "Requires Windows 98 or better",
           so I installed Linux.
-----------------------------------------------

[PATCH 2/3] lockmeter: lockmeter fix for generic_read_trylock

[Ray Bryant]

Update lockmeter.c with generic_raw_read_trylock fix.

Signed-off-by: Ray Bryant <raybry@sgi.com>

Index: linux-2.6.9-rc2-mm1/kernel/lockmeter.c
===================================================================
--- linux-2.6.9-rc2-mm1.orig/kernel/lockmeter.c	2004-09-16 12:03:20.000000000 -0700
+++ linux-2.6.9-rc2-mm1/kernel/lockmeter.c	2004-09-16 12:29:08.000000000 -0700
@@ -1213,6 +1213,18 @@
  * except for the fact tht calls to _raw_ routines are replaced by
  * corresponding calls to the _metered_ routines
  */
+
+/*
+ * Generic declaration of the raw read_trylock() function,
+ * architectures are supposed to optimize this:
+ */
+int __lockfunc generic_raw_read_trylock(rwlock_t *lock)
+{
+	_metered_read_lock(lock, __builtin_return_address(0));
+	return 1;
+}
+EXPORT_SYMBOL(generic_raw_read_trylock);
+
 int __lockfunc _spin_trylock(spinlock_t *lock)
 {
 	preempt_disable();

-- 
Best Regards,
Ray
-----------------------------------------------
Ray Bryant                       raybry@sgi.com
The box said: "Requires Windows 98 or better",
           so I installed Linux.
-----------------------------------------------

[PATCH 3/3] lockmeter: lockmeter fix for inline in_lock_functions()

[Ray Bryant]

Fix to lockmeter.c for inline in_lock_functions().
(Only required if in_lock_functions() is modified to be inline.)


Signed-off-by: Ray Bryant <raybry@sgi.com>

Index: linux-2.6.9-rc2-mm1/kernel/lockmeter.c
===================================================================
--- linux-2.6.9-rc2-mm1.orig/kernel/lockmeter.c	2004-09-16 12:29:08.000000000 -0700
+++ linux-2.6.9-rc2-mm1/kernel/lockmeter.c	2004-09-16 12:53:57.000000000 -0700
@@ -1510,13 +1510,3 @@
 	return 0;
 }
 EXPORT_SYMBOL(_spin_trylock_bh);
-
-int in_lock_functions(unsigned long addr)
-{
-	/* Linker adds these: start and end of __lockfunc functions */
-	extern char __lock_text_start[], __lock_text_end[];
-
-	return addr >= (unsigned long)__lock_text_start
-	&& addr < (unsigned long)__lock_text_end;
-}
-EXPORT_SYMBOL(in_lock_functions);

-- 
Best Regards,
Ray
-----------------------------------------------
Ray Bryant                       raybry@sgi.com
The box said: "Requires Windows 98 or better",
           so I installed Linux.
-----------------------------------------------

Re: [PATCH 2/3] lockmeter: lockmeter fix for generic_read_trylock

[Ray Bryant]

Zwane Mwaikambo wrote:
> On Thu, 16 Sep 2004, Ray Bryant wrote:
> 
> 
>>Update lockmeter.c with generic_raw_read_trylock fix.
>>
>>+ * Generic declaration of the raw read_trylock() function,
>>+ * architectures are supposed to optimize this:
>>+ */
>>+int __lockfunc generic_raw_read_trylock(rwlock_t *lock)
>>+{
>>+	_metered_read_lock(lock, __builtin_return_address(0));
>>+	return 1;
>>+}
> 
> 
> What's really going on here? I'm slightly confused by the 
> _metered_read_lock usage.
> 
> Thanks,
> 	Zwane
> 
> 

Yeah, I think overly patterned matched on this one.  I just
grabbed the code from spinlock.c and do what I normally do to
make it lockmetered, but in this case since its a _raw_ routine, I
should have left it alone.  It just needs to be duplicated in
lockmeter.c, not lockmeter'd.

So that middle line there should be:

	_raw_read_lock(lock);

instead.  I'll get this straightend out with Andrew shortly.
-- 
Best Regards,
Ray
-----------------------------------------------
                   Ray Bryant
512-453-9679 (work)         512-507-7807 (cell)
raybry@sgi.com             raybry@austin.rr.com
The box said: "Requires Windows 98 or better",
            so I installed Linux.
-----------------------------------------------

Re: [PATCH 0/3] lockmeter: fixes

[Christoph Hellwig]

On Thu, Sep 16, 2004 at 04:03:44PM -0700, Ray Bryant wrote:
> Andrew,
> 
> The first patch in this series is a replacement patch for the prempt-fix
> patch I sent earlier this morning.  There was a missing paren in that
> previous version.

Any chance you could stop lockmeter patching around in fs/proc/proc_misc.c?
procfs files can be created easily from individual drivers.

Re: [PATCH 0/3] lockmeter: fixes

[Ray Bryant]

Christoph Hellwig wrote:
> On Thu, Sep 16, 2004 at 04:03:44PM -0700, Ray Bryant wrote:
> 
>>Andrew,
>>
>>The first patch in this series is a replacement patch for the prempt-fix
>>patch I sent earlier this morning.  There was a missing paren in that
>>previous version.
> 
> 
> Any chance you could stop lockmeter patching around in fs/proc/proc_misc.c?
> procfs files can be created easily from individual drivers.
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
Hi Christoph,

Yes, I'll take a look at it as soon as I get done chasing the COOL bits changes.

Is there a specific example you can point me at as to how others have done this?

Thanks,
-- 
Best Regards,
Ray
-----------------------------------------------
                   Ray Bryant
512-453-9679 (work)         512-507-7807 (cell)
raybry@sgi.com             raybry@austin.rr.com
The box said: "Requires Windows 98 or better",
            so I installed Linux.
-----------------------------------------------

Re: [PATCH 0/3] lockmeter: fixes

[Christoph Hellwig]

> Yes, I'll take a look at it as soon as I get done chasing the COOL bits changes.
> 
> Is there a specific example you can point me at as to how others have done this?

Just grep for create_proc_entry - lots of drivers are using this.  In fact I wonder
whether a miscdevice wouldn't be the better choice for lockmeter, but given that'd
need userland changes let's stay away from that for now.

Re: [PATCH 0/3] lockmeter: fixes

[Alexander Nyberg]

> Andrew,
> 
> The first patch in this series is a replacement patch for the prempt-fix
> patch I sent earlier this morning.  There was a missing paren in that
> previous version.
> 
> Adding this to 2.6.9-rc2-mm1 (just after lockmeter-2.patch) fixes the
> preempt compile problem on ia64, at least.  However, I then started 
> getting a missing symbol for generic_raw_read_trylock.  Hence the second
> patch of this series.

Hi Ray

I need the following to compile on x86-64, works fine otherwise.


--- mm/kernel/lockmeter.c.orig	2004-09-19 12:49:52.000000000 +0200
+++ mm/kernel/lockmeter.c	2004-09-19 12:50:25.000000000 +0200
@@ -1510,3 +1510,13 @@ int __lockfunc _spin_trylock_bh(spinlock
 	return 0;
 }
 EXPORT_SYMBOL(_spin_trylock_bh);
+
+int in_lock_functions(unsigned long addr)
+{
+	/* Linker adds these: start and end of __lockfunc functions */
+	extern char __lock_text_start[], __lock_text_end[];
+
+	return addr >= (unsigned long)__lock_text_start
+		&& addr < (unsigned long)__lock_text_end;
+}
+EXPORT_SYMBOL(in_lock_functions);
--- mm/include/asm-x86_64/spinlock.h.orig	2004-09-19 12:31:32.000000000 +0200
+++ mm/include/asm-x86_64/spinlock.h	2004-09-19 13:04:56.117109248 +0200
@@ -232,16 +232,6 @@ static inline void _raw_write_lock(rwloc
 #define _raw_read_unlock(rw)		asm volatile("lock ; incl %0" :"=m" ((rw)->lock) : : "memory")
 #define _raw_write_unlock(rw)	asm volatile("lock ; addl $" RW_LOCK_BIAS_STR ",%0":"=m" ((rw)->lock) : : "memory")
 
-static inline int _raw_read_trylock(rwlock_t *lock)
-{
-	atomic_t *count = (atomic_t *)lock;
-	atomic_dec(count);
-	if (atomic_read(count) < RW_LOCK_BIAS)
-		return 1;
-	atomic_inc(count);
-	return 0;
-}
-
 static inline int _raw_write_trylock(rwlock_t *lock)
 {
 	atomic_t *count = (atomic_t *)lock;
@@ -262,6 +252,16 @@ static inline int _raw_read_trylock(rwlo
 	atomic_inc(count);
 	return 0;
 }
+#else
+static inline int _raw_read_trylock(rwlock_t *lock)
+{
+	atomic_t *count = (atomic_t *)lock;
+	atomic_dec(count);
+	if (atomic_read(count) < RW_LOCK_BIAS)
+		return 1;
+	atomic_inc(count);
+	return 0;
+}
 #endif
 
 #if defined(CONFIG_LOCKMETER) && defined(CONFIG_HAVE_DEC_LOCK)
 

Re: [PATCH 0/3] lockmeter: fixes

[Ray Bryant]

Alexander Nyberg wrote:
>>Andrew,
>>
>>The first patch in this series is a replacement patch for the prempt-fix
>>patch I sent earlier this morning.  There was a missing paren in that
>>previous version.
>>
>>Adding this to 2.6.9-rc2-mm1 (just after lockmeter-2.patch) fixes the
>>preempt compile problem on ia64, at least.  However, I then started 
>>getting a missing symbol for generic_raw_read_trylock.  Hence the second
>>patch of this series.
> 
> 
> Hi Ray
> 
> I need the following to compile on x86-64, works fine otherwise.
> 
> 
> --- mm/kernel/lockmeter.c.orig	2004-09-19 12:49:52.000000000 +0200
> +++ mm/kernel/lockmeter.c	2004-09-19 12:50:25.000000000 +0200
> @@ -1510,3 +1510,13 @@ int __lockfunc _spin_trylock_bh(spinlock
>  	return 0;
>  }
>  EXPORT_SYMBOL(_spin_trylock_bh);
> +
> +int in_lock_functions(unsigned long addr)
> +{
> +	/* Linker adds these: start and end of __lockfunc functions */
> +	extern char __lock_text_start[], __lock_text_end[];
> +
> +	return addr >= (unsigned long)__lock_text_start
> +		&& addr < (unsigned long)__lock_text_end;
> +}
> +EXPORT_SYMBOL(in_lock_functions);
> --- mm/include/asm-x86_64/spinlock.h.orig	2004-09-19 12:31:32.000000000 +0200
> +++ mm/include/asm-x86_64/spinlock.h	2004-09-19 13:04:56.117109248 +0200
> @@ -232,16 +232,6 @@ static inline void _raw_write_lock(rwloc
>  #define _raw_read_unlock(rw)		asm volatile("lock ; incl %0" :"=m" ((rw)->lock) : : "memory")
>  #define _raw_write_unlock(rw)	asm volatile("lock ; addl $" RW_LOCK_BIAS_STR ",%0":"=m" ((rw)->lock) : : "memory")
>  
> -static inline int _raw_read_trylock(rwlock_t *lock)
> -{
> -	atomic_t *count = (atomic_t *)lock;
> -	atomic_dec(count);
> -	if (atomic_read(count) < RW_LOCK_BIAS)
> -		return 1;
> -	atomic_inc(count);
> -	return 0;
> -}
> -
>  static inline int _raw_write_trylock(rwlock_t *lock)
>  {
>  	atomic_t *count = (atomic_t *)lock;
> @@ -262,6 +252,16 @@ static inline int _raw_read_trylock(rwlo
>  	atomic_inc(count);
>  	return 0;
>  }
> +#else
> +static inline int _raw_read_trylock(rwlock_t *lock)
> +{
> +	atomic_t *count = (atomic_t *)lock;
> +	atomic_dec(count);
> +	if (atomic_read(count) < RW_LOCK_BIAS)
> +		return 1;
> +	atomic_inc(count);
> +	return 0;
> +}
>  #endif
>  
>  #if defined(CONFIG_LOCKMETER) && defined(CONFIG_HAVE_DEC_LOCK)
>  
> 
> 

Hi Alex,

There were a flurry of spinlock and corresponding lockmeter changes on 
Thursday and Friday.  One of the patches was to make in_lock_functions()
an inline, so that solves the first problem you found above.  A second
one that Andrew found was the duplicate definition of _raw_read_trylock(),
which is what I think the second part of your patch is.

I'm currently working on testing a lockmeter rollup patch.  I'll add the 
x86_64 _raw_read_trylock() fix unless Andrew has already got that covered.

Thanks,

-- 
Best Regards,
Ray
-----------------------------------------------
                   Ray Bryant
512-453-9679 (work)         512-507-7807 (cell)
raybry@sgi.com             raybry@austin.rr.com
The box said: "Requires Windows 98 or better",
            so I installed Linux.
-----------------------------------------------