All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mao Han <han_mao@c-sky.com>
To: Nick Kossifidis <mick@ics.forth.gr>
Cc: linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: Perf-related compilation issues
Date: Thu, 24 Oct 2019 14:01:17 +0800	[thread overview]
Message-ID: <20191024060116.GA786@vmh-VirtualBox> (raw)
In-Reply-To: <1bba622b-1f59-d21b-f396-d9c1a021dc3a@ics.forth.gr>

On Wed, Oct 23, 2019 at 06:15:43PM +0300, Nick Kossifidis wrote:
> Hello all,
> 
> a) Compiling the current fixes branch with a minimal config I get the
> following error:
> 
> riscv64-unknown-linux-gnu-ld: arch/riscv/kernel/perf_callchain.o: in
> function `.L0 ':
> perf_callchain.c:(.text+0x16a): undefined reference to `walk_stackframe'
> make: *** [Makefile:1074: vmlinux] Error 1
> 
> 
> I've removed the static delcaration of walk_stackframe on stackframe.c
> and marked walk_stackframe as extern on perf_callchain.c to fix the
> above issue.
>

Beside the compile problem caused by:
[PATCH v3 5/8] riscv: mark some code and data as file-static
similar issue may happen when CONFIG_FRAME_POINTER is not defined.
I didn't see the CONFIG_FRAME_POINTER in stacktrace.c, and the 
conditional for !CONFIG_FRAME_POINTER looks quite strange, keep
adding the sp and read pc from that?

        ksp = (unsigned long *)sp;
        while (!kstack_end(ksp)) {
                if (__kernel_text_address(pc) && unlikely(fn(pc, arg)))
                        break;
                pc = (*ksp++) - 0x4;
        }

A conditional for perf_callchain_kernel might be properly to fix that.
--- a/arch/riscv/kernel/perf_callchain.c
+++ b/arch/riscv/kernel/perf_callchain.c
+#ifdef CONFIG_FRAME_POINTER
 void notrace walk_stackframe(struct task_struct *task,
        struct pt_regs *regs, bool (*fn)(unsigned long, void *), void *arg);
 void perf_callchain_kernel(struct perf_callchain_entry_ctx *entry,
@@ -92,3 +93,4 @@ void perf_callchain_kernel(struct perf_callchain_entry_ctx *entry,
 
        walk_stackframe(NULL, regs, fill_callchain, entry);
 }
+#endif


> 
> b) Then If I compile the kernel without CONFIG_RISCV_BASE_PMU I get
> 
> 
> ./arch/riscv/include/asm/perf_event.h:26:2: error: #error "Please
> provide a valid RISCV_MAX_COUNTERS for the PMU."
>  #error "Please provide a valid RISCV_MAX_COUNTERS for the PMU."
> 
> 
> I noticed that the only place where CONFIG_RISCV_BASE_PMU is checked is
> on perf_event.h and only for this parameter that's not defined anywhere
> else. So for now if one tries to compile the kernel without PMU the
> kernel won't compile + I don't see how unsetting this saves code size as
> the config description says.
> 

The content inside perf_event.h mostly relate to HW PMU. Other
architectures normally put them inside perf_event.c or pmu.h, they are
not compiled when CONFIG_PERF_EVENTS is selected and HW PMU is not
selected, user can use software event under this configuration.
Base pmu shouldn't be registed when it is defined in dts.
I think it can be fixed by put the content inside perf_event.h
into a riscv_pmu.h, only compile perf_event.c when CONFIG_RISCV_BASE_PMU
is selected or add conditional inside it:

--- a/arch/riscv/kernel/perf_event.c
+++ b/arch/riscv/kernel/perf_event.c
@@ -474,12 +474,13 @@ int __init init_hw_perf_events(void)
        if (node) {
                of_id = of_match_node(riscv_pmu_of_ids, node);
 
-               if (of_id)
+               if (of_id) {
                        riscv_pmu = of_id->data;
+                       perf_pmu_register(riscv_pmu->pmu, "cpu", PERF_TYPE_RAW);
+               }
                of_node_put(node);
        }
 
-       perf_pmu_register(riscv_pmu->pmu, "cpu", PERF_TYPE_RAW);
        return 0;
--- a/arch/riscv/kernel/Makefile
+++ b/arch/riscv/kernel/Makefile
@@ -38,7 +38,7 @@ obj-$(CONFIG_MODULE_SECTIONS) += module-sections.o
 obj-$(CONFIG_FUNCTION_TRACER)  += mcount.o ftrace.o
 obj-$(CONFIG_DYNAMIC_FTRACE)   += mcount-dyn.o
 
-obj-$(CONFIG_PERF_EVENTS)      += perf_event.o
+obj-$(CONFIG_RISCV_BASE_PMU)   += perf_event.o
 obj-$(CONFIG_PERF_EVENTS)      += perf_callchain.o
 obj-$(CONFIG_HAVE_PERF_REGS)   += perf_regs.o

Thanks,
Mao Han

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

WARNING: multiple messages have this Message-ID (diff)
From: Mao Han <han_mao@c-sky.com>
To: Nick Kossifidis <mick@ics.forth.gr>
Cc: linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: Perf-related compilation issues
Date: Thu, 24 Oct 2019 14:01:17 +0800	[thread overview]
Message-ID: <20191024060116.GA786@vmh-VirtualBox> (raw)
In-Reply-To: <1bba622b-1f59-d21b-f396-d9c1a021dc3a@ics.forth.gr>

On Wed, Oct 23, 2019 at 06:15:43PM +0300, Nick Kossifidis wrote:
> Hello all,
> 
> a) Compiling the current fixes branch with a minimal config I get the
> following error:
> 
> riscv64-unknown-linux-gnu-ld: arch/riscv/kernel/perf_callchain.o: in
> function `.L0 ':
> perf_callchain.c:(.text+0x16a): undefined reference to `walk_stackframe'
> make: *** [Makefile:1074: vmlinux] Error 1
> 
> 
> I've removed the static delcaration of walk_stackframe on stackframe.c
> and marked walk_stackframe as extern on perf_callchain.c to fix the
> above issue.
>

Beside the compile problem caused by:
[PATCH v3 5/8] riscv: mark some code and data as file-static
similar issue may happen when CONFIG_FRAME_POINTER is not defined.
I didn't see the CONFIG_FRAME_POINTER in stacktrace.c, and the 
conditional for !CONFIG_FRAME_POINTER looks quite strange, keep
adding the sp and read pc from that?

        ksp = (unsigned long *)sp;
        while (!kstack_end(ksp)) {
                if (__kernel_text_address(pc) && unlikely(fn(pc, arg)))
                        break;
                pc = (*ksp++) - 0x4;
        }

A conditional for perf_callchain_kernel might be properly to fix that.
--- a/arch/riscv/kernel/perf_callchain.c
+++ b/arch/riscv/kernel/perf_callchain.c
+#ifdef CONFIG_FRAME_POINTER
 void notrace walk_stackframe(struct task_struct *task,
        struct pt_regs *regs, bool (*fn)(unsigned long, void *), void *arg);
 void perf_callchain_kernel(struct perf_callchain_entry_ctx *entry,
@@ -92,3 +93,4 @@ void perf_callchain_kernel(struct perf_callchain_entry_ctx *entry,
 
        walk_stackframe(NULL, regs, fill_callchain, entry);
 }
+#endif


> 
> b) Then If I compile the kernel without CONFIG_RISCV_BASE_PMU I get
> 
> 
> ./arch/riscv/include/asm/perf_event.h:26:2: error: #error "Please
> provide a valid RISCV_MAX_COUNTERS for the PMU."
>  #error "Please provide a valid RISCV_MAX_COUNTERS for the PMU."
> 
> 
> I noticed that the only place where CONFIG_RISCV_BASE_PMU is checked is
> on perf_event.h and only for this parameter that's not defined anywhere
> else. So for now if one tries to compile the kernel without PMU the
> kernel won't compile + I don't see how unsetting this saves code size as
> the config description says.
> 

The content inside perf_event.h mostly relate to HW PMU. Other
architectures normally put them inside perf_event.c or pmu.h, they are
not compiled when CONFIG_PERF_EVENTS is selected and HW PMU is not
selected, user can use software event under this configuration.
Base pmu shouldn't be registed when it is defined in dts.
I think it can be fixed by put the content inside perf_event.h
into a riscv_pmu.h, only compile perf_event.c when CONFIG_RISCV_BASE_PMU
is selected or add conditional inside it:

--- a/arch/riscv/kernel/perf_event.c
+++ b/arch/riscv/kernel/perf_event.c
@@ -474,12 +474,13 @@ int __init init_hw_perf_events(void)
        if (node) {
                of_id = of_match_node(riscv_pmu_of_ids, node);
 
-               if (of_id)
+               if (of_id) {
                        riscv_pmu = of_id->data;
+                       perf_pmu_register(riscv_pmu->pmu, "cpu", PERF_TYPE_RAW);
+               }
                of_node_put(node);
        }
 
-       perf_pmu_register(riscv_pmu->pmu, "cpu", PERF_TYPE_RAW);
        return 0;
--- a/arch/riscv/kernel/Makefile
+++ b/arch/riscv/kernel/Makefile
@@ -38,7 +38,7 @@ obj-$(CONFIG_MODULE_SECTIONS) += module-sections.o
 obj-$(CONFIG_FUNCTION_TRACER)  += mcount.o ftrace.o
 obj-$(CONFIG_DYNAMIC_FTRACE)   += mcount-dyn.o
 
-obj-$(CONFIG_PERF_EVENTS)      += perf_event.o
+obj-$(CONFIG_RISCV_BASE_PMU)   += perf_event.o
 obj-$(CONFIG_PERF_EVENTS)      += perf_callchain.o
 obj-$(CONFIG_HAVE_PERF_REGS)   += perf_regs.o

Thanks,
Mao Han

  parent reply	other threads:[~2019-10-24  6:01 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-23 15:15 Perf-related compilation issues Nick Kossifidis
2019-10-23 23:52 ` Alan Kao
2019-10-24  2:23 ` Paul Walmsley
2019-10-24  6:01 ` Mao Han [this message]
2019-10-24  6:01   ` Mao Han

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191024060116.GA786@vmh-VirtualBox \
    --to=han_mao@c-sky.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=mick@ics.forth.gr \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.