| 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
| ||
|
Date: Wed, 15 Feb 2023 09:46:10 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "peterz@...radead.org" <peterz@...radead.org>
CC: "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"Hansen, Dave" <dave.hansen@...el.com>,
"david@...hat.com" <david@...hat.com>,
"bagasdotme@...il.com" <bagasdotme@...il.com>,
"ak@...ux.intel.com" <ak@...ux.intel.com>,
"Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
"kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
"Chatre, Reinette" <reinette.chatre@...el.com>,
"Christopherson,, Sean" <seanjc@...gle.com>,
"pbonzini@...hat.com" <pbonzini@...hat.com>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"Yamahata, Isaku" <isaku.yamahata@...el.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-mm@...ck.org" <linux-mm@...ck.org>,
"tj@...nel.org" <tj@...nel.org>,
"Luck, Tony" <tony.luck@...el.com>,
"Shahar, Sagi" <sagis@...gle.com>,
"imammedo@...hat.com" <imammedo@...hat.com>,
"Gao, Chao" <chao.gao@...el.com>,
"Brown, Len" <len.brown@...el.com>,
"sathyanarayanan.kuppuswamy@...ux.intel.com"
<sathyanarayanan.kuppuswamy@...ux.intel.com>,
"Huang, Ying" <ying.huang@...el.com>,
"Williams, Dan J" <dan.j.williams@...el.com>
Subject: Re: [PATCH v9 07/18] x86/virt/tdx: Do TDX module per-cpu
initialization
> > +
> > + for_each_online_cpu(cpu)
>
> I think you need to skip some flushes too. Given we skip setting
> work->func, this will go WARN, see __flush_work().
I missed. Thanks for pointing out.
>
> > + flush_work(per_cpu_ptr(works, cpu));
> > +
>
[...]
> That said, I find it jarring that the schedule_on*() family doesn't have
> a void* argument to the function, like the smp_*() family has. So how
> about something like the below (equally untested). It preserves the
> current semantics, but allows a work function to cast to schedule_work
> and access ->info if it so desires.
Yes agreed. Your code below looks indeed better. Thanks!
Would you mind send me a patch so I can include to this series, or would you
mind get it merged to tip/x86/tdx (or other branch I am not sure) so I can
rebase?
Appreciate your help!
>
>
> diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> index a0143dd24430..5e97111322b2 100644
> --- a/include/linux/workqueue.h
> +++ b/include/linux/workqueue.h
> @@ -103,6 +103,11 @@ struct work_struct {
> #endif
> };
>
> +struct schedule_work {
> + struct work_struct work;
> + void *info;
> +};
> +
> #define WORK_DATA_INIT() ATOMIC_LONG_INIT((unsigned long)WORK_STRUCT_NO_POOL)
> #define WORK_DATA_STATIC_INIT() \
> ATOMIC_LONG_INIT((unsigned long)(WORK_STRUCT_NO_POOL | WORK_STRUCT_STATIC))
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 07895deca271..c73bb8860bbc 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -51,6 +51,7 @@
> #include <linux/sched/isolation.h>
> #include <linux/nmi.h>
> #include <linux/kvm_para.h>
> +#include <linux/smp.h>
>
> #include "workqueue_internal.h"
>
> @@ -3302,43 +3303,64 @@ bool cancel_delayed_work_sync(struct delayed_work *dwork)
> }
> EXPORT_SYMBOL(cancel_delayed_work_sync);
>
> -/**
> - * schedule_on_each_cpu - execute a function synchronously on each online CPU
> - * @func: the function to call
> - *
> - * schedule_on_each_cpu() executes @func on each online CPU using the
> - * system workqueue and blocks until all CPUs have completed.
> - * schedule_on_each_cpu() is very slow.
> - *
> - * Return:
> - * 0 on success, -errno on failure.
> - */
> -int schedule_on_each_cpu(work_func_t func)
> +int schedule_on_each_cpu_cond_locked(work_func_t func, smp_cond_func_t cond_func, void *info)
> {
> + struct schedule_work __percpu *works;
> int cpu;
> - struct work_struct __percpu *works;
>
> - works = alloc_percpu(struct work_struct);
> + works = alloc_percpu(struct schedule_work);
> if (!works)
> return -ENOMEM;
>
> - cpus_read_lock();
> -
> for_each_online_cpu(cpu) {
> - struct work_struct *work = per_cpu_ptr(works, cpu);
> + struct schedule_work *work = per_cpu_ptr(works, cpu);
>
> - INIT_WORK(work, func);
> - schedule_work_on(cpu, work);
> + if (cond_func && !cond_func(cpu, info))
> + continue;
> +
> + INIT_WORK(&work->work, func);
> + work->info = info;
> + schedule_work_on(cpu, &work->work);
> }
>
> - for_each_online_cpu(cpu)
> - flush_work(per_cpu_ptr(works, cpu));
> + for_each_online_cpu(cpu) {
> + struct schedule_work *work = per_cpu_ptr(works, cpu);
> +
> + if (work->work.func)
> + flush_work(&work->work);
> + }
>
> - cpus_read_unlock();
> free_percpu(works);
> return 0;
> }
>
> +int schedule_on_each_cpu_cond(work_func_t func, smp_cond_func_t cond_func, void *info)
> +{
> + int ret;
> +
> + cpus_read_lock();
> + ret = schedule_on_each_cpu_cond_locked(func, cond, info);
> + cpus_read_unlock();
> +
> + return ret;
> +}
> +
> +/**
> + * schedule_on_each_cpu - execute a function synchronously on each online CPU
> + * @func: the function to call
> + *
> + * schedule_on_each_cpu() executes @func on each online CPU using the
> + * system workqueue and blocks until all CPUs have completed.
> + * schedule_on_each_cpu() is very slow.
> + *
> + * Return:
> + * 0 on success, -errno on failure.
> + */
> +int schedule_on_each_cpu(work_func_t func)
> +{
> + return schedule_on_each_cpu_cond(func, NULL, NULL);
> +}
> +
> /**
> * execute_in_process_context - reliably execute the routine with user context
> * @fn: the function to execute
Powered by blists - more mailing lists