Re: [PATCH v9 07/18] x86/virt/tdx: Do TDX module per-cpu initialization

[Huang, Kai]

> > +
> > +       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