linux-kernel - Re: [PATCH 4/6] hw-breakpoints: Rewrite the hw-breakpoints layer on top of perf events

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 8 Nov 2009 23:02:05 +0530
From:	"K.Prasad" <prasad@...ux.vnet.ibm.com>
To:	Frederic Weisbecker <fweisbec@...il.com>
Cc:	Ingo Molnar <mingo@...e.hu>, LKML <linux-kernel@...r.kernel.org>,
	Alan Stern <stern@...land.harvard.edu>,
	Peter Zijlstra <peterz@...radead.org>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Jan Kiszka <jan.kiszka@....de>,
	Jiri Slaby <jirislaby@...il.com>,
	Li Zefan <lizf@...fujitsu.com>, Avi Kivity <avi@...hat.com>,
	Paul Mackerras <paulus@...ba.org>,
	Mike Galbraith <efault@....de>,
	Masami Hiramatsu <mhiramat@...hat.com>,
	Paul Mundt <lethal@...ux-sh.org>
Subject: Re: [PATCH 4/6] hw-breakpoints: Rewrite the hw-breakpoints layer
	on top of perf events

On Thu, Nov 05, 2009 at 10:06:55PM +0100, Frederic Weisbecker wrote:
> On Thu, Nov 05, 2009 at 09:04:04PM +0530, K.Prasad wrote:
> > On Tue, Nov 03, 2009 at 08:11:12PM +0100, Frederic Weisbecker wrote:
> > > +/*
> > > + * Kernel breakpoints are not associated with any particular thread.
> > > + */
> > > +extern struct perf_event *
> > > +register_wide_hw_breakpoint_cpu(unsigned long addr,
> > > +				int len,
> > > +				int type,
> > > +				perf_callback_t triggered,
> > > +				int cpu,
> > 
> > Can't it be cpumask_t instead of int cpu? Given that per-cpu breakpoints
> > will be implemented, it should be very different to implement them for a
> > subset of cpus too.
> 
> I can't figure out any usecase where we want to only bind to,
> say, cpu 1 and 3 or any kind of such strange combination.
> 
> Either we want a wide breakpoint, or we want to profile
> a single cpu, but I don't imagine we need a middle case.
 
When we originally had this discussion on LKML, one of the use-cases
cited was http://lkml.org/lkml/2009/7/29/243. I can't see why such
need should be restricted to a given CPU only, rather than a subset of
CPUs (say 'x' is a variable normally read/written-to in the interrupt
path, and if the said interrupt is has a cpu affinity to a subset of
cpus only).

Although in the normal case, this feature could be implemented later, in
case of breakpoints we accept that as input from the user (and hence
part of the well-defined interface), so it is better to design it for
a subset of CPUs from start. The logic isn't very different and given that
there are plenty of helper routines in cpumask.h the implementation is easy too.

> 
> > > -static DEFINE_SPINLOCK(hw_breakpoint_lock);
> > 
> > Wouldn't you want to hold this lock while checking for system-wide
> > availability of debug registers (during rollbacks) to avoid contenders
> > from checking for the same simultaneously?
> 
> 
> If we want to lock such path, we probably more likely want a mutex.
> Registering a breakpoint is not a fastpath and also perf does
> some sleepable things while creating a counter.
> 
> The check to register constraints, which is part of this path,
> is itself a mutex.
> 
> But we'll probably need something NMI safe in the future so
> that it can be used without any problem by kgdb.
> 

I suspect that it will be required for cpu-hotplug handler, where
previously load_debug_registers() was called from a softirq context.

> 
> > <snipped>
> > 
> > > -int register_kernel_hw_breakpoint(struct hw_breakpoint *bp)
> > > +struct perf_event **
> > > +register_wide_hw_breakpoint(unsigned long addr,
> > > +			    int len,
> > > +			    int type,
> > > +			    perf_callback_t triggered,
> > > +			    bool active)
> > >  {
> > > -	int rc;
> > > +	struct perf_event **cpu_events, **pevent, *bp;
> > > +	long err;
> > > +	int cpu;
> > > +
> > > +	cpu_events = alloc_percpu(typeof(*cpu_events));
> > > +	if (!cpu_events)
> > > +		return ERR_PTR(-ENOMEM);
> > > 
> > > -	rc = arch_validate_hwbkpt_settings(bp, NULL);
> > > -	if (rc)
> > > -		return rc;
> > > +	for_each_possible_cpu(cpu) {
> > > +		pevent = per_cpu_ptr(cpu_events, cpu);
> > > +		bp = register_kernel_hw_breakpoint_cpu(addr, len, type,
> > > +					triggered, cpu, active);
> > > 
> > 
> > I'm assuming that there'd be an implementation for system-wide
> > perf-events (and hence breakpoints) in the forthcoming version(s) of
> > this patchset.
> 
> 
> If that becomes a necessary feature, then yeah.
> 
> 

Apart from the several benefits of having system-wide perf-events,
implementing them in the first iteration itself will
help us fully realise the cost of perf-events + hw-breakpoint
integration! When implemented, perf-events will also be ready to
accomodate future users (apart from bp and perf-top) having a
need for system-wide counter.

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/