Make is_optimized() return a tri-state and avoid return through
argument. This simplifies things a little.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/kernel/uprobes.c | 34 +++++++++++++---------------------
1 file changed, 13 insertions(+), 21 deletions(-)
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -1047,7 +1047,7 @@ static bool __is_optimized(uprobe_opcode
return __in_uprobe_trampoline(vaddr + 5 + call->raddr);
}
-static int is_optimized(struct mm_struct *mm, unsigned long vaddr, bool *optimized)
+static int is_optimized(struct mm_struct *mm, unsigned long vaddr)
{
uprobe_opcode_t insn[5];
int err;
@@ -1055,8 +1055,7 @@ static int is_optimized(struct mm_struct
err = copy_from_vaddr(mm, vaddr, &insn, 5);
if (err)
return err;
- *optimized = __is_optimized((uprobe_opcode_t *)&insn, vaddr);
- return 0;
+ return __is_optimized((uprobe_opcode_t *)&insn, vaddr);
}
static bool should_optimize(struct arch_uprobe *auprobe)
@@ -1069,17 +1068,14 @@ int set_swbp(struct arch_uprobe *auprobe
unsigned long vaddr)
{
if (should_optimize(auprobe)) {
- bool optimized = false;
- int err;
-
/*
* We could race with another thread that already optimized the probe,
* so let's not overwrite it with int3 again in this case.
*/
- err = is_optimized(vma->vm_mm, vaddr, &optimized);
- if (err)
- return err;
- if (optimized)
+ int ret = is_optimized(vma->vm_mm, vaddr);
+ if (ret < 0)
+ return ret;
+ if (ret)
return 0;
}
return uprobe_write_opcode(auprobe, vma, vaddr, UPROBE_SWBP_INSN,
@@ -1090,17 +1086,13 @@ int set_orig_insn(struct arch_uprobe *au
unsigned long vaddr)
{
if (test_bit(ARCH_UPROBE_FLAG_CAN_OPTIMIZE, &auprobe->flags)) {
- struct mm_struct *mm = vma->vm_mm;
- bool optimized = false;
- int err;
-
- err = is_optimized(mm, vaddr, &optimized);
- if (err)
- return err;
- if (optimized) {
- err = swbp_unoptimize(auprobe, vma, vaddr);
- WARN_ON_ONCE(err);
- return err;
+ int ret = is_optimized(vma->vm_mm, vaddr);
+ if (ret < 0)
+ return ret;
+ if (ret) {
+ ret = swbp_unoptimize(auprobe, vma, vaddr);
+ WARN_ON_ONCE(ret);
+ return ret;
}
}
return uprobe_write_opcode(auprobe, vma, vaddr, *(uprobe_opcode_t *)&auprobe->insn,On Thu, 21 Aug 2025 14:28:24 +0200
Peter Zijlstra <peterz@infradead.org> wrote:
> Make is_optimized() return a tri-state and avoid return through
> argument. This simplifies things a little.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> arch/x86/kernel/uprobes.c | 34 +++++++++++++---------------------
> 1 file changed, 13 insertions(+), 21 deletions(-)
>
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -1047,7 +1047,7 @@ static bool __is_optimized(uprobe_opcode
> return __in_uprobe_trampoline(vaddr + 5 + call->raddr);
> }
>
> -static int is_optimized(struct mm_struct *mm, unsigned long vaddr, bool *optimized)
> +static int is_optimized(struct mm_struct *mm, unsigned long vaddr)
> {
> uprobe_opcode_t insn[5];
> int err;
> @@ -1055,8 +1055,7 @@ static int is_optimized(struct mm_struct
> err = copy_from_vaddr(mm, vaddr, &insn, 5);
> if (err)
> return err;
> - *optimized = __is_optimized((uprobe_opcode_t *)&insn, vaddr);
> - return 0;
> + return __is_optimized((uprobe_opcode_t *)&insn, vaddr);
> }
>
> static bool should_optimize(struct arch_uprobe *auprobe)
> @@ -1069,17 +1068,14 @@ int set_swbp(struct arch_uprobe *auprobe
> unsigned long vaddr)
> {
> if (should_optimize(auprobe)) {
> - bool optimized = false;
> - int err;
> -
> /*
> * We could race with another thread that already optimized the probe,
> * so let's not overwrite it with int3 again in this case.
> */
> - err = is_optimized(vma->vm_mm, vaddr, &optimized);
> - if (err)
> - return err;
> - if (optimized)
> + int ret = is_optimized(vma->vm_mm, vaddr);
> + if (ret < 0)
> + return ret;
> + if (ret)
> return 0;
Looks like you should swap over 0 and 1.
That would then be: if (ret <= 0) return ret;
David
> }
> return uprobe_write_opcode(auprobe, vma, vaddr, UPROBE_SWBP_INSN,
> @@ -1090,17 +1086,13 @@ int set_orig_insn(struct arch_uprobe *au
> unsigned long vaddr)
> {
> if (test_bit(ARCH_UPROBE_FLAG_CAN_OPTIMIZE, &auprobe->flags)) {
> - struct mm_struct *mm = vma->vm_mm;
> - bool optimized = false;
> - int err;
> -
> - err = is_optimized(mm, vaddr, &optimized);
> - if (err)
> - return err;
> - if (optimized) {
> - err = swbp_unoptimize(auprobe, vma, vaddr);
> - WARN_ON_ONCE(err);
> - return err;
> + int ret = is_optimized(vma->vm_mm, vaddr);
> + if (ret < 0)
> + return ret;
> + if (ret) {
> + ret = swbp_unoptimize(auprobe, vma, vaddr);
> + WARN_ON_ONCE(ret);
> + return ret;
> }
> }
> return uprobe_write_opcode(auprobe, vma, vaddr, *(uprobe_opcode_t *)&auprobe->insn,
>
>
>
On Tue, Aug 26, 2025 at 06:51:58AM +0100, David Laight wrote:
> On Thu, 21 Aug 2025 14:28:24 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
>
> > Make is_optimized() return a tri-state and avoid return through
> > argument. This simplifies things a little.
> >
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> > arch/x86/kernel/uprobes.c | 34 +++++++++++++---------------------
> > 1 file changed, 13 insertions(+), 21 deletions(-)
> >
> > --- a/arch/x86/kernel/uprobes.c
> > +++ b/arch/x86/kernel/uprobes.c
> > @@ -1047,7 +1047,7 @@ static bool __is_optimized(uprobe_opcode
> > return __in_uprobe_trampoline(vaddr + 5 + call->raddr);
> > }
> >
> > -static int is_optimized(struct mm_struct *mm, unsigned long vaddr, bool *optimized)
> > +static int is_optimized(struct mm_struct *mm, unsigned long vaddr)
> > {
> > uprobe_opcode_t insn[5];
> > int err;
> > @@ -1055,8 +1055,7 @@ static int is_optimized(struct mm_struct
> > err = copy_from_vaddr(mm, vaddr, &insn, 5);
> > if (err)
> > return err;
> > - *optimized = __is_optimized((uprobe_opcode_t *)&insn, vaddr);
> > - return 0;
> > + return __is_optimized((uprobe_opcode_t *)&insn, vaddr);
> > }
> >
> > static bool should_optimize(struct arch_uprobe *auprobe)
> > @@ -1069,17 +1068,14 @@ int set_swbp(struct arch_uprobe *auprobe
> > unsigned long vaddr)
> > {
> > if (should_optimize(auprobe)) {
> > - bool optimized = false;
> > - int err;
> > -
> > /*
> > * We could race with another thread that already optimized the probe,
> > * so let's not overwrite it with int3 again in this case.
> > */
> > - err = is_optimized(vma->vm_mm, vaddr, &optimized);
> > - if (err)
> > - return err;
> > - if (optimized)
> > + int ret = is_optimized(vma->vm_mm, vaddr);
> > + if (ret < 0)
> > + return ret;
> > + if (ret)
> > return 0;
>
> Looks like you should swap over 0 and 1.
> That would then be: if (ret <= 0) return ret;
hum, but if it's not optimized (ret == 0) we need to follow up with
installing breakpoint through following uprobe_write_opcode call
also I noticed we mix int/bool return, perhaps we could do fix below
jirka
---
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 0a8c0a4a5423..853abb2a5638 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -1064,7 +1064,7 @@ static int is_optimized(struct mm_struct *mm, unsigned long vaddr)
err = copy_from_vaddr(mm, vaddr, &insn, 5);
if (err)
return err;
- return __is_optimized((uprobe_opcode_t *)&insn, vaddr);
+ return __is_optimized((uprobe_opcode_t *)&insn, vaddr) ? 1 : 0;
}
static bool should_optimize(struct arch_uprobe *auprobe)
On Tue, Aug 26, 2025 at 10:25:29AM +0200, Jiri Olsa wrote:
> On Tue, Aug 26, 2025 at 06:51:58AM +0100, David Laight wrote:
> > On Thu, 21 Aug 2025 14:28:24 +0200
> > Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > > Make is_optimized() return a tri-state and avoid return through
> > > argument. This simplifies things a little.
> > >
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > ---
> > > arch/x86/kernel/uprobes.c | 34 +++++++++++++---------------------
> > > 1 file changed, 13 insertions(+), 21 deletions(-)
> > >
> > > --- a/arch/x86/kernel/uprobes.c
> > > +++ b/arch/x86/kernel/uprobes.c
> > > @@ -1047,7 +1047,7 @@ static bool __is_optimized(uprobe_opcode
> > > return __in_uprobe_trampoline(vaddr + 5 + call->raddr);
> > > }
> > >
> > > -static int is_optimized(struct mm_struct *mm, unsigned long vaddr, bool *optimized)
> > > +static int is_optimized(struct mm_struct *mm, unsigned long vaddr)
> > > {
> > > uprobe_opcode_t insn[5];
> > > int err;
> > > @@ -1055,8 +1055,7 @@ static int is_optimized(struct mm_struct
> > > err = copy_from_vaddr(mm, vaddr, &insn, 5);
> > > if (err)
> > > return err;
> > > - *optimized = __is_optimized((uprobe_opcode_t *)&insn, vaddr);
> > > - return 0;
> > > + return __is_optimized((uprobe_opcode_t *)&insn, vaddr);
> > > }
> > >
> > > static bool should_optimize(struct arch_uprobe *auprobe)
> > > @@ -1069,17 +1068,14 @@ int set_swbp(struct arch_uprobe *auprobe
> > > unsigned long vaddr)
> > > {
> > > if (should_optimize(auprobe)) {
> > > - bool optimized = false;
> > > - int err;
> > > -
> > > /*
> > > * We could race with another thread that already optimized the probe,
> > > * so let's not overwrite it with int3 again in this case.
> > > */
> > > - err = is_optimized(vma->vm_mm, vaddr, &optimized);
> > > - if (err)
> > > - return err;
> > > - if (optimized)
> > > + int ret = is_optimized(vma->vm_mm, vaddr);
> > > + if (ret < 0)
> > > + return ret;
> > > + if (ret)
> > > return 0;
> >
> > Looks like you should swap over 0 and 1.
> > That would then be: if (ret <= 0) return ret;
>
> hum, but if it's not optimized (ret == 0) we need to follow up with
> installing breakpoint through following uprobe_write_opcode call
ah u meant to swap the whole thing.. got it
>
> also I noticed we mix int/bool return, perhaps we could do fix below
>
> jirka
>
>
> ---
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 0a8c0a4a5423..853abb2a5638 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -1064,7 +1064,7 @@ static int is_optimized(struct mm_struct *mm, unsigned long vaddr)
> err = copy_from_vaddr(mm, vaddr, &insn, 5);
> if (err)
> return err;
> - return __is_optimized((uprobe_opcode_t *)&insn, vaddr);
> + return __is_optimized((uprobe_opcode_t *)&insn, vaddr) ? 1 : 0;
> }
>
> static bool should_optimize(struct arch_uprobe *auprobe)
On Tue, Aug 26, 2025 at 06:51:58AM +0100, David Laight wrote:
> > @@ -1069,17 +1068,14 @@ int set_swbp(struct arch_uprobe *auprobe
> > unsigned long vaddr)
> > {
> > if (should_optimize(auprobe)) {
> > - bool optimized = false;
> > - int err;
> > -
> > /*
> > * We could race with another thread that already optimized the probe,
> > * so let's not overwrite it with int3 again in this case.
> > */
> > - err = is_optimized(vma->vm_mm, vaddr, &optimized);
> > - if (err)
> > - return err;
> > - if (optimized)
> > + int ret = is_optimized(vma->vm_mm, vaddr);
> > + if (ret < 0)
> > + return ret;
> > + if (ret)
> > return 0;
>
> Looks like you should swap over 0 and 1.
> That would then be: if (ret <= 0) return ret;
I considered that, but that was actually more confusing. Yes the return
check is neat, but urgh.
The tri-state return is:
<0 -- error
0 -- false
1 -- true
and that is converted to the 'normal' convention:
<0 -- error
0 -- success
Making that intermediate:
<0 -- error
0 -- true
1 -- false
is just asking for trouble later.
On Tue, 26 Aug 2025 10:18:40 +0200
Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Aug 26, 2025 at 06:51:58AM +0100, David Laight wrote:
>
> > > @@ -1069,17 +1068,14 @@ int set_swbp(struct arch_uprobe *auprobe
> > > unsigned long vaddr)
> > > {
> > > if (should_optimize(auprobe)) {
> > > - bool optimized = false;
> > > - int err;
> > > -
> > > /*
> > > * We could race with another thread that already optimized the probe,
> > > * so let's not overwrite it with int3 again in this case.
> > > */
> > > - err = is_optimized(vma->vm_mm, vaddr, &optimized);
> > > - if (err)
> > > - return err;
> > > - if (optimized)
> > > + int ret = is_optimized(vma->vm_mm, vaddr);
> > > + if (ret < 0)
> > > + return ret;
> > > + if (ret)
> > > return 0;
> >
> > Looks like you should swap over 0 and 1.
> > That would then be: if (ret <= 0) return ret;
>
> I considered that, but that was actually more confusing. Yes the return
> check is neat, but urgh.
>
> The tri-state return is:
>
> <0 -- error
> 0 -- false
> 1 -- true
>
> and that is converted to the 'normal' convention:
>
> <0 -- error
> 0 -- success
>
>
> Making that intermediate:
>
> <0 -- error
> 0 -- true
> 1 -- false
>
> is just asking for trouble later.
I'm sure the function name could be changed to make it all work :-)
David
The following commit has been merged into the perf/core branch of tip:
Commit-ID: fd54052b60cf6e73cf918fd8653cd7a5c84b0cc3
Gitweb: https://git.kernel.org/tip/fd54052b60cf6e73cf918fd8653cd7a5c84b0cc3
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Wed, 20 Aug 2025 12:37:22 +02:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Thu, 21 Aug 2025 20:09:21 +02:00
uprobes/x86: Optimize is_optimize()
Make is_optimized() return a tri-state and avoid return through
argument. This simplifies things a little.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20250821123656.823296198@infradead.org
---
arch/x86/kernel/uprobes.c | 34 +++++++++++++---------------------
1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 580989d..3b46a89 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -1047,7 +1047,7 @@ static bool __is_optimized(uprobe_opcode_t *insn, unsigned long vaddr)
return __in_uprobe_trampoline(vaddr + 5 + call->raddr);
}
-static int is_optimized(struct mm_struct *mm, unsigned long vaddr, bool *optimized)
+static int is_optimized(struct mm_struct *mm, unsigned long vaddr)
{
uprobe_opcode_t insn[5];
int err;
@@ -1055,8 +1055,7 @@ static int is_optimized(struct mm_struct *mm, unsigned long vaddr, bool *optimiz
err = copy_from_vaddr(mm, vaddr, &insn, 5);
if (err)
return err;
- *optimized = __is_optimized((uprobe_opcode_t *)&insn, vaddr);
- return 0;
+ return __is_optimized((uprobe_opcode_t *)&insn, vaddr);
}
static bool should_optimize(struct arch_uprobe *auprobe)
@@ -1069,17 +1068,14 @@ int set_swbp(struct arch_uprobe *auprobe, struct vm_area_struct *vma,
unsigned long vaddr)
{
if (should_optimize(auprobe)) {
- bool optimized = false;
- int err;
-
/*
* We could race with another thread that already optimized the probe,
* so let's not overwrite it with int3 again in this case.
*/
- err = is_optimized(vma->vm_mm, vaddr, &optimized);
- if (err)
- return err;
- if (optimized)
+ int ret = is_optimized(vma->vm_mm, vaddr);
+ if (ret < 0)
+ return ret;
+ if (ret)
return 0;
}
return uprobe_write_opcode(auprobe, vma, vaddr, UPROBE_SWBP_INSN,
@@ -1090,17 +1086,13 @@ int set_orig_insn(struct arch_uprobe *auprobe, struct vm_area_struct *vma,
unsigned long vaddr)
{
if (test_bit(ARCH_UPROBE_FLAG_CAN_OPTIMIZE, &auprobe->flags)) {
- struct mm_struct *mm = vma->vm_mm;
- bool optimized = false;
- int err;
-
- err = is_optimized(mm, vaddr, &optimized);
- if (err)
- return err;
- if (optimized) {
- err = swbp_unoptimize(auprobe, vma, vaddr);
- WARN_ON_ONCE(err);
- return err;
+ int ret = is_optimized(vma->vm_mm, vaddr);
+ if (ret < 0)
+ return ret;
+ if (ret) {
+ ret = swbp_unoptimize(auprobe, vma, vaddr);
+ WARN_ON_ONCE(ret);
+ return ret;
}
}
return uprobe_write_opcode(auprobe, vma, vaddr, *(uprobe_opcode_t *)&auprobe->insn,© 2016 - 2025 Red Hat, Inc.