OProfile

module/x86/op_model_ppro.c is missing a:

#include "op_msr.h"

I will try it out on the p3 and athlon machines.

-Will



graydon@... wrote:
> hi,
> 
> attached is another go at the model-specific x86 backend cleanup
> patch, in further preparation for landing the P4 patch. it applies to
> the cvs head, builds and runs on my P4 with RTC mode; but that's not
> saying much. If someone with a ppro / athlon could confirm the changes
> did not disrupt those backends I'd appreciate it. there is some chance
> of that, with the moving-around of things.
> 
> differences from last round of this patch:
> 
>   - some more constification
>   - fixed some spacing / formatting
>   - removed model-fetch call from within NMI, assume it's set
>   - move MSR addresses to contiguous arrays to remain more 
>     cache friendly during frequent NMIs
> 
> any further comments / suggestions?
> 
> -graydon

At Fri, 20 Sep 2002 14:34:20 -0400,
William Cohen wrote:

> module/x86/op_model_ppro.c is missing a:
> 
> #include "op_msr.h"

Ha! nice catch. it *is* missing, but I wondered how it could possibly
compile without that. it seems that

/usr/src/linux-2.4.19/include/asm/msr.h

includes these lines too:

#define MSR_P6_PERFCTR0                 0xc1
#define MSR_P6_PERFCTR1                 0xc2
#define MSR_P6_EVNTSEL0                 0x186
#define MSR_P6_EVNTSEL1                 0x187

should we change the #defines we're using, or should I just put in the
apporiate #include "op_msr.h" and have it not-override the kernel
definitions?

-graydon

graydon@... wrote:
> At Fri, 20 Sep 2002 14:34:20 -0400,
> William Cohen wrote:
> 
> 
>>module/x86/op_model_ppro.c is missing a:
>>
>>#include "op_msr.h"
> 
> 
> Ha! nice catch. it *is* missing, but I wondered how it could possibly
> compile without that. it seems that
> 
> /usr/src/linux-2.4.19/include/asm/msr.h
> 
> includes these lines too:
> 
> #define MSR_P6_PERFCTR0                 0xc1
> #define MSR_P6_PERFCTR1                 0xc2
> #define MSR_P6_EVNTSEL0                 0x186
> #define MSR_P6_EVNTSEL1                 0x187
> 
> should we change the #defines we're using, or should I just put in the
> apporiate #include "op_msr.h" and have it not-override the kernel
> definitions?
> 

just include op_msr.h, all #defines in this file are on the form:

#ifndef MSR_P6_PERFCTR0
#define MSR_P6_PERFCTR0 0xc1
#endif

It's less annoying to reuse the same name

I'll take care about compatibility with previous kernel
version (but only on un-patched kernel)

regardss,
Phil

One of the concerns is being able to compile this on older kernels. The 
2.4.18 kernel doesn't appear to have the MSR_P6_* defined. The code 
needs to compile on earlier versions of the kernel. If the values are 
the same (they should be), I would leave the names the same.

I tried the patch on P3 and athlon and driver didn't work. :( I verified 
that the unpatched version of oprofile I used worked, so there is 
something in the patch that isn't right. I don't know what right now.

-Will

graydon@... wrote:
> At Fri, 20 Sep 2002 14:34:20 -0400,
> William Cohen wrote:
> 
> 
>>module/x86/op_model_ppro.c is missing a:
>>
>>#include "op_msr.h"
> 
> 
> Ha! nice catch. it *is* missing, but I wondered how it could possibly
> compile without that. it seems that
> 
> /usr/src/linux-2.4.19/include/asm/msr.h
> 
> includes these lines too:
> 
> #define MSR_P6_PERFCTR0                 0xc1
> #define MSR_P6_PERFCTR1                 0xc2
> #define MSR_P6_EVNTSEL0                 0x186
> #define MSR_P6_EVNTSEL1                 0x187
> 
> should we change the #defines we're using, or should I just put in the
> apporiate #include "op_msr.h" and have it not-override the kernel
> definitions?
> 
> -graydon
>

graydon@... wrote:
> hi,
> 
> attached is another go at the model-specific x86 backend cleanup
> patch, in further preparation for landing the P4 patch. it applies to
> the cvs head, builds and runs on my P4 with RTC mode; but that's not
> saying much. If someone with a ppro / athlon could confirm the changes
> did not disrupt those backends I'd appreciate it. there is some chance
> of that, with the moving-around of things.
> 
> differences from last round of this patch:
> 
>   - some more constification
>   - fixed some spacing / formatting
>   - removed model-fetch call from within NMI, assume it's set
>   - move MSR addresses to contiguous arrays to remain more 
>     cache friendly during frequent NMIs
> 
> any further comments / suggestions?
> 

> --- module/x86/op_nmi.c	Sat Sep  7 14:19:39 2002
> +++ module/x86/op_nmi.c	Fri Sep 20 10:49:55 2002

[...]


> +static int pmc_init(void)
> +{
> +	int err = 0;	
> +	if ((err = smp_call_function(pmc_save_registers, NULL, 0, 1))) {
> +		smp_call_function(pmc_restore_registers, NULL, 0, 1);

don't restore: if save fail, save can be done only partially and anyway
we don't change anything in msr.


But I think it doesn't explain my problem at module load:

general protection fault: 0000
CPU:    0
EIP:    0010:[<d8961b95>]    Not tainted
EFLAGS: 00010246
eax: cb0cd520   ebx: 00000000   ecx: 00000002   edx: d8962980
esi: 00000000   edi: cb0cd520   ebp: c142e320   esp: c2953efc
ds: 0018   es: 0018   ss: 0018
Process oprofiled (pid: 5006, stackpage=c2953000)
Stack: d89653fc 00000000 d89658c0 c142e320 d89658c0 cb0cd520 d8960dbb 
d89659c0
        d8960dcb 00000000 d89600d6 00000000 c26ce6e0 cb924bc0 d895fd6b 
00000000
        c0130882 cb924bc0 c26ce6e0 c26ce6e0 cb924bc0 ffffffe9 c012f955 
cb924bc0
Call Trace:    [<d89653fc>] [<d89658c0>] [<d89658c0>] [<d8960dbb>] 
[<d89659c0>]
   [<d8960dcb>] [<d89600d6>] [<d895fd6b>] [<c0130882>] [<c012f955>] 
[<c012f86a>]
   [<c012fb9a>] [<c010853f>]

Code: 0f 32 25 00 00 20 00 0f 30 43 83 fb 01 7e ee 8b 44 24 1c 31


00002b1c <ppro_setup_ctrs>:
     2b1c:       83 ec 08                sub    $0x8,%esp
     2b1f:       55                      push   %ebp
     2b20:       57                      push   %edi
     2b21:       56                      push   %esi
     2b22:       53                      push   %ebx
     2b23:       8b 44 24 1c             mov    0x1c(%esp,1),%eax
     2b27:       31 db                   xor    %ebx,%ebx
     2b29:       8b 40 08                mov    0x8(%eax),%eax
     2b2c:       89 44 24 14             mov    %eax,0x14(%esp,1)
     2b30:       89 c7                   mov    %eax,%edi
     2b32:       8b 0c 9f                mov    (%edi,%ebx,4),%ecx
--> 2b35:       0f 32                   rdmsr
     2b37:       25 00 00 20 00          and    $0x200000,%eax
     2b3c:       0f 30                   wrmsr
     2b3e:       43                      inc    %ebx
     2b3f:       83 fb 01                cmp    $0x1,%ebx
     2b42:       7e ee                   jle    2b32static void


ppro_setup_ctrs(struct op_msrs const * const msrs)
{
	uint low, high;
	int i;

	/* clear all counters */
	for (i = 0 ; i < PPRO_NUM_CONTROLS; ++i) {
--> 
	PPRO_CTRL_READ (low, high, msrs, i);
	i == %ebx = 0, msr number == %ecx = 2 ??

It can occur if init is not done but I don't notice any obvious
error in code.

regards,
Phil

William Cohen wrote:
 > One of the concerns is being able to compile this on older kernels. The
 > 2.4.18 kernel doesn't appear to have the MSR_P6_* defined. The code
 > needs to compile on earlier versions of the kernel. If the values are
 > the same (they should be), I would leave the names the same.

that's handled by op_msr.h, anyway don't take care for now
for backward compatiblity, I have the necessary stuff to
check that and I'll provide correction, if necessary, later.

 >
 > I tried the patch on P3 and athlon and driver didn't work. :( I verified
 > that the unpatched version of oprofile I used worked, so there is
 > something in the patch that isn't right. I don't know what right now.

me too, if athlon doesn't work its probably a problem of
initialisation of msr number problem. I got an msr
number == 2 for the first control register on PIII.
See my other mail for details. I'm unsure than Graydon
can test this patch on PIII/Athlon. Graydon ?

regards,
Phil

graydon@... wrote:

ok catch it: conjunction of a typo and thinko

>  
> -static int pmc_init(void)
> +static int pmc_save_registers(void *dummy) 

+static void

see later

>  {
> -	int i;

[...]

> +
> +	msrs->counters.saved = kmalloc (n_ctrls * 
> +					sizeof (struct op_saved_msr), 
> +					GFP_KERNEL);
> +	if (! msrs->controls.saved) 
> +		goto fault;

typo: if (!msrs->counters.saved)

so we returned -EFAULT in all case but ...


> +static int pmc_init(void)
> +{
> +	int err = 0;	
> +	if ((err = smp_call_function(pmc_save_registers, NULL, 0, 1))) {
> +		smp_call_function(pmc_restore_registers, NULL, 0, 1);

I already point to not restore if save fail in previous mail.

The second problem here is the err returned by sm_call_function();

extern int smp_call_function (void (*func) (void *info), void *info,
			      int retry, int wait);

you can't return en error from the called function, the return
code is probably something like (only smp) failure to delivery
the function to all cpus. That's compile on UP because
smp_call_funtion() is a macro on UP :/

So init was failed silently then we try to use incorrect msr
number, boom ...

****

Now another problem (yeps ...)

$ op_start .. ; op_dump ; op_time
$ no samples files found (try running op_dump ?)

oprofiled.log verbose mode:

blha blah...
Read buffer of 0 entries.

but that's an another story ;)

the module can be loaded/unloaded now so it will
more easy to catch this once.

regards,
Phil

graydon@... wrote:

 > hi,

hello, the final try (I hope)

I prefer attach the new diff including the previous fix.


*****

op_time --> no samples files

cat /proc/sys/dev/oprofile/nr_interrupts
0


> +static void ppro_start(struct op_msrs const * const msrs)
> +{
> +	uint low,high;
> +	PPRO_CTR_READ(low, high, msrs, 0);
> +	PPRO_CTR_SET_ACTIVE(low);
> +	PPRO_CTR_WRITE(low, msrs, 0);


PPRO_CTR --> PPRO_CTRL, modifying the macro name for
SET_ACTIVE etc... ditto for ppro_stop / athlon_start
/ athlon_stop

ATHLON specific part not tested, can someone test ? If you get
problem look athlon_start/athlon_stop. We need also a compile with
a SMP kernel to catch if it exist other smp_call_function type
mismatch.

****

the patch is not consistent about spacing:

			PPRO_CTR_WRITE(sysctl.ctr[i].count, msrs, i);

			PPRO_CTRL_READ (low, high, msrs, i);

> +	kfree (msrs->controls.saved);
> +	kfree (msrs->controls.addrs);
> +	kfree (msrs->counters.saved);
> +	kfree (msrs->counters.addrs);

what about removing all space between macro name and (,
ditto for functions ?


*****

btw op_model_xxx.c are 120/130 lines and a lot of cleaner than
the old. It's really more easy now to debug the specific model
stuff.

regards,
Phil

sorry the last diff was missing the new file, this once
apply cleanly against cvs and compile/work with 2.4.19/PIII

regards,
Phil

At Sat, 21 Sep 2002 04:44:34 +0200,
Philippe Elie wrote:

> sorry the last diff was missing the new file, this once
> apply cleanly against cvs and compile/work with 2.4.19/PIII

I just applied this and confirmed that with your changes it works on
athlon, as well as P4-using-RTC. I think it's good now.

thanks a lot for the fixes. I went out to get some food and when I got
back in you had it all done!

-graydon