Re: [PATCH 5/7] hw-breakpoints: Rewrite the hw-breakpoints layeron top of perf events

From: K.Prasad
Date: Sun Nov 08 2009 - 12:31:30 EST


> #include <asm/smpboot_hooks.h>
> @@ -328,7 +327,6 @@ notrace static void __cpuinit start_secondary(void *unused)
> x86_cpuinit.setup_percpu_clockev();
>
> wmb();
> - load_debug_registers();
> cpu_idle();
> }
>

So, will the breakpoint values be stored in per-cpu variables and be
restored when a cpu (from the list of for_each_possible_cpu) eventually
turns online due to cpu-hotplug or resume from hibernate (as originally
done by load_debug_registers())?

A few more observations....

int reserve_bp_slot(struct perf_event *bp)
{
...
....
if (!bp->attr.pinned) {
/*
* If there are already flexible counters here,
* there is at least one slot reserved for all
* of them. Just join the party.
*
* Otherwise, check there is at least one free slot
*/
if (!slots.flexible && slots.pinned == HBP_NUM) {
ret = -ENOSPC;
goto end;
}

/* Flexible counters need to keep at least one slot */
} else if (slots.pinned + (!!slots.flexible) == HBP_NUM) {
ret = -ENOSPC;
goto end;
}
..
...
}

It appears that you're reserving one slot for the non-pinned breakpoint
requests, which I'm afraid wouldn't play well with PPC64 (having one
DABR). Even with x86, I can't understand why we should allow the use of
only 3 registers for ptrace requests (which are all pinned) on every
system even if perf-events will never be used on them.


int __register_perf_hw_breakpoint(struct perf_event *bp)
{
..
...
if (!bp->attr.disabled)
ret = arch_validate_hwbkpt_settings(bp, bp->ctx->task);

return ret;
}

Can't the arch_validate_() check be done unconditionally?

struct perf_event *
modify_user_hw_breakpoint(struct perf_event *bp,
unsigned long addr,
int len,
int type,
perf_callback_t triggered,
struct task_struct *tsk,
bool active)
{
/*
* FIXME: do it without unregistering
* - We don't want to lose our slot
* - If the new bp is incorrect, don't lose the older one
*/
unregister_hw_breakpoint(bp);

return register_user_hw_breakpoint(addr, len, type, triggered,
tsk, active);
}

Given that modify_user_hw_breakpoint() is used by ptrace, there's a risk
of breaking/changing ptrace's behaviour i.e. new ptrace failure
scenarios (while modifying DR0-DR3) which may not be acceptable to
user-space applications. I presume that you intend to address the FIXME
during the next few iterations itself...

void arch_uninstall_hw_breakpoint(struct perf_event *bp)
{
...
....
dr7 = &__get_cpu_var(dr7);
*dr7 &= ~encode_dr7(i, info->len, info->type);

set_debugreg(*dr7, 7);
}

The &= ~encode_dr7 would unset the GE flag in DR7 (encoded as
DR_GLOBAL_SLOWDOWN) which, I think is unintended.

struct pmu perf_ops_bp = {
.enable = arch_install_hw_breakpoint,
.disable = arch_uninstall_hw_breakpoint,
.read = hw_breakpoint_pmu_read,
.unthrottle = hw_breakpoint_pmu_unthrottle
};

So the enable/disable points to those routines that clear the debug
register off its breakpoint values but still don't seem to release the
slot (even temporarily)....i.e. nr_bp_flexible is unchanged
Will perf-events automatically re-use the disabled slot for breakpoint
requests from other perf-event instances?

Thanks,
K.Prasad

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