Re: [PATCH] module: ppc64 module CRC relocation fix causes perf issues

On 07/17/2013 11:00:45 PM, Anton Blanchard wrote:

Hi Scott,

quoted

What specifically should I do to test it?

Could you double check perf annotate works? I'm 99% sure it will but
that is what was failing on ppc64.

I'm not really sure what it's supposed to look like when "perf  
annotate" works.  It spits a bunch of unreadable[1] dark-blue-on-black  
assembly code at me, all with "0.00 :" in the left column.

Oh, wait -- some lines have "100.00 : " on the left, in  
even-more-unreadable dark-red-on-black.

Apart from the annoying colors, is there anything specific I should be  
looking for?  Some sort of error message, or output that actually makes  
sense?

I've attached the output from "perf annotate" and "perf report".   
perf.data was generated by "perf record find /usr > /dev/null" on an  
NFS root (which took a few seconds to complete), so the large amount of  
__alloc_skb makes some sense, but the way perf annotate shows 100% on  
one instruction in each function seems odd.

-Scott

[1] ...unless I crank the brightness up on my monitor to the point  
where whites are blinding, or redirect the output to a file so the  
colors go away.

On Fri, Jul 19, 2013 at 05:59:30PM -0500, Scott Wood wrote:

On 07/17/2013 11:00:45 PM, Anton Blanchard wrote:

quoted

Hi Scott,

quoted

What specifically should I do to test it?

Could you double check perf annotate works? I'm 99% sure it will but
that is what was failing on ppc64.

I'm not really sure what it's supposed to look like when "perf
annotate" works.  It spits a bunch of unreadable[1]
dark-blue-on-black assembly code at me, all with "0.00 :" in the
left column.

Oh, wait -- some lines have "100.00 : " on the left, in
even-more-unreadable dark-red-on-black.

Apart from the annoying colors, is there anything specific I should
be looking for?  Some sort of error message, or output that actually
makes sense?

The colours look fine on my terminal, so I don't know what you've done
there. If you care you can use "--stdio" to use the plainer interface,
though it still uses colours.

That output looks fine in terms of the bug Anton was chasing. As far as
only ever hitting one instruction that does look weird.

cheers

Hi Scott,

I'm not really sure what it's supposed to look like when "perf  
annotate" works.  It spits a bunch of unreadable[1]
dark-blue-on-black assembly code at me, all with "0.00 :" in the left
column.

Oh, wait -- some lines have "100.00 : " on the left, in  
even-more-unreadable dark-red-on-black.

Apart from the annoying colors, is there anything specific I should
be looking for?  Some sort of error message, or output that actually
makes sense?

Thanks for testing! Ben, I think the patch is good to go.

Anton

On Thu, 2013-07-25 at 08:34 +1000, Anton Blanchard wrote:

quoted

Apart from the annoying colors, is there anything specific I should
be looking for?  Some sort of error message, or output that actually
makes sense?

Thanks for testing! Ben, I think the patch is good to go.

Sent it yesterday to Linus, it's upstream already :-)

Cheers,
Ben.

On Thu, Jul 25, 2013 at 09:14:25AM +1000, Benjamin Herrenschmidt wrote:

On Thu, 2013-07-25 at 08:34 +1000, Anton Blanchard wrote:

quoted

quoted

Apart from the annoying colors, is there anything specific I should
be looking for?  Some sort of error message, or output that actually
makes sense?

Thanks for testing! Ben, I think the patch is good to go.

Sent it yesterday to Linus, it's upstream already :-)

Cheers,
Ben.

Sorry I'm a bit late to the thread, I've ben swamped.  Has someone tested this
with kexec/kdump?  Thats why the origional patch was created, because when kexec
loads the kernel at a different physical address, the relocations messed with
the module crc's, and modules couldn't load during the kexec boot.  Assuming
that kernaddr_start gets set appropriately during boot, using PHYSICAL_START
should be fine, but I wanted to check, and don't currently have access to a
powerpc system to do so.
Neil

Hi Neil,

Sorry I'm a bit late to the thread, I've ben swamped.  Has someone
tested this with kexec/kdump?  Thats why the origional patch was
created, because when kexec loads the kernel at a different physical
address, the relocations messed with the module crc's, and modules
couldn't load during the kexec boot.  Assuming that kernaddr_start
gets set appropriately during boot, using PHYSICAL_START should be
fine, but I wanted to check, and don't currently have access to a
powerpc system to do so. Neil

I tested a relocatable kernel forced to run at a non zero physical
address (ie basically kdump). I verified CRCs were bad with your
original patch backed out, and were good with this patch applied.

Anton

On Fri, Jul 26, 2013 at 11:19:13AM +1000, Anton Blanchard wrote:

Hi Neil,

quoted

Sorry I'm a bit late to the thread, I've ben swamped.  Has someone
tested this with kexec/kdump?  Thats why the origional patch was
created, because when kexec loads the kernel at a different physical
address, the relocations messed with the module crc's, and modules
couldn't load during the kexec boot.  Assuming that kernaddr_start
gets set appropriately during boot, using PHYSICAL_START should be
fine, but I wanted to check, and don't currently have access to a
powerpc system to do so. Neil

I tested a relocatable kernel forced to run at a non zero physical
address (ie basically kdump). I verified CRCs were bad with your
original patch backed out, and were good with this patch applied.

Anton

Perfect, sounds like a sufficient test to me.

Acked-by: Neil Horman <nhorman@tuxdriver.com>