[Patch 1/2] HW-BKPT: Allow per-cpu kernel-space Hardware Breakpoint requests

[Patch 1/2] HW-BKPT: Allow per-cpu kernel-space Hardware Breakpoint requests

[K.Prasad]

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

Allow kernel-space hw-breakpoints to be restricted only for a subset of
CPUs using a cpumask field in 'struct hw_breakpoint'.

Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 arch/x86/kernel/hw_breakpoint.c     |    6 +
 include/asm-generic/hw_breakpoint.h |    2 
 kernel/hw_breakpoint.c              |  119 +++++++++++++++++++++++++-----------
 3 files changed, 92 insertions(+), 35 deletions(-)

Index: linux-2.6-tip.hbkpt/arch/x86/kernel/hw_breakpoint.c
===================================================================
--- linux-2.6-tip.hbkpt.orig/arch/x86/kernel/hw_breakpoint.c
+++ linux-2.6-tip.hbkpt/arch/x86/kernel/hw_breakpoint.c
@@ -78,7 +78,7 @@ void arch_update_kernel_hw_breakpoint(vo
 	set_debugreg(0UL, 7);
 
 	for (i = hbp_kernel_pos; i < HBP_NUM; i++) {
-		per_cpu(this_hbp_kernel[i], cpu) = bp = hbp_kernel[i];
+		bp = per_cpu(this_hbp_kernel[i], cpu);
 		if (bp) {
 			temp_kdr7 |= encode_dr7(i, bp->info.len, bp->info.type);
 			set_debugreg(bp->info.address, i);
@@ -207,6 +207,10 @@ int arch_validate_hwbkpt_settings(struct
 	unsigned int align;
 	int ret = -EINVAL;
 
+	/* User-space breakpoints cannot be restricted to a subset of CPUs */
+	if (tsk && bp->cpumask)
+		return ret;
+
 	switch (bp->info.type) {
 	/*
 	 * Ptrace-refactoring code
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
@@ -8,6 +8,7 @@
 #ifdef	__KERNEL__
 #include <linux/list.h>
 #include <linux/types.h>
+#include <linux/cpumask.h>
 #include <linux/kallsyms.h>
 
 /**
@@ -102,6 +103,7 @@
  */
 struct hw_breakpoint {
 	void (*triggered)(struct hw_breakpoint *, struct pt_regs *);
+	const cpumask_t *cpumask;
 	struct arch_hw_breakpoint info;
 };
 
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
@@ -47,14 +47,7 @@
  */
 static DEFINE_SPINLOCK(hw_breakpoint_lock);
 
-/* Array of kernel-space breakpoint structures */
-struct hw_breakpoint *hbp_kernel[HBP_NUM];
-
-/*
- * Per-processor copy of hbp_kernel[]. Used only when hbp_kernel is being
- * modified but we need the older copy to handle any hbp exceptions. It will
- * sync with hbp_kernel[] value after updation is done through IPIs.
- */
+/* Per-cpu copy of HW-breakpoint structure */
 DEFINE_PER_CPU(struct hw_breakpoint*, this_hbp_kernel[HBP_NUM]);
 
 /*
@@ -72,6 +65,9 @@ unsigned int hbp_kernel_pos = HBP_NUM;
  */
 unsigned int hbp_user_refcount[HBP_NUM];
 
+/* An array denoting the number of consumed HW Breakpoints on each CPU */
+static DEFINE_PER_CPU(int, hbp_consumed);
+
 /*
  * Load the debug registers during startup of a CPU.
  */
@@ -294,6 +290,23 @@ void unregister_user_hw_breakpoint(struc
 }
 EXPORT_SYMBOL_GPL(unregister_user_hw_breakpoint);
 
+/* Update per-cpu instances of HW Breakpoint structure */
+static void update_each_cpu_kernel_hbp(void *bp_param)
+{
+	int i, cpu = get_cpu();
+	struct hw_breakpoint *bp = (struct hw_breakpoint *)bp_param;
+
+	for (i = HBP_NUM-1; i >= hbp_kernel_pos; i--) {
+		if (per_cpu(this_hbp_kernel[i], cpu))
+			continue;
+		per_cpu(this_hbp_kernel[i], cpu) = bp;
+		per_cpu(hbp_consumed, cpu)++;
+		break;
+	}
+	arch_update_kernel_hw_breakpoint(NULL);
+	put_cpu();
+}
+
 /**
  * register_kernel_hw_breakpoint - register a hardware breakpoint for kernel space
  * @bp: the breakpoint structure to register
@@ -305,27 +318,74 @@ EXPORT_SYMBOL_GPL(unregister_user_hw_bre
 int register_kernel_hw_breakpoint(struct hw_breakpoint *bp)
 {
 	int rc;
+	unsigned int cpu;
 
 	rc = arch_validate_hwbkpt_settings(bp, NULL);
 	if (rc)
 		return rc;
 
+	/* Default to ALL CPUs if cpumask is not specified */
+	if (!bp->cpumask)
+		bp->cpumask = cpu_possible_mask;
+
 	spin_lock_bh(&hw_breakpoint_lock);
 
-	rc = -ENOSPC;
-	/* Check if we are over-committing */
-	if ((hbp_kernel_pos > 0) && (!hbp_user_refcount[hbp_kernel_pos-1])) {
-		hbp_kernel_pos--;
-		hbp_kernel[hbp_kernel_pos] = bp;
-		on_each_cpu(arch_update_kernel_hw_breakpoint, NULL, 1);
-		rc = 0;
+	rc = -EINVAL;
+	for_each_cpu(cpu, bp->cpumask) {
+		/*
+		 * Check if we need a new slot of debug register in every CPU
+		 * i.e. if 'hbp_kernel_pos' needs to be decremented or if the
+		 * request can be serviced by consuming the vacant debug
+		 * registers
+		 */
+		if (per_cpu(hbp_consumed, cpu) == (HBP_NUM - hbp_kernel_pos)) {
+			/* Check if a new slot is available */
+			if ((hbp_kernel_pos == 0) ||
+			    (hbp_user_refcount[hbp_kernel_pos - 1] != 0)) {
+				rc = -ENOSPC;
+				goto err_ret;
+			} else {
+				hbp_kernel_pos--;
+				break;
+			}
+		}
 	}
 
+	if (cpumask_test_cpu(smp_processor_id(), bp->cpumask))
+		update_each_cpu_kernel_hbp(bp);
+	smp_call_function_many(bp->cpumask, update_each_cpu_kernel_hbp, bp, 1);
+	rc = 0;
+
+err_ret:
 	spin_unlock_bh(&hw_breakpoint_lock);
 	return rc;
 }
 EXPORT_SYMBOL_GPL(register_kernel_hw_breakpoint);
 
+/* Removes breakpoint structure from the per-cpu breakpoint data-structure */
+static void remove_each_cpu_kernel_hbp(void *bp_param)
+{
+	int i, j, cpu = get_cpu();
+	struct hw_breakpoint *bp  = (struct hw_breakpoint *)bp_param;
+
+	for (i = HBP_NUM-1; i >= hbp_kernel_pos; i--) {
+		if (per_cpu(this_hbp_kernel[i], cpu) == bp) {
+			/*
+			 * Shift the breakpoint structures by one-position
+			 * above to compact them
+			 */
+			for (j = i; j > hbp_kernel_pos; j--)
+				per_cpu(this_hbp_kernel[j], cpu) =
+					per_cpu(this_hbp_kernel[j-1], cpu);
+			per_cpu(this_hbp_kernel[hbp_kernel_pos], cpu) = NULL;
+			break;
+		}
+	}
+	per_cpu(hbp_consumed, cpu)--;
+	arch_update_kernel_hw_breakpoint(NULL);
+	put_cpu();
+}
+
 /**
  * unregister_kernel_hw_breakpoint - unregister a HW breakpoint for kernel space
  * @bp: the breakpoint structure to unregister
@@ -334,32 +394,23 @@ EXPORT_SYMBOL_GPL(register_kernel_hw_bre
  */
 void unregister_kernel_hw_breakpoint(struct hw_breakpoint *bp)
 {
-	int i, j;
+	int cpu;
 
 	spin_lock_bh(&hw_breakpoint_lock);
 
-	/* Find the 'bp' in our list of breakpoints for kernel */
-	for (i = hbp_kernel_pos; i < HBP_NUM; i++)
-		if (bp == hbp_kernel[i])
-			break;
-
-	/* Check if we did not find a match for 'bp'. If so return early */
-	if (i == HBP_NUM) {
-		spin_unlock_bh(&hw_breakpoint_lock);
-		return;
-	}
+	if (cpumask_test_cpu(smp_processor_id(), bp->cpumask))
+		remove_each_cpu_kernel_hbp(bp);
+	smp_call_function_many(bp->cpumask, remove_each_cpu_kernel_hbp, bp, 1);
+	for_each_possible_cpu(cpu)
+		if (per_cpu(hbp_consumed, cpu) == (HBP_NUM - hbp_kernel_pos))
+			goto ret_path;
 
-	/*
-	 * We'll shift the breakpoints one-level above to compact if
-	 * unregistration creates a hole
-	 */
-	for (j = i; j > hbp_kernel_pos; j--)
-		hbp_kernel[j] = hbp_kernel[j-1];
+	if (bp->cpumask == cpu_possible_mask)
+		bp->cpumask = NULL;
 
-	hbp_kernel[hbp_kernel_pos] = NULL;
-	on_each_cpu(arch_update_kernel_hw_breakpoint, NULL, 1);
 	hbp_kernel_pos++;
 
+ret_path:
 	spin_unlock_bh(&hw_breakpoint_lock);
 }
 EXPORT_SYMBOL_GPL(unregister_kernel_hw_breakpoint);

[Patch 2/2] HW-BKPT: Allow kernel breakpoints to be modified through a new API

[K.Prasad]

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

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@linux.vnet.ibm.com>
---
 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)
 {

Re: [Patch 2/2] HW-BKPT: Allow kernel breakpoints to be modified through a new API

[Frederic Weisbecker]

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@linux.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)
>  {
>