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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
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
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 >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
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. > > StefanOh, 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 >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
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 >
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.
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. >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
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 >
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 >> > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
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
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 > >> > > > > > >
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel