Message-ID: <20090826040917.GC6245@nowhere>
Date:	Wed, 26 Aug 2009 06:09:19 +0200
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	"K.Prasad" <prasad@...ux.vnet.ibm.com>
Cc:	LKML <linux-kernel@...r.kernel.org>, Ingo Molnar <mingo@...e.hu>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Lai Jiangshan <laijs@...fujitsu.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>,
	Alan Stern <stern@...land.harvard.edu>
Subject: Re: [Patch 2/2] HW-BKPT: Allow kernel breakpoints to be modified
	through a new API

On Wed, Aug 26, 2009 at 01:51:59AM +0530, K.Prasad wrote:
> Introduce modify_kernel_hw_breakpoint() API that can quickly change the
> characteristics (such as address, length, type but not cpumask) of a
> kernel-space breakpoint without having to unregister first and then
> re-register it.
> 
> Signed-off-by: K.Prasad <prasad@...ux.vnet.ibm.com>


Hmm... You will hate me but... While looking more closely
at the pmu management in perf, I realize I've made a mistake by requesting
this functionnality.

struct pmu are built of some callbacks, especially enable() and disable().
These callbacks are called by perf when a task/group is sched in/out.

The breakpoint API then doesn't need to be able to live-modify breakpoint
properties.
Instead, what we need is a way to disarm/rearm a bp without actually unregister
it (we don't want to lose its slot until it gets rearmed).

So eventually we need two callbacks: 

- disable_kernel_breakpoint() that just disarm the breakpoint in the
  hardware level without unregistering it in the software level.
  When a profiled task group is sched out, the counter becomes
  inactive, waiting to be sched in later, that's why the bp
  must still be registered at this time (but inactive).

- enable_kernel_breakpoint() that rearms it

Your first patch that handles the per cpu breakpoints + these two callbacks
are the only thing we need for the perf integration.

Once we get that, it's up to perf to handle the enable/disable for
its own needs. Of course we already have the per task breakpoints
that could handle that, but it looks easier and better to let perf
decide whenever it needs the pmu to be bound or not.

So, I'm sorry but we won't need this one patch (but we still need
the first). And you can also forget about the breakpoint inheritance
across clone() calls, hardware register rewrite on schedule() only while
switching to separate task groups (although that could enhance/optimize
this API).
These things are already handled by perf.

I should have looked more closely into the PMUs before requesting
you this patch. My bad...

Thanks,
Frederic.

> ---
>  include/asm-generic/hw_breakpoint.h |    2 +
>  kernel/hw_breakpoint.c              |   49 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
> 
> Index: linux-2.6-tip.hbkpt/include/asm-generic/hw_breakpoint.h
> ===================================================================
> --- linux-2.6-tip.hbkpt.orig/include/asm-generic/hw_breakpoint.h
> +++ linux-2.6-tip.hbkpt/include/asm-generic/hw_breakpoint.h
> @@ -133,6 +133,8 @@ extern void unregister_user_hw_breakpoin
>   * Kernel breakpoints are not associated with any particular thread.
>   */
>  extern int register_kernel_hw_breakpoint(struct hw_breakpoint *bp);
> +extern int modify_kernel_hw_breakpoint(struct hw_breakpoint *old_bp,
> +					struct hw_breakpoint *new_bp);
>  extern void unregister_kernel_hw_breakpoint(struct hw_breakpoint *bp);
>  
>  extern unsigned int hbp_kernel_pos;
> Index: linux-2.6-tip.hbkpt/kernel/hw_breakpoint.c
> ===================================================================
> --- linux-2.6-tip.hbkpt.orig/kernel/hw_breakpoint.c
> +++ linux-2.6-tip.hbkpt/kernel/hw_breakpoint.c
> @@ -362,6 +362,55 @@ err_ret:
>  }
>  EXPORT_SYMBOL_GPL(register_kernel_hw_breakpoint);
>  
> +/**
> + * modify_kernel_hw_breakpoint - modify characteristics of a previously registered breakpoint request
> + * @old_bp: pointer to the registered breakpoint structure
> + * @new_bp: pointer to the breakpoint structure that replaces @old_bp
> + *
> + */
> +int modify_kernel_hw_breakpoint(struct hw_breakpoint *old_bp,
> +				   struct hw_breakpoint *new_bp)
> +{
> +	int i, rc;
> +	unsigned int cpu;
> +	const cpumask_t *new_cpumask = new_bp->cpumask;
> +
> +	/* Default to ALL CPUs if cpumask is not specified */
> +	if (!new_cpumask)
> +		new_cpumask = new_bp->cpumask = cpu_possible_mask;
> +	/*
> +	 * The user cannot modify the cpumask of the registered breakpoint
> +	 * It requires non-trivial amount of code and new data-structures to
> +	 * allow a change in cpumask value. The user must instead 'unregister'
> +	 * and re-register a new breakpoint if 'cpumask' should be changed
> +	 */
> +	if (!cpumask_equal(old_bp->cpumask, new_cpumask))
> +		return -EINVAL;
> +
> +	rc = arch_validate_hwbkpt_settings(new_bp, NULL);
> +	if (rc)
> +		return rc;
> +
> +	spin_lock_bh(&hw_breakpoint_lock);
> +	for_each_cpu(cpu, new_cpumask) {
> +		for (i = HBP_NUM-1; i >= hbp_kernel_pos; i--) {
> +			if (per_cpu(this_hbp_kernel[i], cpu) == old_bp) {
> +				per_cpu(this_hbp_kernel[i], cpu) = new_bp;
> +				break;
> +			}
> +		}
> +	}
> +
> +	if (cpumask_test_cpu(smp_processor_id(), new_cpumask))
> +		arch_update_kernel_hw_breakpoint(NULL);
> +	smp_call_function_many(new_cpumask,
> +				arch_update_kernel_hw_breakpoint, NULL, 1);
> +
> +	spin_unlock_bh(&hw_breakpoint_lock);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(modify_kernel_hw_breakpoint);
> +
>  /* Removes breakpoint structure from the per-cpu breakpoint data-structure */
>  static void remove_each_cpu_kernel_hbp(void *bp_param)
>  {
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists