linux-kernel - Re: [PATCH resend] smp: avoid sending needless IPI in smp_call_function

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]
Date:   Mon, 22 May 2017 16:04:57 +0800
From:   Aaron Lu <aaron.lu@...el.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Tim Chen <tim.c.chen@...ux.intel.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Huang Ying <ying.huang@...el.com>
Subject: Re: [PATCH resend] smp: avoid sending needless IPI in
 smp_call_function_many()

On Fri, May 19, 2017 at 12:59:54PM +0200, Peter Zijlstra wrote:
> On Fri, May 19, 2017 at 03:53:31PM +0800, Aaron Lu wrote:
> > Inter-Processor-Interrupt(IPI) is needed when a page is unmapped and the
> > process' mm_cpumask() shows the process has ever run on other CPUs. page
> > migration, page reclaim all need IPIs. The number of IPI needed to send
> > to different CPUs is especially large for multi-threaded workload since
> > mm_cpumask() is per process.
> > 
> > For smp_call_function_many(), whenever a CPU queues a CSD to a target
> > CPU, it will send an IPI to let the target CPU to handle the work.
> > This isn't necessary - we need only send IPI when queueing a CSD
> > to an empty call_single_queue.
> > 
> > The reason:
> > flush_smp_call_function_queue() that is called upon a CPU receiving an
> > IPI will empty the queue and then handle all of the CSDs there. So if
> > the target CPU's call_single_queue is not empty, we know that:
> > i.  An IPI for the target CPU has already been sent by 'previous queuers';
> > ii. flush_smp_call_function_queue() hasn't emptied that CPU's queue yet.
> > Thus, it's safe for us to just queue our CSD there without sending an
> > addtional IPI. And for the 'previous queuers', we can limit it to the
> > first queuer.
> > 
> > To demonstrate the effect of this patch, a multi-thread workload that
> > spawns 80 threads to equally consume 100G memory is used. This is tested
> > on a 2 node broadwell-EP which has 44cores/88threads and 32G memory. So
> > after 32G memory is used up, page reclaiming starts to happen a lot.
> > 
> > With this patch, IPI number dropped 88% and throughput increased about
> > 15% for the above workload.
> > 
> > Signed-off-by: Aaron Lu <aaron.lu@...el.com>
> 
> Seems fine to me, I'll queue it.
> 
> 
> > @@ -434,6 +442,7 @@ void smp_call_function_many(const struct cpumask *mask,
> >  	if (unlikely(!cpumask_weight(cfd->cpumask)))
> >  		return;
> >  
> > +	cpumask_clear(cfd->cpumask_ipi);
> >  	for_each_cpu(cpu, cfd->cpumask) {
> >  		struct call_single_data *csd = per_cpu_ptr(cfd->csd, cpu);
> >  
> > @@ -442,11 +451,12 @@ void smp_call_function_many(const struct cpumask *mask,
> >  			csd->flags |= CSD_FLAG_SYNCHRONOUS;
> >  		csd->func = func;
> >  		csd->info = info;
> > -		llist_add(&csd->llist, &per_cpu(call_single_queue, cpu));
> > +		if (llist_add(&csd->llist, &per_cpu(call_single_queue, cpu)))
> > +			cpumask_set_cpu(cpu, cfd->cpumask_ipi);
> >  	}
> 
> But looking at this I wonder why cpumask_{set,clear}_cpu() are atomic
> ops while most other cpumask ops are not.
> 
> This seems to suggest we want __cpumask_{set,clear}_cpu() and use those
> here. Because those LOCK prefixes sure are pointless.

Sounds reasonable to me.

> 
> >  
> >  	/* Send a message to all CPUs in the map */
> > -	arch_send_call_function_ipi_mask(cfd->cpumask);
> > +	arch_send_call_function_ipi_mask(cfd->cpumask_ipi);
> >  
> >  	if (wait) {
> >  		for_each_cpu(cpu, cfd->cpumask) {
> 
> Something like so on top I suppose.
> 
> Anybody?

With the below patch applied on top, the workload still works fine.
Performance is about the same.

Thanks,
Aaron
 
> ---
>  include/linux/cpumask.h |   11 +++++++++++
>  kernel/smp.c            |    4 ++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -293,6 +293,12 @@ static inline void cpumask_set_cpu(unsig
>  	set_bit(cpumask_check(cpu), cpumask_bits(dstp));
>  }
>  
> +static inline void __cpumask_set_cpu(unsigned int cpu, struct cpumask *dstp)
> +{
> +	__set_bit(cpumask_check(cpu), cpumask_bits(dstp));
> +}
> +
> +
>  /**
>   * cpumask_clear_cpu - clear a cpu in a cpumask
>   * @cpu: cpu number (< nr_cpu_ids)
> @@ -303,6 +309,11 @@ static inline void cpumask_clear_cpu(int
>  	clear_bit(cpumask_check(cpu), cpumask_bits(dstp));
>  }
>  
> +static inline void __cpumask_clear_cpu(int cpu, struct cpumask *dstp)
> +{
> +	__clear_bit(cpumask_check(cpu), cpumask_bits(dstp));
> +}
> +
>  /**
>   * cpumask_test_cpu - test for a cpu in a cpumask
>   * @cpu: cpu number (< nr_cpu_ids)
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -436,7 +436,7 @@ void smp_call_function_many(const struct
>  	cfd = this_cpu_ptr(&cfd_data);
>  
>  	cpumask_and(cfd->cpumask, mask, cpu_online_mask);
> -	cpumask_clear_cpu(this_cpu, cfd->cpumask);
> +	__cpumask_clear_cpu(this_cpu, cfd->cpumask);
>  
>  	/* Some callers race with other cpus changing the passed mask */
>  	if (unlikely(!cpumask_weight(cfd->cpumask)))
> @@ -452,7 +452,7 @@ void smp_call_function_many(const struct
>  		csd->func = func;
>  		csd->info = info;
>  		if (llist_add(&csd->llist, &per_cpu(call_single_queue, cpu)))
> -			cpumask_set_cpu(cpu, cfd->cpumask_ipi);
> +			__cpumask_set_cpu(cpu, cfd->cpumask_ipi);
>  	}
>  
>  	/* Send a message to all CPUs in the map */