Re: [Xen-devel] Question about apic ipi interface

[Ben Guthro <ben@xxxxxxxxxx>]

On Wed, May 8, 2013 at 4:26 PM, Stefan Bader <stefan.bader@xxxxxxxxxxxxx> wrote:
> On 23.04.2013 12:07, Stefan Bader wrote:
>> I was looking at some older patch and there is one thing I do not understand.
>>
>> commit f447d56d36af18c5104ff29dcb1327c0c0ac3634
>>     xen: implement apic ipi interface
>>
>> Specifically there the implementation of xen_send_IPI_mask_allbutself().
>>
>> void xen_send_IPI_mask_allbutself(const struct cpumask *mask,
>>                                 int vector)
>> {
>>         unsigned cpu;
>>         unsigned int this_cpu = smp_processor_id();
>>
>>         if (!(num_online_cpus() > 1))
>>                 return;
>>
>>         for_each_cpu_and(cpu, mask, cpu_online_mask) {
>>                 if (this_cpu == cpu)
>>                         continue;
>>
>>                 xen_smp_send_call_function_single_ipi(cpu);
>>         }
>> }
>>
>> Why is this using xen_smp_send_call_function_single_ipi()? This dumps the
>> supplied vector and always uses XEN_CALL_FUNCTION_SINGLE_VECTOR. In contrast 
>> the
>> xen_send_IPI_all() and xen_send_IPI_self() keep the (mapped) vector.
>>
>> Mildly wondering about whether call function would need special casing (just
>> because xen_smp_send_call_function_ipi() is special). But I don't have the 
>> big
>> picture there.
>>
>
> This never got really answered, so lets try this: Does the following patch 
> seem
> to make sense? I know, it has not caused any obvious regressions but at least
> this would look more in agreement with the other code. It has not blown up on 
> a
> normal boot either.
> Ben, is there a simple way that I would trigger the problem you had?

Stefan,

As mentioned before, Ming Lin took my patches from a different
context, and re-worked them for a different patch series

I originally wrote the patch in question, trying to get kgdb to work
with Xen, which I never completed:
http://wiki.xen.org/wiki/Xen_Development_Projects#dom0_kgdb_support

Lin Ming originally introduced this commit to fix "perf top" soft lockups:
http://lists.xen.org/archives/html/xen-devel/2012-04/msg01024.html

This commit was also a modification of the code that I originally
wrote, so I don't really have the final say on it, since it was
modified after I wrote it.
Here is how I transferred the patch, and the context at the time:
http://www.kernelhub.org/?p=2&msg=17979


IIRC, kgdb uses IPI to do the CPU roundup, to get into a single
processor context. Without the xen IPI implementation, it would never
round up the CPUs.
That is my best recollection. I apologize that it is not better.

I don't see anything wrong with your fix, as long as it does not
reintroduce any regressions in the "perf top" problems that Lin Ming
addressed.

Ben

>
> -Stefan
>
>
> From e13703426f367c618f2984d376289b197a8c0402 Mon Sep 17 00:00:00 2001
> From: Stefan Bader <stefan.bader@xxxxxxxxxxxxx>
> Date: Wed, 8 May 2013 16:37:35 +0200
> Subject: [PATCH] xen: Clean up apic ipi interface
>
> Commit f447d56d36af18c5104ff29dcb1327c0c0ac3634 introduced the
> implementation of the PV apic ipi interface. But there were some
> odd things (it seems none of which cause really any issue but
> maybe they should be cleaned up anyway):
>  - xen_send_IPI_mask_allbutself (and by that xen_send_IPI_allbutself)
>    ignore the passed in vector and only use the CALL_FUNCTION_SINGLE
>    vector. While xen_send_IPI_all and xen_send_IPI_mask use the vector.
>  - physflat_send_IPI_allbutself is declared unnecessarily. It is never
>    used.
>
> This patch tries to clean up those things.
>
> Signed-off-by: Stefan Bader <stefan.bader@xxxxxxxxxxxxx>
> ---
>  arch/x86/xen/smp.c |   10 ++++------
>  arch/x86/xen/smp.h |    1 -
>  2 files changed, 4 insertions(+), 7 deletions(-)
> diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
> index 8ff3799..fb44426 100644
> --- a/arch/x86/xen/smp.c
> +++ b/arch/x86/xen/smp.c
> @@ -576,24 +576,22 @@ void xen_send_IPI_mask_allbutself(const struct cpumask 
> *mask,
>  {
>         unsigned cpu;
>         unsigned int this_cpu = smp_processor_id();
> +       int xen_vector = xen_map_vector(vector);
>
> -       if (!(num_online_cpus() > 1))
> +       if (!(num_online_cpus() > 1) || (xen_vector < 0))
>                 return;
>
>         for_each_cpu_and(cpu, mask, cpu_online_mask) {
>                 if (this_cpu == cpu)
>                         continue;
>
> -               xen_smp_send_call_function_single_ipi(cpu);
> +               xen_send_IPI_one(cpu, xen_vector);
>         }
>  }
>
>  void xen_send_IPI_allbutself(int vector)
>  {
> -       int xen_vector = xen_map_vector(vector);
> -
> -       if (xen_vector >= 0)
> -               xen_send_IPI_mask_allbutself(cpu_online_mask, xen_vector);
> +       xen_send_IPI_mask_allbutself(cpu_online_mask, vector);
>  }
>
>  static irqreturn_t xen_call_function_interrupt(int irq, void *dev_id)
> diff --git a/arch/x86/xen/smp.h b/arch/x86/xen/smp.h
> index 8981a76..c7c2d89 100644
> --- a/arch/x86/xen/smp.h
> +++ b/arch/x86/xen/smp.h
> @@ -5,7 +5,6 @@ extern void xen_send_IPI_mask(const struct cpumask *mask,
>  extern void xen_send_IPI_mask_allbutself(const struct cpumask *mask,
>                                 int vector);
>  extern void xen_send_IPI_allbutself(int vector);
> -extern void physflat_send_IPI_allbutself(int vector);
>  extern void xen_send_IPI_all(int vector);
>  extern void xen_send_IPI_self(int vector);
>
> --
> 1.7.9.5
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel