[PATCH 00/13] perf: Employ SBRM to simplify error handling -- batch 1/3

[PATCH 00/13] perf: Employ SBRM to simplify error handling -- batch 1/3

[Peter Zijlstra]

Hi,

These few patches start the process of using Scope Based Resource Management
(SBRM) for the perf core.

As per the subject, I have cut all the patches I have for this into 3 separate
parts so as to not overload you all with some 35+ patches.

I plan to post the next batch in about a week or so, we'll see. I'm hoping we
can get them all merged in time for 6.8.

The full pile can be admired here:

  git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git perf/guards

[PATCH 01/13] perf: Simplify perf_event_alloc() error path

[Peter Zijlstra]

The error cleanup sequence in perf_event_alloc() is a subset of the
existing _free_event() function (it must of course be).

Split this out into __free_event() and simplify the error path.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/perf_event.h |    1 
 kernel/events/core.c       |  129 ++++++++++++++++++++++-----------------------
 2 files changed, 66 insertions(+), 64 deletions(-)

--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -634,6 +634,7 @@ struct swevent_hlist {
 #define PERF_ATTACH_ITRACE	0x10
 #define PERF_ATTACH_SCHED_CB	0x20
 #define PERF_ATTACH_CHILD	0x40
+#define PERF_ATTACH_EXCLUSIVE	0x80
 
 struct bpf_prog;
 struct perf_cgroup;
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5094,6 +5094,8 @@ static int exclusive_event_init(struct p
 			return -EBUSY;
 	}
 
+	event->attach_state |= PERF_ATTACH_EXCLUSIVE;
+
 	return 0;
 }
 
@@ -5101,14 +5103,13 @@ static void exclusive_event_destroy(stru
 {
 	struct pmu *pmu = event->pmu;
 
-	if (!is_exclusive_pmu(pmu))
-		return;
-
 	/* see comment in exclusive_event_init() */
 	if (event->attach_state & PERF_ATTACH_TASK)
 		atomic_dec(&pmu->exclusive_cnt);
 	else
 		atomic_inc(&pmu->exclusive_cnt);
+
+	event->attach_state &= ~PERF_ATTACH_EXCLUSIVE;
 }
 
 static bool exclusive_event_match(struct perf_event *e1, struct perf_event *e2)
@@ -5143,38 +5144,22 @@ static bool exclusive_event_installable(
 static void perf_addr_filters_splice(struct perf_event *event,
 				       struct list_head *head);
 
-static void _free_event(struct perf_event *event)
+/* vs perf_event_alloc() error */
+static void __free_event(struct perf_event *event)
 {
-	irq_work_sync(&event->pending_irq);
-
-	unaccount_event(event);
-
-	security_perf_event_free(event);
-
-	if (event->rb) {
-		/*
-		 * Can happen when we close an event with re-directed output.
-		 *
-		 * Since we have a 0 refcount, perf_mmap_close() will skip
-		 * over us; possibly making our ring_buffer_put() the last.
-		 */
-		mutex_lock(&event->mmap_mutex);
-		ring_buffer_attach(event, NULL);
-		mutex_unlock(&event->mmap_mutex);
-	}
-
-	if (is_cgroup_event(event))
-		perf_detach_cgroup(event);
-
 	if (!event->parent) {
 		if (event->attr.sample_type & PERF_SAMPLE_CALLCHAIN)
 			put_callchain_buffers();
 	}
 
-	perf_event_free_bpf_prog(event);
-	perf_addr_filters_splice(event, NULL);
 	kfree(event->addr_filter_ranges);
 
+	if (event->attach_state & PERF_ATTACH_EXCLUSIVE)
+		exclusive_event_destroy(event);
+
+	if (is_cgroup_event(event))
+		perf_detach_cgroup(event);
+
 	if (event->destroy)
 		event->destroy(event);
 
@@ -5185,22 +5170,56 @@ static void _free_event(struct perf_even
 	if (event->hw.target)
 		put_task_struct(event->hw.target);
 
-	if (event->pmu_ctx)
+	if (event->pmu_ctx) {
+		/*
+		 * put_pmu_ctx() needs an event->ctx reference, because of
+		 * epc->ctx.
+		 */
+		WARN_ON_ONCE(!event->ctx);
+		WARN_ON_ONCE(event->pmu_ctx->ctx != event->ctx);
 		put_pmu_ctx(event->pmu_ctx);
+	}
 
 	/*
-	 * perf_event_free_task() relies on put_ctx() being 'last', in particular
-	 * all task references must be cleaned up.
+	 * perf_event_free_task() relies on put_ctx() being 'last', in
+	 * particular all task references must be cleaned up.
 	 */
 	if (event->ctx)
 		put_ctx(event->ctx);
 
-	exclusive_event_destroy(event);
-	module_put(event->pmu->module);
+	if (event->pmu)
+		module_put(event->pmu->module);
 
 	call_rcu(&event->rcu_head, free_event_rcu);
 }
 
+/* vs perf_event_alloc() success */
+static void _free_event(struct perf_event *event)
+{
+	irq_work_sync(&event->pending_irq);
+
+	unaccount_event(event);
+
+	security_perf_event_free(event);
+
+	if (event->rb) {
+		/*
+		 * Can happen when we close an event with re-directed output.
+		 *
+		 * Since we have a 0 refcount, perf_mmap_close() will skip
+		 * over us; possibly making our ring_buffer_put() the last.
+		 */
+		mutex_lock(&event->mmap_mutex);
+		ring_buffer_attach(event, NULL);
+		mutex_unlock(&event->mmap_mutex);
+	}
+
+	perf_event_free_bpf_prog(event);
+	perf_addr_filters_splice(event, NULL);
+
+	__free_event(event);
+}
+
 /*
  * Used to free events which have a known refcount of 1, such as in error paths
  * where the event isn't exposed yet and inherited events.
@@ -11591,8 +11610,10 @@ static int perf_try_init_event(struct pm
 			event->destroy(event);
 	}
 
-	if (ret)
+	if (ret) {
+		event->pmu = NULL;
 		module_put(pmu->module);
+	}
 
 	return ret;
 }
@@ -11918,7 +11939,7 @@ perf_event_alloc(struct perf_event_attr
 	 * See perf_output_read().
 	 */
 	if (attr->inherit && (attr->sample_type & PERF_SAMPLE_READ))
-		goto err_ns;
+		goto err;
 
 	if (!has_branch_stack(event))
 		event->attr.branch_sample_type = 0;
@@ -11926,7 +11947,7 @@ perf_event_alloc(struct perf_event_attr
 	pmu = perf_init_event(event);
 	if (IS_ERR(pmu)) {
 		err = PTR_ERR(pmu);
-		goto err_ns;
+		goto err;
 	}
 
 	/*
@@ -11936,24 +11957,24 @@ perf_event_alloc(struct perf_event_attr
 	 */
 	if (pmu->task_ctx_nr == perf_invalid_context && (task || cgroup_fd != -1)) {
 		err = -EINVAL;
-		goto err_pmu;
+		goto err;
 	}
 
 	if (event->attr.aux_output &&
 	    !(pmu->capabilities & PERF_PMU_CAP_AUX_OUTPUT)) {
 		err = -EOPNOTSUPP;
-		goto err_pmu;
+		goto err;
 	}
 
 	if (cgroup_fd != -1) {
 		err = perf_cgroup_connect(cgroup_fd, event, attr, group_leader);
 		if (err)
-			goto err_pmu;
+			goto err;
 	}
 
 	err = exclusive_event_init(event);
 	if (err)
-		goto err_pmu;
+		goto err;
 
 	if (has_addr_filter(event)) {
 		event->addr_filter_ranges = kcalloc(pmu->nr_addr_filters,
@@ -11961,7 +11982,7 @@ perf_event_alloc(struct perf_event_attr
 						    GFP_KERNEL);
 		if (!event->addr_filter_ranges) {
 			err = -ENOMEM;
-			goto err_per_task;
+			goto err;
 		}
 
 		/*
@@ -11986,41 +12007,21 @@ perf_event_alloc(struct perf_event_attr
 		if (event->attr.sample_type & PERF_SAMPLE_CALLCHAIN) {
 			err = get_callchain_buffers(attr->sample_max_stack);
 			if (err)
-				goto err_addr_filters;
+				goto err;
 		}
 	}
 
 	err = security_perf_event_alloc(event);
 	if (err)
-		goto err_callchain_buffer;
+		goto err;
 
 	/* symmetric to unaccount_event() in _free_event() */
 	account_event(event);
 
 	return event;
 
-err_callchain_buffer:
-	if (!event->parent) {
-		if (event->attr.sample_type & PERF_SAMPLE_CALLCHAIN)
-			put_callchain_buffers();
-	}
-err_addr_filters:
-	kfree(event->addr_filter_ranges);
-
-err_per_task:
-	exclusive_event_destroy(event);
-
-err_pmu:
-	if (is_cgroup_event(event))
-		perf_detach_cgroup(event);
-	if (event->destroy)
-		event->destroy(event);
-	module_put(pmu->module);
-err_ns:
-	if (event->hw.target)
-		put_task_struct(event->hw.target);
-	call_rcu(&event->rcu_head, free_event_rcu);
-
+err:
+	__free_event(event);
 	return ERR_PTR(err);
 }
 

Re: [PATCH 01/13] perf: Simplify perf_event_alloc() error path

[Jiri Olsa]

On Thu, Nov 02, 2023 at 04:09:20PM +0100, Peter Zijlstra wrote:

SNIP

> @@ -11936,24 +11957,24 @@ perf_event_alloc(struct perf_event_attr
>  	 */
>  	if (pmu->task_ctx_nr == perf_invalid_context && (task || cgroup_fd != -1)) {
>  		err = -EINVAL;
> -		goto err_pmu;
> +		goto err;
>  	}
>  
>  	if (event->attr.aux_output &&
>  	    !(pmu->capabilities & PERF_PMU_CAP_AUX_OUTPUT)) {
>  		err = -EOPNOTSUPP;
> -		goto err_pmu;
> +		goto err;
>  	}
>  
>  	if (cgroup_fd != -1) {
>  		err = perf_cgroup_connect(cgroup_fd, event, attr, group_leader);
>  		if (err)
> -			goto err_pmu;
> +			goto err;
>  	}
>  
>  	err = exclusive_event_init(event);
>  	if (err)
> -		goto err_pmu;
> +		goto err;
>  
>  	if (has_addr_filter(event)) {
>  		event->addr_filter_ranges = kcalloc(pmu->nr_addr_filters,
> @@ -11961,7 +11982,7 @@ perf_event_alloc(struct perf_event_attr
>  						    GFP_KERNEL);
>  		if (!event->addr_filter_ranges) {
>  			err = -ENOMEM;
> -			goto err_per_task;
> +			goto err;
>  		}
>  
>  		/*
> @@ -11986,41 +12007,21 @@ perf_event_alloc(struct perf_event_attr
>  		if (event->attr.sample_type & PERF_SAMPLE_CALLCHAIN) {
>  			err = get_callchain_buffers(attr->sample_max_stack);
>  			if (err)
> -				goto err_addr_filters;
> +				goto err;
>  		}
>  	}
>  
>  	err = security_perf_event_alloc(event);
>  	if (err)
> -		goto err_callchain_buffer;
> +		goto err;
>  
>  	/* symmetric to unaccount_event() in _free_event() */
>  	account_event(event);
>  
>  	return event;
>  
> -err_callchain_buffer:
> -	if (!event->parent) {
> -		if (event->attr.sample_type & PERF_SAMPLE_CALLCHAIN)
> -			put_callchain_buffers();
> -	}

hum, so this is now called all the time via __free_event, but it should
be called only if we passed get_callchain_buffers call.. this could screw
up nr_callchain_events number eventually no?

jirka

> -err_addr_filters:
> -	kfree(event->addr_filter_ranges);
> -
> -err_per_task:
> -	exclusive_event_destroy(event);
> -
> -err_pmu:
> -	if (is_cgroup_event(event))
> -		perf_detach_cgroup(event);
> -	if (event->destroy)
> -		event->destroy(event);
> -	module_put(pmu->module);
> -err_ns:
> -	if (event->hw.target)
> -		put_task_struct(event->hw.target);
> -	call_rcu(&event->rcu_head, free_event_rcu);
> -
> +err:
> +	__free_event(event);
>  	return ERR_PTR(err);
>  }
>  
> 
> 

Re: [PATCH 01/13] perf: Simplify perf_event_alloc() error path

[Namhyung Kim]

Hi Jiri and Peter,

On Fri, Nov 3, 2023 at 5:38 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Thu, Nov 02, 2023 at 04:09:20PM +0100, Peter Zijlstra wrote:
>
> SNIP
>
> > @@ -11936,24 +11957,24 @@ perf_event_alloc(struct perf_event_attr
> >        */
> >       if (pmu->task_ctx_nr == perf_invalid_context && (task || cgroup_fd != -1)) {
> >               err = -EINVAL;
> > -             goto err_pmu;
> > +             goto err;
> >       }
> >
> >       if (event->attr.aux_output &&
> >           !(pmu->capabilities & PERF_PMU_CAP_AUX_OUTPUT)) {
> >               err = -EOPNOTSUPP;
> > -             goto err_pmu;
> > +             goto err;
> >       }
> >
> >       if (cgroup_fd != -1) {
> >               err = perf_cgroup_connect(cgroup_fd, event, attr, group_leader);
> >               if (err)
> > -                     goto err_pmu;
> > +                     goto err;
> >       }
> >
> >       err = exclusive_event_init(event);
> >       if (err)
> > -             goto err_pmu;
> > +             goto err;
> >
> >       if (has_addr_filter(event)) {
> >               event->addr_filter_ranges = kcalloc(pmu->nr_addr_filters,
> > @@ -11961,7 +11982,7 @@ perf_event_alloc(struct perf_event_attr
> >                                                   GFP_KERNEL);
> >               if (!event->addr_filter_ranges) {
> >                       err = -ENOMEM;
> > -                     goto err_per_task;
> > +                     goto err;
> >               }
> >
> >               /*
> > @@ -11986,41 +12007,21 @@ perf_event_alloc(struct perf_event_attr
> >               if (event->attr.sample_type & PERF_SAMPLE_CALLCHAIN) {
> >                       err = get_callchain_buffers(attr->sample_max_stack);
> >                       if (err)
> > -                             goto err_addr_filters;
> > +                             goto err;
> >               }
> >       }
> >
> >       err = security_perf_event_alloc(event);
> >       if (err)
> > -             goto err_callchain_buffer;
> > +             goto err;
> >
> >       /* symmetric to unaccount_event() in _free_event() */
> >       account_event(event);
> >
> >       return event;
> >
> > -err_callchain_buffer:
> > -     if (!event->parent) {
> > -             if (event->attr.sample_type & PERF_SAMPLE_CALLCHAIN)
> > -                     put_callchain_buffers();
> > -     }
>
> hum, so this is now called all the time via __free_event, but it should
> be called only if we passed get_callchain_buffers call.. this could screw
> up nr_callchain_events number eventually no?

Looks like so.

>
> jirka
>
> > -err_addr_filters:
> > -     kfree(event->addr_filter_ranges);
> > -
> > -err_per_task:
> > -     exclusive_event_destroy(event);
> > -
> > -err_pmu:
> > -     if (is_cgroup_event(event))
> > -             perf_detach_cgroup(event);
> > -     if (event->destroy)
> > -             event->destroy(event);
> > -     module_put(pmu->module);

I'm afraid of this part.  If it failed at perf_init_event(), it might
call event->destroy() already.  I saw you cleared event->pmu
in the failure path.  Maybe we need the same thing for the
event->destroy.

Thanks,
Namhyung


> > -err_ns:
> > -     if (event->hw.target)
> > -             put_task_struct(event->hw.target);
> > -     call_rcu(&event->rcu_head, free_event_rcu);
> > -
> > +err:
> > +     __free_event(event);
> >       return ERR_PTR(err);
> >  }
> >
> >
> >

Re: [PATCH 01/13] perf: Simplify perf_event_alloc() error path

[Peter Zijlstra]

On Fri, Nov 03, 2023 at 12:50:19PM -0700, Namhyung Kim wrote:

> > > -err_pmu:
> > > -     if (is_cgroup_event(event))
> > > -             perf_detach_cgroup(event);
> > > -     if (event->destroy)
> > > -             event->destroy(event);
> > > -     module_put(pmu->module);
> 
> I'm afraid of this part.  If it failed at perf_init_event(), it might
> call event->destroy() already.  I saw you cleared event->pmu
> in the failure path.  Maybe we need the same thing for the
> event->destroy.

In that case event->destroy will not yet have been set, no?

And once it is set, we should call it.

Re: [PATCH 01/13] perf: Simplify perf_event_alloc() error path

[Namhyung Kim]

On Wed, Nov 15, 2023 at 1:58 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Fri, Nov 03, 2023 at 12:50:19PM -0700, Namhyung Kim wrote:
>
> > > > -err_pmu:
> > > > -     if (is_cgroup_event(event))
> > > > -             perf_detach_cgroup(event);
> > > > -     if (event->destroy)
> > > > -             event->destroy(event);
> > > > -     module_put(pmu->module);
> >
> > I'm afraid of this part.  If it failed at perf_init_event(), it might
> > call event->destroy() already.  I saw you cleared event->pmu
> > in the failure path.  Maybe we need the same thing for the
> > event->destroy.
>
> In that case event->destroy will not yet have been set, no?

In perf_try_init_event(), it calls event->destroy() if set when it
has EXTENDED_REGS or NO_EXCLUDE capabilities and returns an error.

Thanks,
Namhyung

Re: [PATCH 01/13] perf: Simplify perf_event_alloc() error path

[Peter Zijlstra]

On Fri, Nov 03, 2023 at 01:38:05PM +0100, Jiri Olsa wrote:

> > -err_callchain_buffer:
> > -	if (!event->parent) {
> > -		if (event->attr.sample_type & PERF_SAMPLE_CALLCHAIN)
> > -			put_callchain_buffers();
> > -	}
> 
> hum, so this is now called all the time via __free_event, but it should
> be called only if we passed get_callchain_buffers call.. this could screw
> up nr_callchain_events number eventually no?

Yes, good catch, thanks!

Something like the below should handle that, no?

---
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -628,14 +628,15 @@ struct swevent_hlist {
 	struct rcu_head			rcu_head;
 };
 
-#define PERF_ATTACH_CONTEXT	0x01
-#define PERF_ATTACH_GROUP	0x02
-#define PERF_ATTACH_TASK	0x04
-#define PERF_ATTACH_TASK_DATA	0x08
-#define PERF_ATTACH_ITRACE	0x10
-#define PERF_ATTACH_SCHED_CB	0x20
-#define PERF_ATTACH_CHILD	0x40
-#define PERF_ATTACH_EXCLUSIVE	0x80
+#define PERF_ATTACH_CONTEXT	0x0001
+#define PERF_ATTACH_GROUP	0x0002
+#define PERF_ATTACH_TASK	0x0004
+#define PERF_ATTACH_TASK_DATA	0x0008
+#define PERF_ATTACH_ITRACE	0x0010
+#define PERF_ATTACH_SCHED_CB	0x0020
+#define PERF_ATTACH_CHILD	0x0040
+#define PERF_ATTACH_EXCLUSIVE	0x0080
+#define PERF_ATTACH_CALLCHAIN	0x0100
 
 struct bpf_prog;
 struct perf_cgroup;
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5166,10 +5166,8 @@ static void perf_addr_filters_splice(str
 /* vs perf_event_alloc() error */
 static void __free_event(struct perf_event *event)
 {
-	if (!event->parent) {
-		if (event->attr.sample_type & PERF_SAMPLE_CALLCHAIN)
-			put_callchain_buffers();
-	}
+	if (event->attach_state & PERF_ATTACH_CALLCHAIN)
+		put_callchain_buffers();
 
 	kfree(event->addr_filter_ranges);
 
@@ -12065,6 +12063,7 @@ perf_event_alloc(struct perf_event_attr
 			err = get_callchain_buffers(attr->sample_max_stack);
 			if (err)
 				goto err;
+			event->attach_state |= PERF_ATTACH_CALLCHAIN;
 		}
 	}
 

[PATCH 02/13] perf: Simplify perf_pmu_register() error path

[Peter Zijlstra]

The error path of perf_pmu_register() is of course very similar to a
subset of perf_pmu_unregister(). Extract this common part in
perf_pmu_free() and simplify things.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/events/core.c |   51 ++++++++++++++++++++++++---------------------------
 1 file changed, 24 insertions(+), 27 deletions(-)

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -11426,20 +11426,35 @@ static int pmu_dev_alloc(struct pmu *pmu
 static struct lock_class_key cpuctx_mutex;
 static struct lock_class_key cpuctx_lock;
 
+static void perf_pmu_free(struct pmu *pmu)
+{
+	free_percpu(pmu->pmu_disable_count);
+	if (pmu->type >= 0)
+		idr_remove(&pmu_idr, pmu->type);
+	if (pmu_bus_running && pmu->dev && pmu->dev != PMU_NULL_DEV) {
+		if (pmu->nr_addr_filters)
+			device_remove_file(pmu->dev, &dev_attr_nr_addr_filters);
+		device_del(pmu->dev);
+		put_device(pmu->dev);
+	}
+	free_pmu_context(pmu);
+}
+
 int perf_pmu_register(struct pmu *pmu, const char *name, int type)
 {
 	int cpu, ret, max = PERF_TYPE_MAX;
 
+	pmu->type = -1;
+
 	mutex_lock(&pmus_lock);
 	ret = -ENOMEM;
 	pmu->pmu_disable_count = alloc_percpu(int);
 	if (!pmu->pmu_disable_count)
 		goto unlock;
 
-	pmu->type = -1;
 	if (WARN_ONCE(!name, "Can not register anonymous pmu.\n")) {
 		ret = -EINVAL;
-		goto free_pdc;
+		goto free;
 	}
 
 	pmu->name = name;
@@ -11449,23 +11464,22 @@ int perf_pmu_register(struct pmu *pmu, c
 
 	ret = idr_alloc(&pmu_idr, pmu, max, 0, GFP_KERNEL);
 	if (ret < 0)
-		goto free_pdc;
+		goto free;
 
 	WARN_ON(type >= 0 && ret != type);
 
-	type = ret;
-	pmu->type = type;
+	pmu->type = ret;
 
 	if (pmu_bus_running && !pmu->dev) {
 		ret = pmu_dev_alloc(pmu);
 		if (ret)
-			goto free_idr;
+			goto free;
 	}
 
 	ret = -ENOMEM;
 	pmu->cpu_pmu_context = alloc_percpu(struct perf_cpu_pmu_context);
 	if (!pmu->cpu_pmu_context)
-		goto free_dev;
+		goto free;
 
 	for_each_possible_cpu(cpu) {
 		struct perf_cpu_pmu_context *cpc;
@@ -11511,17 +11525,8 @@ int perf_pmu_register(struct pmu *pmu, c
 
 	return ret;
 
-free_dev:
-	if (pmu->dev && pmu->dev != PMU_NULL_DEV) {
-		device_del(pmu->dev);
-		put_device(pmu->dev);
-	}
-
-free_idr:
-	idr_remove(&pmu_idr, pmu->type);
-
-free_pdc:
-	free_percpu(pmu->pmu_disable_count);
+free:
+	perf_pmu_free(pmu);
 	goto unlock;
 }
 EXPORT_SYMBOL_GPL(perf_pmu_register);
@@ -11538,15 +11543,7 @@ void perf_pmu_unregister(struct pmu *pmu
 	synchronize_srcu(&pmus_srcu);
 	synchronize_rcu();
 
-	free_percpu(pmu->pmu_disable_count);
-	idr_remove(&pmu_idr, pmu->type);
-	if (pmu_bus_running && pmu->dev && pmu->dev != PMU_NULL_DEV) {
-		if (pmu->nr_addr_filters)
-			device_remove_file(pmu->dev, &dev_attr_nr_addr_filters);
-		device_del(pmu->dev);
-		put_device(pmu->dev);
-	}
-	free_pmu_context(pmu);
+	perf_pmu_free(pmu);
 	mutex_unlock(&pmus_lock);
 }
 EXPORT_SYMBOL_GPL(perf_pmu_unregister);

[PATCH 03/13] perf: Simplify perf_fget_light()

[Peter Zijlstra]

Introduce fdnull and use it to simplify perf_fget_light() to either
return a valid struct fd or not -- much like fdget() itself.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/file.h |    7 ++++++-
 kernel/events/core.c |   22 +++++++++++-----------
 2 files changed, 17 insertions(+), 12 deletions(-)

--- a/include/linux/file.h
+++ b/include/linux/file.h
@@ -59,6 +59,8 @@ static inline struct fd __to_fd(unsigned
 	return (struct fd){(struct file *)(v & ~3),v & 3};
 }
 
+#define fdnull __to_fd(0)
+
 static inline struct fd fdget(unsigned int fd)
 {
 	return __to_fd(__fdget(fd));
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5802,18 +5802,17 @@ EXPORT_SYMBOL_GPL(perf_event_period);
 
 static const struct file_operations perf_fops;
 
-static inline int perf_fget_light(int fd, struct fd *p)
+static inline struct fd perf_fdget(int fd)
 {
 	struct fd f = fdget(fd);
 	if (!f.file)
-		return -EBADF;
+		return fdnull;
 
 	if (f.file->f_op != &perf_fops) {
 		fdput(f);
-		return -EBADF;
+		return fdnull;
 	}
-	*p = f;
-	return 0;
+	return f;
 }
 
 static int perf_event_set_output(struct perf_event *event,
@@ -5864,10 +5863,9 @@ static long _perf_ioctl(struct perf_even
 		int ret;
 		if (arg != -1) {
 			struct perf_event *output_event;
-			struct fd output;
-			ret = perf_fget_light(arg, &output);
-			if (ret)
-				return ret;
+			struct fd output = perf_fdget(arg);
+			if (!output.file)
+				return -EBADF;
 			output_event = output.file->private_data;
 			ret = perf_event_set_output(event, output_event);
 			fdput(output);
@@ -12401,9 +12399,11 @@ SYSCALL_DEFINE5(perf_event_open,
 		return event_fd;
 
 	if (group_fd != -1) {
-		err = perf_fget_light(group_fd, &group);
-		if (err)
+		group = perf_fdget(group_fd);
+		if (!group.file) {
+			err = -EBADF;
 			goto err_fd;
+		}
 		group_leader = group.file->private_data;
 		if (flags & PERF_FLAG_FD_OUTPUT)
 			output_event = group_leader;

[PATCH 04/13] perf: Simplify event_function*()

[Peter Zijlstra]

Use guards to reduce gotos and simplify control flow.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/events/core.c |   36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -214,6 +214,19 @@ struct event_function_struct {
 	void *data;
 };
 
+typedef struct {
+	struct perf_cpu_context *cpuctx;
+	struct perf_event_context *ctx;
+} class_perf_ctx_lock_t;
+
+static inline void class_perf_ctx_lock_destructor(class_perf_ctx_lock_t *_T)
+{ perf_ctx_unlock(_T->cpuctx, _T->ctx); }
+
+static inline class_perf_ctx_lock_t
+class_perf_ctx_lock_constructor(struct perf_cpu_context *cpuctx,
+				struct perf_event_context *ctx)
+{ perf_ctx_lock(cpuctx, ctx); return (class_perf_ctx_lock_t){ cpuctx, ctx }; }
+
 static int event_function(void *info)
 {
 	struct event_function_struct *efs = info;
@@ -221,20 +234,17 @@ static int event_function(void *info)
 	struct perf_event_context *ctx = event->ctx;
 	struct perf_cpu_context *cpuctx = this_cpu_ptr(&perf_cpu_context);
 	struct perf_event_context *task_ctx = cpuctx->task_ctx;
-	int ret = 0;
 
 	lockdep_assert_irqs_disabled();
+	guard(perf_ctx_lock)(cpuctx, task_ctx);
 
-	perf_ctx_lock(cpuctx, task_ctx);
 	/*
 	 * Since we do the IPI call without holding ctx->lock things can have
 	 * changed, double check we hit the task we set out to hit.
 	 */
 	if (ctx->task) {
-		if (ctx->task != current) {
-			ret = -ESRCH;
-			goto unlock;
-		}
+		if (ctx->task != current)
+			return -ESRCH;
 
 		/*
 		 * We only use event_function_call() on established contexts,
@@ -254,10 +264,8 @@ static int event_function(void *info)
 	}
 
 	efs->func(event, cpuctx, ctx, efs->data);
-unlock:
-	perf_ctx_unlock(cpuctx, task_ctx);
 
-	return ret;
+	return 0;
 }
 
 static void event_function_call(struct perf_event *event, event_f func, void *data)
@@ -329,11 +337,11 @@ static void event_function_local(struct
 		task_ctx = ctx;
 	}
 
-	perf_ctx_lock(cpuctx, task_ctx);
+	guard(perf_ctx_lock)(cpuctx, task_ctx);
 
 	task = ctx->task;
 	if (task == TASK_TOMBSTONE)
-		goto unlock;
+		return;
 
 	if (task) {
 		/*
@@ -343,18 +351,16 @@ static void event_function_local(struct
 		 */
 		if (ctx->is_active) {
 			if (WARN_ON_ONCE(task != current))
-				goto unlock;
+				return;
 
 			if (WARN_ON_ONCE(cpuctx->task_ctx != ctx))
-				goto unlock;
+				return;
 		}
 	} else {
 		WARN_ON_ONCE(&cpuctx->ctx != ctx);
 	}
 
 	func(event, cpuctx, ctx, data);
-unlock:
-	perf_ctx_unlock(cpuctx, task_ctx);
 }
 
 #define PERF_FLAG_ALL (PERF_FLAG_FD_NO_GROUP |\

[PATCH 05/13] perf: Simplify perf_cgroup_connect()

[Peter Zijlstra]

Use CLASS to reduce gotos and simplify control flow.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/file.h |    2 +-
 kernel/events/core.c |   19 ++++++++-----------
 2 files changed, 9 insertions(+), 12 deletions(-)

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -936,22 +936,20 @@ static inline int perf_cgroup_connect(in
 {
 	struct perf_cgroup *cgrp;
 	struct cgroup_subsys_state *css;
-	struct fd f = fdget(fd);
-	int ret = 0;
+	int ret;
 
+	CLASS(fd, f)(fd);
 	if (!f.file)
 		return -EBADF;
 
 	css = css_tryget_online_from_dir(f.file->f_path.dentry,
 					 &perf_event_cgrp_subsys);
-	if (IS_ERR(css)) {
-		ret = PTR_ERR(css);
-		goto out;
-	}
+	if (IS_ERR(css))
+		return PTR_ERR(css);
 
 	ret = perf_cgroup_ensure_storage(event, css);
 	if (ret)
-		goto out;
+		return ret;
 
 	cgrp = container_of(css, struct perf_cgroup, css);
 	event->cgrp = cgrp;
@@ -963,11 +961,10 @@ static inline int perf_cgroup_connect(in
 	 */
 	if (group_leader && group_leader->cgrp != cgrp) {
 		perf_detach_cgroup(event);
-		ret = -EINVAL;
+		return -EINVAL;
 	}
-out:
-	fdput(f);
-	return ret;
+
+	return 0;
 }
 
 static inline void

[PATCH 06/13] perf; Simplify event_sched_in()

[Peter Zijlstra]

Use guards to reduce gotos and simplify control flow.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/events/core.c |   13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1153,6 +1153,8 @@ void perf_pmu_enable(struct pmu *pmu)
 		pmu->pmu_enable(pmu);
 }
 
+DEFINE_GUARD(perf_pmu_disable, struct pmu *, perf_pmu_disable(_T), perf_pmu_enable(_T))
+
 static void perf_assert_pmu_disabled(struct pmu *pmu)
 {
 	WARN_ON_ONCE(*this_cpu_ptr(pmu->pmu_disable_count) == 0);
@@ -2489,7 +2491,6 @@ event_sched_in(struct perf_event *event,
 {
 	struct perf_event_pmu_context *epc = event->pmu_ctx;
 	struct perf_cpu_pmu_context *cpc = this_cpu_ptr(epc->pmu->cpu_pmu_context);
-	int ret = 0;
 
 	WARN_ON_ONCE(event->ctx != ctx);
 
@@ -2517,15 +2518,14 @@ event_sched_in(struct perf_event *event,
 		event->hw.interrupts = 0;
 	}
 
-	perf_pmu_disable(event->pmu);
+	guard(perf_pmu_disable)(event->pmu);
 
 	perf_log_itrace_start(event);
 
 	if (event->pmu->add(event, PERF_EF_START)) {
 		perf_event_set_state(event, PERF_EVENT_STATE_INACTIVE);
 		event->oncpu = -1;
-		ret = -EAGAIN;
-		goto out;
+		return -EAGAIN;
 	}
 
 	if (!is_software_event(event))
@@ -2536,10 +2536,7 @@ event_sched_in(struct perf_event *event,
 	if (event->attr.exclusive)
 		cpc->exclusive = 1;
 
-out:
-	perf_pmu_enable(event->pmu);
-
-	return ret;
+	return 0;
 }
 
 static int

[PATCH 07/13] perf: Simplify: __perf_install_in_context()

[Peter Zijlstra]

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/events/core.c |   21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2732,13 +2732,13 @@ static int  __perf_install_in_context(vo
 	struct perf_cpu_context *cpuctx = this_cpu_ptr(&perf_cpu_context);
 	struct perf_event_context *task_ctx = cpuctx->task_ctx;
 	bool reprogram = true;
-	int ret = 0;
 
-	raw_spin_lock(&cpuctx->ctx.lock);
-	if (ctx->task) {
-		raw_spin_lock(&ctx->lock);
+	if (ctx->task)
 		task_ctx = ctx;
 
+	guard(perf_ctx_lock)(cpuctx, task_ctx);
+
+	if (ctx->task) {
 		reprogram = (ctx->task == current);
 
 		/*
@@ -2748,14 +2748,10 @@ static int  __perf_install_in_context(vo
 		 * If its not running, we don't care, ctx->lock will
 		 * serialize against it becoming runnable.
 		 */
-		if (task_curr(ctx->task) && !reprogram) {
-			ret = -ESRCH;
-			goto unlock;
-		}
+		if (task_curr(ctx->task) && !reprogram)
+			return -ESRCH;
 
 		WARN_ON_ONCE(reprogram && cpuctx->task_ctx && cpuctx->task_ctx != ctx);
-	} else if (task_ctx) {
-		raw_spin_lock(&task_ctx->lock);
 	}
 
 #ifdef CONFIG_CGROUP_PERF
@@ -2778,10 +2774,7 @@ static int  __perf_install_in_context(vo
 		add_event_to_ctx(event, ctx);
 	}
 
-unlock:
-	perf_ctx_unlock(cpuctx, task_ctx);
-
-	return ret;
+	return 0;
 }
 
 static bool exclusive_event_installable(struct perf_event *event,

Re: [PATCH 07/13] perf: Simplify: __perf_install_in_context()

[Namhyung Kim]

You may want to remove ":" after Simplify in the subject line
here and the next patch.

Thanks,
Namhyung

Re: [PATCH 07/13] perf: Simplify: __perf_install_in_context()

[Peter Zijlstra]

On Fri, Nov 03, 2023 at 06:08:06PM -0700, Namhyung Kim wrote:
> You may want to remove ":" after Simplify in the subject line
> here and the next patch.

Duh, typing hard. Thanks!

[PATCH 08/13] perf: Simplify: *perf_event_{dis,en}able*()

[Peter Zijlstra]

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/events/core.c |   51 ++++++++++++++++++++++-----------------------------
 1 file changed, 22 insertions(+), 29 deletions(-)

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2415,7 +2415,7 @@ static void __perf_event_disable(struct
 		update_cgrp_time_from_event(event);
 	}
 
-	perf_pmu_disable(event->pmu_ctx->pmu);
+	guard(perf_pmu_disable)(event->pmu_ctx->pmu);
 
 	if (event == event->group_leader)
 		group_sched_out(event, ctx);
@@ -2424,8 +2424,6 @@ static void __perf_event_disable(struct
 
 	perf_event_set_state(event, PERF_EVENT_STATE_OFF);
 	perf_cgroup_event_disable(event, ctx);
-
-	perf_pmu_enable(event->pmu_ctx->pmu);
 }
 
 /*
@@ -2446,12 +2444,10 @@ static void _perf_event_disable(struct p
 {
 	struct perf_event_context *ctx = event->ctx;
 
-	raw_spin_lock_irq(&ctx->lock);
-	if (event->state <= PERF_EVENT_STATE_OFF) {
-		raw_spin_unlock_irq(&ctx->lock);
-		return;
+	scoped_guard (raw_spinlock_irq, &ctx->lock) {
+		if (event->state <= PERF_EVENT_STATE_OFF)
+			return;
 	}
-	raw_spin_unlock_irq(&ctx->lock);
 
 	event_function_call(event, __perf_event_disable, NULL);
 }
@@ -2955,32 +2951,29 @@ static void _perf_event_enable(struct pe
 {
 	struct perf_event_context *ctx = event->ctx;
 
-	raw_spin_lock_irq(&ctx->lock);
-	if (event->state >= PERF_EVENT_STATE_INACTIVE ||
-	    event->state <  PERF_EVENT_STATE_ERROR) {
-out:
-		raw_spin_unlock_irq(&ctx->lock);
-		return;
-	}
+	scoped_guard (raw_spinlock_irq, &ctx->lock) {
+		if (event->state >= PERF_EVENT_STATE_INACTIVE ||
+		    event->state <  PERF_EVENT_STATE_ERROR)
+			return;
 
-	/*
-	 * If the event is in error state, clear that first.
-	 *
-	 * That way, if we see the event in error state below, we know that it
-	 * has gone back into error state, as distinct from the task having
-	 * been scheduled away before the cross-call arrived.
-	 */
-	if (event->state == PERF_EVENT_STATE_ERROR) {
 		/*
-		 * Detached SIBLING events cannot leave ERROR state.
+		 * If the event is in error state, clear that first.
+		 *
+		 * That way, if we see the event in error state below, we know that it
+		 * has gone back into error state, as distinct from the task having
+		 * been scheduled away before the cross-call arrived.
 		 */
-		if (event->event_caps & PERF_EV_CAP_SIBLING &&
-		    event->group_leader == event)
-			goto out;
+		if (event->state == PERF_EVENT_STATE_ERROR) {
+			/*
+			 * Detached SIBLING events cannot leave ERROR state.
+			 */
+			if (event->event_caps & PERF_EV_CAP_SIBLING &&
+			    event->group_leader == event)
+				return;
 
-		event->state = PERF_EVENT_STATE_OFF;
+			event->state = PERF_EVENT_STATE_OFF;
+		}
 	}
-	raw_spin_unlock_irq(&ctx->lock);
 
 	event_function_call(event, __perf_event_enable, NULL);
 }

[PATCH 09/13] perf: Simplify perf_event_modify_attr()

[Peter Zijlstra]

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/events/core.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3172,7 +3172,7 @@ static int perf_event_modify_attr(struct
 
 	WARN_ON_ONCE(event->ctx->parent_ctx);
 
-	mutex_lock(&event->child_mutex);
+	guard(mutex)(&event->child_mutex);
 	/*
 	 * Event-type-independent attributes must be copied before event-type
 	 * modification, which will validate that final attributes match the
@@ -3181,16 +3181,16 @@ static int perf_event_modify_attr(struct
 	perf_event_modify_copy_attr(&event->attr, attr);
 	err = func(event, attr);
 	if (err)
-		goto out;
+		return err;
+
 	list_for_each_entry(child, &event->child_list, child_list) {
 		perf_event_modify_copy_attr(&child->attr, attr);
 		err = func(child, attr);
 		if (err)
-			goto out;
+			return err;
 	}
-out:
-	mutex_unlock(&event->child_mutex);
-	return err;
+
+	return 0;
 }
 
 static void __pmu_ctx_sched_out(struct perf_event_pmu_context *pmu_ctx,

[PATCH 10/13] perf: Simplify perf_event_context_sched_in()

[Peter Zijlstra]

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/events/core.c |   38 +++++++++++++++-----------------------
 1 file changed, 15 insertions(+), 23 deletions(-)

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -713,6 +713,9 @@ static void perf_ctx_enable(struct perf_
 	}
 }
 
+DEFINE_GUARD(perf_ctx_disable, struct perf_event_context *,
+	     perf_ctx_disable(_T, false), perf_ctx_enable(_T, false))
+
 static void ctx_sched_out(struct perf_event_context *ctx, enum event_type_t event_type);
 static void ctx_sched_in(struct perf_event_context *ctx, enum event_type_t event_type);
 
@@ -3903,31 +3906,27 @@ static void perf_event_context_sched_in(
 	struct perf_cpu_context *cpuctx = this_cpu_ptr(&perf_cpu_context);
 	struct perf_event_context *ctx;
 
-	rcu_read_lock();
+	guard(rcu)();
+
 	ctx = rcu_dereference(task->perf_event_ctxp);
 	if (!ctx)
-		goto rcu_unlock;
-
-	if (cpuctx->task_ctx == ctx) {
-		perf_ctx_lock(cpuctx, ctx);
-		perf_ctx_disable(ctx, false);
-
-		perf_ctx_sched_task_cb(ctx, true);
-
-		perf_ctx_enable(ctx, false);
-		perf_ctx_unlock(cpuctx, ctx);
-		goto rcu_unlock;
-	}
+		return;
 
-	perf_ctx_lock(cpuctx, ctx);
+	guard(perf_ctx_lock)(cpuctx, ctx);
 	/*
 	 * We must check ctx->nr_events while holding ctx->lock, such
 	 * that we serialize against perf_install_in_context().
 	 */
 	if (!ctx->nr_events)
-		goto unlock;
+		return;
+
+	guard(perf_ctx_disable)(ctx);
+
+	if (cpuctx->task_ctx == ctx) {
+		perf_ctx_sched_task_cb(ctx, true);
+		return;
+	}
 
-	perf_ctx_disable(ctx, false);
 	/*
 	 * We want to keep the following priority order:
 	 * cpu pinned (that don't need to move), task pinned,
@@ -3947,13 +3946,6 @@ static void perf_event_context_sched_in(
 
 	if (!RB_EMPTY_ROOT(&ctx->pinned_groups.tree))
 		perf_ctx_enable(&cpuctx->ctx, false);
-
-	perf_ctx_enable(ctx, false);
-
-unlock:
-	perf_ctx_unlock(cpuctx, ctx);
-rcu_unlock:
-	rcu_read_unlock();
 }
 
 /*

[PATCH 11/13] perf: Simplify perf_adjust_freq_unthr_context()

[Peter Zijlstra]

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/events/core.c |   51 +++++++++++++++++++++++----------------------------
 1 file changed, 23 insertions(+), 28 deletions(-)

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4090,7 +4090,7 @@ perf_adjust_freq_unthr_context(struct pe
 	if (!(ctx->nr_freq || unthrottle))
 		return;
 
-	raw_spin_lock(&ctx->lock);
+	guard(raw_spinlock)(&ctx->lock);
 
 	list_for_each_entry_rcu(event, &ctx->event_list, event_entry) {
 		if (event->state != PERF_EVENT_STATE_ACTIVE)
@@ -4100,7 +4100,7 @@ perf_adjust_freq_unthr_context(struct pe
 		if (!event_filter_match(event))
 			continue;
 
-		perf_pmu_disable(event->pmu);
+		guard(perf_pmu_disable)(event->pmu);
 
 		hwc = &event->hw;
 
@@ -4110,34 +4110,29 @@ perf_adjust_freq_unthr_context(struct pe
 			event->pmu->start(event, 0);
 		}
 
-		if (!event->attr.freq || !event->attr.sample_freq)
-			goto next;
+		if (event->attr.freq && event->attr.sample_freq) {
+			/*
+			 * stop the event and update event->count
+			 */
+			event->pmu->stop(event, PERF_EF_UPDATE);
+
+			now = local64_read(&event->count);
+			delta = now - hwc->freq_count_stamp;
+			hwc->freq_count_stamp = now;
+
+			/*
+			 * restart the event
+			 * reload only if value has changed
+			 * we have stopped the event so tell that
+			 * to perf_adjust_period() to avoid stopping it
+			 * twice.
+			 */
+			if (delta > 0)
+				perf_adjust_period(event, period, delta, false);
 
-		/*
-		 * stop the event and update event->count
-		 */
-		event->pmu->stop(event, PERF_EF_UPDATE);
-
-		now = local64_read(&event->count);
-		delta = now - hwc->freq_count_stamp;
-		hwc->freq_count_stamp = now;
-
-		/*
-		 * restart the event
-		 * reload only if value has changed
-		 * we have stopped the event so tell that
-		 * to perf_adjust_period() to avoid stopping it
-		 * twice.
-		 */
-		if (delta > 0)
-			perf_adjust_period(event, period, delta, false);
-
-		event->pmu->start(event, delta > 0 ? PERF_EF_RELOAD : 0);
-	next:
-		perf_pmu_enable(event->pmu);
+			event->pmu->start(event, delta > 0 ? PERF_EF_RELOAD : 0);
+		}
 	}
-
-	raw_spin_unlock(&ctx->lock);
 }
 
 /*

Re: [PATCH 11/13] perf: Simplify perf_adjust_freq_unthr_context()

[Namhyung Kim]

On Thu, Nov 2, 2023 at 8:32 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  kernel/events/core.c |   51 +++++++++++++++++++++++----------------------------
>  1 file changed, 23 insertions(+), 28 deletions(-)
>
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -4090,7 +4090,7 @@ perf_adjust_freq_unthr_context(struct pe
>         if (!(ctx->nr_freq || unthrottle))
>                 return;
>
> -       raw_spin_lock(&ctx->lock);
> +       guard(raw_spinlock)(&ctx->lock);
>
>         list_for_each_entry_rcu(event, &ctx->event_list, event_entry) {
>                 if (event->state != PERF_EVENT_STATE_ACTIVE)
> @@ -4100,7 +4100,7 @@ perf_adjust_freq_unthr_context(struct pe
>                 if (!event_filter_match(event))
>                         continue;
>
> -               perf_pmu_disable(event->pmu);
> +               guard(perf_pmu_disable)(event->pmu);
>
>                 hwc = &event->hw;
>
> @@ -4110,34 +4110,29 @@ perf_adjust_freq_unthr_context(struct pe
>                         event->pmu->start(event, 0);
>                 }
>
> -               if (!event->attr.freq || !event->attr.sample_freq)
> -                       goto next;
> +               if (event->attr.freq && event->attr.sample_freq) {

Any reason for this change?  I think we can just change the
'goto next' to 'continue', no?

Also I think this code needs changes to optimize the access.
A similar reason for the cgroup switch, it accesses all the
pmu/events in the context even before checking the sampling
frequency.  This is bad for uncore PMUs (and KVM too).

But this is a different issue..

Thanks,
Namhyung


> +                       /*
> +                        * stop the event and update event->count
> +                        */
> +                       event->pmu->stop(event, PERF_EF_UPDATE);
> +
> +                       now = local64_read(&event->count);
> +                       delta = now - hwc->freq_count_stamp;
> +                       hwc->freq_count_stamp = now;
> +
> +                       /*
> +                        * restart the event
> +                        * reload only if value has changed
> +                        * we have stopped the event so tell that
> +                        * to perf_adjust_period() to avoid stopping it
> +                        * twice.
> +                        */
> +                       if (delta > 0)
> +                               perf_adjust_period(event, period, delta, false);
>
> -               /*
> -                * stop the event and update event->count
> -                */
> -               event->pmu->stop(event, PERF_EF_UPDATE);
> -
> -               now = local64_read(&event->count);
> -               delta = now - hwc->freq_count_stamp;
> -               hwc->freq_count_stamp = now;
> -
> -               /*
> -                * restart the event
> -                * reload only if value has changed
> -                * we have stopped the event so tell that
> -                * to perf_adjust_period() to avoid stopping it
> -                * twice.
> -                */
> -               if (delta > 0)
> -                       perf_adjust_period(event, period, delta, false);
> -
> -               event->pmu->start(event, delta > 0 ? PERF_EF_RELOAD : 0);
> -       next:
> -               perf_pmu_enable(event->pmu);
> +                       event->pmu->start(event, delta > 0 ? PERF_EF_RELOAD : 0);
> +               }
>         }
> -
> -       raw_spin_unlock(&ctx->lock);
>  }
>
>  /*
>
>

Re: [PATCH 11/13] perf: Simplify perf_adjust_freq_unthr_context()

[Peter Zijlstra]

On Fri, Nov 03, 2023 at 06:14:32PM -0700, Namhyung Kim wrote:
> On Thu, Nov 2, 2023 at 8:32 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> >
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > ---
> >  kernel/events/core.c |   51 +++++++++++++++++++++++----------------------------
> >  1 file changed, 23 insertions(+), 28 deletions(-)
> >
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -4090,7 +4090,7 @@ perf_adjust_freq_unthr_context(struct pe
> >         if (!(ctx->nr_freq || unthrottle))
> >                 return;
> >
> > -       raw_spin_lock(&ctx->lock);
> > +       guard(raw_spinlock)(&ctx->lock);
> >
> >         list_for_each_entry_rcu(event, &ctx->event_list, event_entry) {
> >                 if (event->state != PERF_EVENT_STATE_ACTIVE)
> > @@ -4100,7 +4100,7 @@ perf_adjust_freq_unthr_context(struct pe
> >                 if (!event_filter_match(event))
> >                         continue;
> >
> > -               perf_pmu_disable(event->pmu);
> > +               guard(perf_pmu_disable)(event->pmu);
> >
> >                 hwc = &event->hw;
> >
> > @@ -4110,34 +4110,29 @@ perf_adjust_freq_unthr_context(struct pe
> >                         event->pmu->start(event, 0);
> >                 }
> >
> > -               if (!event->attr.freq || !event->attr.sample_freq)
> > -                       goto next;
> > +               if (event->attr.freq && event->attr.sample_freq) {
> 
> Any reason for this change?  I think we can just change the
> 'goto next' to 'continue', no?

Linus initially got confused about the life-time of for-loop scopes, but
yeah, this could be continue just fine.

> Also I think this code needs changes to optimize the access.
> A similar reason for the cgroup switch, it accesses all the
> pmu/events in the context even before checking the sampling
> frequency.  This is bad for uncore PMUs (and KVM too).
> 
> But this is a different issue..

Right, lets do that in another patch. Also, there seems to be a problem
with the cgroup thing :/

[PATCH 12/13] perf: Simplify perf_event_*_on_exec()

[Peter Zijlstra]

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/events/core.c |   88 +++++++++++++++++++++++----------------------------
 1 file changed, 40 insertions(+), 48 deletions(-)

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4318,39 +4318,36 @@ static void perf_event_enable_on_exec(st
 	enum event_type_t event_type = 0;
 	struct perf_cpu_context *cpuctx;
 	struct perf_event *event;
-	unsigned long flags;
 	int enabled = 0;
 
-	local_irq_save(flags);
-	if (WARN_ON_ONCE(current->perf_event_ctxp != ctx))
-		goto out;
-
-	if (!ctx->nr_events)
-		goto out;
-
-	cpuctx = this_cpu_ptr(&perf_cpu_context);
-	perf_ctx_lock(cpuctx, ctx);
-	ctx_sched_out(ctx, EVENT_TIME);
-
-	list_for_each_entry(event, &ctx->event_list, event_entry) {
-		enabled |= event_enable_on_exec(event, ctx);
-		event_type |= get_event_type(event);
+	scoped_guard (irqsave) {
+		if (WARN_ON_ONCE(current->perf_event_ctxp != ctx))
+			return;
+
+		if (!ctx->nr_events)
+			return;
+
+		cpuctx = this_cpu_ptr(&perf_cpu_context);
+		guard(perf_ctx_lock)(cpuctx, ctx);
+
+		ctx_sched_out(ctx, EVENT_TIME);
+
+		list_for_each_entry(event, &ctx->event_list, event_entry) {
+			enabled |= event_enable_on_exec(event, ctx);
+			event_type |= get_event_type(event);
+		}
+
+		/*
+		 * Unclone and reschedule this context if we enabled any event.
+		 */
+		if (enabled) {
+			clone_ctx = unclone_ctx(ctx);
+			ctx_resched(cpuctx, ctx, event_type);
+		} else {
+			ctx_sched_in(ctx, EVENT_TIME);
+		}
 	}
 
-	/*
-	 * Unclone and reschedule this context if we enabled any event.
-	 */
-	if (enabled) {
-		clone_ctx = unclone_ctx(ctx);
-		ctx_resched(cpuctx, ctx, event_type);
-	} else {
-		ctx_sched_in(ctx, EVENT_TIME);
-	}
-	perf_ctx_unlock(cpuctx, ctx);
-
-out:
-	local_irq_restore(flags);
-
 	if (clone_ctx)
 		put_ctx(clone_ctx);
 }
@@ -4367,34 +4364,29 @@ static void perf_event_remove_on_exec(st
 {
 	struct perf_event_context *clone_ctx = NULL;
 	struct perf_event *event, *next;
-	unsigned long flags;
 	bool modified = false;
 
-	mutex_lock(&ctx->mutex);
+	scoped_guard (mutex, &ctx->mutex) {
+		if (WARN_ON_ONCE(ctx->task != current))
+			return;
 
-	if (WARN_ON_ONCE(ctx->task != current))
-		goto unlock;
+		list_for_each_entry_safe(event, next, &ctx->event_list, event_entry) {
+			if (!event->attr.remove_on_exec)
+				continue;
 
-	list_for_each_entry_safe(event, next, &ctx->event_list, event_entry) {
-		if (!event->attr.remove_on_exec)
-			continue;
+			if (!is_kernel_event(event))
+				perf_remove_from_owner(event);
 
-		if (!is_kernel_event(event))
-			perf_remove_from_owner(event);
+			modified = true;
 
-		modified = true;
+			perf_event_exit_event(event, ctx);
+		}
 
-		perf_event_exit_event(event, ctx);
+		guard(raw_spinlock_irqsave)(&ctx->lock);
+		if (modified)
+			clone_ctx = unclone_ctx(ctx);
 	}
 
-	raw_spin_lock_irqsave(&ctx->lock, flags);
-	if (modified)
-		clone_ctx = unclone_ctx(ctx);
-	raw_spin_unlock_irqrestore(&ctx->lock, flags);
-
-unlock:
-	mutex_unlock(&ctx->mutex);
-
 	if (clone_ctx)
 		put_ctx(clone_ctx);
 }

[PATCH 13/13] perf: Simplify *perf_event_read*()

[Peter Zijlstra]

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 kernel/events/core.c |   54 ++++++++++++++++-----------------------------------
 1 file changed, 17 insertions(+), 37 deletions(-)

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4434,7 +4434,8 @@ static void __perf_event_read(void *info
 	if (ctx->task && cpuctx->task_ctx != ctx)
 		return;
 
-	raw_spin_lock(&ctx->lock);
+	guard(raw_spinlock)(&ctx->lock);
+
 	if (ctx->is_active & EVENT_TIME) {
 		update_context_time(ctx);
 		update_cgrp_time_from_event(event);
@@ -4445,12 +4446,12 @@ static void __perf_event_read(void *info
 		perf_event_update_sibling_time(event);
 
 	if (event->state != PERF_EVENT_STATE_ACTIVE)
-		goto unlock;
+		return;
 
 	if (!data->group) {
 		pmu->read(event);
 		data->ret = 0;
-		goto unlock;
+		return;
 	}
 
 	pmu->start_txn(pmu, PERF_PMU_TXN_READ);
@@ -4468,9 +4469,6 @@ static void __perf_event_read(void *info
 	}
 
 	data->ret = pmu->commit_txn(pmu);
-
-unlock:
-	raw_spin_unlock(&ctx->lock);
 }
 
 static inline u64 perf_event_count(struct perf_event *event)
@@ -4501,32 +4499,25 @@ static void calc_timer_values(struct per
 int perf_event_read_local(struct perf_event *event, u64 *value,
 			  u64 *enabled, u64 *running)
 {
-	unsigned long flags;
 	int event_oncpu;
 	int event_cpu;
-	int ret = 0;
-
 	/*
 	 * Disabling interrupts avoids all counter scheduling (context
 	 * switches, timer based rotation and IPIs).
 	 */
-	local_irq_save(flags);
+	guard(irqsave)();
 
 	/*
 	 * It must not be an event with inherit set, we cannot read
 	 * all child counters from atomic context.
 	 */
-	if (event->attr.inherit) {
-		ret = -EOPNOTSUPP;
-		goto out;
-	}
+	if (event->attr.inherit)
+		return -EOPNOTSUPP;
 
 	/* If this is a per-task event, it must be for current */
 	if ((event->attach_state & PERF_ATTACH_TASK) &&
-	    event->hw.target != current) {
-		ret = -EINVAL;
-		goto out;
-	}
+	    event->hw.target != current)
+		return -EINVAL;
 
 	/*
 	 * Get the event CPU numbers, and adjust them to local if the event is
@@ -4537,16 +4528,12 @@ int perf_event_read_local(struct perf_ev
 
 	/* If this is a per-CPU event, it must be for this CPU */
 	if (!(event->attach_state & PERF_ATTACH_TASK) &&
-	    event_cpu != smp_processor_id()) {
-		ret = -EINVAL;
-		goto out;
-	}
+	    event_cpu != smp_processor_id())
+		return -EINVAL;
 
 	/* If this is a pinned event it must be running on this CPU */
-	if (event->attr.pinned && event_oncpu != smp_processor_id()) {
-		ret = -EBUSY;
-		goto out;
-	}
+	if (event->attr.pinned && event_oncpu != smp_processor_id())
+		return -EBUSY;
 
 	/*
 	 * If the event is currently on this CPU, its either a per-task event,
@@ -4566,10 +4553,8 @@ int perf_event_read_local(struct perf_ev
 		if (running)
 			*running = __running;
 	}
-out:
-	local_irq_restore(flags);
 
-	return ret;
+	return 0;
 }
 
 static int perf_event_read(struct perf_event *event, bool group)
@@ -4603,7 +4588,7 @@ static int perf_event_read(struct perf_e
 			.ret = 0,
 		};
 
-		preempt_disable();
+		guard(preempt)();
 		event_cpu = __perf_event_read_cpu(event, event_cpu);
 
 		/*
@@ -4617,19 +4602,15 @@ static int perf_event_read(struct perf_e
 		 * after this.
 		 */
 		(void)smp_call_function_single(event_cpu, __perf_event_read, &data, 1);
-		preempt_enable();
 		ret = data.ret;
 
 	} else if (state == PERF_EVENT_STATE_INACTIVE) {
 		struct perf_event_context *ctx = event->ctx;
-		unsigned long flags;
 
-		raw_spin_lock_irqsave(&ctx->lock, flags);
+		guard(raw_spinlock_irqsave)(&ctx->lock);
 		state = event->state;
-		if (state != PERF_EVENT_STATE_INACTIVE) {
-			raw_spin_unlock_irqrestore(&ctx->lock, flags);
+		if (state != PERF_EVENT_STATE_INACTIVE)
 			goto again;
-		}
 
 		/*
 		 * May read while context is not active (e.g., thread is
@@ -4643,7 +4624,6 @@ static int perf_event_read(struct perf_e
 		perf_event_update_time(event);
 		if (group)
 			perf_event_update_sibling_time(event);
-		raw_spin_unlock_irqrestore(&ctx->lock, flags);
 	}
 
 	return ret;