Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1440571 > unrolled thread

[patch 22/66] bus: arm-ccn: convert to hotplug statemachine

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [patch 22/66] bus: arm-ccn: convert to hotplug statemachine Anna-Maria Gleixner <anna-maria@linutronix.de> - 2016-07-11 14:50 +0200
    Re: [patch 22/66] bus: arm-ccn: convert to hotplug statemachine Pawel Moll <pawel.moll@arm.com> - 2016-07-12 12:10 +0200
    Re: [patch 22/66] bus: arm-ccn: convert to hotplug statemachine Pawel Moll <pawel.moll@arm.com> - 2016-07-12 13:20 +0200
      Re: [patch 22/66] bus: arm-ccn: convert to hotplug statemachine Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-07-12 13:30 +0200

#1440571 — [patch 22/66] bus: arm-ccn: convert to hotplug statemachine

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Install the callbacks via the state machine and let the core invoke
the callbacks on the already online CPUs.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Pawel Moll <pawel.moll@arm.com>
Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
---
 drivers/bus/arm-ccn.c      |   47 ++++++++++++++++++++-------------------------
 include/linux/cpuhotplug.h |    2 +
 2 files changed, 23 insertions(+), 26 deletions(-)

--- a/drivers/bus/arm-ccn.c
+++ b/drivers/bus/arm-ccn.c
@@ -167,7 +167,6 @@ struct arm_ccn_dt {
 	struct hrtimer hrtimer;
 
 	cpumask_t cpu;
-	struct notifier_block cpu_nb;
 
 	struct pmu pmu;
 };
@@ -1171,30 +1170,23 @@ static enum hrtimer_restart arm_ccn_pmu_
 }
 
 
-static int arm_ccn_pmu_cpu_notifier(struct notifier_block *nb,
-		unsigned long action, void *hcpu)
+static struct arm_ccn_dt *cpuhp_armccn_dt;
+static int arm_ccn_pmu_offline_cpu(unsigned int cpu)
 {
-	struct arm_ccn_dt *dt = container_of(nb, struct arm_ccn_dt, cpu_nb);
+	struct arm_ccn_dt *dt = cpuhp_armccn_dt;
 	struct arm_ccn *ccn = container_of(dt, struct arm_ccn, dt);
-	unsigned int cpu = (long)hcpu; /* for (long) see kernel/cpu.c */
 	unsigned int target;
 
-	switch (action & ~CPU_TASKS_FROZEN) {
-	case CPU_DOWN_PREPARE:
-		if (!cpumask_test_and_clear_cpu(cpu, &dt->cpu))
-			break;
-		target = cpumask_any_but(cpu_online_mask, cpu);
-		if (target >= nr_cpu_ids)
-			break;
-		perf_pmu_migrate_context(&dt->pmu, cpu, target);
-		cpumask_set_cpu(target, &dt->cpu);
-		if (ccn->irq)
-			WARN_ON(irq_set_affinity_hint(ccn->irq, &dt->cpu) != 0);
-	default:
-		break;
-	}
-
-	return NOTIFY_OK;
+	if (!cpumask_test_and_clear_cpu(cpu, &dt->cpu))
+		return 0;
+	target = cpumask_any_but(cpu_online_mask, cpu);
+	if (target >= nr_cpu_ids)
+		return 0;
+	perf_pmu_migrate_context(&dt->pmu, cpu, target);
+	cpumask_set_cpu(target, &dt->cpu);
+	if (ccn->irq)
+		WARN_ON(irq_set_affinity_hint(ccn->irq, &dt->cpu) != 0);
+	return 0;
 }
 
 
@@ -1270,9 +1262,10 @@ static int arm_ccn_pmu_init(struct arm_c
 	 * ... and change the selection when it goes offline. Priority is
 	 * picked to have a chance to migrate events before perf is notified.
 	 */
-	ccn->dt.cpu_nb.notifier_call = arm_ccn_pmu_cpu_notifier;
-	ccn->dt.cpu_nb.priority = CPU_PRI_PERF + 1,
-	err = register_cpu_notifier(&ccn->dt.cpu_nb);
+	cpuhp_armccn_dt = &ccn->dt;
+	err = cpuhp_setup_state(CPUHP_AP_PERF_ARM_CCN_ONLINE,
+				"AP_PERF_ARM_CCN_ONLINE", NULL,
+				arm_ccn_pmu_offline_cpu);
 	if (err)
 		goto error_cpu_notifier;
 
@@ -1293,7 +1286,8 @@ static int arm_ccn_pmu_init(struct arm_c
 
 error_pmu_register:
 error_set_affinity:
-	unregister_cpu_notifier(&ccn->dt.cpu_nb);
+	cpuhp_remove_state_nocalls(CPUHP_AP_PERF_ARM_CCN_ONLINE);
+	cpuhp_armccn_dt = NULL;
 error_cpu_notifier:
 	ida_simple_remove(&arm_ccn_pmu_ida, ccn->dt.id);
 	for (i = 0; i < ccn->num_xps; i++)
@@ -1308,7 +1302,8 @@ static void arm_ccn_pmu_cleanup(struct a
 
 	if (ccn->irq)
 		irq_set_affinity_hint(ccn->irq, NULL);
-	unregister_cpu_notifier(&ccn->dt.cpu_nb);
+	cpuhp_remove_state_nocalls(CPUHP_AP_PERF_ARM_CCN_ONLINE);
+	cpuhp_armccn_dt = NULL;
 	for (i = 0; i < ccn->num_xps; i++)
 		writel(0, ccn->xp[i].base + CCN_XP_DT_CONTROL);
 	writel(0, ccn->dt.base + CCN_DT_PMCR);
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -30,6 +30,7 @@ enum cpuhp_state {
 	CPUHP_AP_PERF_X86_AMD_IBS_STARTING,
 	CPUHP_AP_PERF_X86_CQM_STARTING,
 	CPUHP_AP_PERF_X86_CSTATE_STARTING,
+	CPUHP_AP_PERF_XTENSA_STARTING,
 	CPUHP_AP_NOTIFY_STARTING,
 	CPUHP_AP_ONLINE,
 	CPUHP_TEARDOWN_CPU,
@@ -46,6 +47,7 @@ enum cpuhp_state {
 	CPUHP_AP_PERF_S390_CF_ONLINE,
 	CPUHP_AP_PERF_S390_SF_ONLINE,
 	CPUHP_AP_PERF_ARM_CCI_ONLINE,
+	CPUHP_AP_PERF_ARM_CCN_ONLINE,
 	CPUHP_AP_NOTIFY_ONLINE,
 	CPUHP_AP_ONLINE_DYN,
 	CPUHP_AP_ONLINE_DYN_END		= CPUHP_AP_ONLINE_DYN + 30,

[toc] | [next] | [standalone]

#1441210

On Mon, 2016-07-11 at 12:28 +0000, Anna-Maria Gleixner wrote:
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> Install the callbacks via the state machine and let the core invoke
> the callbacks on the already online CPUs.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>

Although I won't shed a tear over the notifiers going, there's a
problem with this patch...

> ---
>  drivers/bus/arm-ccn.c      |   47 ++++++++++++++++++++--------------
> -----------
>  include/linux/cpuhotplug.h |    2 +
>  2 files changed, 23 insertions(+), 26 deletions(-)
> 
> --- a/drivers/bus/arm-ccn.c
> +++ b/drivers/bus/arm-ccn.c
> @@ -167,7 +167,6 @@ struct arm_ccn_dt {
>  	struct hrtimer hrtimer;
>  
>  	cpumask_t cpu;
> -	struct notifier_block cpu_nb;
>  
>  	struct pmu pmu;
>  };

Notice that here each instance of CCN (unlikely as it is today, the
code was written with the assumption that there's more than one
interconnect ring in the system) get its own notifier block...

> @@ -1171,30 +1170,23 @@ static enum hrtimer_restart arm_ccn_pmu_
>  }
>  
>  
> -static int arm_ccn_pmu_cpu_notifier(struct notifier_block *nb,
> -		unsigned long action, void *hcpu)
> +static struct arm_ccn_dt *cpuhp_armccn_dt;
> +static int arm_ccn_pmu_offline_cpu(unsigned int cpu)
>  {
> -	struct arm_ccn_dt *dt = container_of(nb, struct arm_ccn_dt,
> cpu_nb);
> +	struct arm_ccn_dt *dt = cpuhp_armccn_dt;
>  	struct arm_ccn *ccn = container_of(dt, struct arm_ccn, dt);
> -	unsigned int cpu = (long)hcpu; /* for (long) see


... but here (and in all the rest of this change) it's replaced by a
static pointer to a single instance.

> @@ -1270,9 +1262,10 @@ static int arm_ccn_pmu_init(struct arm_c
>  	 * ... and change the selection when it goes offline.
> Priority is
>  	 * picked to have a chance to migrate events before perf is
> notified.
>  	 */
> -	ccn->dt.cpu_nb.notifier_call = arm_ccn_pmu_cpu_notifier;
> -	ccn->dt.cpu_nb.priority = CPU_PRI_PERF + 1,
> -	err = register_cpu_notifier(&ccn->dt.cpu_nb);
> +	cpuhp_armccn_dt = &ccn->dt;

Even without checking if the pointer has been already set.

> +	err = cpuhp_setup_state(CPUHP_AP_PERF_ARM_CCN_ONLINE,
> +				"AP_PERF_ARM_CCN_ONLINE", NULL,
> +				arm_ccn_pmu_offline_cpu);
>  	if (err)
>  		goto error_cpu_notifier;
>  
> @@ -1293,7 +1286,8 @@ static int arm_ccn_pmu_init(struct arm_c
>  
>  error_pmu_register:
>  error_set_affinity:
> -	unregister_cpu_notifier(&ccn->dt.cpu_nb);
> +	cpuhp_remove_state_nocalls(CPUHP_AP_PERF_ARM_CCN_ONLINE);
> +	cpuhp_armccn_dt = NULL;
  error_cpu_notifier:
>  	ida_simple_remove(&arm_ccn_pmu_ida, ccn->dt.id);
>  	for (i = 0; i < ccn->num_xps; i++)
> @@ -1308,7 +1302,8 @@ static void arm_ccn_pmu_cleanup(struct a
>  
>  	if (ccn->irq)
>  		irq_set_affinity_hint(ccn->irq, NULL);
> -	unregister_cpu_notifier(&ccn->dt.cpu_nb);
> +	cpuhp_remove_state_nocalls(CPUHP_AP_PERF_ARM_CCN_ONLINE);
> +	cpuhp_armccn_dt = NULL;

Same (only the other way round) here...

> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -30,6 +30,7 @@ enum cpuhp_state {
>  	CPUHP_AP_PERF_X86_AMD_IBS_STARTING,
>  	CPUHP_AP_PERF_X86_CQM_STARTING,
>  	CPUHP_AP_PERF_X86_CSTATE_STARTING,
> +	CPUHP_AP_PERF_XTENSA_STARTING,
>  	CPUHP_AP_NOTIFY_STARTING,
>  	CPUHP_AP_ONLINE,
>  	CPUHP_TEARDOWN_CPU,

That chunk does not belong here, does it?

Regards

Pawel

[toc] | [prev] | [next] | [standalone]

#1441259

On Mon, 2016-07-11 at 12:28 +0000, Anna-Maria Gleixner wrote:
> @@ -1270,9 +1262,10 @@ static int arm_ccn_pmu_init(struct arm_c
>  	 * ... and change the selection when it goes offline.
> Priority is
>  	 * picked to have a chance to migrate events before perf is
> notified.
>  	 */
> -	ccn->dt.cpu_nb.notifier_call = arm_ccn_pmu_cpu_notifier;
> -	ccn->dt.cpu_nb.priority = CPU_PRI_PERF + 1,
> -	err = register_cpu_notifier(&ccn->dt.cpu_nb);
> +	cpuhp_armccn_dt = &ccn->dt;
> +	err = cpuhp_setup_state(CPUHP_AP_PERF_ARM_CCN_ONLINE,
> +				"AP_PERF_ARM_CCN_ONLINE", NULL,
> +				arm_ccn_pmu_offline_cpu);
>  	if (err)
>  		goto error_cpu_notifier;

Also, unless I'm missing something obvious, it seems that the callback
will be executed for CPUs going online? I'm definitely interested in my
"current" CPU going down, in order to migrate my handlers somewhere
else.

Help?

Pawel

[toc] | [prev] | [next] | [standalone]

#1441263

On 07/12/2016 01:16 PM, Pawel Moll wrote:
> On Mon, 2016-07-11 at 12:28 +0000, Anna-Maria Gleixner wrote:
>> @@ -1270,9 +1262,10 @@ static int arm_ccn_pmu_init(struct arm_c
>>  	 * ... and change the selection when it goes offline.
>> Priority is
>>  	 * picked to have a chance to migrate events before perf is
>> notified.
>>  	 */
>> -	ccn->dt.cpu_nb.notifier_call = arm_ccn_pmu_cpu_notifier;
>> -	ccn->dt.cpu_nb.priority = CPU_PRI_PERF + 1,
>> -	err = register_cpu_notifier(&ccn->dt.cpu_nb);
>> +	cpuhp_armccn_dt = &ccn->dt;
>> +	err = cpuhp_setup_state(CPUHP_AP_PERF_ARM_CCN_ONLINE,
>> +				"AP_PERF_ARM_CCN_ONLINE", NULL,
>> +				arm_ccn_pmu_offline_cpu);
>>  	if (err)
>>  		goto error_cpu_notifier;
> 
> Also, unless I'm missing something obvious, it seems that the callback
> will be executed for CPUs going online? I'm definitely interested in my
> "current" CPU going down, in order to migrate my handlers somewhere
> else.
> 
> Help?

cpuhp_setup_state() gets two callbacks, online followed by offline
(argument three and four). The online callback is NULL so you have only
one offline callback.

> Pawel

Sebastian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web