* [Patch 1/3] HW-BKPT: Allow per-cpu kernel-space Hardware Breakpoint requests
[not found] <20090826200840.118253312@linux.vnet.ibm.com>
@ 2009-08-26 20:14 ` K.Prasad
2009-08-26 20:14 ` [Patch 2/3] HW-BKPT: Allow kernel breakpoints to be modified through a new API K.Prasad
2009-08-26 20:15 ` [Patch 3/3] HW-BKPT: Enable/disable the breakpoints when still registered K.Prasad
2 siblings, 0 replies; 6+ messages in thread
From: K.Prasad @ 2009-08-26 20:14 UTC (permalink / raw)
To: LKML, Frederic Weisbecker
Cc: Ingo Molnar, Peter Zijlstra, Lai Jiangshan, Steven Rostedt,
Mathieu Desnoyers, Alan Stern, 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);
^ permalink raw reply [flat|nested] 6+ messages in thread* [Patch 2/3] HW-BKPT: Allow kernel breakpoints to be modified through a new API
[not found] <20090826200840.118253312@linux.vnet.ibm.com>
2009-08-26 20:14 ` [Patch 1/3] HW-BKPT: Allow per-cpu kernel-space Hardware Breakpoint requests K.Prasad
@ 2009-08-26 20:14 ` K.Prasad
2009-08-26 20:15 ` [Patch 3/3] HW-BKPT: Enable/disable the breakpoints when still registered K.Prasad
2 siblings, 0 replies; 6+ messages in thread
From: K.Prasad @ 2009-08-26 20:14 UTC (permalink / raw)
To: LKML, Frederic Weisbecker
Cc: Ingo Molnar, Peter Zijlstra, Lai Jiangshan, Steven Rostedt,
Mathieu Desnoyers, Alan Stern, K.Prasad
[-- Attachment #1: modify_kernel_hbp_02 --]
[-- Type: text/plain, Size: 3109 bytes --]
Introduce modify_kernel_hw_breakpoint() API that can quickly change the
characteristics (such as address, length, type) 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)
{
^ permalink raw reply [flat|nested] 6+ messages in thread* [Patch 3/3] HW-BKPT: Enable/disable the breakpoints when still registered
[not found] <20090826200840.118253312@linux.vnet.ibm.com>
2009-08-26 20:14 ` [Patch 1/3] HW-BKPT: Allow per-cpu kernel-space Hardware Breakpoint requests K.Prasad
2009-08-26 20:14 ` [Patch 2/3] HW-BKPT: Allow kernel breakpoints to be modified through a new API K.Prasad
@ 2009-08-26 20:15 ` K.Prasad
2009-08-27 5:55 ` Ananth N Mavinakayanahalli
2 siblings, 1 reply; 6+ messages in thread
From: K.Prasad @ 2009-08-26 20:15 UTC (permalink / raw)
To: LKML, Frederic Weisbecker
Cc: Ingo Molnar, Peter Zijlstra, Lai Jiangshan, Steven Rostedt,
Mathieu Desnoyers, Alan Stern, K.Prasad
[-- Attachment #1: enable_disable_patch_03 --]
[-- Type: text/plain, Size: 4495 bytes --]
Allow breakpoints to be enabled/disabled without yielding the
breakpoint request through a new API - enable_hw_breakpoint().
Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
arch/x86/kernel/hw_breakpoint.c | 12 ++++++----
include/asm-generic/hw_breakpoint.h | 9 +++++++
kernel/hw_breakpoint.c | 41 ++++++++++++++++++++++++++++++++++++
3 files changed, 57 insertions(+), 5 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
@@ -79,10 +79,11 @@ void arch_update_kernel_hw_breakpoint(vo
for (i = hbp_kernel_pos; i < HBP_NUM; i++) {
bp = per_cpu(this_hbp_kernel[i], cpu);
- if (bp) {
+ if (!bp)
+ continue;
+ set_debugreg(bp->info.address, i);
+ if (bp->enabled)
temp_kdr7 |= encode_dr7(i, bp->info.len, bp->info.type);
- set_debugreg(bp->info.address, i);
- }
}
/* No need to set DR6. Update the debug registers with kernel-space
@@ -282,8 +283,9 @@ void arch_update_user_hw_breakpoint(int
thread->debugreg7 &= ~dr7_masks[pos];
if (bp) {
thread->debugreg[pos] = bp->info.address;
- thread->debugreg7 |= encode_dr7(pos, bp->info.len,
- bp->info.type);
+ if (bp->enabled)
+ thread->debugreg7 |= encode_dr7(pos, bp->info.len,
+ bp->info.type);
} else
thread->debugreg[pos] = 0;
}
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
@@ -102,6 +102,12 @@
* ----------------------------------------------------------------------
*/
struct hw_breakpoint {
+ /*
+ * Denotes if a breakpoint is currently enabled in physical debug
+ * registers. Not to be set directly by the end-user. Must be
+ * operated through enable_hw_breakpoint() API only.
+ */
+ unsigned int enabled;
void (*triggered)(struct hw_breakpoint *, struct pt_regs *);
const cpumask_t *cpumask;
struct arch_hw_breakpoint info;
@@ -137,6 +143,9 @@ extern int modify_kernel_hw_breakpoint(s
struct hw_breakpoint *new_bp);
extern void unregister_kernel_hw_breakpoint(struct hw_breakpoint *bp);
+extern void enable_hw_breakpoint(struct hw_breakpoint *bp,
+ struct task_struct *tsk, unsigned int enabled);
+
extern unsigned int hbp_kernel_pos;
#endif /* __KERNEL__ */
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
@@ -351,6 +351,7 @@ int register_kernel_hw_breakpoint(struct
}
}
+ bp->enabled = 1;
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);
@@ -464,6 +465,46 @@ ret_path:
}
EXPORT_SYMBOL_GPL(unregister_kernel_hw_breakpoint);
+/**
+ * enable_hw_breakpoint - enable/disable a previous registered breakpoint
+ * @bp: the breakpoint structure to unregister
+ * @tsk: pointer to 'task_struct' of the process (for user-space breakpoints)
+ * @enabled: zero to disable, any positive integer to enable the breakpoint
+ *
+ * Enable or disable a breakpoint without actually losing the registration
+ */
+void enable_hw_breakpoint(struct hw_breakpoint *bp, struct task_struct *tsk,
+ unsigned int enabled)
+{
+ int i;
+ struct thread_struct *thread = &(tsk->thread);
+
+ spin_lock_bh(&hw_breakpoint_lock);
+
+ bp->enabled = enabled;
+ /* Enable/Disable the kernel-space breakpoint */
+ if (!tsk) {
+ if (cpumask_test_cpu(smp_processor_id(), bp->cpumask))
+ arch_update_kernel_hw_breakpoint(NULL);
+ smp_call_function_many(bp->cpumask,
+ arch_update_kernel_hw_breakpoint, NULL, 1);
+ goto out;
+ }
+
+ /* Enable/disable the user-space breakpoint */
+ for (i = 0; i < hbp_kernel_pos; i++) {
+ if (thread->hbp[i] != bp)
+ continue;
+ arch_update_user_hw_breakpoint(i, tsk);
+ if (tsk == current)
+ arch_install_thread_hw_breakpoint(tsk);
+ break;
+ }
+out:
+ spin_unlock_bh(&hw_breakpoint_lock);
+}
+EXPORT_SYMBOL_GPL(enable_hw_breakpoint);
+
static struct notifier_block hw_breakpoint_exceptions_nb = {
.notifier_call = hw_breakpoint_exceptions_notify,
/* we need to be notified first */
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Patch 3/3] HW-BKPT: Enable/disable the breakpoints when still registered
2009-08-26 20:15 ` [Patch 3/3] HW-BKPT: Enable/disable the breakpoints when still registered K.Prasad
@ 2009-08-27 5:55 ` Ananth N Mavinakayanahalli
2009-08-28 19:06 ` K.Prasad
0 siblings, 1 reply; 6+ messages in thread
From: Ananth N Mavinakayanahalli @ 2009-08-27 5:55 UTC (permalink / raw)
To: K.Prasad
Cc: LKML, Frederic Weisbecker, Ingo Molnar, Peter Zijlstra,
Lai Jiangshan, Steven Rostedt, Mathieu Desnoyers, Alan Stern
On Thu, Aug 27, 2009 at 01:45:06AM +0530, K.Prasad wrote:
...
> struct hw_breakpoint {
> + /*
> + * Denotes if a breakpoint is currently enabled in physical debug
> + * registers. Not to be set directly by the end-user. Must be
> + * operated through enable_hw_breakpoint() API only.
> + */
> + unsigned int enabled;
bool?
...
> +void enable_hw_breakpoint(struct hw_breakpoint *bp, struct task_struct *tsk,
> + unsigned int enabled)
> +{
> + int i;
> + struct thread_struct *thread = &(tsk->thread);
> +
> + spin_lock_bh(&hw_breakpoint_lock);
> +
> + bp->enabled = enabled;
> + /* Enable/Disable the kernel-space breakpoint */
> + if (!tsk) {
> + if (cpumask_test_cpu(smp_processor_id(), bp->cpumask))
> + arch_update_kernel_hw_breakpoint(NULL);
> + smp_call_function_many(bp->cpumask,
> + arch_update_kernel_hw_breakpoint, NULL, 1);
> + goto out;
> + }
> +
> + /* Enable/disable the user-space breakpoint */
> + for (i = 0; i < hbp_kernel_pos; i++) {
> + if (thread->hbp[i] != bp)
> + continue;
> + arch_update_user_hw_breakpoint(i, tsk);
> + if (tsk == current)
> + arch_install_thread_hw_breakpoint(tsk);
> + break;
> + }
> +out:
> + spin_unlock_bh(&hw_breakpoint_lock);
> +}
> +EXPORT_SYMBOL_GPL(enable_hw_breakpoint);
Not sure if its cleaner to have enable_hw_breakpoint() and
disable_hw_breakpoint() rather than one overloaded call.
Ananth
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Patch 3/3] HW-BKPT: Enable/disable the breakpoints when still registered
2009-08-27 5:55 ` Ananth N Mavinakayanahalli
@ 2009-08-28 19:06 ` K.Prasad
0 siblings, 0 replies; 6+ messages in thread
From: K.Prasad @ 2009-08-28 19:06 UTC (permalink / raw)
To: Ananth N Mavinakayanahalli
Cc: LKML, Frederic Weisbecker, Ingo Molnar, Peter Zijlstra,
Lai Jiangshan, Steven Rostedt, Mathieu Desnoyers, Alan Stern
On Thu, Aug 27, 2009 at 11:25:08AM +0530, Ananth N Mavinakayanahalli wrote:
> On Thu, Aug 27, 2009 at 01:45:06AM +0530, K.Prasad wrote:
>
> ...
>
> > struct hw_breakpoint {
> > + /*
> > + * Denotes if a breakpoint is currently enabled in physical debug
> > + * registers. Not to be set directly by the end-user. Must be
> > + * operated through enable_hw_breakpoint() API only.
> > + */
> > + unsigned int enabled;
>
> bool?
>
> ...
>
A 'bool' data-type doesn't really provide advantages in terms of the
bytes consumed as far as I can see...leaving it as 'unsigned int' would
allow any positive integer to be considered as a 'enable' request.
However considering the change due to the second comment, and for the
fact that the data-type is more intuitive I'll change them to 'bool'.
> > +void enable_hw_breakpoint(struct hw_breakpoint *bp, struct task_struct *tsk,
> > + unsigned int enabled)
> > +{
> > + int i;
> > + struct thread_struct *thread = &(tsk->thread);
> > +
> > + spin_lock_bh(&hw_breakpoint_lock);
> > +
> > + bp->enabled = enabled;
> > + /* Enable/Disable the kernel-space breakpoint */
> > + if (!tsk) {
> > + if (cpumask_test_cpu(smp_processor_id(), bp->cpumask))
> > + arch_update_kernel_hw_breakpoint(NULL);
> > + smp_call_function_many(bp->cpumask,
> > + arch_update_kernel_hw_breakpoint, NULL, 1);
> > + goto out;
> > + }
> > +
> > + /* Enable/disable the user-space breakpoint */
> > + for (i = 0; i < hbp_kernel_pos; i++) {
> > + if (thread->hbp[i] != bp)
> > + continue;
> > + arch_update_user_hw_breakpoint(i, tsk);
> > + if (tsk == current)
> > + arch_install_thread_hw_breakpoint(tsk);
> > + break;
> > + }
> > +out:
> > + spin_unlock_bh(&hw_breakpoint_lock);
> > +}
> > +EXPORT_SYMBOL_GPL(enable_hw_breakpoint);
>
> Not sure if its cleaner to have enable_hw_breakpoint() and
> disable_hw_breakpoint() rather than one overloaded call.
>
The plan was to have two separate interfaces at first, but then it
turned out that the code was largely similar...tempting them to be
clubbed together.
But after looking at 'struct pmu' (whose callbacks are intended to map
to these interfaces), I see that two distinct interfaces would be
better (there's a enable/disable callback in struct pmu too).
I shall post a new version of this patch including more changes.
Thanks,
K.Prasad
^ permalink raw reply [flat|nested] 6+ messages in thread