Date:	Sat, 21 Mar 2009 22:56:18 +0530
From:	"K.Prasad" <prasad@...ux.vnet.ibm.com>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	Ingo Molnar <mingo@...e.hu>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Benjamin Herrenschmidt <benh@....ibm.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Maneesh Soni <maneesh@...ibm.com>,
	Roland McGrath <roland@...hat.com>,
	Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [Patch 01/11] Introducing generic hardware breakpoint handler
	interfaces

On Fri, Mar 20, 2009 at 10:33:26AM -0400, Alan Stern wrote:
> On Fri, 20 Mar 2009, K.Prasad wrote:
> 
> > This patch introduces two new files hw_breakpoint.[ch] which defines the 
> > generic interfaces to use hardware breakpoint infrastructure of the system. 
> 
> Prasad:
> 
> I'm sorry to say this is full of mistakes.  So far I have looked only 
> at patch 01/11, but it's not good.
> 

After you pointed out, I realise that the code in load_debug_registers()
is an overkill and unregister_kernel_hw_breakpoint() has an obvious 
error which should have caught my attention. My next revision should 
fix them.

> > + * Kernel breakpoints grow downwards, starting from HB_NUM
> > + * 'hbkpt_kernel_pos' denotes lowest numbered breakpoint register occupied for
> > + * kernel-space request
> > + */
> > +unsigned int hbkpt_kernel_pos;
> 
> This doesn't make much sense.  All you need to know is which registers
> are in use; all others are available.
> 

As explained by Maneesh earlier, we compact the kernel-space requests
into registers (HB_NUM - 1) to hbkpt_kernel_pos. The kernel-space
requests aren't specific to any given register number too, and so
compaction would be suitable for this case (unlike when implemented for
user-space which might need virtualisation of registers).

> For example, suppose the kernel allocated breakpoints 3, 2, and 1, and
> then deallocated 2.  Then bp 2 would be available for use, even though
> 2 > 1.
> 
> It's also a poor choice of name.  Everywhere else (in my patches,
> anyway) the code refers to hardware breakpoints using the abbreviation
> "hwbp" or "hw_breakpoint".  There's no reason suddenly to start using
> "hbkpt".
> 

I began using 'hbkpt' as a shorter naming convention (the longer one
being hw_breakpoint) without being really conscious of the 'hwbkpt'
usage by you (even some of the previous iterations contained them in
samples/hw_breakpoint and ftrace-plugin).

Well, I will rename my shorter name to 'hwbkpt' for uniformity.

> > +/* An array containing refcount of threads using a given bkpt register */
> > +unsigned int hbkpt_user_max_refcount[HB_NUM];
> 
> Why did you put "max" in the name?  Isn't this just a simple refcount?
> 

Ok. It will be hbkpt_user_refcount[].

> > +/* One higher than the highest counted user-space breakpoint register */
> > +unsigned int hbkpt_user_max;
> 
> Likewise, this variable isn't really needed.  It's just one more than
> the largest i such that hbkpt_user_max_refcount[i] > 0.
> 

It acts like a cache for determining the user-space breakpoint boundary.
It is used for sanity checks and in its absence we will have to compute from
hbkpt_user_max_refcount[] everytime.

> > +/*
> > + * Install the debug register values for a new thread.
> > + */
> > +void switch_to_thread_hw_breakpoint(struct task_struct *tsk)
> > +{
> > +	/* Set the debug register */
> 
> Set _which_ debug register?
> 

Will change it to read:
/* Set all debug registers used by 'tsk' */

> > +	arch_install_thread_hbkpt(tsk);
> > +	last_debugged_task = current;
> > +
> > +	put_cpu_no_resched();
> 
> What's this line doing here?  It looks like something you forgot to
> erase.
> 
> > +}
> > +
> > +/*
> > + * Install the debug register values for just the kernel, no thread.
> > + */
> > +void switch_to_none_hw_breakpoint(void)
> > +{
> > +	arch_install_none();
> > +	put_cpu_no_resched();
> 
> Same for this line.
> 

These are carriages from the previous code. They are still invoked from
the same places (such as flush_thread_hw_breakpoint(),
hw_breakpoint_handler()) and hence I didn't analyse it enough to see if
they were to be removed.

However, having found that preempt_count() is already zero at places where
these are called I think they can be removed.

> > +}
> > +
> > +/*
> > + * Load the debug registers during startup of a CPU.
> > + */
> > +void load_debug_registers(void)
> > +{
> > +	int i;
> > +	unsigned long flags;
> > +
> > +	/* Prevent IPIs for new kernel breakpoint updates */
> > +	local_irq_save(flags);
> > +
> > +	for (i = hbkpt_kernel_pos; i < HB_NUM; i++)
> > +		if (hbkpt_kernel[i])
> > +			on_each_cpu(arch_install_kernel_hbkpt,
> > +				(void *)hbkpt_kernel[i], 0);
> 
> This is completely wrong.  First of all, it's dumb to send multiple
> IPIs (one for each iteration through the loop).  Second, this routine
> shouldn't send any IPIs at all!  It gets invoked when a CPU is
> starting up and wants to load its _own_ debug registers -- not tell
> another CPU to load anything.
> 

As I agreed before, it is an overkill (given the design of
arch_install_kernel_hbkpt()). I will create a new
arch_update_kernel_hbkpt(pos, bp) that will install breakpoints only
on the CPU starting up.

> > +	if (current->thread.dr7)
> > +		arch_install_thread_hbkpt(current);
> > +
> > +	local_irq_restore(flags);
> > +}
> > +
> > +/*
> > + * Erase all the hardware breakpoint info associated with a thread.
> > + *
> > + * If tsk != current then tsk must not be usable (for example, a
> > + * child being cleaned up from a failed fork).
> > + */
> > +void flush_thread_hw_breakpoint(struct task_struct *tsk)
> > +{
> > +	int i;
> > +	struct thread_struct *thread = &(tsk->thread);
> > +
> > +	mutex_lock(&hw_breakpoint_mutex);
> > +
> > +	/* Let the breakpoints know they are being uninstalled */
> 
> This comment looks like a leftover which should have been erased.
> 
> > +/*
> > + * Validate the settings in a hw_breakpoint structure.
> > + */
> > +static int validate_settings(struct hw_breakpoint *bp, struct task_struct *tsk)
> > +{
> > +	int ret;
> > +	unsigned int align;
> > +
> > +	ret = arch_validate_hwbkpt_settings(bp, &align, tsk);
> > +	if (ret < 0)
> > +		goto err;
> > +
> > +	/* Check that the low-order bits of the address are appropriate
> > +	 * for the alignment implied by len.
> > +	 */
> > +	if (bp->info.address & align)
> > +		return -EINVAL;
> > +
> > +	/* Check that the virtual address is in the proper range */
> > +	if (tsk) {
> > +		if (!arch_check_va_in_userspace(bp->info.address, tsk))
> > +			return -EFAULT;
> > +	} else {
> > +		if (!arch_check_va_in_kernelspace(bp->info.address))
> > +			return -EFAULT;
> > +	}
> 
> Roland pointed out that these checks need to take into account the
> length of the breakpoint.  For example, in
> arch_check_va_in_userspace() it isn't sufficient for the start of the
> breakpoint region to be a userspace address; the end of the
> breakpoint region must also be in userspace.
> 

Ok. Will do something like:
return (va <= (TASK_SIZE - (hw_breakpoint_length * word_size)));

> > + err:
> > +	return ret;
> > +}
> > +
> > +int __register_user_hw_breakpoint(int pos, struct task_struct *tsk,
> > +					struct hw_breakpoint *bp)
> > +{
> > +	struct thread_struct *thread = &(tsk->thread);
> > +	int rc;
> > +
> > +	/* Do not overcommit. Fail if kernel has used the hbkpt registers */
> > +	if (pos >= hbkpt_kernel_pos)
> > +		return -ENOSPC;
> 
> In fact you should fail if the debug register is already in use,
> regardless of whether it is being used by a kernel breakpoint.  And you 
> shouldn't check against hbkpt_kernel_pos; you should check whether 
> hbkpt_kernel[pos] is NULL and thread->hbkpt[pos] is NULL.
> 

As explained before, the intended design was like this:

ample layout:
hbkpt_kernel_pos = 1
hbkpt_user_max = 1

---------------------------------------------------------------------
|                |                |                |                |
|       DR3      |       DR2      |       DR1      |       DR0      |
|                |                |                |                |
---------------------------------------------------------------------
^                                                  ^                ^
|                                                  |                |
-----------------kernel-space addresses-------------------user-------

After removing breakpoint in say DR2, compaction occurs.
New layout will be:
hbkpt_kernel_pos = 2
hbkpt_user_max = 1

---------------------------------------------------------------------
|                |                |                |                |
|       DR3      |       DR2      |       DR1      |       DR0      |
|                |                |                |                |
---------------------------------------------------------------------
^                                 ^                ^                ^
|                                 |                |                |
-----------------kernel------------------empty-----------user--------

The above design, in my opinion is intuitive, allows re-use of
uninstalled registers and is simple to implement.

What was missing in the sent patch was the updation of dr7 and dr[pos]
register after compaction. I will add them in the next iteration of the
patch.

> > +
> > +	rc = validate_settings(bp, tsk);
> > +	if (rc)
> > +		return rc;
> > +
> > +	thread->hbkpt[pos] = bp;
> > +	thread->hbkpt_num_installed++;
> > +	hbkpt_user_max_refcount[pos]++;
> > +	/* 'tsk' is the thread that uses max number of hbkpt registers */
> 
> This is a bad comment.  It sounds like it's saying that "tsk" is
> defined as the thread using the maximum number of breakpoints, rather
> than being defined as the thread for which the breakpoint is being
> registered.
> 
> Besides, there's no reason to keep track of which thread uses the max 
> number of breakpoints anyway.  Not to mention the fact that you don't 
> update hbkpt_user_max when its thread exits.
> 

We don't keep track of the thread (in the sense the task_struct) but
'hbkpt_user_max' is used for validating requests and book-keeping. As
Maneesh mentioned before flush_thread_hw_breakpoint() updates
'hbkpt_user_max'.

I can change it to read like the one below if it sounds better to you.

/* 
 * 'tsk' uses more number of registers than 'hbkpt_user_max'. Update
 * the same.
 */

> > +	if (hbkpt_user_max < thread->hbkpt_num_installed)
> > +		hbkpt_user_max++;
> 
> At this point I got tired of looking, but it seems obvious that the new 
> patch series needs a bunch of improvements.
> 
> Alan Stern
>

As mentioned before the next iteration would contain the changes I've
discussed above.

Thanks,
K.Prasad
 
--
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