From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757278AbcJZCEj (ORCPT ); Tue, 25 Oct 2016 22:04:39 -0400 Received: from mail-pf0-f196.google.com ([209.85.192.196]:33769 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753339AbcJZCEg (ORCPT ); Tue, 25 Oct 2016 22:04:36 -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> <1c4adb8a-f7f7-3474-273a-edf34f575b8d@gmail.com> <580116EB.7050106@arm.com> <36c09045-62e0-93b4-1974-3f3c4c761c01@arm.com> Cc: tglx@linutronix.de, jason@lakedaemon.net, linux-kernel@vger.kernel.org, cs.os.kernel@gmail.com From: Cheng Chao Message-ID: <5ccf6268-c68b-7b3f-1762-5852b40a9058@gmail.com> Date: Wed, 26 Oct 2016 10:04:44 +0800 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:45.0) Gecko/20100101 Thunderbird/45.0 MIME-Version: 1.0 In-Reply-To: <36c09045-62e0-93b4-1974-3f3c4c761c01@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 on 10/25/2016 06:09 PM, Marc Zyngier wrote: > On 15/10/16 08:23, Cheng Chao wrote: >> On 10/15/2016 01:33 AM, Marc Zyngier wrote: >>>> 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. >>> >>> It looks to me like you're barking up the wrong tree. We have >>> NAPI-enabled network drivers for this exact reason, and adding more >>> interrupts to an already overloaded system doesn't strike me as going in >>> the right direction. May I suggest that you look at integrating NAPI >>> into your E1 driver? >>> >> >> great, NAPI maybe is a good option, I can try to use NAPI. thank you. >> >> In other hand, gic_set_affinity sets only one cpu to handle interrupt, >> that really makes me a little confused, why does GIC's driver not like >> the others(MPIC, APIC etc) to support many cpus to handle interrupt? >> >> It seems that the GIC's driver constrain too much. > > There is several drawbacks to this: > - Cache impacts and power efficiency, as already mentioned > - Not virtualizable (you cannot efficiently implement this in a > hypervisor that emulates a GICv2 distributor) > - Doesn't scale (you cannot go beyond 8 CPUs) > > I strongly suggest you give NAPI a go, and only then consider > delivering interrupts to multiple CPUs, because multiple CPU > delivery is not future proof. > Thanks again, the E1 driver with NAPI is on the right track. >> I think it is more reasonable to let user decide what to do. >> >> If I care about the power etc, then I only echo single cpu to >> /proc/irq/xx/smp_affinity, but if I expect more than one cpu to handle >> one special interrupt, I can echo 'what I expect cpus' to >> /proc/irq/xx/smp_affinity. > > If that's what you really want, a better patch may be something like this: > I hope the GIC'c driver is more flexible, and gic_set_affinity() doesn't constrain to set only one cpu. the GIC supports to distribute more than one cpu after all. > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index d6c404b..b301d72 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -326,20 +326,25 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, > { > void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3); > unsigned int cpu, shift = (gic_irq(d) % 4) * 8; > - u32 val, mask, bit; > - unsigned long flags; > + u32 val, mask, bit = 0; > + unsigned long flags, aff = 0; > > - if (!force) > - cpu = cpumask_any_and(mask_val, cpu_online_mask); > - else > - cpu = cpumask_first(mask_val); > + for_each_cpu(cpu, mask_val) { > + if (force) { > + aff = 1 << cpu; > + break; > + } > + > + aff |= cpu_online(cpu) << cpu; > + } > > - if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids) > + if (!aff) > return -EINVAL; > > gic_lock_irqsave(flags); > mask = 0xff << shift; > - bit = gic_cpu_map[cpu] << shift; > + for_each_set_bit(cpu, &aff, nr_cpu_ids) > + bit |= gic_cpu_map[cpu] << shift; > val = readl_relaxed(reg) & ~mask; > writel_relaxed(val | bit, reg); > gic_unlock_irqrestore(flags); > this patch is more better than before. a little check add. diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 58e5b4e..b3d0f07 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -326,20 +326,28 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, { void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3); unsigned int cpu, shift = (gic_irq(d) % 4) * 8; - u32 val, mask, bit; - unsigned long flags; + u32 val, mask, bit = 0; + unsigned long flags, aff = 0; - if (!force) - cpu = cpumask_any_and(mask_val, cpu_online_mask); - else - cpu = cpumask_first(mask_val); + for_each_cpu(cpu, mask_val) { + if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids) + break; + + if (force) { + aff = 1 << cpu; + break; + } + + aff |= cpu_online(cpu) << cpu; + } - if (cpu >= NR_GIC_CPU_IF || cpu >= nr_cpu_ids) + if (!aff) return -EINVAL; gic_lock_irqsave(flags); mask = 0xff << shift; - bit = gic_cpu_map[cpu] << shift; + for_each_set_bit(cpu, &aff, nr_cpu_ids) + bit |= gic_cpu_map[cpu] << shift; val = readl_relaxed(reg) & ~mask; writel_relaxed(val | bit, reg); gic_unlock_irqrestore(flags); > Thanks, > > M. > Thanks, Cheng