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
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 ([email protected]) 651-683-5302
Principal Engineer SGI - Silicon Graphics, Inc.
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
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
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
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
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
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
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
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
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
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
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