linux-kernel - Re: [PATCH v3 41/44] metag: OProfile

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 for Android: free password hash cracker in your pocket

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

Message-ID: <50EFDEA8.2070505@imgtec.com>
Date:	Fri, 11 Jan 2013 09:43:04 +0000
From:	James Hogan <james.hogan@...tec.com>
To:	Maynard Johnson <maynardj@...ibm.com>
CC:	<linux-kernel@...r.kernel.org>, <linux-arch@...r.kernel.org>,
	<oprofile-list@...ts.sf.net>
Subject: Re: [PATCH v3 41/44] metag: OProfile

Hi Maynard,

On 10/01/13 17:12, Maynard Johnson wrote:
>> +static void kernel_backtrace_fp(unsigned long *fp, unsigned long *stack,
>> +				unsigned int depth)
>> +{
...
>> +#ifdef CONFIG_KALLSYMS
>> +		/* If we've reached TBIBoingVec then we're at an interrupt
>> +		 * entry point or a syscall entry point. The frame pointer
>> +		 * points to a pt_regs which can be used to continue tracing on
>> +		 * the other side of the boing.
>> +		 */
>> +		if (tbi_boing_size && addr >= tbi_boing_addr &&
>> +				addr < tbi_boing_addr + tbi_boing_size) {
>> +			struct pt_regs *regs = (struct pt_regs *)fp;
>> +			/* OProfile doesn't understand backtracing into
>> +			 * userland.
>> +			 */
> Since we can only get into kernel_backtrace_fp if user_mode(regs) == 0, why the if-statement?

Because this regs comes from the stack, so it could be a userland
context from the point of entry into the kernel.

>> +			if (!user_mode(regs) && --depth) {
>> +				oprofile_add_trace(regs->ctx.CurrPC);
>> +				metag_backtrace(regs, depth);
>> +			}
...
>> +
>> +/*
>> + * Unfortunately we don't have a native exception or interrupt for counter
>> + * overflow.
>> + *
>> + * OProfile on the other hand likes to have samples taken periodically, so
>> + * for now we just piggyback the timer interrupt to get the expected
>> + * behavior.
>> + */
>> +
> I presume an oprofile userspace patch is forthcoming.

Yes, unfortunately it's not included in our public buildroot tree yet,
but there's a slightly older patch (semantically identical) available in
the Pure ONE Flow release from http://www.pure.com/gpl/ in
metag-buildroot2/package/oprofile/oprofile-0.9.4-003-metag.patch and
also attached to this email.

It could probably do with some updating tbh though...

> As you probably know already, each event definition in oprofile
userspace requires a minimum 'count' value, which is the number of
events to occur before taking a sample.  With your userspace patch, you
should try to set min count values such that the fastest arrival rate
for the given event can be caught within (or near) one timer tick.

Yes, it appears the values are all left at 1000 in the patch which is
likely suboptimal.

>> +static int meta_timer_notify(struct pt_regs *regs)
>> +{
>> +	int i;
>> +	u32 val, total_val, sub_val;
>> +	u32 enabled_threads;
>> +
>> +	for (i = 0; i < NR_CNTRS; i++) {
>> +		if (!ctr[i].enabled)
>> +			continue;
>> +
>> +		/* Disable performance monitoring. */
>> +		enabled_threads = meta_read_counter(i);
>> +		meta_write_counter(i, 0);
>> +
>> +		sub_val = total_val = val = enabled_threads & PERF_COUNT_BITS;
>> +
>> +		if (val >= ctr[i].count) {
>> +			while (val > ctr[i].count) {
>> +				oprofile_add_sample(regs, i);
> I don't see a good reason for adding multiple samples using the same regs values.  As a matter of fact, it could really skew results under certain conditions.

I suspect the reasoning was to give more weight if the count is higher.
If that's not the usual way I'll change it.

Thanks for giving it a look.

Cheers
James

View attachment "oprofile-0.9.4-003-metag.patch" of type "text/x-patch" (4545 bytes)