Messages in this thread Patch in this message | | | Date | Fri, 19 May 2017 12:59:54 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH resend] smp: avoid sending needless IPI in smp_call_function_many() |
| |
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@intel.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.
> > /* 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?
--- 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 */
| |