linux-kernel - Re: [PATCH 6/6] smp: Cleanup smp_call

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200617090051.GF2531@hirez.programming.kicks-ass.net>
Date:   Wed, 17 Jun 2020 11:00:51 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Christoph Hellwig <hch@...radead.org>
Cc:     mingo@...nel.org, tglx@...utronix.de, linux-kernel@...r.kernel.org,
        juri.lelli@...hat.com, vincent.guittot@...aro.org,
        dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
        mgorman@...e.de, paulmck@...nel.org, frederic@...nel.org,
        tsbogend@...ha.franken.de, axboe@...nel.dk, rjw@...ysocki.net,
        daniel.lezcano@...aro.org, dchickles@...vell.com,
        davem@...emloft.net, kuba@...nel.org, daniel.thompson@...aro.org,
        gerald.schaefer@...ibm.com
Subject: Re: [PATCH 6/6] smp: Cleanup smp_call_function*()

On Wed, Jun 17, 2020 at 01:23:49AM -0700, Christoph Hellwig wrote:
> > -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?

Yes. The way it works is that it initializes the variable that ends up
in the .data..percpu section and that's copied when we create the
actual per-cpu things.

> > @@ -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.

Agreed, except that won't work for things like cfd_data, csd_data and
csd_stack in smp.c. It might be possible to rework some of that, but
that's going to be further surgery.

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

Yes, I know. The merge shouldn't be too difficult, but if that's landed
in a git tree meanwhile, I can try and pull that in.

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

That mess exists because of the alignment thing. IIRC you can't use the
sizeof() of a struct you're still declaring.