All of lore.kernel.org
 help / color / mirror / Atom feed
* contention on profile_lock
@ 2004-11-02 19:52 Jesse Barnes
  2004-11-02 20:02 ` Jack Steiner
  0 siblings, 1 reply; 13+ messages in thread
From: Jesse Barnes @ 2004-11-02 19:52 UTC (permalink / raw)
  To: wli, linux-kernel; +Cc: steiner, edwardsg

Hmm, the last patch you sent me worked ok, so I'm not sure why we're seeing 
problems with profiling now.  There seems to be very heavy contention on 
profile_lock since profile_hook is called unconditionally every timer tick.  
Should it only be called if profiling is enabled?  Is there a way we can 
check the notifier list to see if it's empty before calling it or something?  
The only user appears to be oprofile timer based profiling, so in the general 
case we're taking the profile_lock and not doing anything.

Thanks,
Jesse

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: contention on profile_lock
  2004-11-02 19:52 contention on profile_lock Jesse Barnes
@ 2004-11-02 20:02 ` Jack Steiner
  2004-11-02 21:42   ` Jesse Barnes
  0 siblings, 1 reply; 13+ messages in thread
From: Jack Steiner @ 2004-11-02 20:02 UTC (permalink / raw)
  To: Jesse Barnes; +Cc: wli, linux-kernel, edwardsg

On Tue, Nov 02, 2004 at 11:52:15AM -0800, Jesse Barnes wrote:
> Hmm, the last patch you sent me worked ok, so I'm not sure why we're seeing 
> problems with profiling now.  There seems to be very heavy contention on 
> profile_lock since profile_hook is called unconditionally every timer tick.  
> Should it only be called if profiling is enabled?  Is there a way we can 
> check the notifier list to see if it's empty before calling it or something?  
> The only user appears to be oprofile timer based profiling, so in the general 
> case we're taking the profile_lock and not doing anything.
> 
> Thanks,
> Jesse

Calling profile_hook() only if the notifier list is non-empty seems like a good
step but I don't think that is the complete fix. We need to be able to
enable profiling without killing performance.



-- 
Thanks

Jack Steiner (steiner@sgi.com)          651-683-5302
Principal Engineer                      SGI - Silicon Graphics, Inc.



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: contention on profile_lock
  2004-11-02 20:02 ` Jack Steiner
@ 2004-11-02 21:42   ` Jesse Barnes
  2004-11-04 19:56     ` Jesse Barnes
  0 siblings, 1 reply; 13+ messages in thread
From: Jesse Barnes @ 2004-11-02 21:42 UTC (permalink / raw)
  To: Jack Steiner; +Cc: wli, linux-kernel, edwardsg

On Tuesday, November 2, 2004 12:02 pm, Jack Steiner wrote:
> On Tue, Nov 02, 2004 at 11:52:15AM -0800, Jesse Barnes wrote:
> > Hmm, the last patch you sent me worked ok, so I'm not sure why we're
> > seeing problems with profiling now.  There seems to be very heavy
> > contention on profile_lock since profile_hook is called unconditionally
> > every timer tick. Should it only be called if profiling is enabled?  Is
> > there a way we can check the notifier list to see if it's empty before
> > calling it or something? The only user appears to be oprofile timer based
> > profiling, so in the general case we're taking the profile_lock and not
> > doing anything.
> >
> > Thanks,
> > Jesse
>
> Calling profile_hook() only if the notifier list is non-empty seems like a
> good step but I don't think that is the complete fix. We need to be able to
> enable profiling without killing performance.

Agreed.  Dipankar already suggested RCUifying the notifier list, but another 
option would be to simply check to see if oprofile timer based profiling is 
enabled since it seems to be the only user.  That would turn a lock into a 
read-mostly variable at least.

Jesse

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: contention on profile_lock
  2004-11-02 21:42   ` Jesse Barnes
@ 2004-11-04 19:56     ` Jesse Barnes
  2004-11-04 20:12       ` William Lee Irwin III
  0 siblings, 1 reply; 13+ messages in thread
From: Jesse Barnes @ 2004-11-04 19:56 UTC (permalink / raw)
  To: Jack Steiner; +Cc: wli, linux-kernel, edwardsg

[-- Attachment #1: Type: text/plain, Size: 575 bytes --]

On Tuesday, November 2, 2004 1:42 pm, Jesse Barnes wrote:
> Agreed.  Dipankar already suggested RCUifying the notifier list, but
> another option would be to simply check to see if oprofile timer based
> profiling is enabled since it seems to be the only user.  That would turn a
> lock into a read-mostly variable at least.

..but since I haven't heard from Dipankar, here's a patch that removes the 
profile_hook notifier list altogether in favor of a simple flag that controls 
whether or not to call the oprofile timer routine directly.  Does it look ok?

Thanks,
Jesse


[-- Attachment #2: remove-profile-notifier-list-2.patch --]
[-- Type: text/plain, Size: 3332 bytes --]

===== drivers/oprofile/timer_int.c 1.8 vs edited =====
--- 1.8/drivers/oprofile/timer_int.c	2004-09-03 16:55:27 -07:00
+++ edited/drivers/oprofile/timer_int.c	2004-11-04 11:53:42 -08:00
@@ -14,32 +14,29 @@
 #include <linux/profile.h>
 #include <linux/init.h>
 #include <asm/ptrace.h>
+
+int oprofile_timer;
  
-static int timer_notify(struct notifier_block * self, unsigned long val, void * data)
+int oprofile_timer_notify(struct pt_regs *regs)
 {
-	struct pt_regs * regs = (struct pt_regs *)data;
 	int cpu = smp_processor_id();
 	unsigned long eip = profile_pc(regs);
  
 	oprofile_add_sample(eip, !user_mode(regs), 0, cpu);
 	return 0;
 }
- 
- 
-static struct notifier_block timer_notifier = {
-	.notifier_call	= timer_notify,
-};
- 
 
 static int timer_start(void)
 {
-	return register_profile_notifier(&timer_notifier);
+	oprofile_timer = 1;
+	return 0;
 }
 
 
 static void timer_stop(void)
 {
-	unregister_profile_notifier(&timer_notifier);
+	oprofile_timer = 0;
+	wmb();
 }
 
 
===== include/linux/oprofile.h 1.10 vs edited =====
--- 1.10/include/linux/oprofile.h	2004-06-24 01:56:02 -07:00
+++ edited/include/linux/oprofile.h	2004-11-04 11:06:29 -08:00
@@ -13,6 +13,7 @@
 #ifndef OPROFILE_H
 #define OPROFILE_H
 
+#include <linux/config.h>
 #include <linux/types.h>
 #include <linux/spinlock.h>
 #include <asm/atomic.h>
@@ -105,5 +106,14 @@
 
 /** lock for read/write safety */
 extern spinlock_t oprofilefs_lock;
+
+#ifdef CONFIG_OPROFILE
+extern int oprofile_timer; /* bool for the oprofile timer */
+extern int oprofile_timer_notify(struct pt_regs *);
+#else
+#define oprofile_timer 0
+static inline int oprofile_timer_notify(struct pt_regs *) { return 0; }
+#endif
+
  
 #endif /* OPROFILE_H */
===== kernel/profile.c 1.14 vs edited =====
--- 1.14/kernel/profile.c	2004-10-19 02:40:31 -07:00
+++ edited/kernel/profile.c	2004-11-04 11:10:10 -08:00
@@ -22,6 +22,7 @@
 #include <linux/cpumask.h>
 #include <linux/cpu.h>
 #include <linux/profile.h>
+#include <linux/oprofile.h>
 #include <linux/highmem.h>
 #include <asm/sections.h>
 #include <asm/semaphore.h>
@@ -168,38 +169,6 @@
 	return err;
 }
 
-static struct notifier_block * profile_listeners;
-static rwlock_t profile_lock = RW_LOCK_UNLOCKED;
- 
-int register_profile_notifier(struct notifier_block * nb)
-{
-	int err;
-	write_lock_irq(&profile_lock);
-	err = notifier_chain_register(&profile_listeners, nb);
-	write_unlock_irq(&profile_lock);
-	return err;
-}
-
-
-int unregister_profile_notifier(struct notifier_block * nb)
-{
-	int err;
-	write_lock_irq(&profile_lock);
-	err = notifier_chain_unregister(&profile_listeners, nb);
-	write_unlock_irq(&profile_lock);
-	return err;
-}
-
-
-void profile_hook(struct pt_regs * regs)
-{
-	read_lock(&profile_lock);
-	notifier_call_chain(&profile_listeners, 0, regs);
-	read_unlock(&profile_lock);
-}
-
-EXPORT_SYMBOL_GPL(register_profile_notifier);
-EXPORT_SYMBOL_GPL(unregister_profile_notifier);
 EXPORT_SYMBOL_GPL(task_handoff_register);
 EXPORT_SYMBOL_GPL(task_handoff_unregister);
 
@@ -394,8 +363,8 @@
 
 void profile_tick(int type, struct pt_regs *regs)
 {
-	if (type == CPU_PROFILING)
-		profile_hook(regs);
+	if (type == CPU_PROFILING && oprofile_timer)
+		oprofile_timer_notify(regs);
 	if (!user_mode(regs) && cpu_isset(smp_processor_id(), prof_cpu_mask))
 		profile_hit(type, (void *)profile_pc(regs));
 }

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: contention on profile_lock
  2004-11-04 19:56     ` Jesse Barnes
@ 2004-11-04 20:12       ` William Lee Irwin III
  2004-11-04 20:49         ` Jesse Barnes
  0 siblings, 1 reply; 13+ messages in thread
From: William Lee Irwin III @ 2004-11-04 20:12 UTC (permalink / raw)
  To: Jesse Barnes; +Cc: Jack Steiner, linux-kernel, edwardsg

On Thu, Nov 04, 2004 at 11:56:23AM -0800, Jesse Barnes wrote:
> ..but since I haven't heard from Dipankar, here's a patch that removes the 
> profile_hook notifier list altogether in favor of a simple flag that controls 
> whether or not to call the oprofile timer routine directly.  Does it look ok?

This looks reasonable to me.


-- wli

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: contention on profile_lock
  2004-11-04 20:12       ` William Lee Irwin III
@ 2004-11-04 20:49         ` Jesse Barnes
  2004-11-04 21:51           ` John Levon
  2004-11-04 21:55           ` Jesse Barnes
  0 siblings, 2 replies; 13+ messages in thread
From: Jesse Barnes @ 2004-11-04 20:49 UTC (permalink / raw)
  To: William Lee Irwin III; +Cc: Jack Steiner, linux-kernel, edwardsg, dipankar

On Thursday, November 4, 2004 12:12 pm, William Lee Irwin III wrote:
> On Thu, Nov 04, 2004 at 11:56:23AM -0800, Jesse Barnes wrote:
> > ..but since I haven't heard from Dipankar, here's a patch that removes
> > the profile_hook notifier list altogether in favor of a simple flag that
> > controls whether or not to call the oprofile timer routine directly. 
> > Does it look ok?
>
> This looks reasonable to me.

John pointed out that this breaks modules.  Would registering and 
unregistering a function pointer thus be module safe?  Dipankar, hopefully 
you have something better?

static int timer_start(void)
{
 /* Setup the callback pointer */
 oprofile_timer_notify = oprofile_timer;
 return 0;
}


static void timer_stop(void)
{
 /* Tear down the callback pointer after sync_kernel */
 synchronize_kernel();
 oprofile_timer_notify = NULL;
}

Thanks,
Jesse

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: contention on profile_lock
  2004-11-04 20:49         ` Jesse Barnes
@ 2004-11-04 21:51           ` John Levon
  2004-11-04 22:08             ` Jesse Barnes
  2004-11-04 21:55           ` Jesse Barnes
  1 sibling, 1 reply; 13+ messages in thread
From: John Levon @ 2004-11-04 21:51 UTC (permalink / raw)
  To: Jesse Barnes
  Cc: William Lee Irwin III, Jack Steiner, linux-kernel, edwardsg,
	dipankar

On Thu, Nov 04, 2004 at 12:49:21PM -0800, Jesse Barnes wrote:

> John pointed out that this breaks modules.  Would registering and 
> unregistering a function pointer thus be module safe?  Dipankar, hopefully 
> you have something better?
> 
> static int timer_start(void)
> {
>  /* Setup the callback pointer */
>  oprofile_timer_notify = oprofile_timer;
>  return 0;
> }

Surely something like (profile.c):

funcptr_t timer_hook;

static int register_timer_hook(funcptr_t hook)
{
	if (timer_hook)
		return -EBUSY;
	timer_hook = hook;
}

static void unregister_timer_hook(funcptr_t hook)
{
	WARN_ON(hook != timer_hook);
	timer_hook = NULL;
	/* make sure all CPUs see the NULL hook */
	synchronize_kernel();
}

john

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: contention on profile_lock
  2004-11-04 20:49         ` Jesse Barnes
  2004-11-04 21:51           ` John Levon
@ 2004-11-04 21:55           ` Jesse Barnes
  2004-11-04 22:16             ` Dipankar Sarma
  2004-11-04 22:21             ` John Levon
  1 sibling, 2 replies; 13+ messages in thread
From: Jesse Barnes @ 2004-11-04 21:55 UTC (permalink / raw)
  To: William Lee Irwin III
  Cc: Jack Steiner, linux-kernel, edwardsg, dipankar, levon

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

On Thursday, November 4, 2004 12:49 pm, Jesse Barnes wrote:
> John pointed out that this breaks modules.  Would registering and
> unregistering a function pointer thus be module safe?  Dipankar, hopefully
> you have something better?
>
> static int timer_start(void)
> {
>  /* Setup the callback pointer */
>  oprofile_timer_notify = oprofile_timer;
>  return 0;
> }
>
>
> static void timer_stop(void)
> {
>  /* Tear down the callback pointer after sync_kernel */
>  synchronize_kernel();
>  oprofile_timer_notify = NULL;
> }

Ok, here's another one that uses this approach.  Hopefully it satisfies all 
parties?  (Compile tested only so far.)

Thanks,
Jesse

[-- Attachment #2: remove-profile-notifier-list-3.patch --]
[-- Type: text/plain, Size: 3493 bytes --]

===== drivers/oprofile/timer_int.c 1.8 vs edited =====
--- 1.8/drivers/oprofile/timer_int.c	2004-09-03 16:55:27 -07:00
+++ edited/drivers/oprofile/timer_int.c	2004-11-04 13:54:13 -08:00
@@ -13,33 +13,31 @@
 #include <linux/oprofile.h>
 #include <linux/profile.h>
 #include <linux/init.h>
+#include <linux/rcupdate.h>
 #include <asm/ptrace.h>
  
-static int timer_notify(struct notifier_block * self, unsigned long val, void * data)
+static int timer_notify(struct pt_regs *regs)
 {
-	struct pt_regs * regs = (struct pt_regs *)data;
 	int cpu = smp_processor_id();
 	unsigned long eip = profile_pc(regs);
  
 	oprofile_add_sample(eip, !user_mode(regs), 0, cpu);
 	return 0;
 }
- 
- 
-static struct notifier_block timer_notifier = {
-	.notifier_call	= timer_notify,
-};
- 
 
 static int timer_start(void)
 {
-	return register_profile_notifier(&timer_notifier);
+	/* Setup the callback pointer */
+	oprofile_timer_notify = timer_notify;
+	return 0;
 }
 
 
 static void timer_stop(void)
 {
-	unregister_profile_notifier(&timer_notifier);
+	/* Tear down the callback pointer after sync_kernel */
+	synchronize_kernel();
+	oprofile_timer_notify = NULL;
 }
 
 
===== include/linux/oprofile.h 1.10 vs edited =====
--- 1.10/include/linux/oprofile.h	2004-06-24 01:56:02 -07:00
+++ edited/include/linux/oprofile.h	2004-11-04 13:54:04 -08:00
@@ -105,5 +105,8 @@
 
 /** lock for read/write safety */
 extern spinlock_t oprofilefs_lock;
+
+/* Timer based profiling hook */
+extern int (*oprofile_timer_notify)(struct pt_regs *);
  
 #endif /* OPROFILE_H */
===== kernel/profile.c 1.14 vs edited =====
--- 1.14/kernel/profile.c	2004-10-19 02:40:31 -07:00
+++ edited/kernel/profile.c	2004-11-04 13:50:36 -08:00
@@ -22,6 +22,7 @@
 #include <linux/cpumask.h>
 #include <linux/cpu.h>
 #include <linux/profile.h>
+#include <linux/oprofile.h>
 #include <linux/highmem.h>
 #include <asm/sections.h>
 #include <asm/semaphore.h>
@@ -34,6 +35,9 @@
 #define NR_PROFILE_HIT		(PAGE_SIZE/sizeof(struct profile_hit))
 #define NR_PROFILE_GRP		(NR_PROFILE_HIT/PROFILE_GRPSZ)
 
+/* Oprofile timer tick hook */
+int (*oprofile_timer_notify)(struct pt_regs *);
+
 static atomic_t *prof_buffer;
 static unsigned long prof_len, prof_shift;
 static int prof_on;
@@ -168,38 +172,6 @@
 	return err;
 }
 
-static struct notifier_block * profile_listeners;
-static rwlock_t profile_lock = RW_LOCK_UNLOCKED;
- 
-int register_profile_notifier(struct notifier_block * nb)
-{
-	int err;
-	write_lock_irq(&profile_lock);
-	err = notifier_chain_register(&profile_listeners, nb);
-	write_unlock_irq(&profile_lock);
-	return err;
-}
-
-
-int unregister_profile_notifier(struct notifier_block * nb)
-{
-	int err;
-	write_lock_irq(&profile_lock);
-	err = notifier_chain_unregister(&profile_listeners, nb);
-	write_unlock_irq(&profile_lock);
-	return err;
-}
-
-
-void profile_hook(struct pt_regs * regs)
-{
-	read_lock(&profile_lock);
-	notifier_call_chain(&profile_listeners, 0, regs);
-	read_unlock(&profile_lock);
-}
-
-EXPORT_SYMBOL_GPL(register_profile_notifier);
-EXPORT_SYMBOL_GPL(unregister_profile_notifier);
 EXPORT_SYMBOL_GPL(task_handoff_register);
 EXPORT_SYMBOL_GPL(task_handoff_unregister);
 
@@ -394,8 +366,8 @@
 
 void profile_tick(int type, struct pt_regs *regs)
 {
-	if (type == CPU_PROFILING)
-		profile_hook(regs);
+	if (type == CPU_PROFILING && oprofile_timer_notify)
+		oprofile_timer_notify(regs);
 	if (!user_mode(regs) && cpu_isset(smp_processor_id(), prof_cpu_mask))
 		profile_hit(type, (void *)profile_pc(regs));
 }

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: contention on profile_lock
  2004-11-04 21:51           ` John Levon
@ 2004-11-04 22:08             ` Jesse Barnes
  0 siblings, 0 replies; 13+ messages in thread
From: Jesse Barnes @ 2004-11-04 22:08 UTC (permalink / raw)
  To: John Levon
  Cc: William Lee Irwin III, Jack Steiner, linux-kernel, edwardsg,
	dipankar

On Thursday, November 4, 2004 1:51 pm, John Levon wrote:
> On Thu, Nov 04, 2004 at 12:49:21PM -0800, Jesse Barnes wrote:
> > John pointed out that this breaks modules.  Would registering and
> > unregistering a function pointer thus be module safe?  Dipankar,
> > hopefully you have something better?
> >
> > static int timer_start(void)
> > {
> >  /* Setup the callback pointer */
> >  oprofile_timer_notify = oprofile_timer;
> >  return 0;
> > }
>
> Surely something like (profile.c):
>
> funcptr_t timer_hook;
>
> static int register_timer_hook(funcptr_t hook)
> {
>  if (timer_hook)
>   return -EBUSY;
>  timer_hook = hook;
> }
>
> static void unregister_timer_hook(funcptr_t hook)
> {
>  WARN_ON(hook != timer_hook);
>  timer_hook = NULL;
>  /* make sure all CPUs see the NULL hook */
>  synchronize_kernel();
> }

Yes, that's much better.  Will post another one shortly.  Thanks.

Jesse

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: contention on profile_lock
  2004-11-04 21:55           ` Jesse Barnes
@ 2004-11-04 22:16             ` Dipankar Sarma
  2004-11-04 22:21             ` John Levon
  1 sibling, 0 replies; 13+ messages in thread
From: Dipankar Sarma @ 2004-11-04 22:16 UTC (permalink / raw)
  To: Jesse Barnes
  Cc: William Lee Irwin III, Jack Steiner, linux-kernel, edwardsg,
	levon

On Thu, Nov 04, 2004 at 01:55:27PM -0800, Jesse Barnes wrote:
> On Thursday, November 4, 2004 12:49 pm, Jesse Barnes wrote:
> > John pointed out that this breaks modules.  Would registering and
>  
>  static void timer_stop(void)
>  {
> -	unregister_profile_notifier(&timer_notifier);
> +	/* Tear down the callback pointer after sync_kernel */
> +	synchronize_kernel();
> +	oprofile_timer_notify = NULL;
>  }
>  
>  
> +/* Oprofile timer tick hook */
> +int (*oprofile_timer_notify)(struct pt_regs *);
> +
>  static atomic_t *prof_buffer;
>  static unsigned long prof_len, prof_shift;
>  static int prof_on;
> @@ -168,38 +172,6 @@
>  	return err;
>  }
>  


> -void profile_hook(struct pt_regs * regs)
> -{
> -	read_lock(&profile_lock);
> -	notifier_call_chain(&profile_listeners, 0, regs);
> -	read_unlock(&profile_lock);
> -}
> -
> -EXPORT_SYMBOL_GPL(register_profile_notifier);
> -EXPORT_SYMBOL_GPL(unregister_profile_notifier);
>  EXPORT_SYMBOL_GPL(task_handoff_register);
>  EXPORT_SYMBOL_GPL(task_handoff_unregister);
>  
> @@ -394,8 +366,8 @@
>  
>  void profile_tick(int type, struct pt_regs *regs)
>  {
> -	if (type == CPU_PROFILING)
> -		profile_hook(regs);
> +	if (type == CPU_PROFILING && oprofile_timer_notify)
> +		oprofile_timer_notify(regs);
>  	if (!user_mode(regs) && cpu_isset(smp_processor_id(), prof_cpu_mask))
>  		profile_hit(type, (void *)profile_pc(regs));
>  }


Or you could just do -

void profile_tick(int type, struct pt_regs *regs)
{
	int (*timer_notify)(struct pt_regs *);

	timer_notify = oprofile_timer_notify;
	smp_read_barrier_depends();
	if (type == CPU_PROFILING && timer_notify) {
		timer_notify(regs);
	}
	if (!user_mode(regs) && cpu_isset(smp_processor_id(), prof_cpu_mask))
		profile_hit(type, (void *)profile_pc(regs));
}

And avoid the synchronize_kernel(). But I think the synchronize_kernel()
thing is cleaner.

I am looking at the notifier chain mechanism itself and its
lack of proper locking. That is a different story.

Thanks
Dipankar

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: contention on profile_lock
  2004-11-04 21:55           ` Jesse Barnes
  2004-11-04 22:16             ` Dipankar Sarma
@ 2004-11-04 22:21             ` John Levon
  2004-11-04 22:27               ` Jesse Barnes
  1 sibling, 1 reply; 13+ messages in thread
From: John Levon @ 2004-11-04 22:21 UTC (permalink / raw)
  To: Jesse Barnes
  Cc: William Lee Irwin III, Jack Steiner, linux-kernel, edwardsg,
	dipankar

On Thu, Nov 04, 2004 at 01:55:27PM -0800, Jesse Barnes wrote:

> +/* Oprofile timer tick hook */
> +int (*oprofile_timer_notify)(struct pt_regs *);

How is the module going to access this if you don't EXPORT_SYMBOL_GPL()
it ?

Do you have some specific objection to keeping the register/unregister
functions as I showed?

john

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: contention on profile_lock
  2004-11-04 22:21             ` John Levon
@ 2004-11-04 22:27               ` Jesse Barnes
  2004-11-04 22:52                 ` John Levon
  0 siblings, 1 reply; 13+ messages in thread
From: Jesse Barnes @ 2004-11-04 22:27 UTC (permalink / raw)
  To: John Levon
  Cc: William Lee Irwin III, Jack Steiner, linux-kernel, edwardsg,
	dipankar

[-- Attachment #1: Type: text/plain, Size: 509 bytes --]

On Thursday, November 4, 2004 2:21 pm, John Levon wrote:
> On Thu, Nov 04, 2004 at 01:55:27PM -0800, Jesse Barnes wrote:
> > +/* Oprofile timer tick hook */
> > +int (*oprofile_timer_notify)(struct pt_regs *);
>
> How is the module going to access this if you don't EXPORT_SYMBOL_GPL()
> it ?
>
> Do you have some specific objection to keeping the register/unregister
> functions as I showed?

Nope, not at all, just the locking :).  How does this one look?  I think I've 
exported the right symbols.

Jesse


[-- Attachment #2: remove-profile-notifier-list-5.patch --]
[-- Type: text/plain, Size: 4397 bytes --]

===== drivers/oprofile/timer_int.c 1.8 vs edited =====
--- 1.8/drivers/oprofile/timer_int.c	2004-09-03 16:55:27 -07:00
+++ edited/drivers/oprofile/timer_int.c	2004-11-04 14:24:59 -08:00
@@ -15,31 +15,24 @@
 #include <linux/init.h>
 #include <asm/ptrace.h>
  
-static int timer_notify(struct notifier_block * self, unsigned long val, void * data)
+static int timer_notify(struct pt_regs *regs)
 {
-	struct pt_regs * regs = (struct pt_regs *)data;
 	int cpu = smp_processor_id();
 	unsigned long eip = profile_pc(regs);
  
 	oprofile_add_sample(eip, !user_mode(regs), 0, cpu);
 	return 0;
 }
- 
- 
-static struct notifier_block timer_notifier = {
-	.notifier_call	= timer_notify,
-};
- 
 
 static int timer_start(void)
 {
-	return register_profile_notifier(&timer_notifier);
+	return register_timer_hook(timer_notify);
 }
 
 
 static void timer_stop(void)
 {
-	unregister_profile_notifier(&timer_notifier);
+	unregister_timer_hook(timer_notify);
 }
 
 
===== include/linux/profile.h 1.11 vs edited =====
--- 1.11/include/linux/profile.h	2004-08-26 23:42:56 -07:00
+++ edited/include/linux/profile.h	2004-11-04 14:21:00 -08:00
@@ -53,13 +53,13 @@
 int profile_event_register(enum profile_type, struct notifier_block * n);
 int profile_event_unregister(enum profile_type, struct notifier_block * n);
 
-int register_profile_notifier(struct notifier_block * nb);
-int unregister_profile_notifier(struct notifier_block * nb);
+int register_timer_hook(int (*hook)(struct pt_regs *));
+void unregister_timer_hook(int (*hook)(struct pt_regs *));
 
-struct pt_regs;
+/* Timer based profiling hook */
+extern int (*timer_hook)(struct pt_regs *);
 
-/* profiling hook activated on each timer interrupt */
-void profile_hook(struct pt_regs * regs);
+struct pt_regs;
 
 #else
 
@@ -87,17 +87,15 @@
 #define profile_handoff_task(a) (0)
 #define profile_munmap(a) do { } while (0)
 
-static inline int register_profile_notifier(struct notifier_block * nb)
+static inline int register_timer_hook(int (*hook)(struct pt_regs *))
 {
 	return -ENOSYS;
 }
 
-static inline int unregister_profile_notifier(struct notifier_block * nb)
+static inline void unregister_timer_hook(int (*hook)(struct pt_regs *))
 {
-	return -ENOSYS;
+	return;
 }
-
-#define profile_hook(regs) do { } while (0)
 
 #endif /* CONFIG_PROFILING */
 
===== kernel/profile.c 1.14 vs edited =====
--- 1.14/kernel/profile.c	2004-10-19 02:40:31 -07:00
+++ edited/kernel/profile.c	2004-11-04 14:23:38 -08:00
@@ -34,6 +34,9 @@
 #define NR_PROFILE_HIT		(PAGE_SIZE/sizeof(struct profile_hit))
 #define NR_PROFILE_GRP		(NR_PROFILE_HIT/PROFILE_GRPSZ)
 
+/* Oprofile timer tick hook */
+int (*timer_hook)(struct pt_regs *);
+
 static atomic_t *prof_buffer;
 static unsigned long prof_len, prof_shift;
 static int prof_on;
@@ -168,38 +171,24 @@
 	return err;
 }
 
-static struct notifier_block * profile_listeners;
-static rwlock_t profile_lock = RW_LOCK_UNLOCKED;
- 
-int register_profile_notifier(struct notifier_block * nb)
+int register_timer_hook(int (*hook)(struct pt_regs *))
 {
-	int err;
-	write_lock_irq(&profile_lock);
-	err = notifier_chain_register(&profile_listeners, nb);
-	write_unlock_irq(&profile_lock);
-	return err;
+	if (timer_hook)
+		return -EBUSY;
+	timer_hook = hook;
+	return 0;
 }
 
-
-int unregister_profile_notifier(struct notifier_block * nb)
-{
-	int err;
-	write_lock_irq(&profile_lock);
-	err = notifier_chain_unregister(&profile_listeners, nb);
-	write_unlock_irq(&profile_lock);
-	return err;
-}
-
-
-void profile_hook(struct pt_regs * regs)
+void unregister_timer_hook(int (*hook)(struct pt_regs *))
 {
-	read_lock(&profile_lock);
-	notifier_call_chain(&profile_listeners, 0, regs);
-	read_unlock(&profile_lock);
+	WARN_ON(hook != timer_hook);
+	timer_hook = NULL;
+	/* make sure all CPUs see the NULL hook */
+	synchronize_kernel();
 }
 
-EXPORT_SYMBOL_GPL(register_profile_notifier);
-EXPORT_SYMBOL_GPL(unregister_profile_notifier);
+EXPORT_SYMBOL_GPL(register_timer_hook);
+EXPORT_SYMBOL_GPL(unregister_timer_hook);
 EXPORT_SYMBOL_GPL(task_handoff_register);
 EXPORT_SYMBOL_GPL(task_handoff_unregister);
 
@@ -394,8 +383,8 @@
 
 void profile_tick(int type, struct pt_regs *regs)
 {
-	if (type == CPU_PROFILING)
-		profile_hook(regs);
+	if (type == CPU_PROFILING && timer_hook)
+		timer_hook(regs);
 	if (!user_mode(regs) && cpu_isset(smp_processor_id(), prof_cpu_mask))
 		profile_hit(type, (void *)profile_pc(regs));
 }

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: contention on profile_lock
  2004-11-04 22:27               ` Jesse Barnes
@ 2004-11-04 22:52                 ` John Levon
  0 siblings, 0 replies; 13+ messages in thread
From: John Levon @ 2004-11-04 22:52 UTC (permalink / raw)
  To: Jesse Barnes
  Cc: William Lee Irwin III, Jack Steiner, linux-kernel, edwardsg,
	dipankar

On Thu, Nov 04, 2004 at 02:27:05PM -0800, Jesse Barnes wrote:

> Nope, not at all, just the locking :).  How does this one look?  I think I've 
> exported the right symbols.

Looks fine to me

regards
john

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2004-11-04 23:06 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-11-02 19:52 contention on profile_lock Jesse Barnes
2004-11-02 20:02 ` Jack Steiner
2004-11-02 21:42   ` Jesse Barnes
2004-11-04 19:56     ` Jesse Barnes
2004-11-04 20:12       ` William Lee Irwin III
2004-11-04 20:49         ` Jesse Barnes
2004-11-04 21:51           ` John Levon
2004-11-04 22:08             ` Jesse Barnes
2004-11-04 21:55           ` Jesse Barnes
2004-11-04 22:16             ` Dipankar Sarma
2004-11-04 22:21             ` John Levon
2004-11-04 22:27               ` Jesse Barnes
2004-11-04 22:52                 ` John Levon

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.