Re: [PATCH] perf: xgene: Add CPU hotplug support

[Mark Rutland]

On Wed, Aug 15, 2018 at 11:31:35AM -0700, Hoan Tran wrote:
> This patch adds CPU hotplug support where the PMU migrates the context to
> another online CPU when its CPU is offline.
> 
> It fixes the below issue where the user does offline the CPU which is assigned
> to this PMU.
> 
> Assuming, CPU0 is assigned for this PMU. When the user does offline CPU0
>         [root@(none) ~]# echo 0 > /sys/devices/system/cpu/cpu0/online
> This PMU does not work anymore and shows the below error.
>         [root@(none) ~]# perf stat -a -e l3c0/cycle-count/,l3c0/write/ sleep 1
>         Error:
>         The sys_perf_event_open() syscall returned with 19 (No such device) for event (l3c0/cycle-count/).
>         /bin/dmesg may provide additional information.
>         No CONFIG_PERF_EVENTS=y kernel support configured?
> 
> With this patch, when CPU0 is offline, PMU migrates to another online CPU and
> works on that CPU.
> 
> Signed-off-by: Hoan Tran <hoan.tran@xxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/perf/xgene_pmu.c   | 71 ++++++++++++++++++++++++++++++++++++++++++----
>  include/linux/cpuhotplug.h |  1 +
>  2 files changed, 66 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/perf/xgene_pmu.c b/drivers/perf/xgene_pmu.c
> index 0e31f13..248a3f7 100644
> --- a/drivers/perf/xgene_pmu.c
> +++ b/drivers/perf/xgene_pmu.c
> @@ -21,6 +21,7 @@
> 
>  #include <linux/acpi.h>
>  #include <linux/clk.h>
> +#include <linux/cpuhotplug.h>
>  #include <linux/cpumask.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> @@ -130,12 +131,14 @@ struct xgene_pmu_ops {
> 
>  struct xgene_pmu {
>         struct device *dev;
> +       struct hlist_node node;
>         int version;
>         void __iomem *pcppmu_csr;
>         u32 mcb_active_mask;
>         u32 mc_active_mask;
>         u32 l3c_active_mask;
>         cpumask_t cpu;
> +       int irq;
>         raw_spinlock_t lock;
>         const struct xgene_pmu_ops *ops;
>         struct list_head l3cpmus;
> @@ -1806,6 +1809,53 @@ static const struct acpi_device_id xgene_pmu_acpi_match[] = {
>  MODULE_DEVICE_TABLE(acpi, xgene_pmu_acpi_match);
>  #endif
> 
> +static int xgene_pmu_online_cpu(unsigned int cpu, struct hlist_node *node)
> +{
> +       struct xgene_pmu *xgene_pmu = hlist_entry_safe(node, struct xgene_pmu,
> +                                                      node);
> +
> +       if (cpumask_empty(&xgene_pmu->cpu))
> +               cpumask_set_cpu(cpu, &xgene_pmu->cpu);
> +
> +       /* Overflow interrupt also should use the same CPU */
> +       WARN_ON(irq_set_affinity(xgene_pmu->irq, &xgene_pmu->cpu));
> +
> +       return 0;
> +}
> +
> +static int xgene_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
> +{
> +       struct xgene_pmu *xgene_pmu = hlist_entry_safe(node, struct xgene_pmu,
> +                                                      node);
> +       struct xgene_pmu_dev_ctx *ctx;
> +       unsigned int target;
> +
> +       if (!cpumask_test_and_clear_cpu(cpu, &xgene_pmu->cpu))
> +               return 0;
> +       target = cpumask_any_but(cpu_online_mask, cpu);
> +       if (target >= nr_cpu_ids)
> +               return 0;
> +
> +       list_for_each_entry(ctx, &xgene_pmu->mcpmus, next) {
> +               perf_pmu_migrate_context(&ctx->pmu_dev->pmu, cpu, target);
> +       }
> +       list_for_each_entry(ctx, &xgene_pmu->mcbpmus, next) {
> +               perf_pmu_migrate_context(&ctx->pmu_dev->pmu, cpu, target);
> +       }
> +       list_for_each_entry(ctx, &xgene_pmu->l3cpmus, next) {
> +               perf_pmu_migrate_context(&ctx->pmu_dev->pmu, cpu, target);
> +       }
> +       list_for_each_entry(ctx, &xgene_pmu->iobpmus, next) {
> +               perf_pmu_migrate_context(&ctx->pmu_dev->pmu, cpu, target);
> +       }
> +
> +       cpumask_set_cpu(target, &xgene_pmu->cpu);
> +       /* Overflow interrupt also should use the same CPU */
> +       WARN_ON(irq_set_affinity(xgene_pmu->irq, &xgene_pmu->cpu));
> +
> +       return 0;
> +}
> +
>  static int xgene_pmu_probe(struct platform_device *pdev)
>  {
>         const struct xgene_pmu_data *dev_data;
> @@ -1815,6 +1865,14 @@ static int xgene_pmu_probe(struct platform_device *pdev)
>         int irq, rc;
>         int version;
> 
> +       /* Install a hook to update the reader CPU in case it goes offline */
> +       rc = cpuhp_setup_state_multi(CPUHP_AP_PERF_XGENE_ONLINE,
> +                                     "CPUHP_AP_PERF_XGENE_ONLINE",
> +                                     xgene_pmu_online_cpu,
> +                                     xgene_pmu_offline_cpu);
> +       if (rc)
> +               return rc;
> +
>         xgene_pmu = devm_kzalloc(&pdev->dev, sizeof(*xgene_pmu), GFP_KERNEL);
>         if (!xgene_pmu)
>                 return -ENOMEM;
> @@ -1865,6 +1923,7 @@ static int xgene_pmu_probe(struct platform_device *pdev)
>                 dev_err(&pdev->dev, "No IRQ resource\n");
>                 return -EINVAL;
>         }
> +
>         rc = devm_request_irq(&pdev->dev, irq, xgene_pmu_isr,
>                                 IRQF_NOBALANCING | IRQF_NO_THREAD,
>                                 dev_name(&pdev->dev), xgene_pmu);
> @@ -1873,6 +1932,8 @@ static int xgene_pmu_probe(struct platform_device *pdev)
>                 return rc;
>         }
> 
> +       xgene_pmu->irq = irq;
> +
>         raw_spin_lock_init(&xgene_pmu->lock);
> 
>         /* Check for active MCBs and MCUs */
> @@ -1883,13 +1944,11 @@ static int xgene_pmu_probe(struct platform_device *pdev)
>                 xgene_pmu->mc_active_mask = 0x1;
>         }
> 
> -       /* Pick one core to use for cpumask attributes */
> -       cpumask_set_cpu(smp_processor_id(), &xgene_pmu->cpu);
> -
> -       /* Make sure that the overflow interrupt is handled by this CPU */
> -       rc = irq_set_affinity(irq, &xgene_pmu->cpu);
> +       /* Add this instance to the list used by the hotplug callback */
> +       rc = cpuhp_state_add_instance(CPUHP_AP_PERF_XGENE_ONLINE,
> +                                     &xgene_pmu->node);
>         if (rc) {
> -               dev_err(&pdev->dev, "Failed to set interrupt affinity!\n");
> +               dev_err(&pdev->dev, "Error %d registering hotplug", rc);
>                 return rc;
>         }

You also need to remove the cpuhp instances when unregistering the PMUs
in xgene_pmu_dev_cleanup().

Otherwise this looks fine to me, on the assumption the system PMU is
accessible from all CPUs in the system (e.g. it's not lcoal to a socket
or anything like that).

Thanks,
Mark.