| Message ID | 19127.8400.376239.586120@drongo.ozlabs.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Paul Mackerras |
| Headers | show |
* Paul Mackerras <paulus@samba.org> wrote: > Commit 5622f295 ("x86, perf_counter, bts: Optimize BTS overflow > handling") removed the regs field from struct perf_sample_data and > added a regs parameter to perf_counter_overflow(). This breaks the > build on powerpc as reported by Sachin Sant: > > arch/powerpc/kernel/perf_counter.c: In function 'record_and_restart': > arch/powerpc/kernel/perf_counter.c:1165: error: unknown field 'regs' specified in initializer > cc1: warnings being treated as errors > arch/powerpc/kernel/perf_counter.c:1165: error: initialization makes integer from pointer without a cast > arch/powerpc/kernel/perf_counter.c:1173: error: too few arguments to function 'perf_counter_overflow' > make[1]: *** [arch/powerpc/kernel/perf_counter.o] Error 1 > make: *** [arch/powerpc/kernel] Error 2 > > This adjusts arch/powerpc/kernel/perf_counter.c to correspond with the > new struct perf_sample_data and perf_counter_overflow(). > > Reported-by: Sachin Sant <sachinp@in.ibm.com> > Signed-off-by: Paul Mackerras <paulus@samba.org> Applied, thanks Paul. > --- > > I missed this problem when the "x86, perf_counter, bts: Optimize BTS > overflow handling" patch was posted because the headline made it seem > entirely x86-specific, and the changes to struct perf_sample_data and > perf_counter_overflow() were not mentioned in the changelog. > > Markus, please take care in future to mention it in the changelog if > your patches touch definitions used by other architectures. If you > could go so far as to use grep a bit more and fix up other > architectures' callsites for the things you're changing, that would be > very much appreciated. Thanks. Yes, that should be done in general - still, nothing beats actual testing. Paul, you might also want to test the perfcounter bits of -tip on PowerPC a bit more frequently - this patch was there for 5 days before i sent it to Linus. Cross-builds didnt catch it as perfcounters isnt enabled by default in any of the powerpc defconfigs: phoenix:~/linux/linux> grep -w CONFIG_PERF_COUNTERS arch/powerpc/configs/* arch/powerpc/configs/adder875_defconfig:# CONFIG_PERF_COUNTERS is not set arch/powerpc/configs/c2k_defconfig:# CONFIG_PERF_COUNTERS is not set arch/powerpc/configs/ep8248e_defconfig:# CONFIG_PERF_COUNTERS is not set arch/powerpc/configs/ep88xc_defconfig:# CONFIG_PERF_COUNTERS is not set arch/powerpc/configs/linkstation_defconfig:# CONFIG_PERF_COUNTERS is not set arch/powerpc/configs/mgcoge_defconfig:# CONFIG_PERF_COUNTERS is not set arch/powerpc/configs/mgsuvd_defconfig:# CONFIG_PERF_COUNTERS is not set arch/powerpc/configs/mpc7448_hpc2_defconfig:# CONFIG_PERF_COUNTERS is not set arch/powerpc/configs/mpc8272_ads_defconfig:# CONFIG_PERF_COUNTERS is not set arch/powerpc/configs/mpc83xx_defconfig:# CONFIG_PERF_COUNTERS is not set arch/powerpc/configs/mpc85xx_defconfig:# CONFIG_PERF_COUNTERS is not set arch/powerpc/configs/mpc85xx_smp_defconfig:# CONFIG_PERF_COUNTERS is not set arch/powerpc/configs/mpc866_ads_defconfig:# CONFIG_PERF_COUNTERS is not set arch/powerpc/configs/mpc86xx_defconfig:# CONFIG_PERF_COUNTERS is not set arch/powerpc/configs/mpc885_ads_defconfig:# CONFIG_PERF_COUNTERS is not set arch/powerpc/configs/pq2fads_defconfig:# CONFIG_PERF_COUNTERS is not set arch/powerpc/configs/prpmc2800_defconfig:# CONFIG_PERF_COUNTERS is not set arch/powerpc/configs/ps3_defconfig:# CONFIG_PERF_COUNTERS is not set arch/powerpc/configs/storcenter_defconfig:# CONFIG_PERF_COUNTERS is not set There's not that many PowerPC users so all extra testing help would be much welcome. Also, enabling them in the powerpc defconfigs would be helpful as well. Thanks, Ingo
* Metzger, Markus T <markus.t.metzger@intel.com> wrote: > >-----Original Message----- > >From: Paul Mackerras [mailto:paulus@samba.org] > >Sent: Monday, September 21, 2009 8:45 AM > > > >Markus, please take care in future to mention it in the changelog if > >your patches touch definitions used by other architectures. If you > >could go so far as to use grep a bit more and fix up other > >architectures' callsites for the things you're changing, that would be > >very much appreciated. Thanks. > > I'm sorry I missed that. > > There's one more place in arch/sparc/. > The below patch should fix it, but I have no means to test it. You also missed a third thing: +static inline int +perf_output_begin(struct perf_output_handle *handle, struct perf_counter *c, + unsigned int size, int nmi, int sample) { } an 'int' function returning void ... Plus all the !PERF_COUNTERS branch of empty inlines is pointless - these facilities are used by perfcounters code only. I fixed that too. > > Index: b/arch/sparc/kernel/perf_counter.c > =================================================================== > --- a/arch/sparc/kernel/perf_counter.c > +++ b/arch/sparc/kernel/perf_counter.c > @@ -493,7 +493,6 @@ static int __kprobes perf_counter_nmi_ha > > regs = args->regs; > > - data.regs = regs; > data.addr = 0; > > cpuc = &__get_cpu_var(cpu_hw_counters); > @@ -513,7 +512,7 @@ static int __kprobes perf_counter_nmi_ha > if (!sparc_perf_counter_set_period(counter, hwc, idx)) > continue; > > - if (perf_counter_overflow(counter, 1, &data)) > + if (perf_counter_overflow(counter, 1, &data, regs)) > sparc_pmu_disable_counter(hwc, idx); > } Looks correct to me and i've also done a Sparc cross build with the fix in place and it builds fine besides the unrelated build error pasted below. I've added it to the other fix and if David acks it will send it to Linus later today. Thanks, Ingo /home/mingo/tip/drivers/video/console/vgacon.c: In function 'vgacon_startup': /home/mingo/tip/drivers/video/console/vgacon.c:516: warning: passing argument 1 of 'scr_readw' discards qualifiers from pointer target type /home/mingo/tip/drivers/video/console/vgacon.c:517: warning: passing argument 1 of 'scr_readw' discards qualifiers from pointer target type /home/mingo/tip/drivers/video/console/vgacon.c:518: warning: passing argument 2 of 'scr_writew' discards qualifiers from pointer target type /home/mingo/tip/drivers/video/console/vgacon.c:519: warning: passing argument 2 of 'scr_writew' discards qualifiers from pointer target type /home/mingo/tip/drivers/video/console/vgacon.c:520: warning: passing argument 1 of 'scr_readw' discards qualifiers from pointer target type /home/mingo/tip/drivers/video/console/vgacon.c:520: warning: passing argument 1 of 'scr_readw' discards qualifiers from pointer target type /home/mingo/tip/drivers/video/console/vgacon.c:521: warning: passing argument 2 of 'scr_writew' discards qualifiers from pointer target type /home/mingo/tip/drivers/video/console/vgacon.c:522: warning: passing argument 2 of 'scr_writew' discards qualifiers from pointer target type /home/mingo/tip/drivers/video/console/vgacon.c:525: warning: passing argument 2 of 'scr_writew' discards qualifiers from pointer target type /home/mingo/tip/drivers/video/console/vgacon.c:526: warning: passing argument 2 of 'scr_writew' discards qualifiers from pointer target type /home/mingo/tip/drivers/video/console/vgacon.c:527: warning: passing argument 1 of 'scr_readw' discards qualifiers from pointer target type /home/mingo/tip/drivers/video/console/vgacon.c:527: warning: passing argument 1 of 'scr_readw' discards qualifiers from pointer target type /home/mingo/tip/drivers/video/console/vgacon.c:528: warning: passing argument 2 of 'scr_writew' discards qualifiers from pointer target type /home/mingo/tip/drivers/video/console/vgacon.c:529: warning: passing argument 2 of 'scr_writew' discards qualifiers from pointer target type /home/mingo/tip/drivers/video/console/vgacon.c:532: warning: passing argument 2 of 'scr_writew' discards qualifiers from pointer target type /home/mingo/tip/drivers/video/console/vgacon.c:533: warning: passing argument 2 of 'scr_writew' discards qualifiers from pointer target type /home/mingo/tip/drivers/video/console/vgacon.c: In function 'vgacon_do_font_op': /home/mingo/tip/drivers/video/console/vgacon.c:1126: error: implicit declaration of function 'vga_writeb' /home/mingo/tip/drivers/video/console/vgacon.c:1129: error: implicit declaration of function 'vga_readb' make[4]: *** [drivers/video/console/vgacon.o] Error 1 make[3]: *** [drivers/video/console] Error 2 make[2]: *** [drivers/video] Error 2 make[2]: *** Waiting for unfinished jobs....
On Mon, Sep 21, 2009 at 09:30:43AM +0200, Ingo Molnar wrote: > > * Metzger, Markus T <markus.t.metzger@intel.com> wrote: > > > >-----Original Message----- > > >From: Paul Mackerras [mailto:paulus@samba.org] > > >Sent: Monday, September 21, 2009 8:45 AM > > > > > > >Markus, please take care in future to mention it in the changelog if > > >your patches touch definitions used by other architectures. If you > > >could go so far as to use grep a bit more and fix up other > > >architectures' callsites for the things you're changing, that would be > > >very much appreciated. Thanks. > > > > I'm sorry I missed that. > > > > There's one more place in arch/sparc/. > > The below patch should fix it, but I have no means to test it. > > You also missed a third thing: > > +static inline int > +perf_output_begin(struct perf_output_handle *handle, struct perf_counter *c, > + unsigned int size, int nmi, int sample) { } > > an 'int' function returning void ... > > Plus all the !PERF_COUNTERS branch of empty inlines is pointless - these > facilities are used by perfcounters code only. I fixed that too. Hi Ingo, did you fix all of these warnings for !PERF_COUNTERS? include/linux/perf_counter.h: In function 'perf_output_begin': include/linux/perf_counter.h:854: warning: no return statement in function returning non-void include/linux/perf_counter.h: At top level: include/linux/perf_counter.h:863: warning: 'struct perf_sample_data' declared inside parameter list include/linux/perf_counter.h:863: warning: its scope is only this definition or declaration, which is probably not what you want include/linux/perf_counter.h:868: warning: 'struct perf_sample_data' declared inside parameter list
Ingo Molnar writes: > Paul, you might also want to test the perfcounter bits of -tip on > PowerPC a bit more frequently - this patch was there for 5 days before i > sent it to Linus. Yes, I'll try to do that in future. I hope I didn't come across as blaming anyone for anything - that wasn't my intention at all. > Cross-builds didnt catch it as perfcounters isnt enabled by default in > any of the powerpc defconfigs: I'll get that fixed too. Paul.
diff --git a/arch/powerpc/kernel/perf_counter.c b/arch/powerpc/kernel/perf_counter.c index 7ceefaf..5ccf9bc 100644 --- a/arch/powerpc/kernel/perf_counter.c +++ b/arch/powerpc/kernel/perf_counter.c @@ -1162,7 +1162,6 @@ static void record_and_restart(struct perf_counter *counter, unsigned long val, */ if (record) { struct perf_sample_data data = { - .regs = regs, .addr = 0, .period = counter->hw.last_period, }; @@ -1170,7 +1169,7 @@ static void record_and_restart(struct perf_counter *counter, unsigned long val, if (counter->attr.sample_type & PERF_SAMPLE_ADDR) perf_get_data_addr(regs, &data.addr); - if (perf_counter_overflow(counter, nmi, &data)) { + if (perf_counter_overflow(counter, nmi, &data, regs)) { /* * Interrupts are coming too fast - throttle them * by setting the counter to 0, so it will be
Commit 5622f295 ("x86, perf_counter, bts: Optimize BTS overflow handling") removed the regs field from struct perf_sample_data and added a regs parameter to perf_counter_overflow(). This breaks the build on powerpc as reported by Sachin Sant: arch/powerpc/kernel/perf_counter.c: In function 'record_and_restart': arch/powerpc/kernel/perf_counter.c:1165: error: unknown field 'regs' specified in initializer cc1: warnings being treated as errors arch/powerpc/kernel/perf_counter.c:1165: error: initialization makes integer from pointer without a cast arch/powerpc/kernel/perf_counter.c:1173: error: too few arguments to function 'perf_counter_overflow' make[1]: *** [arch/powerpc/kernel/perf_counter.o] Error 1 make: *** [arch/powerpc/kernel] Error 2 This adjusts arch/powerpc/kernel/perf_counter.c to correspond with the new struct perf_sample_data and perf_counter_overflow(). Reported-by: Sachin Sant <sachinp@in.ibm.com> Signed-off-by: Paul Mackerras <paulus@samba.org> --- I missed this problem when the "x86, perf_counter, bts: Optimize BTS overflow handling" patch was posted because the headline made it seem entirely x86-specific, and the changes to struct perf_sample_data and perf_counter_overflow() were not mentioned in the changelog. Markus, please take care in future to mention it in the changelog if your patches touch definitions used by other architectures. If you could go so far as to use grep a bit more and fix up other architectures' callsites for the things you're changing, that would be very much appreciated. Thanks.