From: Brian R. <bc...@co...> - 2005-07-14 16:48:04
Attachments:
oprof-ppc64-backtrace.patch
|
Attached please find a patch to support backtracing on ppc64. Currently, support is only implemented for Power5, although it should be easy to adapt to Power4, but I don't have the hardware to try. I would really like to hear any comments that anyone has on the code. --Brian |
From: John L. <le...@mo...> - 2005-07-18 18:52:37
|
On Thu, Jul 14, 2005 at 12:48:04PM -0400, Brian Rogan wrote: > Attached please find a patch to support backtracing on ppc64. Currently, > support is only implemented for Power5, although it should be easy to adapt > to Power4, but I don't have the hardware to try. Not knowing anything about ppc64, my only comment is that your code isn't in Linux coding style in some places, in particular if statements look like this: if (!something()) { not the other variations you have. regards, john |
From: Anton B. <an...@sa...> - 2005-07-21 18:02:06
|
Hi Brian, > Attached please find a patch to support backtracing on ppc64. Currently, > support is only implemented for Power5, although it should be easy to adapt > to Power4, but I don't have the hardware to try. Cool! > I would really like to hear any comments that anyone has on the code. John already mentioned coding style, we tend to be pretty particular about following it in the kernel. Other than that I have a few suggestions. > +static unsigned int user_putsp32(unsigned int sp, int *is_first) { > + unsigned int stack_frame[2], rv; > + unsigned long t = sp; > + > + /* If the page isn't accessible, then we've done all that we can do, > + * a partial stack trace is better than none. > + */ > + rv = __copy_from_user_inatomic(stack_frame, (void *) t, sizeof(stack_frame)); We should probably use this method even on x86, the spin_trylock of the page_table_lock together with a check if the page is present hack is pretty nasty stuff. > @@ -261,13 +261,11 @@ static void power4_handle_interrupt(stru > struct op_counter_config *ctr) > { > unsigned long pc; > - int is_kernel; > int val; > int i; > unsigned int mmcr0; > > pc = get_pc(regs); > - is_kernel = get_kernel(pc); > > /* set the PMM bit (see comment below) */ > mtmsrd(mfmsr() | MSR_PMM); > @@ -276,7 +274,7 @@ static void power4_handle_interrupt(stru > val = ctr_read(i); > if (val < 0) { > if (oprofile_running && ctr[i].enabled) { > - oprofile_add_pc(pc, is_kernel, i); > + oprofile_add_sample(regs, i); > ctr_write(i, reset_value[i]); > } else { > ctr_write(i, 0); This changes behaviour here. get_kernel uses some bits in the performance monitor hardware to tell if the sample came from userspace or kernel. Since the performance monitor interrupt is not an NMI on ppc64 we could take the exception a long way after it tripped. Changing to oprofile_add_sample means we look at the regs of the exception, not when the original performance monitor interrupt happened. I wonder if we can change oprofile_add_sample to take another argument which mirrors the old is_kernel stuff. In fact on top of this Id love to add a third "hypervisor" state so I can send hypervisor samples out to userspace instead of the current hack where we throw them into a magic function (for details check out arch/ppc64/oprofile/op_model_power4.c:hypervisor_bucket()) Anton |
From: John L. <le...@mo...> - 2005-07-21 18:46:54
|
On Fri, Jul 22, 2005 at 03:51:01AM +1000, Anton Blanchard wrote: > > + rv = __copy_from_user_inatomic(stack_frame, (void *) t, sizeof(stack_frame)); > > We should probably use this method even on x86, the spin_trylock of the > page_table_lock together with a check if the page is present hack is > pretty nasty stuff. You're probably right. Note that ARM is using the copy_from_user thing too, though not this exact form. > I wonder if we can change oprofile_add_sample to take another argument > which mirrors the old is_kernel stuff. In fact on top of this Id love to > add a third "hypervisor" state so I can send hypervisor samples out to > userspace instead of the current hack where we throw them into a magic > function (for details check out > arch/ppc64/oprofile/op_model_power4.c:hypervisor_bucket()) Sounds reasonable. Note that the OProfile Xen port has a similar need I think, though I forget the hack they had for it. Certainly it's worth talking to them. regards, john |
From: Brian R. <bc...@co...> - 2005-07-22 22:24:03
Attachments:
oprof-ppc64-backtrace-v2.patch
|
Anton, > > John already mentioned coding style, we tend to be pretty particular > about following it in the kernel. Other than that I have a few > suggestions. I got rid of all of the style issues that I could find, tell me if there any additional issues in this release. > > { > > unsigned long pc; > > - int is_kernel; > > int val; > > int i; > > unsigned int mmcr0; > > > > pc = get_pc(regs); > > - is_kernel = get_kernel(pc); > > > > /* set the PMM bit (see comment below) */ > > mtmsrd(mfmsr() | MSR_PMM); > > @@ -276,7 +274,7 @@ static void power4_handle_interrupt(stru > > val = ctr_read(i); > > if (val < 0) { > > if (oprofile_running && ctr[i].enabled) { > > - oprofile_add_pc(pc, is_kernel, i); > > + oprofile_add_sample(regs, i); > > ctr_write(i, reset_value[i]); > > } else { > > ctr_write(i, 0); > > This changes behaviour here. get_kernel uses some bits in the > performance monitor hardware to tell if the sample came from userspace > or kernel. Since the performance monitor interrupt is not an NMI on > ppc64 we could take the exception a long way after it tripped. Changing > to oprofile_add_sample means we look at the regs of the exception, not > when the original performance monitor interrupt happened. The attached patch should fix these issues. John, please tell me if you don't think this method is sufficiently generic, or you want to do it another way. > > I wonder if we can change oprofile_add_sample to take another argument > which mirrors the old is_kernel stuff. In fact on top of this Id love to > add a third "hypervisor" state so I can send hypervisor samples out to > userspace instead of the current hack where we throw them into a magic > function (for details check out > arch/ppc64/oprofile/op_model_power4.c:hypervisor_bucket()) This certainly wouldn't be difficult. I'm not sure that there's any good way to do it without breaking old oprofiled's (i.e. ones that call abort if they see escape codes >= LAST_CODE), as the only way I could imagine doing it would rely on creating an additional escape code. I don't know if this is a big issue or not. --Brian |