Re: [Patch 1/1] Introduce register_user_hbp_by_pid() and unregister_user_hbp_by_pid()

[Frederic Weisbecker <fweisbec@gmail.com>]

On Tue, Dec 22, 2009 at 12:16:31AM +0530, K.Prasad wrote:
> On Fri, Dec 18, 2009 at 09:47:48PM +0100, Frederic Weisbecker wrote:
> > > +
> > > +	do {
> > > +		mutex_lock(&start->perf_event_mutex);
> > > +		list_for_each_entry_safe(bp, temp_bp, &start->perf_event_list,
> > > +					 owner_entry) {
> > > +			if (bp->attr.type != PERF_TYPE_BREAKPOINT)
> > > +				continue;
> > > +			unregister_hw_breakpoint(bp);
> > > +			break;
> > 
> > Do you really want to unregister all of them? What about those
> > that may have been registered using perf syscall?
> > 
> 
> Seems that I got influenced heavily by the PPC64 design (was coding
> simultaneously for it :-)...will accept a "struct perf_event_attr *" and
> use that to identify appropriate breakpoint.


:)

 
> > And this function needs rcu too.
> > 
> > I don't see any in-kernel user for this new feature.
> > That would be required to integrate it.
> >
> 
> The proposed interfaces, as obvious, are mere wrappers over existing
> (un)register_user_* interfaces, and don't do anything vastly different
> in order to demonstrate them separately.
> 
> I can get a sample kernel module ready - that consumes pid and user-space
> address to track write accesses, if you prefer it.


Ok. The code looks good and useful.

But the usual philosophy in the kernel is to not add code
that is left unused upstream. And samples don't substitute a user.
I'm not sure this is a good idea to merge this.