perf/arm-ni: IRQ improvements

From: Shouping Wang <[email protected]>

NI-700 has a distinct PMU interrupt output for each Clock Domain, however some integrations may still combine these together externally. The initial driver didn't attempt to support this, in anticipation of a more general solution for IRQ sharing between system PMU instances, but that's still a way off, so let's make this intermediate step for now to at least allow sharing IRQs within an individual NI instance.

Now that CPU affinity and migration are cleaned up, it's fairly straightforward to adopt similar logic to arm-cmn, to identify CDs with a common interrupt and loop over them directly in the handler.

Signed-off-by Shouping Wang <[email protected]>

[ rm: Rework for affinity handling, cosmetics, new commit message ]

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/perf/arm-ni.c | 78 ++++++++++++++++++++++++++++---------------
 1 file changed, 51 insertions(+), 27 deletions(-)

drivers/perf/arm-ni.c +0 -0

index 168750e78fc4..ca033c8785ff 100644

drivers/perf/arm-ni.c +0 -0

 	int num_units;
 	int irq;
 	struct pmu pmu;

@@ -448,33 +449,37 @@ static irqreturn_t arm_ni_handle_irq(int irq, void *dev_id)
 {

drivers/perf/arm-ni.c +20 -20

+		u32 reg = readl_relaxed(cd->pmu_base + NI_PMOVSCLR);
+
+		if (reg & (1U << NI_CCNT_IDX)) {
+			ret = IRQ_HANDLED;
+			if (!(WARN_ON(!cd->ccnt))) {
+				arm_ni_event_read(cd->ccnt);
+				arm_ni_init_ccnt(cd);
+			}
 		}
-	}
-	for (int i = 0; i < NI_NUM_COUNTERS; i++) {
-		if (!(reg & (1U << i)))
-			continue;
-		ret = IRQ_HANDLED;
-		if (!(WARN_ON(!cd->evcnt[i]))) {
-			arm_ni_event_read(cd->evcnt[i]);
-			arm_ni_init_evcnt(cd, i);
+		for (int i = 0; i < NI_NUM_COUNTERS; i++) {
+			if (!(reg & (1U << i)))
+				continue;
+			ret = IRQ_HANDLED;
+			if (!(WARN_ON(!cd->evcnt[i]))) {
+				arm_ni_event_read(cd->evcnt[i]);
+				arm_ni_init_evcnt(cd, i);
+			}
 		}
+		writel_relaxed(reg, cd->pmu_base + NI_PMOVSCLR);
+		if (!cd->irq_friend)
+			return ret;
+		cd += cd->irq_friend;
 	}
-	writel_relaxed(reg, cd->pmu_base + NI_PMOVSCLR);
-	return ret;
 }
 
 static int arm_ni_init_cd(struct arm_ni *ni, struct arm_ni_node *node, u64 res_start)
 {
 	struct arm_ni_cd *cd = ni->cds + node->id;
 	const char *name;
-	int err;
 
 	cd->id = node->id;
 	cd->num_units = node->num_components;

@@ -534,20 +539,11 @@ static int arm_ni_init_cd(struct arm_ni *ni, struct arm_ni_node *node, u64 res_s
 		       cd->pmu_base + NI_PMCR);
 	writel_relaxed(U32_MAX, cd->pmu_base + NI_PMCNTENCLR);
 	writel_relaxed(U32_MAX, cd->pmu_base + NI_PMOVSCLR);
-	writel_relaxed(U32_MAX, cd->pmu_base + NI_PMINTENSET);
 
 	cd->irq = platform_get_irq(to_platform_device(ni->dev), cd->id);
 	if (cd->irq < 0)
 		return cd->irq;
 
-	err = devm_request_irq(ni->dev, cd->irq, arm_ni_handle_irq,
-			       IRQF_NOBALANCING | IRQF_NO_THREAD,
-			       dev_name(ni->dev), cd);
-	if (err)
-		return err;
-
-	irq_set_affinity(cd->irq, cpumask_of(ni->cpu));
-
 	cd->pmu = (struct pmu) {
 		.module = THIS_MODULE,
 		.parent = ni->dev,

@@ -593,6 +589,30 @@ static void arm_ni_probe_domain(void __iomem *base, struct arm_ni_node *node)
 	node->num_components = readl_relaxed(base + NI_CHILD_NODE_INFO);

Quoted 20 lines · Robin Murphy

From: Shouping Wang <[email protected]>

NI-700 has a distinct PMU interrupt output for each Clock Domain, however some integrations may still combine these together externally. The initial driver didn't attempt to support this, in anticipation of a more general solution for IRQ sharing between system PMU instances, but that's still a way off, so let's make this intermediate step for now to at least allow sharing IRQs within an individual NI instance.

Now that CPU affinity and migration are cleaned up, it's fairly straightforward to adopt similar logic to arm-cmn, to identify CDs with a common interrupt and loop over them directly in the handler.

Signed-off-by Shouping Wang <[email protected]>

[ rm: Rework for affinity handling, cosmetics, new commit message ]

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/perf/arm-ni.c | 78 ++++++++++++++++++++++++++++---------------
  1 file changed, 51 insertions(+), 27 deletions(-)

drivers/perf/arm-ni.c struct arm_ni_unit { · line 105

Quoted 9 lines

drivers/perf/arm-ni.c +1 -0

index 168750e78fc4..ca033c8785ff 100644
@@ -102,6 +102,7 @@ struct arm_ni_unit {
  struct arm_ni_cd {
  	void __iomem *pmu_base;
  	u16 id;
+	s8 irq_friend;

Why not just store the pointer to the cd as opposed to a relative offset?

Quoted 9 lines

drivers/perf/arm-ni.c +0 -1

  	int num_units;
  	int irq;
  	struct pmu pmu;

@@ -448,33 +449,37 @@ static irqreturn_t arm_ni_handle_irq(int irq, void *dev_id)
  {
  	struct arm_ni_cd *cd = dev_id;
  	irqreturn_t ret = IRQ_NONE;
-	u32 reg = readl_relaxed(cd->pmu_base + NI_PMOVSCLR);
 

drivers/perf/arm-ni.c static irqreturn_t arm_ni_handle_irq(int irq, void *dev_id) · line 453

Quoted 6 lines

drivers/perf/arm-ni.c +1 -5

-	if (reg & (1U << NI_CCNT_IDX)) {
-		ret = IRQ_HANDLED;
-		if (!(WARN_ON(!cd->ccnt))) {
-			arm_ni_event_read(cd->ccnt);
-			arm_ni_init_ccnt(cd);
+	for (;;) {

Could this be a do { ... } while (cd) loop instead?

Quoted 68 lines

drivers/perf/arm-ni.c +20 -20

+		u32 reg = readl_relaxed(cd->pmu_base + NI_PMOVSCLR);
+
+		if (reg & (1U << NI_CCNT_IDX)) {
+			ret = IRQ_HANDLED;
+			if (!(WARN_ON(!cd->ccnt))) {
+				arm_ni_event_read(cd->ccnt);
+				arm_ni_init_ccnt(cd);
+			}
  		}
-	}
-	for (int i = 0; i < NI_NUM_COUNTERS; i++) {
-		if (!(reg & (1U << i)))
-			continue;
-		ret = IRQ_HANDLED;
-		if (!(WARN_ON(!cd->evcnt[i]))) {
-			arm_ni_event_read(cd->evcnt[i]);
-			arm_ni_init_evcnt(cd, i);
+		for (int i = 0; i < NI_NUM_COUNTERS; i++) {
+			if (!(reg & (1U << i)))
+				continue;
+			ret = IRQ_HANDLED;
+			if (!(WARN_ON(!cd->evcnt[i]))) {
+				arm_ni_event_read(cd->evcnt[i]);
+				arm_ni_init_evcnt(cd, i);
+			}
  		}
+		writel_relaxed(reg, cd->pmu_base + NI_PMOVSCLR);
+		if (!cd->irq_friend)
+			return ret;
+		cd += cd->irq_friend;
  	}
-	writel_relaxed(reg, cd->pmu_base + NI_PMOVSCLR);
-	return ret;
  }
 
  static int arm_ni_init_cd(struct arm_ni *ni, struct arm_ni_node *node, u64 res_start)
  {
  	struct arm_ni_cd *cd = ni->cds + node->id;
  	const char *name;
-	int err;
 
  	cd->id = node->id;
  	cd->num_units = node->num_components;

@@ -534,20 +539,11 @@ static int arm_ni_init_cd(struct arm_ni *ni, struct arm_ni_node *node, u64 res_s
  		       cd->pmu_base + NI_PMCR);
  	writel_relaxed(U32_MAX, cd->pmu_base + NI_PMCNTENCLR);
  	writel_relaxed(U32_MAX, cd->pmu_base + NI_PMOVSCLR);
-	writel_relaxed(U32_MAX, cd->pmu_base + NI_PMINTENSET);
 
  	cd->irq = platform_get_irq(to_platform_device(ni->dev), cd->id);
  	if (cd->irq < 0)
  		return cd->irq;
 
-	err = devm_request_irq(ni->dev, cd->irq, arm_ni_handle_irq,
-			       IRQF_NOBALANCING | IRQF_NO_THREAD,
-			       dev_name(ni->dev), cd);
-	if (err)
-		return err;
-
-	irq_set_affinity(cd->irq, cpumask_of(ni->cpu));
-
  	cd->pmu = (struct pmu) {
  		.module = THIS_MODULE,
  		.parent = ni->dev,

@@ -593,6 +589,30 @@ static void arm_ni_probe_domain(void __iomem *base, struct arm_ni_node *node)
  	node->num_components = readl_relaxed(base + NI_CHILD_NODE_INFO);
  }
 

drivers/perf/arm-ni.c static void arm_ni_probe_domain(void __iomem *base, struct arm_ni_node *node) · line 599

Quoted 8 lines

drivers/perf/arm-ni.c +8 -0

+static int arm_ni_init_irqs(struct arm_ni *ni)
+{
+	int err;
+
+	ni_for_each_cd(ni, cd) {
+		for (struct arm_ni_cd *prev = cd; prev-- > ni->cds; ) {
+			if (prev->irq == cd->irq) {
+				prev->irq_friend = cd - prev;

Can't this race with the read of irq_friend in the interrupt handler?

Similarly, how do you handle the race between the interrupt firing on one CD and the friend CD being torn down on e.g. the remove path?

drivers/perf/arm-ni.c static void arm_ni_probe_domain(void __iomem *base, struct arm_ni_node *node) · line 605

Quoted 6 lines

drivers/perf/arm-ni.c +6 -0

+				goto enable_irq;
+			}
+		}
+		err = devm_request_irq(ni->dev, cd->irq, arm_ni_handle_irq,
+				       IRQF_NOBALANCING | IRQF_NO_THREAD,
+				       dev_name(ni->dev), cd);

What's the reason not to use IRQF_SHARED and register multiple handlers? I'm sure there is one, but capturing that in a comment would help to justify the manual multiplexing.

Quoted 20 lines · Will Deacon

Quoted 19 lines · Robin Murphy

From: Shouping Wang <[email protected]>

NI-700 has a distinct PMU interrupt output for each Clock Domain, however some integrations may still combine these together externally. The initial driver didn't attempt to support this, in anticipation of a more general solution for IRQ sharing between system PMU instances, but that's still a way off, so let's make this intermediate step for now to at least allow sharing IRQs within an individual NI instance.

Now that CPU affinity and migration are cleaned up, it's fairly straightforward to adopt similar logic to arm-cmn, to identify CDs with a common interrupt and loop over them directly in the handler.

Signed-off-by Shouping Wang <[email protected]>

[ rm: Rework for affinity handling, cosmetics, new commit message ]

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/perf/arm-ni.c | 78 ++++++++++++++++++++++++++++---------------
  1 file changed, 51 insertions(+), 27 deletions(-)

Quoted 5 lines

Quoted 5 lines

drivers/perf/arm-ni.c +2 -0

@@ -593,6 +589,30 @@ static void arm_ni_probe_domain(void __iomem *base, struct arm_ni_node *node)
  	node->num_components = readl_relaxed(base + NI_CHILD_NODE_INFO);
  }
+static int arm_ni_init_irqs(struct arm_ni *ni)
+{

Quoted 8 lines

drivers/perf/arm-ni.c static void arm_ni_probe_domain(void __iomem *base, struct arm_ni_node *node) · line 599

Quoted 6 lines

drivers/perf/arm-ni.c +6 -0

+	int err;
+
+	ni_for_each_cd(ni, cd) {
+		for (struct arm_ni_cd *prev = cd; prev-- > ni->cds; ) {
+			if (prev->irq == cd->irq) {
+				prev->irq_friend = cd - prev;

Can't this race with the read of irq_friend in the interrupt handler?

Not in any way that matters. Any IRQ at this point would be a spurious one left latched at the interrupt controller after we've already reset all the PMUs. The handler can hardly observe a torn partial store of 1 byte, so either it'll see a valid irq_friend or none. Either way we'll eventually return IRQ_NONE, EOI the phantom IRQ and be done.

I'm not entertaining the idea of somehow being preempted here for long enough for userspace to notice the already-registered PMUs (even though the module load hasn't yet finished...), open an event and have it count enough to genuinely overflow, because that would be in the order of at least tens of seconds if not minutes.

Hi all,

While the previous attempt[1] was missing some important aspects, it's a valid problem to solve, so I figured I'd take a look myself. This ended up turning into a slightly wider cleanup, but I think the end result is nice enough.

As usual I don't have anything to test this on, so if there are any silly mistakes left that the compiler couldn't catch please do let me know.

Thanks,
Robin.

[1] https://lore.kernel.org/all/[email protected]/

Robin Murphy (2):
perf/arm-ni: Set initial IRQ affinity
perf/arm-ni: Consolidate CPU affinity handling

Shouping Wang (1):
perf/arm-ni: Support sharing IRQs within an NI instance

 drivers/perf/arm-ni.c | 148 ++++++++++++++++++++++++------------------
 1 file changed, 84 insertions(+), 64 deletions(-)

While the previous attempt[1] was missing some important aspects, it's a valid problem to solve, so I figured I'd take a look myself. This ended up turning into a slightly wider cleanup, but I think the end result is nice enough.

As usual I don't have anything to test this on, so if there are any silly mistakes left that the compiler couldn't catch please do let me know.

[...]