| 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: <29655aae-e1fd-4f3a-88f9-033034943ddd@linux.intel.com>
Date: Wed, 12 Mar 2025 15:52:06 -0400
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Peter Zijlstra <peterz@...radead.org>
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 2025-03-12 3:18 p.m., Peter Zijlstra wrote:
> 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?
Ah, yes
>
>> + }
>> + 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?
>
Yes. I will do more test and send a V9.
Thanks,
Kan
> ---
> --- 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