Re: [1/2] PATCH Kernel watchpoint interface-2.6.9-rc4-mm1

[Andi Kleen]

> +config DEBUGREG
> +	bool "Global Debug Registers"

I agree with Keith that it shouldn't be user visible. I would always
enable it in fact.

> +{
> +	int i;
> +	if (flag == DR_ALLOC_LOCAL) {

[...] This all would be simpler if you used lib/idr.c, no?

> +int dr_free(int regnum)
> +{
> +	spin_lock(&dr_lock);
> +	if (regnum >= DR_MAX || dr_list[regnum].flag == DR_UNUSED) {
> +		spin_unlock(&dr_lock);
> +		return -1;

This should printk

> +#ifdef CONFIG_DEBUGREG
> +{
> +	/*
> +	 * Don't reload global debug registers. Don't touch the global debug
> +	 * register settings in dr7.
> +	 */
> +	unsigned long next_dr7 = next->debugreg[7];
> +	if (unlikely(next_dr7)) {
> +		if (DR7_L0(next_dr7)) loaddebug(next, 0);
> +		if (DR7_L1(next_dr7)) loaddebug(next, 1);
> +		if (DR7_L2(next_dr7)) loaddebug(next, 2);
> +		if (DR7_L3(next_dr7)) loaddebug(next, 3);

I would do this differently - check instead if the registers
are different between the tasks and only reload when different.
This will make updating/freeing more expensive because
you will need to change all tasks, but imho it's worth it.

And then no ifdefs please.

>  	 */
>  clear_dr7:
> -	__asm__("movl %0,%%db7"
> -		: /* no output */
> -		: "r" (0));
> +	load_process_dr7(0);
>  	CHK_REMOTE_DEBUG(1,SIGTRAP,error_code,regs,)

That's mm (and should go away anyways because debug notifiers are better) 
I would do the patch against mainline so that it can be actually merged.

-Andi
-
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/