* [PATCH 0/2] arm-cc*: ensure perf synchronisation
@ 2016-06-28 17:50 Mark Rutland
2016-06-28 17:50 ` [PATCH 1/2] arm-ccn: " Mark Rutland
2016-06-28 17:50 ` [PATCH 2/2] arm-cci: " Mark Rutland
0 siblings, 2 replies; 9+ messages in thread
From: Mark Rutland @ 2016-06-28 17:50 UTC (permalink / raw)
To: linux-arm-kernel
As discovered during review of the X-Gene SoC PMU [1], the arm-cc{i,n} drivers
don't ensure that IRQ balancers don't migrate interrupts.
This is problematic for the perf core code, which requires mutual exclusion of
certain operations (e.g. event rotation, cross-calls, and irq handling) to be
provided by disabling IRQs, which only works if all operations occur on the
same CPU. This is also required for safe (lockless) manipulation of some data
structures.
To avoid this problem, we must request interrupts with IRQF_NOBALANCING, as is
already the case for CPU PMU drivers which make use of interrupts.
To ensure synchronisation between IRQ handlers and other manipulation of said
data structures or HW state, we must also ensure that the interrupt handlers
are not threaded, by requesting them with IRQF_NO_THREAD, as is already the
case for CPU PMU drivers.
Thanks,
Mark.
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/439589.html
Mark Rutland (2):
arm-ccn: ensure perf synchronisation
arm-cci: ensure perf synchronisation
drivers/bus/arm-cci.c | 3 ++-
drivers/bus/arm-ccn.c | 5 +++--
2 files changed, 5 insertions(+), 3 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/2] arm-ccn: ensure perf synchronisation 2016-06-28 17:50 [PATCH 0/2] arm-cc*: ensure perf synchronisation Mark Rutland @ 2016-06-28 17:50 ` Mark Rutland 2016-07-04 13:39 ` Pawel Moll 2016-06-28 17:50 ` [PATCH 2/2] arm-cci: " Mark Rutland 1 sibling, 1 reply; 9+ messages in thread From: Mark Rutland @ 2016-06-28 17:50 UTC (permalink / raw) To: linux-arm-kernel Currently the IRQ core is permitted to make the CCN PMU IRQ handler threaded, and will allow userspace to change the CPU affinity of the interrupt behind our back. Both of these could violate our synchronisation requirements with the core perf code, which relies upon strict CPU affinity and disabling of interrupts to guarantee mutual exclusion in some cases. As with the CPU PMU drivers, we should request the interrupt with IRQF_NOBALANCING and IRQF_NO_THREAD, to avoid these issues. Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Marc Zyngier <marc.zyngier@arm.com> Cc: Pawel Moll <pawel.moll@arm.com> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> Cc: Will Deacon <will.deacon@arm.com> Cc: Olof Johansson <olof@lixom.net> Cc: Arnd Bergmann <arnd@arndb.de> Cc: <arm@kernel.org> --- drivers/bus/arm-ccn.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/bus/arm-ccn.c b/drivers/bus/arm-ccn.c index acc3eb5..2c99ab2 100644 --- a/drivers/bus/arm-ccn.c +++ b/drivers/bus/arm-ccn.c @@ -1479,8 +1479,9 @@ static int arm_ccn_probe(struct platform_device *pdev) /* Can set 'disable' bits, so can acknowledge interrupts */ writel(CCN_MN_ERRINT_STATUS__PMU_EVENTS__ENABLE, ccn->base + CCN_MN_ERRINT_STATUS); - err = devm_request_irq(ccn->dev, irq, arm_ccn_irq_handler, 0, - dev_name(ccn->dev), ccn); + err = devm_request_irq(ccn->dev, irq, arm_ccn_irq_handler, + IRQF_NOBALANCING | IRQF_NO_THREAD, + dev_name(ccn->dev), ccn); if (err) return err; -- 1.9.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 1/2] arm-ccn: ensure perf synchronisation 2016-06-28 17:50 ` [PATCH 1/2] arm-ccn: " Mark Rutland @ 2016-07-04 13:39 ` Pawel Moll 2016-07-04 13:50 ` Mark Rutland 0 siblings, 1 reply; 9+ messages in thread From: Pawel Moll @ 2016-07-04 13:39 UTC (permalink / raw) To: linux-arm-kernel Dnia 2016-06-28, Tue o godzinie 18:50 +0100, Mark Rutland pisze: > Currently the IRQ core is permitted to make the CCN PMU IRQ handler > threaded, and will allow userspace to change the CPU affinity of the > interrupt behind our back. Both of these could violate our > synchronisation requirements with the core perf code, which relies upon > strict CPU affinity and disabling of interrupts to guarantee mutual > exclusion in some cases. > > As with the CPU PMU drivers, we should request the interrupt with > IRQF_NOBALANCING and IRQF_NO_THREAD, to avoid these issues. > Signed-off-by: Mark Rutland <mark.rutland@arm.com> Acked-by: Pawel Moll <pawel.moll@arm.com> In principle, because still had no chance to test it... Thanks! Pawel ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] arm-ccn: ensure perf synchronisation 2016-07-04 13:39 ` Pawel Moll @ 2016-07-04 13:50 ` Mark Rutland 0 siblings, 0 replies; 9+ messages in thread From: Mark Rutland @ 2016-07-04 13:50 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jul 04, 2016 at 02:39:44PM +0100, Pawel Moll wrote: > Dnia 2016-06-28, Tue o godzinie 18:50 +0100, Mark Rutland pisze: > > Currently the IRQ core is permitted to make the CCN PMU IRQ handler > > threaded, and will allow userspace to change the CPU affinity of the > > interrupt behind our back. Both of these could violate our > > synchronisation requirements with the core perf code, which relies upon > > strict CPU affinity and disabling of interrupts to guarantee mutual > > exclusion in some cases. > > > > As with the CPU PMU drivers, we should request the interrupt with > > IRQF_NOBALANCING and IRQF_NO_THREAD, to avoid these issues. > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > > Acked-by: Pawel Moll <pawel.moll@arm.com> Cheers. I've just sent v2, so I've applied that locally, and it'll appear if a v3 is necessary. > In principle, because still had no chance to test it... Understood. Thanks, Mark. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] arm-cci: ensure perf synchronisation 2016-06-28 17:50 [PATCH 0/2] arm-cc*: ensure perf synchronisation Mark Rutland 2016-06-28 17:50 ` [PATCH 1/2] arm-ccn: " Mark Rutland @ 2016-06-28 17:50 ` Mark Rutland 2016-07-04 10:22 ` Will Deacon 1 sibling, 1 reply; 9+ messages in thread From: Mark Rutland @ 2016-06-28 17:50 UTC (permalink / raw) To: linux-arm-kernel Currently the IRQ core is permitted to make the CCI PMU IRQ handler threaded, and will allow userspace to change the CPU affinity of the interrupt behind our back. Both of these could violate our synchronisation requirements with the core perf code, which relies upon strict CPU affinity and disabling of interrupts to guarantee mutual exclusion in some cases. As with the CPU PMU drivers, we should request the interrupt with IRQF_NOBALANCING and IRQF_NO_THREAD, to avoid these issues. Signed-off-by: Mark Rutland <mark.rutland@arm.com> Cc: Marc Zyngier <marc.zyngier@arm.com> Cc: Punit Agrawal <punit.agrawal@arm.com> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> Cc: Will Deacon <will.deacon@arm.com> Cc: Olof Johansson <olof@lixom.net> Cc: Arnd Bergmann <arnd@arndb.de> Cc: <arm@kernel.org> --- drivers/bus/arm-cci.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c index a49b283..6a8825a 100644 --- a/drivers/bus/arm-cci.c +++ b/drivers/bus/arm-cci.c @@ -881,7 +881,8 @@ static int pmu_request_irq(struct cci_pmu *cci_pmu, irq_handler_t handler) * This should allow handling of non-unique interrupt for the counters. */ for (i = 0; i < cci_pmu->nr_irqs; i++) { - int err = request_irq(cci_pmu->irqs[i], handler, IRQF_SHARED, + int err = request_irq(cci_pmu->irqs[i], handler, + IRQF_SHARED | IRQF_NOBALANCING | IRQF_NO_THREAD, "arm-cci-pmu", cci_pmu); if (err) { dev_err(&pmu_device->dev, "unable to request IRQ%d for ARM CCI PMU counters\n", -- 1.9.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] arm-cci: ensure perf synchronisation 2016-06-28 17:50 ` [PATCH 2/2] arm-cci: " Mark Rutland @ 2016-07-04 10:22 ` Will Deacon 2016-07-04 10:31 ` Mark Rutland 0 siblings, 1 reply; 9+ messages in thread From: Will Deacon @ 2016-07-04 10:22 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jun 28, 2016 at 06:50:18PM +0100, Mark Rutland wrote: > Currently the IRQ core is permitted to make the CCI PMU IRQ handler > threaded, and will allow userspace to change the CPU affinity of the > interrupt behind our back. Both of these could violate our > synchronisation requirements with the core perf code, which relies upon > strict CPU affinity and disabling of interrupts to guarantee mutual > exclusion in some cases. Minor nit, but I think $subject is particularly unhelpful for these two patches. How about "arm-ccX: fix PMU interrupt flags"? > As with the CPU PMU drivers, we should request the interrupt with > IRQF_NOBALANCING and IRQF_NO_THREAD, to avoid these issues. > > Signed-off-by: Mark Rutland <mark.rutland@arm.com> > Cc: Marc Zyngier <marc.zyngier@arm.com> > Cc: Punit Agrawal <punit.agrawal@arm.com> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > Cc: Will Deacon <will.deacon@arm.com> > Cc: Olof Johansson <olof@lixom.net> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: <arm@kernel.org> > --- > drivers/bus/arm-cci.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c > index a49b283..6a8825a 100644 > --- a/drivers/bus/arm-cci.c > +++ b/drivers/bus/arm-cci.c > @@ -881,7 +881,8 @@ static int pmu_request_irq(struct cci_pmu *cci_pmu, irq_handler_t handler) > * This should allow handling of non-unique interrupt for the counters. > */ > for (i = 0; i < cci_pmu->nr_irqs; i++) { > - int err = request_irq(cci_pmu->irqs[i], handler, IRQF_SHARED, Why is this shared and who is the line shared with? We should check that we don't have contradictory IRQ flags in the other irq request path(s). Will ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] arm-cci: ensure perf synchronisation 2016-07-04 10:22 ` Will Deacon @ 2016-07-04 10:31 ` Mark Rutland 2016-07-04 10:44 ` Mark Rutland 2016-07-04 11:16 ` Punit Agrawal 0 siblings, 2 replies; 9+ messages in thread From: Mark Rutland @ 2016-07-04 10:31 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jul 04, 2016 at 11:22:05AM +0100, Will Deacon wrote: > On Tue, Jun 28, 2016 at 06:50:18PM +0100, Mark Rutland wrote: > > Currently the IRQ core is permitted to make the CCI PMU IRQ handler > > threaded, and will allow userspace to change the CPU affinity of the > > interrupt behind our back. Both of these could violate our > > synchronisation requirements with the core perf code, which relies upon > > strict CPU affinity and disabling of interrupts to guarantee mutual > > exclusion in some cases. > > Minor nit, but I think $subject is particularly unhelpful for these two > patches. How about "arm-ccX: fix PMU interrupt flags"? Sure, I'll move over to that wording. > > @@ -881,7 +881,8 @@ static int pmu_request_irq(struct cci_pmu *cci_pmu, irq_handler_t handler) > > * This should allow handling of non-unique interrupt for the counters. > > */ > > for (i = 0; i < cci_pmu->nr_irqs; i++) { > > - int err = request_irq(cci_pmu->irqs[i], handler, IRQF_SHARED, > > Why is this shared and who is the line shared with? We should check that > we don't have contradictory IRQ flags in the other irq request path(s). Hmm... I thought that was so the driver could request the same IRQ multiple times in the case of muxing, but I see we've always had the is_duplicate_irq logic. The IRQF_SHARED flags has also been there since day one, so I'm not sure if that's needed for some platform or whether that was added out of habit. Punit, do you recall if/why IRQF_SHARED was used? I'll take a look at dts and see if I can get rid of it. Thanks, Mark. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] arm-cci: ensure perf synchronisation 2016-07-04 10:31 ` Mark Rutland @ 2016-07-04 10:44 ` Mark Rutland 2016-07-04 11:16 ` Punit Agrawal 1 sibling, 0 replies; 9+ messages in thread From: Mark Rutland @ 2016-07-04 10:44 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jul 04, 2016 at 11:31:31AM +0100, Mark Rutland wrote: > On Mon, Jul 04, 2016 at 11:22:05AM +0100, Will Deacon wrote: > > On Tue, Jun 28, 2016 at 06:50:18PM +0100, Mark Rutland wrote: > > > Currently the IRQ core is permitted to make the CCI PMU IRQ handler > > > threaded, and will allow userspace to change the CPU affinity of the > > > interrupt behind our back. Both of these could violate our > > > synchronisation requirements with the core perf code, which relies upon > > > strict CPU affinity and disabling of interrupts to guarantee mutual > > > exclusion in some cases. > > > > Minor nit, but I think $subject is particularly unhelpful for these two > > patches. How about "arm-ccX: fix PMU interrupt flags"? > > Sure, I'll move over to that wording. > > > > @@ -881,7 +881,8 @@ static int pmu_request_irq(struct cci_pmu *cci_pmu, irq_handler_t handler) > > > * This should allow handling of non-unique interrupt for the counters. > > > */ > > > for (i = 0; i < cci_pmu->nr_irqs; i++) { > > > - int err = request_irq(cci_pmu->irqs[i], handler, IRQF_SHARED, > > > > Why is this shared and who is the line shared with? We should check that > > we don't have contradictory IRQ flags in the other irq request path(s). > > Hmm... I thought that was so the driver could request the same IRQ > multiple times in the case of muxing, but I see we've always had the > is_duplicate_irq logic. > > The IRQF_SHARED flags has also been there since day one, so I'm not sure > if that's needed for some platform or whether that was added out of > habit. > > Punit, do you recall if/why IRQF_SHARED was used? > > I'll take a look at dts and see if I can get rid of it. Tree-wide there only appears to be one instance of the pmu node: [mark at leverpostej:~/src/linux]% git grep 'cci-.*-pmu' -- arch arch/arm/boot/dts/vexpress-v2p-ca15_a7.dts: compatible = "arm,cci-400-pmu,r0"; Which has locally-unique interrupts... pmu at 9000 { compatible = "arm,cci-400-pmu,r0"; reg = <0x9000 0x5000>; interrupts = <0 105 4>, <0 101 4>, <0 102 4>, <0 103 4>, <0 104 4>; }; .. and from reading the A15x2-A7x3 board TRM, they're globally unique (i.e. not shared) too. I'll drop the IRQF_SHARED in v2. Thanks, Mark. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] arm-cci: ensure perf synchronisation 2016-07-04 10:31 ` Mark Rutland 2016-07-04 10:44 ` Mark Rutland @ 2016-07-04 11:16 ` Punit Agrawal 1 sibling, 0 replies; 9+ messages in thread From: Punit Agrawal @ 2016-07-04 11:16 UTC (permalink / raw) To: linux-arm-kernel Mark Rutland <mark.rutland@arm.com> writes: > On Mon, Jul 04, 2016 at 11:22:05AM +0100, Will Deacon wrote: >> On Tue, Jun 28, 2016 at 06:50:18PM +0100, Mark Rutland wrote: >> > Currently the IRQ core is permitted to make the CCI PMU IRQ handler >> > threaded, and will allow userspace to change the CPU affinity of the >> > interrupt behind our back. Both of these could violate our >> > synchronisation requirements with the core perf code, which relies upon >> > strict CPU affinity and disabling of interrupts to guarantee mutual >> > exclusion in some cases. >> >> Minor nit, but I think $subject is particularly unhelpful for these two >> patches. How about "arm-ccX: fix PMU interrupt flags"? > > Sure, I'll move over to that wording. > >> > @@ -881,7 +881,8 @@ static int pmu_request_irq(struct cci_pmu *cci_pmu, irq_handler_t handler) >> > * This should allow handling of non-unique interrupt for the counters. >> > */ >> > for (i = 0; i < cci_pmu->nr_irqs; i++) { >> > - int err = request_irq(cci_pmu->irqs[i], handler, IRQF_SHARED, >> >> Why is this shared and who is the line shared with? We should check that >> we don't have contradictory IRQ flags in the other irq request path(s). > > Hmm... I thought that was so the driver could request the same IRQ > multiple times in the case of muxing, but I see we've always had the > is_duplicate_irq logic. > > The IRQF_SHARED flags has also been there since day one, so I'm not sure > if that's needed for some platform or whether that was added out of > habit. > > Punit, do you recall if/why IRQF_SHARED was used? AFAIR, there was no platform that needed the flag. The IRQF_SHARED flag was included to allow sharing in the eventuality that such a platform did show up. I worked with the assumption that adding the flag was part of good practice for a driver that didn't need exclusive interrupt. Anyways, I haven't come across any platform that's needed that functionality yet. I think it can be safely dropped. Thanks, Punit > > I'll take a look at dts and see if I can get rid of it. > > Thanks, > Mark. > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-07-04 13:50 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-06-28 17:50 [PATCH 0/2] arm-cc*: ensure perf synchronisation Mark Rutland 2016-06-28 17:50 ` [PATCH 1/2] arm-ccn: " Mark Rutland 2016-07-04 13:39 ` Pawel Moll 2016-07-04 13:50 ` Mark Rutland 2016-06-28 17:50 ` [PATCH 2/2] arm-cci: " Mark Rutland 2016-07-04 10:22 ` Will Deacon 2016-07-04 10:31 ` Mark Rutland 2016-07-04 10:44 ` Mark Rutland 2016-07-04 11:16 ` Punit Agrawal
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).