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 <[email protected]>
>
> 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 */
