diff mbox series

[2/6] xen: add helper for calling notifier_call_chain() to common/cpu.c

Message ID 20190318131155.29450-3-jgross@suse.com (mailing list archive)
State Superseded
Headers show
Series xen: simplify suspend/resume handling | expand

Commit Message

Juergen Gross March 18, 2019, 1:11 p.m. UTC
Add a helper cpu_notifier_call_chain() to call notifier_call_chain()
for a cpu with a specified action, returning an errno value.

This avoids coding the same pattern multiple times.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/cpu.c | 50 +++++++++++++++++++++-----------------------------
 1 file changed, 21 insertions(+), 29 deletions(-)

Comments

Dario Faggioli March 25, 2019, 11:56 a.m. UTC | #1
On Mon, 2019-03-18 at 14:11 +0100, Juergen Gross wrote:
> Add a helper cpu_notifier_call_chain() to call notifier_call_chain()
> for a cpu with a specified action, returning an errno value.
> 
> This avoids coding the same pattern multiple times.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

Regards,
Dario
George Dunlap March 27, 2019, 12:25 p.m. UTC | #2
On 3/18/19 1:11 PM, Juergen Gross wrote:
> Add a helper cpu_notifier_call_chain() to call notifier_call_chain()
> for a cpu with a specified action, returning an errno value.
> 
> This avoids coding the same pattern multiple times.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: George Dunlap <george.dunlap@citrix.com>
Andrew Cooper March 27, 2019, 3:39 p.m. UTC | #3
On 18/03/2019 13:11, Juergen Gross wrote:
> Add a helper cpu_notifier_call_chain() to call notifier_call_chain()
> for a cpu with a specified action, returning an errno value.
>
> This avoids coding the same pattern multiple times.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  xen/common/cpu.c | 50 +++++++++++++++++++++-----------------------------
>  1 file changed, 21 insertions(+), 29 deletions(-)
>
> diff --git a/xen/common/cpu.c b/xen/common/cpu.c
> index 836c62f97f..c436c0de7f 100644
> --- a/xen/common/cpu.c
> +++ b/xen/common/cpu.c
> @@ -71,11 +71,18 @@ void __init register_cpu_notifier(struct notifier_block *nb)
>      spin_unlock(&cpu_add_remove_lock);
>  }
>  
> +static int cpu_notifier_call_chain(unsigned int cpu, unsigned long action,
> +                                   struct notifier_block **nb)
> +{
> +    void *hcpu = (void *)(long)cpu;
> +    int notifier_rc = notifier_call_chain(&cpu_chain, action, hcpu, nb);
> +
> +    return (notifier_rc == NOTIFY_DONE) ? 0 : notifier_to_errno(notifier_rc);
> +}
> +
>  static void _take_cpu_down(void *unused)
>  {
> -    void *hcpu = (void *)(long)smp_processor_id();
> -    int notifier_rc = notifier_call_chain(&cpu_chain, CPU_DYING, hcpu, NULL);
> -    BUG_ON(notifier_rc != NOTIFY_DONE);
> +    BUG_ON(cpu_notifier_call_chain(smp_processor_id(), CPU_DYING, NULL));

Where possible, we're trying to remove side effects from macros.

Could I please talk you into writing this as:

int rc = cpu_notifier_call_chain(smp_processor_id(), CPU_DYING, NULL);

BUG_ON(rc);

An alternative might be to have a:

static void cpu_notifier_call_chain_nofail(...)

wrapper as this seems to be a common pattern.  (Ideally longterm, it
might be better to pass the nofail intention into the notifier chain
itself so we can identify which callback had a problem, but thats
definitely not something for here.)

~Andrew
Juergen Gross March 27, 2019, 4:05 p.m. UTC | #4
On 27/03/2019 16:39, Andrew Cooper wrote:
> On 18/03/2019 13:11, Juergen Gross wrote:
>> Add a helper cpu_notifier_call_chain() to call notifier_call_chain()
>> for a cpu with a specified action, returning an errno value.
>>
>> This avoids coding the same pattern multiple times.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  xen/common/cpu.c | 50 +++++++++++++++++++++-----------------------------
>>  1 file changed, 21 insertions(+), 29 deletions(-)
>>
>> diff --git a/xen/common/cpu.c b/xen/common/cpu.c
>> index 836c62f97f..c436c0de7f 100644
>> --- a/xen/common/cpu.c
>> +++ b/xen/common/cpu.c
>> @@ -71,11 +71,18 @@ void __init register_cpu_notifier(struct notifier_block *nb)
>>      spin_unlock(&cpu_add_remove_lock);
>>  }
>>  
>> +static int cpu_notifier_call_chain(unsigned int cpu, unsigned long action,
>> +                                   struct notifier_block **nb)
>> +{
>> +    void *hcpu = (void *)(long)cpu;
>> +    int notifier_rc = notifier_call_chain(&cpu_chain, action, hcpu, nb);
>> +
>> +    return (notifier_rc == NOTIFY_DONE) ? 0 : notifier_to_errno(notifier_rc);
>> +}
>> +
>>  static void _take_cpu_down(void *unused)
>>  {
>> -    void *hcpu = (void *)(long)smp_processor_id();
>> -    int notifier_rc = notifier_call_chain(&cpu_chain, CPU_DYING, hcpu, NULL);
>> -    BUG_ON(notifier_rc != NOTIFY_DONE);
>> +    BUG_ON(cpu_notifier_call_chain(smp_processor_id(), CPU_DYING, NULL));
> 
> Where possible, we're trying to remove side effects from macros.
> 
> Could I please talk you into writing this as:
> 
> int rc = cpu_notifier_call_chain(smp_processor_id(), CPU_DYING, NULL);
> 
> BUG_ON(rc);

Fine with me.


Juergen
diff mbox series

Patch

diff --git a/xen/common/cpu.c b/xen/common/cpu.c
index 836c62f97f..c436c0de7f 100644
--- a/xen/common/cpu.c
+++ b/xen/common/cpu.c
@@ -71,11 +71,18 @@  void __init register_cpu_notifier(struct notifier_block *nb)
     spin_unlock(&cpu_add_remove_lock);
 }
 
+static int cpu_notifier_call_chain(unsigned int cpu, unsigned long action,
+                                   struct notifier_block **nb)
+{
+    void *hcpu = (void *)(long)cpu;
+    int notifier_rc = notifier_call_chain(&cpu_chain, action, hcpu, nb);
+
+    return (notifier_rc == NOTIFY_DONE) ? 0 : notifier_to_errno(notifier_rc);
+}
+
 static void _take_cpu_down(void *unused)
 {
-    void *hcpu = (void *)(long)smp_processor_id();
-    int notifier_rc = notifier_call_chain(&cpu_chain, CPU_DYING, hcpu, NULL);
-    BUG_ON(notifier_rc != NOTIFY_DONE);
+    BUG_ON(cpu_notifier_call_chain(smp_processor_id(), CPU_DYING, NULL));
     __cpu_disable();
 }
 
@@ -87,8 +94,7 @@  static int take_cpu_down(void *arg)
 
 int cpu_down(unsigned int cpu)
 {
-    int err, notifier_rc;
-    void *hcpu = (void *)(long)cpu;
+    int err;
     struct notifier_block *nb = NULL;
 
     if ( !cpu_hotplug_begin() )
@@ -100,12 +106,9 @@  int cpu_down(unsigned int cpu)
         return -EINVAL;
     }
 
-    notifier_rc = notifier_call_chain(&cpu_chain, CPU_DOWN_PREPARE, hcpu, &nb);
-    if ( notifier_rc != NOTIFY_DONE )
-    {
-        err = notifier_to_errno(notifier_rc);
+    err = cpu_notifier_call_chain(cpu, CPU_DOWN_PREPARE, &nb);
+    if ( err )
         goto fail;
-    }
 
     if ( unlikely(system_state < SYS_STATE_active) )
         on_selected_cpus(cpumask_of(cpu), _take_cpu_down, NULL, true);
@@ -115,24 +118,21 @@  int cpu_down(unsigned int cpu)
     __cpu_die(cpu);
     BUG_ON(cpu_online(cpu));
 
-    notifier_rc = notifier_call_chain(&cpu_chain, CPU_DEAD, hcpu, NULL);
-    BUG_ON(notifier_rc != NOTIFY_DONE);
+    BUG_ON(cpu_notifier_call_chain(cpu, CPU_DEAD, NULL));
 
     send_global_virq(VIRQ_PCPU_STATE);
     cpu_hotplug_done();
     return 0;
 
  fail:
-    notifier_rc = notifier_call_chain(&cpu_chain, CPU_DOWN_FAILED, hcpu, &nb);
-    BUG_ON(notifier_rc != NOTIFY_DONE);
+    BUG_ON(cpu_notifier_call_chain(cpu, CPU_DOWN_FAILED, &nb));
     cpu_hotplug_done();
     return err;
 }
 
 int cpu_up(unsigned int cpu)
 {
-    int notifier_rc, err = 0;
-    void *hcpu = (void *)(long)cpu;
+    int err;
     struct notifier_block *nb = NULL;
 
     if ( !cpu_hotplug_begin() )
@@ -144,19 +144,15 @@  int cpu_up(unsigned int cpu)
         return -EINVAL;
     }
 
-    notifier_rc = notifier_call_chain(&cpu_chain, CPU_UP_PREPARE, hcpu, &nb);
-    if ( notifier_rc != NOTIFY_DONE )
-    {
-        err = notifier_to_errno(notifier_rc);
+    err = cpu_notifier_call_chain(cpu, CPU_UP_PREPARE, &nb);
+    if ( err )
         goto fail;
-    }
 
     err = __cpu_up(cpu);
     if ( err < 0 )
         goto fail;
 
-    notifier_rc = notifier_call_chain(&cpu_chain, CPU_ONLINE, hcpu, NULL);
-    BUG_ON(notifier_rc != NOTIFY_DONE);
+    BUG_ON(cpu_notifier_call_chain(cpu, CPU_ONLINE, NULL));
 
     send_global_virq(VIRQ_PCPU_STATE);
 
@@ -164,18 +160,14 @@  int cpu_up(unsigned int cpu)
     return 0;
 
  fail:
-    notifier_rc = notifier_call_chain(&cpu_chain, CPU_UP_CANCELED, hcpu, &nb);
-    BUG_ON(notifier_rc != NOTIFY_DONE);
+    BUG_ON(cpu_notifier_call_chain(cpu, CPU_UP_CANCELED, &nb));
     cpu_hotplug_done();
     return err;
 }
 
 void notify_cpu_starting(unsigned int cpu)
 {
-    void *hcpu = (void *)(long)cpu;
-    int notifier_rc = notifier_call_chain(
-        &cpu_chain, CPU_STARTING, hcpu, NULL);
-    BUG_ON(notifier_rc != NOTIFY_DONE);
+    BUG_ON(cpu_notifier_call_chain(cpu, CPU_STARTING, NULL));
 }
 
 static cpumask_t frozen_cpus;