| Message ID | 20240618194306.1577022-1-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Masami Hiramatsu |
| Headers | show |
| Series | uprobe: Do not use UPROBE_SWBP_INSN as static initializer | expand |
On Tue, Jun 18, 2024 at 12:43 PM Jiri Olsa <jolsa@kernel.org> wrote: > > Nathan reported compilation fail for loongarch arch: > > kernel/events/uprobes.c: In function 'arch_uprobe_trampoline': > arch/loongarch/include/asm/uprobes.h:12:33: error: initializer element is not constant > 12 | #define UPROBE_SWBP_INSN larch_insn_gen_break(BRK_UPROBE_BP) > | ^~~~~~~~~~~~~~~~~~~~ > kernel/events/uprobes.c:1479:39: note: in expansion of macro 'UPROBE_SWBP_INSN' > 1479 | static uprobe_opcode_t insn = UPROBE_SWBP_INSN; > > Loongarch defines UPROBE_SWBP_INSN as function call, so we can't > use it to initialize static variable. > > Cc: Oleg Nesterov <oleg@redhat.com> > Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe") > Reported-by: Nathan Chancellor <nathan@kernel.org> > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > kernel/events/uprobes.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > Can we instead ask loongarch folks to rewrite it to be a constant? Having this as a function call is both an inconvenience and potential performance problem (a minor one, but still). I would imagine it's not hard to hard-code an instruction as a constant here. > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > index 2816e65729ac..6986bd993702 100644 > --- a/kernel/events/uprobes.c > +++ b/kernel/events/uprobes.c > @@ -1476,8 +1476,9 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area) > > void * __weak arch_uprobe_trampoline(unsigned long *psize) > { > - static uprobe_opcode_t insn = UPROBE_SWBP_INSN; > + static uprobe_opcode_t insn; > > + insn = insn ?: UPROBE_SWBP_INSN; > *psize = UPROBE_SWBP_INSN_SIZE; > return &insn; > } > -- > 2.45.1 >
On 06/20, Andrii Nakryiko wrote: > > Can we instead ask loongarch folks to rewrite it to be a constant? > Having this as a function call is both an inconvenience and potential > performance problem (a minor one, but still). I would imagine it's not > hard to hard-code an instruction as a constant here. I was going to ask the same question when I saw the bug report ;) The same for other users of larch_insn_gen_break(). But I can't understand what does it do, it calls emit_break() and git grep -w emit_break finds nothing. Oleg.
On Thu, Jun 20, 2024 at 12:40 PM Oleg Nesterov <oleg@redhat.com> wrote: > > On 06/20, Andrii Nakryiko wrote: > > > > Can we instead ask loongarch folks to rewrite it to be a constant? > > Having this as a function call is both an inconvenience and potential > > performance problem (a minor one, but still). I would imagine it's not > > hard to hard-code an instruction as a constant here. > > I was going to ask the same question when I saw the bug report ;) > The same for other users of larch_insn_gen_break(). > > But I can't understand what does it do, it calls emit_break() and > git grep -w emit_break finds nothing. > It's DEF_EMIT_REG0I15_FORMAT(break, break_op) in arch/loongarch/include/asm/inst.h A bunch of macro magic, but in the end it produces some constant value, of course. > Oleg. >
On 06/20, Andrii Nakryiko wrote: > > On Thu, Jun 20, 2024 at 12:40 PM Oleg Nesterov <oleg@redhat.com> wrote: > > > > But I can't understand what does it do, it calls emit_break() and > > git grep -w emit_break finds nothing. > > > > It's DEF_EMIT_REG0I15_FORMAT(break, break_op) in > arch/loongarch/include/asm/inst.h > > A bunch of macro magic, but in the end it produces some constant > value, of course. I see, thanks! Then perhaps something like below? Oleg. --- x/arch/loongarch/include/asm/uprobes.h +++ x/arch/loongarch/include/asm/uprobes.h @@ -9,7 +9,7 @@ typedef u32 uprobe_opcode_t; #define MAX_UINSN_BYTES 8 #define UPROBE_XOL_SLOT_BYTES MAX_UINSN_BYTES -#define UPROBE_SWBP_INSN larch_insn_gen_break(BRK_UPROBE_BP) +#define UPROBE_SWBP_INSN (uprobe_opcode_t)(BRK_UPROBE_BP | (break_op << 15)) #define UPROBE_SWBP_INSN_SIZE LOONGARCH_INSN_SIZE #define UPROBE_XOLBP_INSN larch_insn_gen_break(BRK_UPROBE_XOLBP) --- x/arch/loongarch/kernel/uprobes.c +++ x/arch/loongarch/kernel/uprobes.c @@ -7,6 +7,13 @@ #define UPROBE_TRAP_NR UINT_MAX +static __init int __ck_insn(void) +{ + BUG_ON(UPROBE_SWBP_INSN != larch_insn_gen_break(BRK_UPROBE_BP)); + return 0; +} +late_initcall(__ck_insn); + int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long addr) {
On Fri, Jun 21, 2024 at 02:01:50PM +0200, Oleg Nesterov wrote: > On 06/20, Andrii Nakryiko wrote: > > > > On Thu, Jun 20, 2024 at 12:40 PM Oleg Nesterov <oleg@redhat.com> wrote: > > > > > > But I can't understand what does it do, it calls emit_break() and > > > git grep -w emit_break finds nothing. > > > > > > > It's DEF_EMIT_REG0I15_FORMAT(break, break_op) in > > arch/loongarch/include/asm/inst.h > > > > A bunch of macro magic, but in the end it produces some constant > > value, of course. > > I see, thanks! > > Then perhaps something like below? lgtm, added loong arch list/folks for context: https://lore.kernel.org/bpf/20240614174822.GA1185149@thelio-3990X/ thanks, jirka > > Oleg. > > > --- x/arch/loongarch/include/asm/uprobes.h > +++ x/arch/loongarch/include/asm/uprobes.h > @@ -9,7 +9,7 @@ typedef u32 uprobe_opcode_t; > #define MAX_UINSN_BYTES 8 > #define UPROBE_XOL_SLOT_BYTES MAX_UINSN_BYTES > > -#define UPROBE_SWBP_INSN larch_insn_gen_break(BRK_UPROBE_BP) > +#define UPROBE_SWBP_INSN (uprobe_opcode_t)(BRK_UPROBE_BP | (break_op << 15)) > #define UPROBE_SWBP_INSN_SIZE LOONGARCH_INSN_SIZE > > #define UPROBE_XOLBP_INSN larch_insn_gen_break(BRK_UPROBE_XOLBP) > --- x/arch/loongarch/kernel/uprobes.c > +++ x/arch/loongarch/kernel/uprobes.c > @@ -7,6 +7,13 @@ > > #define UPROBE_TRAP_NR UINT_MAX > > +static __init int __ck_insn(void) > +{ > + BUG_ON(UPROBE_SWBP_INSN != larch_insn_gen_break(BRK_UPROBE_BP)); > + return 0; > +} > +late_initcall(__ck_insn); > + > int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, > struct mm_struct *mm, unsigned long addr) > { >
On Fri, Jun 21, 2024 at 03:17:58PM +0200, Jiri Olsa wrote: > On Fri, Jun 21, 2024 at 02:01:50PM +0200, Oleg Nesterov wrote: > > On 06/20, Andrii Nakryiko wrote: > > > > > > On Thu, Jun 20, 2024 at 12:40 PM Oleg Nesterov <oleg@redhat.com> wrote: > > > > > > > > But I can't understand what does it do, it calls emit_break() and > > > > git grep -w emit_break finds nothing. > > > > > > > > > > It's DEF_EMIT_REG0I15_FORMAT(break, break_op) in > > > arch/loongarch/include/asm/inst.h > > > > > > A bunch of macro magic, but in the end it produces some constant > > > value, of course. > > > > I see, thanks! > > > > Then perhaps something like below? > > lgtm, added loong arch list/folks ping Oleg, do you want to send formal patch? thanks, jirka > > for context: > https://lore.kernel.org/bpf/20240614174822.GA1185149@thelio-3990X/ > > thanks, > jirka > > > > > Oleg. > > > > > > --- x/arch/loongarch/include/asm/uprobes.h > > +++ x/arch/loongarch/include/asm/uprobes.h > > @@ -9,7 +9,7 @@ typedef u32 uprobe_opcode_t; > > #define MAX_UINSN_BYTES 8 > > #define UPROBE_XOL_SLOT_BYTES MAX_UINSN_BYTES > > > > -#define UPROBE_SWBP_INSN larch_insn_gen_break(BRK_UPROBE_BP) > > +#define UPROBE_SWBP_INSN (uprobe_opcode_t)(BRK_UPROBE_BP | (break_op << 15)) > > #define UPROBE_SWBP_INSN_SIZE LOONGARCH_INSN_SIZE > > > > #define UPROBE_XOLBP_INSN larch_insn_gen_break(BRK_UPROBE_XOLBP) > > --- x/arch/loongarch/kernel/uprobes.c > > +++ x/arch/loongarch/kernel/uprobes.c > > @@ -7,6 +7,13 @@ > > > > #define UPROBE_TRAP_NR UINT_MAX > > > > +static __init int __ck_insn(void) > > +{ > > + BUG_ON(UPROBE_SWBP_INSN != larch_insn_gen_break(BRK_UPROBE_BP)); > > + return 0; > > +} > > +late_initcall(__ck_insn); > > + > > int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, > > struct mm_struct *mm, unsigned long addr) > > { > >
On Thu, 27 Jun 2024 15:44:16 +0200 Jiri Olsa <olsajiri@gmail.com> wrote: > On Fri, Jun 21, 2024 at 03:17:58PM +0200, Jiri Olsa wrote: > > On Fri, Jun 21, 2024 at 02:01:50PM +0200, Oleg Nesterov wrote: > > > On 06/20, Andrii Nakryiko wrote: > > > > > > > > On Thu, Jun 20, 2024 at 12:40 PM Oleg Nesterov <oleg@redhat.com> wrote: > > > > > > > > > > But I can't understand what does it do, it calls emit_break() and > > > > > git grep -w emit_break finds nothing. > > > > > > > > > > > > > It's DEF_EMIT_REG0I15_FORMAT(break, break_op) in > > > > arch/loongarch/include/asm/inst.h > > > > > > > > A bunch of macro magic, but in the end it produces some constant > > > > value, of course. > > > > > > I see, thanks! > > > > > > Then perhaps something like below? > > > > lgtm, added loong arch list/folks > > ping > > Oleg, do you want to send formal patch? > > thanks, > jirka Yes, can you send v2 patch? Thank you, > > > > > for context: > > https://lore.kernel.org/bpf/20240614174822.GA1185149@thelio-3990X/ > > > > thanks, > > jirka > > > > > > > > Oleg. > > > > > > > > > --- x/arch/loongarch/include/asm/uprobes.h > > > +++ x/arch/loongarch/include/asm/uprobes.h > > > @@ -9,7 +9,7 @@ typedef u32 uprobe_opcode_t; > > > #define MAX_UINSN_BYTES 8 > > > #define UPROBE_XOL_SLOT_BYTES MAX_UINSN_BYTES > > > > > > -#define UPROBE_SWBP_INSN larch_insn_gen_break(BRK_UPROBE_BP) > > > +#define UPROBE_SWBP_INSN (uprobe_opcode_t)(BRK_UPROBE_BP | (break_op << 15)) > > > #define UPROBE_SWBP_INSN_SIZE LOONGARCH_INSN_SIZE > > > > > > #define UPROBE_XOLBP_INSN larch_insn_gen_break(BRK_UPROBE_XOLBP) > > > --- x/arch/loongarch/kernel/uprobes.c > > > +++ x/arch/loongarch/kernel/uprobes.c > > > @@ -7,6 +7,13 @@ > > > > > > #define UPROBE_TRAP_NR UINT_MAX > > > > > > +static __init int __ck_insn(void) > > > +{ > > > + BUG_ON(UPROBE_SWBP_INSN != larch_insn_gen_break(BRK_UPROBE_BP)); > > > + return 0; > > > +} > > > +late_initcall(__ck_insn); > > > + > > > int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, > > > struct mm_struct *mm, unsigned long addr) > > > { > > >
On 06/27, Masami Hiramatsu wrote: > > On Thu, 27 Jun 2024 15:44:16 +0200 > Jiri Olsa <olsajiri@gmail.com> wrote: > > > Oleg, do you want to send formal patch? > > > > thanks, > > jirka > > Yes, can you send v2 patch? I was waiting for the comments from loongarch maintainers... OK, will do today, but the patch won't be even compile tested. Oleg.
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index 2816e65729ac..6986bd993702 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -1476,8 +1476,9 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area) void * __weak arch_uprobe_trampoline(unsigned long *psize) { - static uprobe_opcode_t insn = UPROBE_SWBP_INSN; + static uprobe_opcode_t insn; + insn = insn ?: UPROBE_SWBP_INSN; *psize = UPROBE_SWBP_INSN_SIZE; return &insn; }
Nathan reported compilation fail for loongarch arch: kernel/events/uprobes.c: In function 'arch_uprobe_trampoline': arch/loongarch/include/asm/uprobes.h:12:33: error: initializer element is not constant 12 | #define UPROBE_SWBP_INSN larch_insn_gen_break(BRK_UPROBE_BP) | ^~~~~~~~~~~~~~~~~~~~ kernel/events/uprobes.c:1479:39: note: in expansion of macro 'UPROBE_SWBP_INSN' 1479 | static uprobe_opcode_t insn = UPROBE_SWBP_INSN; Loongarch defines UPROBE_SWBP_INSN as function call, so we can't use it to initialize static variable. Cc: Oleg Nesterov <oleg@redhat.com> Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe") Reported-by: Nathan Chancellor <nathan@kernel.org> Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- kernel/events/uprobes.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)