if (cond) {
A;
} else {
B;
}
C;
into
if (cond) {
A;
C;
} else {
B;
C;
}
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
kernel/events/core.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7043,6 +7043,8 @@ static int perf_mmap(struct file *file,
ret = 0;
/* We need the rb to map pages. */
rb = event->rb;
+ perf_mmap_account(vma, user_extra, extra);
+ atomic_inc(&event->mmap_count);
goto unlock;
}
@@ -7083,6 +7085,9 @@ static int perf_mmap(struct file *file,
perf_event_init_userpage(event);
perf_event_update_userpage(event);
ret = 0;
+
+ perf_mmap_account(vma, user_extra, extra);
+ atomic_inc(&event->mmap_count);
} else {
/*
* AUX area mapping: if rb->aux_nr_pages != 0, it's already
@@ -7127,11 +7132,12 @@ static int perf_mmap(struct file *file,
if (rb_has_aux(rb)) {
atomic_inc(&rb->aux_mmap_count);
ret = 0;
- goto unlock;
+ goto aux_success;
}
if (!perf_mmap_calc_limits(vma, &user_extra, &extra)) {
ret = -EPERM;
+ atomic_dec(&rb->mmap_count);
goto unlock;
}
@@ -7142,20 +7148,19 @@ static int perf_mmap(struct file *file,
ret = rb_alloc_aux(rb, event, vma->vm_pgoff, nr_pages,
event->attr.aux_watermark, flags);
- if (!ret) {
- atomic_set(&rb->aux_mmap_count, 1);
- rb->aux_mmap_locked = extra;
+ if (ret) {
+ atomic_dec(&rb->mmap_count);
+ goto unlock;
}
- }
-unlock:
- if (!ret) {
+ atomic_set(&rb->aux_mmap_count, 1);
+ rb->aux_mmap_locked = extra;
+aux_success:
perf_mmap_account(vma, user_extra, extra);
atomic_inc(&event->mmap_count);
- } else if (rb) {
- /* AUX allocation failed */
- atomic_dec(&rb->mmap_count);
}
+
+unlock:
aux_unlock:
if (aux_mutex)
mutex_unlock(aux_mutex);On Tue, Aug 12, 2025 at 12:39:04PM +0200, Peter Zijlstra wrote:
> if (cond) {
> A;
> } else {
> B;
> }
> C;
>
> into
>
> if (cond) {
> A;
> C;
> } else {
> B;
> C;
> }
>
Hm we're doing more than that here though, we're refactoring other stuff at
the same time.
I guess you're speaking broad strokes here, but maybe worth mentioniing the
tricksy hobbitses around the rb_has_aux() bit.
Anyway LGTM so:
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> kernel/events/core.c | 25 +++++++++++++++----------
> 1 file changed, 15 insertions(+), 10 deletions(-)
>
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -7043,6 +7043,8 @@ static int perf_mmap(struct file *file,
> ret = 0;
> /* We need the rb to map pages. */
> rb = event->rb;
> + perf_mmap_account(vma, user_extra, extra);
> + atomic_inc(&event->mmap_count);
> goto unlock;
> }
>
> @@ -7083,6 +7085,9 @@ static int perf_mmap(struct file *file,
> perf_event_init_userpage(event);
> perf_event_update_userpage(event);
> ret = 0;
> +
> + perf_mmap_account(vma, user_extra, extra);
> + atomic_inc(&event->mmap_count);
> } else {
> /*
> * AUX area mapping: if rb->aux_nr_pages != 0, it's already
> @@ -7127,11 +7132,12 @@ static int perf_mmap(struct file *file,
> if (rb_has_aux(rb)) {
> atomic_inc(&rb->aux_mmap_count);
> ret = 0;
> - goto unlock;
> + goto aux_success;
> }
>
> if (!perf_mmap_calc_limits(vma, &user_extra, &extra)) {
> ret = -EPERM;
> + atomic_dec(&rb->mmap_count);
> goto unlock;
> }
>
> @@ -7142,20 +7148,19 @@ static int perf_mmap(struct file *file,
>
> ret = rb_alloc_aux(rb, event, vma->vm_pgoff, nr_pages,
> event->attr.aux_watermark, flags);
> - if (!ret) {
> - atomic_set(&rb->aux_mmap_count, 1);
> - rb->aux_mmap_locked = extra;
> + if (ret) {
You dropped the 'AUX allocation failed' comment here, but honestly I think
that's fine, it's pretty obvious that's the case given the literal previous
line is you trying the AUX allocation... :)
> + atomic_dec(&rb->mmap_count);
> + goto unlock;
> }
> - }
>
> -unlock:
> - if (!ret) {
> + atomic_set(&rb->aux_mmap_count, 1);
> + rb->aux_mmap_locked = extra;
> +aux_success:
> perf_mmap_account(vma, user_extra, extra);
> atomic_inc(&event->mmap_count);
> - } else if (rb) {
> - /* AUX allocation failed */
> - atomic_dec(&rb->mmap_count);
> }
> +
> +unlock:
> aux_unlock:
Hm, this sucks, I assume this is a temporary thing to reduce churn above
and effective prove that the code paths are equivalently going to the same
place?
Which, given the complexity of the code, and the enormous ease with which
you can miss stuff (from personal experience...!), this is probably a
sensible way of doing it.
> if (aux_mutex)
> mutex_unlock(aux_mutex);
>
>
On Wed, Aug 13, 2025 at 06:55:51AM +0100, Lorenzo Stoakes wrote:
> On Tue, Aug 12, 2025 at 12:39:04PM +0200, Peter Zijlstra wrote:
> > if (cond) {
> > A;
> > } else {
> > B;
> > }
> > C;
> >
> > into
> >
> > if (cond) {
> > A;
> > C;
> > } else {
> > B;
> > C;
> > }
> >
>
> Hm we're doing more than that here though, we're refactoring other stuff at
> the same time.
>
> I guess you're speaking broad strokes here, but maybe worth mentioniing the
> tricksy hobbitses around the rb_has_aux() bit.
Does something like so clarify:
Notably C has a success branch and both A and B have two places for
success. For A (rb case), duplicate the success case because later
patches will result in them no longer being identical. For B (aux
case), share using goto (cleaned up later).
> Anyway LGTM so:
>
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> > Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> > kernel/events/core.c | 25 +++++++++++++++----------
> > 1 file changed, 15 insertions(+), 10 deletions(-)
> >
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -7043,6 +7043,8 @@ static int perf_mmap(struct file *file,
> > ret = 0;
> > /* We need the rb to map pages. */
> > rb = event->rb;
> > + perf_mmap_account(vma, user_extra, extra);
> > + atomic_inc(&event->mmap_count);
> > goto unlock;
> > }
> >
> > @@ -7083,6 +7085,9 @@ static int perf_mmap(struct file *file,
> > perf_event_init_userpage(event);
> > perf_event_update_userpage(event);
> > ret = 0;
> > +
> > + perf_mmap_account(vma, user_extra, extra);
> > + atomic_inc(&event->mmap_count);
> > } else {
> > /*
> > * AUX area mapping: if rb->aux_nr_pages != 0, it's already
> > @@ -7127,11 +7132,12 @@ static int perf_mmap(struct file *file,
> > if (rb_has_aux(rb)) {
> > atomic_inc(&rb->aux_mmap_count);
> > ret = 0;
> > - goto unlock;
> > + goto aux_success;
> > }
> >
> > if (!perf_mmap_calc_limits(vma, &user_extra, &extra)) {
> > ret = -EPERM;
> > + atomic_dec(&rb->mmap_count);
> > goto unlock;
> > }
> >
> > @@ -7142,20 +7148,19 @@ static int perf_mmap(struct file *file,
> >
> > ret = rb_alloc_aux(rb, event, vma->vm_pgoff, nr_pages,
> > event->attr.aux_watermark, flags);
> > - if (!ret) {
> > - atomic_set(&rb->aux_mmap_count, 1);
> > - rb->aux_mmap_locked = extra;
> > + if (ret) {
>
> You dropped the 'AUX allocation failed' comment here, but honestly I think
> that's fine, it's pretty obvious that's the case given the literal previous
> line is you trying the AUX allocation... :)
Yes that :-)
On Wed, Aug 13, 2025 at 10:25:24AM +0200, Peter Zijlstra wrote:
> On Wed, Aug 13, 2025 at 06:55:51AM +0100, Lorenzo Stoakes wrote:
> > On Tue, Aug 12, 2025 at 12:39:04PM +0200, Peter Zijlstra wrote:
> > > if (cond) {
> > > A;
> > > } else {
> > > B;
> > > }
> > > C;
> > >
> > > into
> > >
> > > if (cond) {
> > > A;
> > > C;
> > > } else {
> > > B;
> > > C;
> > > }
> > >
> >
> > Hm we're doing more than that here though, we're refactoring other stuff at
> > the same time.
> >
> > I guess you're speaking broad strokes here, but maybe worth mentioniing the
> > tricksy hobbitses around the rb_has_aux() bit.
>
> Does something like so clarify:
>
> Notably C has a success branch and both A and B have two places for
> success. For A (rb case), duplicate the success case because later
> patches will result in them no longer being identical. For B (aux
> case), share using goto (cleaned up later).
Sounds great, thanks!
>
> > Anyway LGTM so:
> >
> > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> >
> > > Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > > ---
> > > kernel/events/core.c | 25 +++++++++++++++----------
> > > 1 file changed, 15 insertions(+), 10 deletions(-)
[snip]
> > > @@ -7142,20 +7148,19 @@ static int perf_mmap(struct file *file,
> > >
> > > ret = rb_alloc_aux(rb, event, vma->vm_pgoff, nr_pages,
> > > event->attr.aux_watermark, flags);
> > > - if (!ret) {
> > > - atomic_set(&rb->aux_mmap_count, 1);
> > > - rb->aux_mmap_locked = extra;
> > > + if (ret) {
> >
> > You dropped the 'AUX allocation failed' comment here, but honestly I think
> > that's fine, it's pretty obvious that's the case given the literal previous
> > line is you trying the AUX allocation... :)
>
> Yes that :-)
:)
The following commit has been merged into the perf/core branch of tip:
Commit-ID: 4118994b33bb628dd9aeb941c5af6f950f1dea90
Gitweb: https://git.kernel.org/tip/4118994b33bb628dd9aeb941c5af6f950f1dea90
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Tue, 12 Aug 2025 12:39:04 +02:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Fri, 15 Aug 2025 13:12:59 +02:00
perf: Move common code into both rb and aux branches
if (cond) {
A;
} else {
B;
}
C;
into
if (cond) {
A;
C;
} else {
B;
C;
}
Notably C has a success branch and both A and B have two places for
success. For A (rb case), duplicate the success case because later
patches will result in them no longer being identical. For B (aux
case), share using goto (cleaned up later).
Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Link: https://lore.kernel.org/r/20250812104019.016252852@infradead.org
---
kernel/events/core.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 085f36f..dfe09b0 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -7043,6 +7043,8 @@ static int perf_mmap(struct file *file, struct vm_area_struct *vma)
ret = 0;
/* We need the rb to map pages. */
rb = event->rb;
+ perf_mmap_account(vma, user_extra, extra);
+ atomic_inc(&event->mmap_count);
goto unlock;
}
@@ -7083,6 +7085,9 @@ static int perf_mmap(struct file *file, struct vm_area_struct *vma)
perf_event_init_userpage(event);
perf_event_update_userpage(event);
ret = 0;
+
+ perf_mmap_account(vma, user_extra, extra);
+ atomic_inc(&event->mmap_count);
} else {
/*
* AUX area mapping: if rb->aux_nr_pages != 0, it's already
@@ -7127,11 +7132,12 @@ static int perf_mmap(struct file *file, struct vm_area_struct *vma)
if (rb_has_aux(rb)) {
atomic_inc(&rb->aux_mmap_count);
ret = 0;
- goto unlock;
+ goto aux_success;
}
if (!perf_mmap_calc_limits(vma, &user_extra, &extra)) {
ret = -EPERM;
+ atomic_dec(&rb->mmap_count);
goto unlock;
}
@@ -7142,20 +7148,19 @@ static int perf_mmap(struct file *file, struct vm_area_struct *vma)
ret = rb_alloc_aux(rb, event, vma->vm_pgoff, nr_pages,
event->attr.aux_watermark, flags);
- if (!ret) {
- atomic_set(&rb->aux_mmap_count, 1);
- rb->aux_mmap_locked = extra;
+ if (ret) {
+ atomic_dec(&rb->mmap_count);
+ goto unlock;
}
- }
-unlock:
- if (!ret) {
+ atomic_set(&rb->aux_mmap_count, 1);
+ rb->aux_mmap_locked = extra;
+aux_success:
perf_mmap_account(vma, user_extra, extra);
atomic_inc(&event->mmap_count);
- } else if (rb) {
- /* AUX allocation failed */
- atomic_dec(&rb->mmap_count);
}
+
+unlock:
aux_unlock:
if (aux_mutex)
mutex_unlock(aux_mutex);© 2016 - 2025 Red Hat, Inc.