Re: [PATCH V2 2/2] xen/arm: implement smp_call_function

[Keir Fraser <keir.xen@gmail.com>]

On 16/04/2013 14:38, "Julien Grall" <julien.grall@citrix.com> wrote:

> Move smp_call_function and on_selected_cpus to common code.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

Please put a Xen-style heading at the top of new file common/smp.c -- see
common/page_alloc.c (among many others) for an example.

Apart from that very minor consideration, the code is totally fine and I
will Ack this patch in advance of your next iteration of it:

Acked-by: Keir Fraser <keir@xen.org>

 -- Keir

> ---
>  xen/arch/arm/gic.c            |    3 ++
>  xen/arch/arm/smp.c            |   11 ++---
>  xen/arch/x86/smp.c            |   88 +++-----------------------------------
>  xen/common/Makefile           |    1 +
>  xen/common/smp.c              |   94
> +++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/gic.h     |    1 +
>  xen/include/asm-x86/hardirq.h |    1 +
>  xen/include/xen/smp.h         |    7 +++
>  8 files changed, 116 insertions(+), 90 deletions(-)
>  create mode 100644 xen/common/smp.c
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 0ef994e..82aa6fe 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -658,6 +658,9 @@ static void do_sgi(struct cpu_user_regs *regs, int
> othercpu, enum gic_sgi sgi)
>      case GIC_SGI_DUMP_STATE:
>          dump_execstate(regs);
>          break;
> +    case GIC_SGI_CALL_FUNCTION:
> +        smp_call_function_interrupt();
> +        break;
>      default:
>          panic("Unhandled SGI %d on CPU%d\n", sgi, smp_processor_id());
>          break;
> diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c
> index a902d84..4042db5 100644
> --- a/xen/arch/arm/smp.c
> +++ b/xen/arch/arm/smp.c
> @@ -11,17 +11,14 @@ void flush_tlb_mask(const cpumask_t *mask)
>      flush_xen_data_tlb();
>  }
>  
> -void smp_call_function(
> -    void (*func) (void *info),
> -    void *info,
> -    int wait)
> +void smp_send_event_check_mask(const cpumask_t *mask)
>  {
> -    panic("%s not implmented\n", __func__);
> +    send_SGI_mask(mask, GIC_SGI_EVENT_CHECK);
>  }
>  
> -void smp_send_event_check_mask(const cpumask_t *mask)
> +void smp_send_call_function_mask(const cpumask_t *mask)
>  {
> -    send_SGI_mask(mask, GIC_SGI_EVENT_CHECK);
> +    send_SGI_mask(mask, GIC_SGI_CALL_FUNCTION);
>  }
>  
>  /*
> diff --git a/xen/arch/x86/smp.c b/xen/arch/x86/smp.c
> index a607531..0433f30 100644
> --- a/xen/arch/x86/smp.c
> +++ b/xen/arch/x86/smp.c
> @@ -269,67 +269,16 @@ void smp_send_event_check_mask(const cpumask_t *mask)
>      send_IPI_mask(mask, EVENT_CHECK_VECTOR);
>  }
>  
> -/*
> - * Structure and data for smp_call_function()/on_selected_cpus().
> - */
> -
> -static void __smp_call_function_interrupt(void);
> -static DEFINE_SPINLOCK(call_lock);
> -static struct call_data_struct {
> -    void (*func) (void *info);
> -    void *info;
> -    int wait;
> -    cpumask_t selected;
> -} call_data;
> -
> -void smp_call_function(
> -    void (*func) (void *info),
> -    void *info,
> -    int wait)
> +void smp_send_call_function_mask(const cpumask_t *mask)
>  {
> -    cpumask_t allbutself;
> -
> -    cpumask_andnot(&allbutself, &cpu_online_map,
> -                   cpumask_of(smp_processor_id()));
> -    on_selected_cpus(&allbutself, func, info, wait);
> -}
> -
> -void on_selected_cpus(
> -    const cpumask_t *selected,
> -    void (*func) (void *info),
> -    void *info,
> -    int wait)
> -{
> -    unsigned int nr_cpus;
> -
> -    ASSERT(local_irq_is_enabled());
> -
> -    spin_lock(&call_lock);
> -
> -    cpumask_copy(&call_data.selected, selected);
> -
> -    nr_cpus = cpumask_weight(&call_data.selected);
> -    if ( nr_cpus == 0 )
> -        goto out;
> -
> -    call_data.func = func;
> -    call_data.info = info;
> -    call_data.wait = wait;
> -
> -    send_IPI_mask(&call_data.selected, CALL_FUNCTION_VECTOR);
> +    send_IPI_mask(mask, CALL_FUNCTION_VECTOR);
>  
> -    if ( cpumask_test_cpu(smp_processor_id(), &call_data.selected) )
> +    if ( cpumask_test_cpu(smp_processor_id(), mask) )
>      {
>          local_irq_disable();
> -        __smp_call_function_interrupt();
> +        smp_call_function_interrupt();
>          local_irq_enable();
>      }
> -
> -    while ( !cpumask_empty(&call_data.selected) )
> -        cpu_relax();
> -
> - out:
> -    spin_unlock(&call_lock);
>  }
>  
>  void __stop_this_cpu(void)
> @@ -390,36 +339,9 @@ void event_check_interrupt(struct cpu_user_regs *regs)
>      this_cpu(irq_count)++;
>  }
>  
> -static void __smp_call_function_interrupt(void)
> -{
> -    void (*func)(void *info) = call_data.func;
> -    void *info = call_data.info;
> -    unsigned int cpu = smp_processor_id();
> -
> -    if ( !cpumask_test_cpu(cpu, &call_data.selected) )
> -        return;
> -
> -    irq_enter();
> -
> -    if ( call_data.wait )
> -    {
> -        (*func)(info);
> -        mb();
> -        cpumask_clear_cpu(cpu, &call_data.selected);
> -    }
> -    else
> -    {
> -        mb();
> -        cpumask_clear_cpu(cpu, &call_data.selected);
> -        (*func)(info);
> -    }
> -
> -    irq_exit();
> -}
> -
>  void call_function_interrupt(struct cpu_user_regs *regs)
>  {
>      ack_APIC_irq();
>      perfc_incr(ipis);
> -    __smp_call_function_interrupt();
> +    smp_call_function_interrupt();
>  }
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 8a0c506..0dc2050 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -26,6 +26,7 @@ obj-y += schedule.o
>  obj-y += shutdown.o
>  obj-y += softirq.o
>  obj-y += sort.o
> +obj-y += smp.o
>  obj-y += spinlock.o
>  obj-y += stop_machine.o
>  obj-y += string.o
> diff --git a/xen/common/smp.c b/xen/common/smp.c
> new file mode 100644
> index 0000000..dcd93ad
> --- /dev/null
> +++ b/xen/common/smp.c
> @@ -0,0 +1,94 @@
> +#include <asm/hardirq.h>
> +#include <asm/processor.h>
> +#include <xen/spinlock.h>
> +#include <xen/smp.h>
> +
> +/*
> + * Structure and data for smp_call_function()/on_selected_cpus().
> + */
> +static DEFINE_SPINLOCK(call_lock);
> +static struct call_data_struct {
> +    void (*func) (void *info);
> +    void *info;
> +    int wait;
> +    cpumask_t selected;
> +} call_data;
> +
> +void smp_call_function(
> +    void (*func) (void *info),
> +    void *info,
> +    int wait)
> +{
> +    cpumask_t allbutself;
> +
> +    cpumask_andnot(&allbutself, &cpu_online_map,
> +                   cpumask_of(smp_processor_id()));
> +    on_selected_cpus(&allbutself, func, info, wait);
> +}
> +
> +void on_selected_cpus(
> +    const cpumask_t *selected,
> +    void (*func) (void *info),
> +    void *info,
> +    int wait)
> +{
> +    unsigned int nr_cpus;
> +
> +    ASSERT(local_irq_is_enabled());
> +
> +    spin_lock(&call_lock);
> +
> +    cpumask_copy(&call_data.selected, selected);
> +
> +    nr_cpus = cpumask_weight(&call_data.selected);
> +    if ( nr_cpus == 0 )
> +        goto out;
> +
> +    call_data.func = func;
> +    call_data.info = info;
> +    call_data.wait = wait;
> +
> +    smp_send_call_function_mask(&call_data.selected);
> +
> +    while ( !cpumask_empty(&call_data.selected) )
> +        cpu_relax();
> +
> +out:
> +    spin_unlock(&call_lock);
> +}
> +
> +void smp_call_function_interrupt(void)
> +{
> +    void (*func)(void *info) = call_data.func;
> +    void *info = call_data.info;
> +    unsigned int cpu = smp_processor_id();
> +
> +    if ( !cpumask_test_cpu(cpu, &call_data.selected) )
> +        return;
> +
> +    irq_enter();
> +
> +    if ( call_data.wait )
> +    {
> +        (*func)(info);
> +        mb();
> +        cpumask_clear_cpu(cpu, &call_data.selected);
> +    }
> +    else
> +    {
> +        mb();
> +        cpumask_clear_cpu(cpu, &call_data.selected);
> +        (*func)(info);
> +    }
> +
> +    irq_exit();
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 4f2c8b8..47354dd 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -173,6 +173,7 @@ extern void gic_restore_state(struct vcpu *v);
>  enum gic_sgi {
>      GIC_SGI_EVENT_CHECK = 0,
>      GIC_SGI_DUMP_STATE  = 1,
> +    GIC_SGI_CALL_FUNCTION = 2,
>  };
>  extern void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi);
>  extern void send_SGI_one(unsigned int cpu, enum gic_sgi sgi);
> diff --git a/xen/include/asm-x86/hardirq.h b/xen/include/asm-x86/hardirq.h
> index e573007..01a41b9 100644
> --- a/xen/include/asm-x86/hardirq.h
> +++ b/xen/include/asm-x86/hardirq.h
> @@ -3,6 +3,7 @@
>  
>  #include <xen/config.h>
>  #include <xen/cache.h>
> +#include <xen/types.h>
>  
>  typedef struct {
> unsigned int __softirq_pending;
> diff --git a/xen/include/xen/smp.h b/xen/include/xen/smp.h
> index e05f390..6febb56 100644
> --- a/xen/include/xen/smp.h
> +++ b/xen/include/xen/smp.h
> @@ -58,6 +58,13 @@ static inline void on_each_cpu(
>      on_selected_cpus(&cpu_online_map, func, info, wait);
>  }
>  
> +/*
> + * Call a function on the current CPU
> + */
> +void smp_call_function_interrupt(void);
> +
> +void smp_send_call_function_mask(const cpumask_t *mask);
> +
>  #define smp_processor_id() raw_smp_processor_id()
>  
>  int alloc_cpu_id(void);