From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756293AbcJNCJF (ORCPT ); Thu, 13 Oct 2016 22:09:05 -0400 Received: from mail-pf0-f193.google.com ([209.85.192.193]:32810 "EHLO mail-pf0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751337AbcJNCI5 (ORCPT ); Thu, 13 Oct 2016 22:08:57 -0400 Subject: Re: [PATCH] irqchip/gic: Enable gic_set_affinity set more than one cpu To: Marc Zyngier References: <1476356234-7570-1-git-send-email-cs.os.kernel@gmail.com> <20161013163140.5f23abce@arm.com> Cc: tglx@linutronix.de, jason@lakedaemon.net, linux-kernel@vger.kernel.org, cs.os.kernel@gmail.com From: Cheng Chao Message-ID: <1c4adb8a-f7f7-3474-273a-edf34f575b8d@gmail.com> Date: Fri, 14 Oct 2016 10:08:10 +0800 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:45.0) Gecko/20100101 Thunderbird/45.0 MIME-Version: 1.0 In-Reply-To: <20161013163140.5f23abce@arm.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Marc, Thanks for your comments. Cheng on 10/13/2016 11:31 PM, Marc Zyngier wrote: > On Thu, 13 Oct 2016 18:57:14 +0800 > Cheng Chao wrote: > >> GIC can distribute an interrupt to more than one cpu, >> but now, gic_set_affinity sets only one cpu to handle interrupt. > > What makes you think this is a good idea? What purpose does it serves? > I can only see drawbacks to this: You're waking up more than one CPU, > wasting power, adding jitter and clobbering the cache. > > I assume you see a benefit to that approach, so can you please spell it > out? > Ok, You are right, but the performance is another point that we should consider. We use E1 device to transmit/receive video stream. we find that E1's interrupt is only on the one cpu that cause this cpu usage is almost 100%, but other cpus is much lower load, so the performance is not good. the cpu is 4-core. so add CONFIG_ARM_GIC_AFFINITY_SINGLE_CPU is better? thus we can make a trade-off between the performance with the power etc. >> >> Signed-off-by: Cheng Chao >> --- >> drivers/irqchip/irq-gic.c | 28 ++++++++++++++++++++++++---- >> 1 file changed, 24 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >> index 58e5b4e..198d33f 100644 >> --- a/drivers/irqchip/irq-gic.c >> +++ b/drivers/irqchip/irq-gic.c >> @@ -328,18 +328,38 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, >> unsigned int cpu, shift = (gic_irq(d) % 4) * 8; >> u32 val, mask, bit; >> unsigned long flags; >> + u32 valid_mask; >> >> - if (!force) >> - cpu = cpumask_any_and(mask_val, cpu_online_mask); >> - else >> + if (!force) { >> + valid_mask = cpumask_bits(mask_val)[0]; >> + valid_mask &= cpumask_bits(cpu_online_mask)[0]; >> + >> + cpu = cpumask_any((struct cpumask *)&valid_mask); > > What is wrong with with cpumask_any_and? > #define cpumask_any_and(mask1, mask2) cpumask_first_and((mask1), (mask2)) #define cpumask_any(srcp) cpumask_first(srcp) There is no wrong with the cpumask_any_and. >> + } else { >> cpu = cpumask_first(mask_val); >> + } >> >> if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids) >> return -EINVAL; >> >> gic_lock_irqsave(flags); >> mask = 0xff << shift; >> - bit = gic_cpu_map[cpu] << shift; >> + >> + if (!force) { >> + bit = 0; >> + >> + for_each_cpu(cpu, (struct cpumask *)&valid_mask) { >> + if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids) >> + break; > > Shouldn't that be an error? > tested, no error. at the beginning, I code such like, cpumask_var_t valid_mask; alloc_cpumask_var(&valid_mask, GFP_KERNEL); cpumask_and(valid_mask, mask_val, cpu_online_mask); for_each_cpu(cpu, valid_mask) { } but alloc_cpumask_var maybe fail, so if (!alloc_cpumask_var(&valid_mask, GFP_KERNEL)) { /* fail*/ } else { } a little more complex. >> + >> + bit |= gic_cpu_map[cpu]; >> + } >> + >> + bit = bit << shift; >> + } else { >> + bit = gic_cpu_map[cpu] << shift; >> + } >> + >> val = readl_relaxed(reg) & ~mask; >> writel_relaxed(val | bit, reg); >> gic_unlock_irqrestore(flags); > > Thanks, > > M. >