| 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
| ||
|
Message-ID: <20091105210652.GF4877@nowhere>
Date: Thu, 5 Nov 2009 22:06:55 +0100
From: Frederic Weisbecker <fweisbec@...il.com>
To: "K.Prasad" <prasad@...ux.vnet.ibm.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 09:04:04PM +0530, K.Prasad wrote:
> On Tue, Nov 03, 2009 at 08:11:12PM +0100, Frederic Weisbecker wrote:
> [snipped]
> >
> > /* Available HW breakpoint length encodings */
> > -#define HW_BREAKPOINT_LEN_1 0x40
> > -#define HW_BREAKPOINT_LEN_2 0x44
> > -#define HW_BREAKPOINT_LEN_4 0x4c
> > -#define HW_BREAKPOINT_LEN_EXECUTE 0x40
> > +#define X86_BREAKPOINT_LEN_1 0x40
> > +#define X86_BREAKPOINT_LEN_2 0x44
> > +#define X86_BREAKPOINT_LEN_4 0x4c
> > +#define X86_BREAKPOINT_LEN_EXECUTE 0x40
> >
>
> It had previously been suggested http://lkml.org/lkml/2009/5/28/554
> that users be allowed to specify the lengths in numerals. Despite having
> some divergent views initially, I see that it would help minimise the
> amount of code required to request a breakpoint if numerals (such as 1,
> 2, 4 and 8 for x86_64) are allowed.
>
> The conversion to encoded values can happen later inside the
> bkpt-specific code.
That's what I did, I've redefined them in linux/hw_breakpoint.h:
#define HW_BREAKPOINT_LEN_1 1
#define HW_BREAKPOINT_LEN_2 2
#define HW_BREAKPOINT_LEN_4 4
And the arch interpret that using its own corresponding values.
> > --- a/include/asm-generic/hw_breakpoint.h
> > +++ /dev/null
>
> Can you split this patch into fine granular ones? It is very difficult
> to review the changes this way.
Sure, I personally don't like either this big monolithic patch, but
it is hard/impossible to split it as we change the whole base of a
subsystem inside.
But this header moving has been done in the v2 and I thought git-format-patch
would detect the rename but the file has probably too much changed.
I'll do another iteration that split up this part.
> > diff --git a/include/linux/hw_breakpoint.h b/include/linux/hw_breakpoint.h
> > new file mode 100644
> > index 0000000..7eba9b9
> > --- /dev/null
> > +++ b/include/linux/hw_breakpoint.h
>
> Have you clubbed file renaming along with changes inside the file?
> Again, it'd be good to have them in separate patches for easy review.
There have been this rename only. But I'll split up this part.
> > +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> > +extern struct perf_event *
> > +register_user_hw_breakpoint(unsigned long addr,
> > + int len,
> > + int type,
> > + perf_callback_t triggered,
> > + struct task_struct *tsk,
> > + bool active);
> > +
>
> I don't understand the benefit behind bringing these parameters into the
> interfaces' prototype. Besides they will make addition of new attributes
> (if needed later) quite cumbersome. Given that these values are
> eventually copied into members of perf_event_attr, I'd suggest that they
> accept a pointer to an instance of the structure.
Yeah, that's a bit intended as a temporary thing. The preffered
way for that would be to pass a pointer to a perf_event_attr
structure.
I plan to do this change incrementally, once we have defined
breakpoints attributes generic enough to support most archs
possibilities.
> > +/* FIXME: only change from the attr, and don't unregister */
> > +extern 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);
> > +
> > +/*
> > + * 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.
> > -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.
> <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.
> Have you tested these changes from perf-events' user-space command?
> Would you like to re-use the patches from here:
> http://lkml.org/lkml/2009/10/29/304 to test them?
Yeah, I have planned to reuse your patches for the perf subcommand
support :)
--
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