| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250312191823.GB10453@noisy.programming.kicks-ass.net>
Date: Wed, 12 Mar 2025 20:18:23 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: kan.liang@...ux.intel.com
Cc: mingo@...hat.com, tglx@...utronix.de, bp@...en8.de, acme@...nel.org,
namhyung@...nel.org, irogers@...gle.com,
linux-kernel@...r.kernel.org, ak@...ux.intel.com,
eranian@...gle.com
Subject: Re: [PATCH V8 2/6] perf: attach/detach PMU specific data
On Wed, Mar 12, 2025 at 11:25:21AM -0700, kan.liang@...ux.intel.com wrote:
> +static int
> +attach_global_ctx_data(struct kmem_cache *ctx_cache)
> +{
> + if (refcount_inc_not_zero(&global_ctx_data_ref))
> + return 0;
> +
> + percpu_down_write(&global_ctx_data_rwsem);
> + if (!refcount_inc_not_zero(&global_ctx_data_ref)) {
> + struct task_struct *g, *p;
> + struct perf_ctx_data *cd;
> + int ret;
> +
> +again:
> + /* Allocate everything */
> + rcu_read_lock();
> + for_each_process_thread(g, p) {
> + cd = rcu_dereference(p->perf_ctx_data);
> + if (cd && !cd->global) {
> + cd->global = 1;
> + if (!refcount_inc_not_zero(&cd->refcount))
> + cd = NULL;
> + }
> + if (!cd) {
> + get_task_struct(p);
> + rcu_read_unlock();
> +
> + ret = attach_task_ctx_data(p, ctx_cache, true);
> + put_task_struct(p);
> + if (ret) {
> + __detach_global_ctx_data();
> + return ret;
AFAICT this returns with global_ctx_data_rwsem taken, no?
> + }
> + goto again;
> + }
> + }
> + rcu_read_unlock();
> +
> + refcount_set(&global_ctx_data_ref, 1);
> + }
> + percpu_up_write(&global_ctx_data_rwsem);
> +
> + return 0;
> +}
Can we rework this with guards? A little something like so?
---
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5233,18 +5233,20 @@ static refcount_t global_ctx_data_ref;
static int
attach_global_ctx_data(struct kmem_cache *ctx_cache)
{
+ struct task_struct *g, *p;
+ struct perf_ctx_data *cd;
+ int ret;
+
if (refcount_inc_not_zero(&global_ctx_data_ref))
return 0;
- percpu_down_write(&global_ctx_data_rwsem);
- if (!refcount_inc_not_zero(&global_ctx_data_ref)) {
- struct task_struct *g, *p;
- struct perf_ctx_data *cd;
- int ret;
+ guard(percpu_write)(&global_ctx_data_rwsem);
+ if (refcount_inc_not_zero(&global_ctx_data_ref))
+ return 0;
again:
- /* Allocate everything */
- rcu_read_lock();
+ /* Allocate everything */
+ scoped_guard (rcu) {
for_each_process_thread(g, p) {
cd = rcu_dereference(p->perf_ctx_data);
if (cd && !cd->global) {
@@ -5254,24 +5256,23 @@ attach_global_ctx_data(struct kmem_cache
}
if (!cd) {
get_task_struct(p);
- rcu_read_unlock();
-
- ret = attach_task_ctx_data(p, ctx_cache, true);
- put_task_struct(p);
- if (ret) {
- __detach_global_ctx_data();
- return ret;
- }
- goto again;
+ goto alloc;
}
}
- rcu_read_unlock();
-
- refcount_set(&global_ctx_data_ref, 1);
}
- percpu_up_write(&global_ctx_data_rwsem);
+
+ refcount_set(&global_ctx_data_ref, 1);
return 0;
+
+alloc:
+ ret = attach_task_ctx_data(p, ctx_cache, true);
+ put_task_struct(p);
+ if (ret) {
+ __detach_global_ctx_data();
+ return ret;
+ }
+ goto again;
}
static int
@@ -5338,15 +5339,12 @@ static void detach_global_ctx_data(void)
if (refcount_dec_not_one(&global_ctx_data_ref))
return;
- percpu_down_write(&global_ctx_data_rwsem);
+ guard(perpcu_write)(&global_ctx_data_rwsem);
if (!refcount_dec_and_test(&global_ctx_data_ref))
- goto unlock;
+ return;
/* remove everything */
__detach_global_ctx_data();
-
-unlock:
- percpu_up_write(&global_ctx_data_rwsem);
}
static void detach_perf_ctx_data(struct perf_event *event)
@@ -8776,9 +8774,9 @@ perf_event_alloc_task_data(struct task_s
if (!ctx_cache)
return;
- percpu_down_read(&global_ctx_data_rwsem);
+ guard(percpu_read)(&global_ctx_data_rwsem);
+ guard(rcu)();
- rcu_read_lock();
cd = rcu_dereference(child->perf_ctx_data);
if (!cd) {
@@ -8787,21 +8785,16 @@ perf_event_alloc_task_data(struct task_s
* when attaching the perf_ctx_data.
*/
if (!refcount_read(&global_ctx_data_ref))
- goto rcu_unlock;
+ return;
rcu_read_unlock();
attach_task_ctx_data(child, ctx_cache, true);
- goto up_rwsem;
+ return;
}
if (!cd->global) {
cd->global = 1;
refcount_inc(&cd->refcount);
}
-
-rcu_unlock:
- rcu_read_unlock();
-up_rwsem:
- percpu_up_read(&global_ctx_data_rwsem);
}
void perf_event_fork(struct task_struct *task)
@@ -13845,9 +13838,8 @@ void perf_event_exit_task(struct task_st
/*
* Detach the perf_ctx_data for the system-wide event.
*/
- percpu_down_read(&global_ctx_data_rwsem);
+ guard(percpu_read)(&global_ctx_data_rwsem);
detach_task_ctx_data(child);
- percpu_up_read(&global_ctx_data_rwsem);
}
static void perf_free_event(struct perf_event *event,
diff --git a/include/linux/percpu-rwsem.h b/include/linux/percpu-rwsem.h
index c012df33a9f0..36f3082f2d82 100644
--- a/include/linux/percpu-rwsem.h
+++ b/include/linux/percpu-rwsem.h
@@ -8,6 +8,7 @@
#include <linux/wait.h>
#include <linux/rcu_sync.h>
#include <linux/lockdep.h>
+#include <linux/cleanup.h>
struct percpu_rw_semaphore {
struct rcu_sync rss;
@@ -125,6 +126,13 @@ extern bool percpu_is_read_locked(struct percpu_rw_semaphore *);
extern void percpu_down_write(struct percpu_rw_semaphore *);
extern void percpu_up_write(struct percpu_rw_semaphore *);
+DEFINE_GUARD(percpu_read, struct perpcu_rw_semaphore *,
+ perpcu_down_read(_T), percpu_up_read(_T))
+DEFINE_GUARD_COND(perpcu_read, _try, percpu_down_read_trylock(_T))
+
+DEFINE_GUARD(percpu_write, struct percpu_rw_semaphore *,
+ percpu_down_write(_T), perpcu_up_write(_T))
+
static inline bool percpu_is_write_locked(struct percpu_rw_semaphore *sem)
{
return atomic_read(&sem->block);
Powered by blists - more mailing lists