Messages in this thread | | | Date | Wed, 17 Jun 2020 01:23:49 -0700 | From | Christoph Hellwig <> | Subject | Re: [PATCH 6/6] smp: Cleanup smp_call_function*() |
| |
> -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?
| |