* Re: Question about apic ipi interface
2013-05-08 16:26 ` Stefan Bader
@ 2013-05-08 17:00 ` Ben Guthro
2013-05-09 8:56 ` Ian Campbell
2013-05-22 19:40 ` Konrad Rzeszutek Wilk
2 siblings, 0 replies; 13+ messages in thread
From: Ben Guthro @ 2013-05-08 17:00 UTC (permalink / raw)
To: Stefan Bader
Cc: Lin Ming, xen-devel@lists.xensource.com, Konrad Rzeszutek Wilk
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: Question about apic ipi interface
2013-05-08 16:26 ` Stefan Bader
2013-05-08 17:00 ` Ben Guthro
@ 2013-05-09 8:56 ` Ian Campbell
2013-05-09 14:33 ` Stefan Bader
2013-05-22 19:40 ` Konrad Rzeszutek Wilk
2 siblings, 1 reply; 13+ messages in thread
From: Ian Campbell @ 2013-05-09 8:56 UTC (permalink / raw)
To: Stefan Bader
Cc: xen-devel@lists.xensource.com, Ben Guthro, Konrad Rzeszutek Wilk,
Lin Ming
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.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: Question about apic ipi interface
2013-05-09 8:56 ` Ian Campbell
@ 2013-05-09 14:33 ` Stefan Bader
0 siblings, 0 replies; 13+ messages in thread
From: Stefan Bader @ 2013-05-09 14:33 UTC (permalink / raw)
To: Ian Campbell
Cc: xen-devel@lists.xensource.com, Ben Guthro, Konrad Rzeszutek Wilk,
Lin Ming
[-- 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Question about apic ipi interface
2013-05-08 16:26 ` Stefan Bader
2013-05-08 17:00 ` Ben Guthro
2013-05-09 8:56 ` Ian Campbell
@ 2013-05-22 19:40 ` Konrad Rzeszutek Wilk
2013-05-23 9:24 ` Stefan Bader
2 siblings, 1 reply; 13+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-05-22 19:40 UTC (permalink / raw)
To: Stefan Bader, zhenzhong.duan
Cc: xen-devel@lists.xensource.com, Ben Guthro, Lin Ming
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: Question about apic ipi interface
2013-05-22 19:40 ` Konrad Rzeszutek Wilk
@ 2013-05-23 9:24 ` Stefan Bader
2013-05-24 14:19 ` Konrad Rzeszutek Wilk
2013-05-28 12:43 ` Konrad Rzeszutek Wilk
0 siblings, 2 replies; 13+ messages in thread
From: Stefan Bader @ 2013-05-23 9:24 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Lin Ming, xen-devel@lists.xensource.com, zhenzhong.duan,
Ben Guthro
[-- 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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: Question about apic ipi interface
2013-05-23 9:24 ` Stefan Bader
@ 2013-05-24 14:19 ` Konrad Rzeszutek Wilk
2013-05-28 12:43 ` Konrad Rzeszutek Wilk
1 sibling, 0 replies; 13+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-05-24 14:19 UTC (permalink / raw)
To: Stefan Bader
Cc: Ben Guthro, xen-devel@lists.xensource.com, zhenzhong.duan,
Lin Ming
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: Question about apic ipi interface
2013-05-23 9:24 ` Stefan Bader
2013-05-24 14:19 ` Konrad Rzeszutek Wilk
@ 2013-05-28 12:43 ` Konrad Rzeszutek Wilk
2013-05-28 12:50 ` Stefan Bader
1 sibling, 1 reply; 13+ messages in thread
From: Konrad Rzeszutek Wilk @ 2013-05-28 12:43 UTC (permalink / raw)
To: Stefan Bader
Cc: Lin Ming, xen-devel@lists.xensource.com, zhenzhong.duan,
Ben Guthro
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
> >>
> >
> >
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: Question about apic ipi interface
2013-05-28 12:43 ` Konrad Rzeszutek Wilk
@ 2013-05-28 12:50 ` Stefan Bader
0 siblings, 0 replies; 13+ messages in thread
From: Stefan Bader @ 2013-05-28 12:50 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Ben Guthro, xen-devel@lists.xensource.com, zhenzhong.duan,
Lin Ming
[-- 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
^ permalink raw reply related [flat|nested] 13+ messages in thread