Re: [PATCH 6/6] smp: Cleanup smp_call_function*()

[Christoph Hellwig <hch@infradead.org>]

> -static DEFINE_PER_CPU(call_single_data_t, backtrace_csd);
> +static DEFINE_PER_CPU(call_single_data_t, backtrace_csd) = CSD_INIT(handle_backtrace, NULL);
>  static struct cpumask backtrace_csd_busy;

Besides the crazy long line: does assigning to a DEFINE_PER_CPU
really work and initialize all the members?

> @@ -178,9 +178,7 @@ static void zpci_handle_fallback_irq(voi
>  		if (atomic_inc_return(&cpu_data->scheduled) > 1)
>  			continue;
>  
> -		cpu_data->csd.func = zpci_handle_remote_irq;
> -		cpu_data->csd.info = &cpu_data->scheduled;
> -		cpu_data->csd.flags = 0;
> +		cpu_data->csd = CSD_INIT(zpci_handle_remote_irq, &cpu_data->scheduled);

This looks weird.  I'd much rather see an initialization ala INIT_WORK:

		INIT_CSD(&cpu_data->csd, zpci_handle_remote_irq,
			 &cpu_data->scheduled);

Also for many smp_call_function_* users it would be trivial and actually
lead to nicer code if the data argument went away and we'd just use
container_of to get to the containing structure.  For the remaining
ones we can trivially general a container strucuture that has the
extra data pointer.

> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -629,9 +629,7 @@ void blk_mq_force_complete_rq(struct req
>  		shared = cpus_share_cache(cpu, ctx->cpu);
>  
>  	if (cpu != ctx->cpu && !shared && cpu_online(ctx->cpu)) {
> -		rq->csd.func = __blk_mq_complete_request_remote;
> -		rq->csd.info = rq;
> -		rq->csd.flags = 0;
> +		rq->csd = CSD_INIT(__blk_mq_complete_request_remote, rq);
>  		smp_call_function_single_async(ctx->cpu, &rq->csd);
>  	} else {
>  		q->mq_ops->complete(rq);
> --- a/block/blk-softirq.c
> +++ b/block/blk-softirq.c
> @@ -57,13 +57,8 @@ static void trigger_softirq(void *data)
>  static int raise_blk_irq(int cpu, struct request *rq)
>  {
>  	if (cpu_online(cpu)) {
> -		call_single_data_t *data = &rq->csd;
> -
> -		data->func = trigger_softirq;
> -		data->info = rq;
> -		data->flags = 0;
> -
> -		smp_call_function_single_async(cpu, data);
> +		rq->csd = CSD_INIT(trigger_softirq, rq);
> +		smp_call_function_single_async(cpu, &rq->csd);
>  		return 0;
>  	}

FYI, I rewrote much of the blk code in this series:

https://lore.kernel.org/linux-block/20200611064452.12353-1-hch@lst.de/T/#t

that you also were Cced on.

>  struct __call_single_data {
> -	union {
> -		struct __call_single_node node;
> -		struct {
> -			struct llist_node llist;
> -			unsigned int flags;
> -		};
> -	};
> +	struct __call_single_node node;
>  	smp_call_func_t func;
>  	void *info;
>  };

Can we rename this to struct call_single_data without the __prefix
and switch all the users you touch anyway away from the typedef?