Question about apic ipi interface

Question about apic ipi interface

[Stefan Bader]

[-- Attachment #1.1: Type: text/plain, Size: 1134 bytes --]

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.

Thanks,
Stefan


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Question about apic ipi interface

[Ben Guthro]

On Tue, Apr 23, 2013 at 11:07 AM, Stefan Bader
<stefan.bader@canonical.com> 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.
>

Adding Lin Ming here, since this was an evolution of an incomplete
implementation of mine that was
ultimately used in a larger context, outside of my original use case
for it (kgdb of dom0) that ultimately
gave me credit for this part of the patch, as part of a larger series.

I must admit that I don't recall the reasoning, if there was one.
It may be an oversight.

This was the original (incomplete) patch, in context:
http://markmail.org/message/d6ca5zfdmiqipurt


Are you seeing issues with the code, or just doing code inspection?

Ben

Re: Question about apic ipi interface

[Stefan Bader]

[-- Attachment #1.1: Type: text/plain, Size: 2230 bytes --]

On 23.04.2013 14:15, Ben Guthro wrote:
> On Tue, Apr 23, 2013 at 11:07 AM, Stefan Bader
> <stefan.bader@canonical.com> 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.
>>
> 
> Adding Lin Ming here, since this was an evolution of an incomplete
> implementation of mine that was
> ultimately used in a larger context, outside of my original use case
> for it (kgdb of dom0) that ultimately
> gave me credit for this part of the patch, as part of a larger series.
> 
> I must admit that I don't recall the reasoning, if there was one.
> It may be an oversight.
> 
> This was the original (incomplete) patch, in context:
> http://markmail.org/message/d6ca5zfdmiqipurt
> 
> 
> Are you seeing issues with the code, or just doing code inspection?

No issues, I was just looking at the patch because we were asked to backport it
to fix another issue (access to the apic IPI functions without checking whether
there is a pointer). Since things did work in most cases before, maybe there is
no real usage. :) I was just curious.

Stefan

> 
> Ben
> 



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Question about apic ipi interface

[Stefan Bader]

[-- Attachment #1.1: Type: text/plain, Size: 2779 bytes --]

On 23.04.2013 14:23, Stefan Bader wrote:
> On 23.04.2013 14:15, Ben Guthro wrote:
>> On Tue, Apr 23, 2013 at 11:07 AM, Stefan Bader
>> <stefan.bader@canonical.com> 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.
>>>
>>
>> Adding Lin Ming here, since this was an evolution of an incomplete
>> implementation of mine that was
>> ultimately used in a larger context, outside of my original use case
>> for it (kgdb of dom0) that ultimately
>> gave me credit for this part of the patch, as part of a larger series.
>>
>> I must admit that I don't recall the reasoning, if there was one.
>> It may be an oversight.
>>
>> This was the original (incomplete) patch, in context:
>> http://markmail.org/message/d6ca5zfdmiqipurt
>>
>>
>> Are you seeing issues with the code, or just doing code inspection?
> 
> No issues, I was just looking at the patch because we were asked to backport it
> to fix another issue (access to the apic IPI functions without checking whether
> there is a pointer). Since things did work in most cases before, maybe there is
> no real usage. :) I was just curious.
> 
> Stefan

Oh, and while looking at it... why does arch/x86/xen/smp.h includes a definition
for physflat_send_IPI_allbutself? (introduced by the same change. If its not
hidden by some hideous macro magic there is only one place that needs it and
that is in the same file (apic_flat_64.c).

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



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Question about apic ipi interface

[Stefan Bader]

[-- Attachment #1.1: Type: text/plain, Size: 4175 bytes --]

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


From e13703426f367c618f2984d376289b197a8c0402 Mon Sep 17 00:00:00 2001
From: Stefan Bader <stefan.bader@canonical.com>
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@canonical.com>
---
 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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Question about apic ipi interface

[Ben Guthro]

On Wed, May 8, 2013 at 4:26 PM, Stefan Bader <stefan.bader@canonical.com> 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@canonical.com>
> 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@canonical.com>
> ---
>  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
>

Re: Question about apic ipi interface

[Ian Campbell]

On Wed, 2013-05-08 at 17:26 +0100, Stefan Bader 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?

It certainly seems too. Although I traced the call chain back to
default_send_IPI_allbutself() which nothing actually seems to call. Or
my grep is faulty...

There is a call path via send_IPI_allbutself which looks to only
potentially be called as:
        apic->send_IPI_allbutself(APIC_DM_NMI);
        apic->send_IPI_allbutself(NMI_VECTOR);
(the other calls are in native_smp_foo which I assume doesn't apply).

xen_map_vector( doesn't seem to handle those two, I'm not sure if that
will constitute a regression since it doesn't seem likely that either of
those would have worked properly with the current code either.

Ian.

Re: Question about apic ipi interface

[Stefan Bader]

[-- Attachment #1.1: Type: text/plain, Size: 2804 bytes --]

On 09.05.2013 10:56, Ian Campbell wrote:
> On Wed, 2013-05-08 at 17:26 +0100, Stefan Bader 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?
> 
> It certainly seems too. Although I traced the call chain back to
> default_send_IPI_allbutself() which nothing actually seems to call. Or
> my grep is faulty...
> 
> There is a call path via send_IPI_allbutself which looks to only
> potentially be called as:
>         apic->send_IPI_allbutself(APIC_DM_NMI);
>         apic->send_IPI_allbutself(NMI_VECTOR);
> (the other calls are in native_smp_foo which I assume doesn't apply).

If I am not missing something send_IPI_allbutself as a function only exists on
non-x86 archs. And as you say the other users seem to be in a function that gets
replaced under Xen PV.

> 
> xen_map_vector( doesn't seem to handle those two, I'm not sure if that
> will constitute a regression since it doesn't seem likely that either of
> those would have worked properly with the current code either.

Yeah best case it would hide unimplemented vectors to be used. With the change
this would at least be obvious. Like I found when realizing that all this only
applies to dom0 because other PV domUs seem to get the noop_apic assigned. So
one can do a "echo l >/proc/sysrq-trigger" and get vector 0x2 (NMI_VECTOR) not
implemented (in dom0). Luckily the only bad thing happening is a delay and no
stack traces being produced.

-Stefan

> 
> Ian.
> 



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Question about apic ipi interface

[Konrad Rzeszutek Wilk]

On Wed, May 08, 2013 at 06:26:40PM +0200, Stefan Bader 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
> 
> 
> From e13703426f367c618f2984d376289b197a8c0402 Mon Sep 17 00:00:00 2001
> From: Stefan Bader <stefan.bader@canonical.com>
> 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@canonical.com>

Looks very similar to 

https://patchwork.kernel.org/patch/2414311/

So two people pointing out the same thing. 
> ---
>  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
> 

Re: Question about apic ipi interface

[Stefan Bader]

[-- Attachment #1.1: Type: text/plain, Size: 4905 bytes --]

On 22.05.2013 21:40, Konrad Rzeszutek Wilk wrote:
> On Wed, May 08, 2013 at 06:26:40PM +0200, Stefan Bader 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
>>
>>
>> From e13703426f367c618f2984d376289b197a8c0402 Mon Sep 17 00:00:00 2001
>> From: Stefan Bader <stefan.bader@canonical.com>
>> 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@canonical.com>
> 
> Looks very similar to 
> 
> https://patchwork.kernel.org/patch/2414311/
> 
> So two people pointing out the same thing. 

Yeah, from this discussion and further looking into it I am relatively sure this
has no visible effect either way because there currently is no user of the "odd"
implementations.

>> ---
>>  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
>>
> 
> 



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Question about apic ipi interface

[Konrad Rzeszutek Wilk]

On Thu, May 23, 2013 at 11:24:56AM +0200, Stefan Bader wrote:
> On 22.05.2013 21:40, Konrad Rzeszutek Wilk wrote:
> > On Wed, May 08, 2013 at 06:26:40PM +0200, Stefan Bader 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
> >>
> >>
> >> From e13703426f367c618f2984d376289b197a8c0402 Mon Sep 17 00:00:00 2001
> >> From: Stefan Bader <stefan.bader@canonical.com>
> >> 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@canonical.com>
> > 
> > Looks very similar to 
> > 
> > https://patchwork.kernel.org/patch/2414311/
> > 
> > So two people pointing out the same thing. 
> 
> Yeah, from this discussion and further looking into it I am relatively sure this
> has no visible effect either way because there currently is no user of the "odd"
> implementations.

<nods> OK, let me commit yours and also add a comment about Zhenzhong's.

> 
> >> ---
> >>  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@lists.xen.org
> http://lists.xen.org/xen-devel

Re: Question about apic ipi interface

[Konrad Rzeszutek Wilk]

On Thu, May 23, 2013 at 11:24:56AM +0200, Stefan Bader wrote:
> On 22.05.2013 21:40, Konrad Rzeszutek Wilk wrote:
> > On Wed, May 08, 2013 at 06:26:40PM +0200, Stefan Bader 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
> >>
> >>
> >> From e13703426f367c618f2984d376289b197a8c0402 Mon Sep 17 00:00:00 2001
> >> From: Stefan Bader <stefan.bader@canonical.com>
> >> 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@canonical.com>
> > 
> > Looks very similar to 
> > 
> > https://patchwork.kernel.org/patch/2414311/
> > 
> > So two people pointing out the same thing. 
> 
> Yeah, from this discussion and further looking into it I am relatively sure this
> has no visible effect either way because there currently is no user of the "odd"
> implementations.

OK, could you resend your patch properly please? Somehow I cannot apply
your patch:

patching file arch/x86/xen/smp.c
patch: **** malformed patch at line 136: ask *mask,


> 
> >> ---
> >>  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
> >>
> > 
> > 
> 
> 

Re: Question about apic ipi interface

[Stefan Bader]

[-- Attachment #1.1.1: Type: text/plain, Size: 3506 bytes --]

On 28.05.2013 14:43, Konrad Rzeszutek Wilk wrote:
> On Thu, May 23, 2013 at 11:24:56AM +0200, Stefan Bader wrote:
>> On 22.05.2013 21:40, Konrad Rzeszutek Wilk wrote:
>>> On Wed, May 08, 2013 at 06:26:40PM +0200, Stefan Bader 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
>>>>
>>>>
>>>> From e13703426f367c618f2984d376289b197a8c0402 Mon Sep 17 00:00:00 2001
>>>> From: Stefan Bader <stefan.bader@canonical.com>
>>>> 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@canonical.com>
>>>
>>> Looks very similar to 
>>>
>>> https://patchwork.kernel.org/patch/2414311/
>>>
>>> So two people pointing out the same thing. 
>>
>> Yeah, from this discussion and further looking into it I am relatively sure this
>> has no visible effect either way because there currently is no user of the "odd"
>> implementations.
> 
> OK, could you resend your patch properly please? Somehow I cannot apply
> your patch:
> 
> patching file arch/x86/xen/smp.c
> patch: **** malformed patch at line 136: ask *mask,
> 

Thunderbird joy... as attachment this time...

-Stefan



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.2: 0001-xen-Clean-up-apic-ipi-interface.patch --]
[-- Type: text/x-diff; name="0001-xen-Clean-up-apic-ipi-interface.patch", Size: 2441 bytes --]

From c687d9bcf7d3b3f2fd81dcf1afbbc1ebb4559f29 Mon Sep 17 00:00:00 2001
From: Stefan Bader <stefan.bader@canonical.com>
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@canonical.com>
---
 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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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