* [PATCH 2 1/4] powerpc: drop the ability to tweak SMT mode at boot time
2014-12-05 15:13 [PATCH 2 0/4] powerpc: don't mess with SMT at boot time Greg Kurz
@ 2014-12-05 15:14 ` Greg Kurz
2014-12-05 18:52 ` Scott Wood
2014-12-05 15:14 ` [PATCH 2 2/4] powerpc: drop smt_enabled_at_boot Greg Kurz
` (3 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Greg Kurz @ 2014-12-05 15:14 UTC (permalink / raw)
To: linuxppc-dev
The smt-enabled kernel parameter basically leaves unwanted cpus executing
in firmware or wherever they happen to be. The very same applies to the
ibm,smt-enabled DT property which is no more used by anything known. These
are hacks that shoudn't be used in a production environment.
Quoting mpe, "there are better ways for firmware to disable SMT".
It also has an evil side effect on the split-core feature for powernv. The
code needs all the cpus to participate to the split mode update: it relies
on smp_send_reschedule() to get offline ones to do so. This doesn't work with
cpus that haven't come up... The consequence is a kernel hang on powernv when
trying to limit the number of hw threads at boot time (e.g. smt-enabled to
anything but 8 on POWER8).
This patch simply removes both the smt-enabled kernel parameter and the
ibm,smt-enabled property for all platforms. The new default is to start
all hw threads. That leaves /sys the only supported API to change SMT
settings.
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
v2: also drop ibm,smt-enabled
arch/powerpc/kernel/setup_64.c | 46 ----------------------------------------
1 file changed, 46 deletions(-)
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 49f553b..29c1845 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -131,57 +131,11 @@ static void setup_tlb_core_data(void)
#ifdef CONFIG_SMP
-static char *smt_enabled_cmdline;
-
-/* Look for ibm,smt-enabled OF option */
static void check_smt_enabled(void)
{
- struct device_node *dn;
- const char *smt_option;
-
/* Default to enabling all threads */
smt_enabled_at_boot = threads_per_core;
-
- /* Allow the command line to overrule the OF option */
- if (smt_enabled_cmdline) {
- if (!strcmp(smt_enabled_cmdline, "on"))
- smt_enabled_at_boot = threads_per_core;
- else if (!strcmp(smt_enabled_cmdline, "off"))
- smt_enabled_at_boot = 0;
- else {
- int smt;
- int rc;
-
- rc = kstrtoint(smt_enabled_cmdline, 10, &smt);
- if (!rc)
- smt_enabled_at_boot =
- min(threads_per_core, smt);
- }
- } else {
- dn = of_find_node_by_path("/options");
- if (dn) {
- smt_option = of_get_property(dn, "ibm,smt-enabled",
- NULL);
-
- if (smt_option) {
- if (!strcmp(smt_option, "on"))
- smt_enabled_at_boot = threads_per_core;
- else if (!strcmp(smt_option, "off"))
- smt_enabled_at_boot = 0;
- }
-
- of_node_put(dn);
- }
- }
-}
-
-/* Look for smt-enabled= cmdline option */
-static int __init early_smt_enabled(char *p)
-{
- smt_enabled_cmdline = p;
- return 0;
}
-early_param("smt-enabled", early_smt_enabled);
#else
#define check_smt_enabled()
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 2 1/4] powerpc: drop the ability to tweak SMT mode at boot time
2014-12-05 15:14 ` [PATCH 2 1/4] powerpc: drop the ability to tweak SMT mode " Greg Kurz
@ 2014-12-05 18:52 ` Scott Wood
2014-12-08 8:23 ` Greg Kurz
2014-12-09 4:11 ` Michael Ellerman
0 siblings, 2 replies; 19+ messages in thread
From: Scott Wood @ 2014-12-05 18:52 UTC (permalink / raw)
To: Greg Kurz; +Cc: linuxppc-dev
On Fri, 2014-12-05 at 16:14 +0100, Greg Kurz wrote:
> The smt-enabled kernel parameter basically leaves unwanted cpus executing
> in firmware or wherever they happen to be. The very same applies to the
> ibm,smt-enabled DT property which is no more used by anything known. These
> are hacks that shoudn't be used in a production environment.
>
> Quoting mpe, "there are better ways for firmware to disable SMT".
Those "better ways" don't apply to Freescale chips, where the OS enables
(or not) SMT without any interaction with firmware. I don't care about
the ibm,smt-enabled property, but can we please keep the smt-enabled
boot option?
> It also has an evil side effect on the split-core feature for powernv. The
> code needs all the cpus to participate to the split mode update: it relies
> on smp_send_reschedule() to get offline ones to do so. This doesn't work with
> cpus that haven't come up... The consequence is a kernel hang on powernv when
> trying to limit the number of hw threads at boot time (e.g. smt-enabled to
> anything but 8 on POWER8).
In that case could you disable the option only on that hardware?
> This patch simply removes both the smt-enabled kernel parameter and the
> ibm,smt-enabled property for all platforms. The new default is to start
> all hw threads. That leaves /sys the only supported API to change SMT
> settings.
How would you use /sys for this? Are you talking about CPU hotplug?
-Scott
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2 1/4] powerpc: drop the ability to tweak SMT mode at boot time
2014-12-05 18:52 ` Scott Wood
@ 2014-12-08 8:23 ` Greg Kurz
2014-12-08 22:39 ` Scott Wood
2014-12-09 4:11 ` Michael Ellerman
1 sibling, 1 reply; 19+ messages in thread
From: Greg Kurz @ 2014-12-08 8:23 UTC (permalink / raw)
To: Scott Wood; +Cc: Michael Ellerman, linuxppc-dev
On Fri, 5 Dec 2014 12:52:45 -0600
Scott Wood <scottwood@freescale.com> wrote:
> On Fri, 2014-12-05 at 16:14 +0100, Greg Kurz wrote:
> > The smt-enabled kernel parameter basically leaves unwanted cpus executing
> > in firmware or wherever they happen to be. The very same applies to the
> > ibm,smt-enabled DT property which is no more used by anything known. These
> > are hacks that shoudn't be used in a production environment.
> >
> > Quoting mpe, "there are better ways for firmware to disable SMT".
>
Hi Scott,
> Those "better ways" don't apply to Freescale chips, where the OS enables
> (or not) SMT without any interaction with firmware. I don't care about
> the ibm,smt-enabled property, but can we please keep the smt-enabled
> boot option?
>
Fair enough for the firmware side, what about CPU hot(un)plug then ?
> > It also has an evil side effect on the split-core feature for powernv. The
> > code needs all the cpus to participate to the split mode update: it relies
> > on smp_send_reschedule() to get offline ones to do so. This doesn't work with
> > cpus that haven't come up... The consequence is a kernel hang on powernv when
> > trying to limit the number of hw threads at boot time (e.g. smt-enabled to
> > anything but 8 on POWER8).
>
> In that case could you disable the option only on that hardware?
>
The fact it breaks only powernv doesn't mean it is a powernv only issue.
The smt-enabled feature is a hack because it leaves some cpus in a undefined
state from a kernel POV. Moreover it drags about 80 lines of code and sits
entirely in common ppc64 code. I would reverse the question then ? Why not
moving smt-enabled code to freescale only ?
> > This patch simply removes both the smt-enabled kernel parameter and the
> > ibm,smt-enabled property for all platforms. The new default is to start
> > all hw threads. That leaves /sys the only supported API to change SMT
> > settings.
>
> How would you use /sys for this? Are you talking about CPU hotplug?
>
Yes. This is the safer way to offline cpus.
> -Scott
>
Cheers.
--
Greg
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2 1/4] powerpc: drop the ability to tweak SMT mode at boot time
2014-12-08 8:23 ` Greg Kurz
@ 2014-12-08 22:39 ` Scott Wood
0 siblings, 0 replies; 19+ messages in thread
From: Scott Wood @ 2014-12-08 22:39 UTC (permalink / raw)
To: Greg Kurz; +Cc: Michael Ellerman, linuxppc-dev
On Mon, 2014-12-08 at 09:23 +0100, Greg Kurz wrote:
> On Fri, 5 Dec 2014 12:52:45 -0600
> Scott Wood <scottwood@freescale.com> wrote:
>
> > On Fri, 2014-12-05 at 16:14 +0100, Greg Kurz wrote:
> > > The smt-enabled kernel parameter basically leaves unwanted cpus executing
> > > in firmware or wherever they happen to be. The very same applies to the
> > > ibm,smt-enabled DT property which is no more used by anything known. These
> > > are hacks that shoudn't be used in a production environment.
> > >
> > > Quoting mpe, "there are better ways for firmware to disable SMT".
> >
>
> Hi Scott,
>
> > Those "better ways" don't apply to Freescale chips, where the OS enables
> > (or not) SMT without any interaction with firmware. I don't care about
> > the ibm,smt-enabled property, but can we please keep the smt-enabled
> > boot option?
> >
>
> Fair enough for the firmware side, what about CPU hot(un)plug then ?
Not yet supported in mainline for e6500 (or maybe it works with
generic_mach_cpu_die which would not be helpful).
Plus, it's more complicated (both to use and how it works internally)
and doesn't avoid having the secondary thread ever run. Sometimes it's
useful to ensure that the second thread has never run when debugging a
problem.
> > > It also has an evil side effect on the split-core feature for powernv. The
> > > code needs all the cpus to participate to the split mode update: it relies
> > > on smp_send_reschedule() to get offline ones to do so. This doesn't work with
> > > cpus that haven't come up... The consequence is a kernel hang on powernv when
> > > trying to limit the number of hw threads at boot time (e.g. smt-enabled to
> > > anything but 8 on POWER8).
> >
> > In that case could you disable the option only on that hardware?
> >
>
> The fact it breaks only powernv doesn't mean it is a powernv only issue.
> The smt-enabled feature is a hack because it leaves some cpus in a undefined
> state from a kernel POV.
I'm aware of an issue where per-cpu threads get created for these CPUs,
which seems like a bug if they were never marked online (it's on my todo
list to investigate further). Are there other issues? It seems like
there ought to be some way to do this right.
> Moreover it drags about 80 lines of code and sits entirely in common
> ppc64 code. I would reverse the question then ? Why not moving
> smt-enabled code to freescale only ?
I'm fine with making it Freescale-only.
-Scott
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2 1/4] powerpc: drop the ability to tweak SMT mode at boot time
2014-12-05 18:52 ` Scott Wood
2014-12-08 8:23 ` Greg Kurz
@ 2014-12-09 4:11 ` Michael Ellerman
2014-12-09 8:53 ` Greg Kurz
2014-12-09 21:04 ` Scott Wood
1 sibling, 2 replies; 19+ messages in thread
From: Michael Ellerman @ 2014-12-09 4:11 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, Greg Kurz
On Fri, 2014-12-05 at 12:52 -0600, Scott Wood wrote:
> On Fri, 2014-12-05 at 16:14 +0100, Greg Kurz wrote:
> > The smt-enabled kernel parameter basically leaves unwanted cpus executing
> > in firmware or wherever they happen to be. The very same applies to the
> > ibm,smt-enabled DT property which is no more used by anything known. These
> > are hacks that shoudn't be used in a production environment.
> >
> > Quoting mpe, "there are better ways for firmware to disable SMT".
>
> Those "better ways" don't apply to Freescale chips, where the OS enables
> (or not) SMT without any interaction with firmware.
But how does it know there even are SMT threads? From the device tree? So
just don't present the threads in the device tree?
cheers
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2 1/4] powerpc: drop the ability to tweak SMT mode at boot time
2014-12-09 4:11 ` Michael Ellerman
@ 2014-12-09 8:53 ` Greg Kurz
2014-12-09 21:04 ` Scott Wood
1 sibling, 0 replies; 19+ messages in thread
From: Greg Kurz @ 2014-12-09 8:53 UTC (permalink / raw)
To: Michael Ellerman; +Cc: Scott Wood, linuxppc-dev
On Tue, 09 Dec 2014 15:11:02 +1100
Michael Ellerman <mpe@ellerman.id.au> wrote:
> On Fri, 2014-12-05 at 12:52 -0600, Scott Wood wrote:
> > On Fri, 2014-12-05 at 16:14 +0100, Greg Kurz wrote:
> > > The smt-enabled kernel parameter basically leaves unwanted cpus executing
> > > in firmware or wherever they happen to be. The very same applies to the
> > > ibm,smt-enabled DT property which is no more used by anything known. These
> > > are hacks that shoudn't be used in a production environment.
> > >
> > > Quoting mpe, "there are better ways for firmware to disable SMT".
> >
> > Those "better ways" don't apply to Freescale chips, where the OS enables
> > (or not) SMT without any interaction with firmware.
>
> But how does it know there even are SMT threads? From the device tree? So
> just don't present the threads in the device tree?
>
> cheers
>
>
Michael,
Maybe we can first kill the cpu_bootable hook in powernv only, for bug fix.
Then we can take time to do the thing right for all platforms. Thoughts ?
--
Greg
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2 1/4] powerpc: drop the ability to tweak SMT mode at boot time
2014-12-09 4:11 ` Michael Ellerman
2014-12-09 8:53 ` Greg Kurz
@ 2014-12-09 21:04 ` Scott Wood
2014-12-09 23:56 ` Michael Ellerman
1 sibling, 1 reply; 19+ messages in thread
From: Scott Wood @ 2014-12-09 21:04 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, Greg Kurz
On Tue, 2014-12-09 at 15:11 +1100, Michael Ellerman wrote:
> On Fri, 2014-12-05 at 12:52 -0600, Scott Wood wrote:
> > On Fri, 2014-12-05 at 16:14 +0100, Greg Kurz wrote:
> > > The smt-enabled kernel parameter basically leaves unwanted cpus executing
> > > in firmware or wherever they happen to be. The very same applies to the
> > > ibm,smt-enabled DT property which is no more used by anything known. These
> > > are hacks that shoudn't be used in a production environment.
> > >
> > > Quoting mpe, "there are better ways for firmware to disable SMT".
> >
> > Those "better ways" don't apply to Freescale chips, where the OS enables
> > (or not) SMT without any interaction with firmware.
>
> But how does it know there even are SMT threads? From the device tree? So
> just don't present the threads in the device tree?
The device tree is for hardware description, not configuration...
-Scott
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2 1/4] powerpc: drop the ability to tweak SMT mode at boot time
2014-12-09 21:04 ` Scott Wood
@ 2014-12-09 23:56 ` Michael Ellerman
2014-12-10 0:14 ` Scott Wood
0 siblings, 1 reply; 19+ messages in thread
From: Michael Ellerman @ 2014-12-09 23:56 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, Greg Kurz
On Tue, 2014-12-09 at 15:04 -0600, Scott Wood wrote:
> On Tue, 2014-12-09 at 15:11 +1100, Michael Ellerman wrote:
> > On Fri, 2014-12-05 at 12:52 -0600, Scott Wood wrote:
> > > On Fri, 2014-12-05 at 16:14 +0100, Greg Kurz wrote:
> > > > The smt-enabled kernel parameter basically leaves unwanted cpus executing
> > > > in firmware or wherever they happen to be. The very same applies to the
> > > > ibm,smt-enabled DT property which is no more used by anything known. These
> > > > are hacks that shoudn't be used in a production environment.
> > > >
> > > > Quoting mpe, "there are better ways for firmware to disable SMT".
> > >
> > > Those "better ways" don't apply to Freescale chips, where the OS enables
> > > (or not) SMT without any interaction with firmware.
> >
> > But how does it know there even are SMT threads? From the device tree? So
> > just don't present the threads in the device tree?
>
> The device tree is for hardware description, not configuration...
Oh please, you're quoting device tree scripture to me now?
cheers
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2 1/4] powerpc: drop the ability to tweak SMT mode at boot time
2014-12-09 23:56 ` Michael Ellerman
@ 2014-12-10 0:14 ` Scott Wood
2014-12-10 2:14 ` Michael Ellerman
0 siblings, 1 reply; 19+ messages in thread
From: Scott Wood @ 2014-12-10 0:14 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, Greg Kurz
On Wed, 2014-12-10 at 10:56 +1100, Michael Ellerman wrote:
> On Tue, 2014-12-09 at 15:04 -0600, Scott Wood wrote:
> > On Tue, 2014-12-09 at 15:11 +1100, Michael Ellerman wrote:
> > > On Fri, 2014-12-05 at 12:52 -0600, Scott Wood wrote:
> > > > On Fri, 2014-12-05 at 16:14 +0100, Greg Kurz wrote:
> > > > > The smt-enabled kernel parameter basically leaves unwanted cpus executing
> > > > > in firmware or wherever they happen to be. The very same applies to the
> > > > > ibm,smt-enabled DT property which is no more used by anything known. These
> > > > > are hacks that shoudn't be used in a production environment.
> > > > >
> > > > > Quoting mpe, "there are better ways for firmware to disable SMT".
> > > >
> > > > Those "better ways" don't apply to Freescale chips, where the OS enables
> > > > (or not) SMT without any interaction with firmware.
> > >
> > > But how does it know there even are SMT threads? From the device tree? So
> > > just don't present the threads in the device tree?
> >
> > The device tree is for hardware description, not configuration...
>
> Oh please, you're quoting device tree scripture to me now?
What benefit is there to ignoring "scripture" here? Going from an easy
to use command line option to needing to mess around with the dts file
is not a usability improvement. If you want to make it Freescale-only,
fine. If you want to push me to fix the problems with the
implementation, fine.
-Scott
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2 1/4] powerpc: drop the ability to tweak SMT mode at boot time
2014-12-10 0:14 ` Scott Wood
@ 2014-12-10 2:14 ` Michael Ellerman
2014-12-10 23:50 ` Scott Wood
0 siblings, 1 reply; 19+ messages in thread
From: Michael Ellerman @ 2014-12-10 2:14 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, Greg Kurz
On Tue, 2014-12-09 at 18:14 -0600, Scott Wood wrote:
> On Wed, 2014-12-10 at 10:56 +1100, Michael Ellerman wrote:
> > On Tue, 2014-12-09 at 15:04 -0600, Scott Wood wrote:
> > > On Tue, 2014-12-09 at 15:11 +1100, Michael Ellerman wrote:
> > > > On Fri, 2014-12-05 at 12:52 -0600, Scott Wood wrote:
> > > > > On Fri, 2014-12-05 at 16:14 +0100, Greg Kurz wrote:
> > > > > > The smt-enabled kernel parameter basically leaves unwanted cpus executing
> > > > > > in firmware or wherever they happen to be. The very same applies to the
> > > > > > ibm,smt-enabled DT property which is no more used by anything known. These
> > > > > > are hacks that shoudn't be used in a production environment.
> > > > > >
> > > > > > Quoting mpe, "there are better ways for firmware to disable SMT".
> > > > >
> > > > > Those "better ways" don't apply to Freescale chips, where the OS enables
> > > > > (or not) SMT without any interaction with firmware.
> > > >
> > > > But how does it know there even are SMT threads? From the device tree? So
> > > > just don't present the threads in the device tree?
> > >
> > > The device tree is for hardware description, not configuration...
> >
> > Oh please, you're quoting device tree scripture to me now?
>
> What benefit is there to ignoring "scripture" here? Going from an easy
> to use command line option to needing to mess around with the dts file
> is not a usability improvement. If you want to make it Freescale-only,
> fine. If you want to push me to fix the problems with the
> implementation, fine.
It's easy to use but it doesn't necessarily work.
You said in your other mail to Greg "Sometimes it's useful to ensure that the
second thread has never run when debugging a problem.".
But you don't know that, for all you know your firmware has started the thread
and it's busy looping somewhere. Perhaps you guys know that your firmware
doesn't do that, but it's still a hack.
We end up with cpus in the present map, but we have no idea where they are or
what they are doing.
So as far as I'm concerned it's only useful as a debugging hack, and one that
we don't really use anymore. But if you guys think it's useful then we'll keep
it.
I'll work out with Greg what the cleanest solution is.
It looks like you only need it on e6500? Which is platforms/85xx I think.
Anywhere else?
cheers
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2 1/4] powerpc: drop the ability to tweak SMT mode at boot time
2014-12-10 2:14 ` Michael Ellerman
@ 2014-12-10 23:50 ` Scott Wood
2014-12-12 5:19 ` Michael Ellerman
0 siblings, 1 reply; 19+ messages in thread
From: Scott Wood @ 2014-12-10 23:50 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, Greg Kurz
On Wed, 2014-12-10 at 13:14 +1100, Michael Ellerman wrote:
> On Tue, 2014-12-09 at 18:14 -0600, Scott Wood wrote:
> > What benefit is there to ignoring "scripture" here? Going from an easy
> > to use command line option to needing to mess around with the dts file
> > is not a usability improvement. If you want to make it Freescale-only,
> > fine. If you want to push me to fix the problems with the
> > implementation, fine.
>
> It's easy to use but it doesn't necessarily work.
>
> You said in your other mail to Greg "Sometimes it's useful to ensure that the
> second thread has never run when debugging a problem.".
>
> But you don't know that, for all you know your firmware has started the thread
> and it's busy looping somewhere. Perhaps you guys know that your firmware
> doesn't do that, but it's still a hack.
I know that our firmware doesn't do that, and I can verify by reading
the relevant register.
> We end up with cpus in the present map, but we have no idea where they are or
> what they are doing.
Can we check smt-enabled a little earlier and refrain from marking the
secondary threads as present if smt is disabled?
> So as far as I'm concerned it's only useful as a debugging hack, and one that
> we don't really use anymore. But if you guys think it's useful then we'll keep
> it.
>
> I'll work out with Greg what the cleanest solution is.
>
> It looks like you only need it on e6500? Which is platforms/85xx I think.
> Anywhere else?
Yes, just e6500.
-Scott
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2 1/4] powerpc: drop the ability to tweak SMT mode at boot time
2014-12-10 23:50 ` Scott Wood
@ 2014-12-12 5:19 ` Michael Ellerman
0 siblings, 0 replies; 19+ messages in thread
From: Michael Ellerman @ 2014-12-12 5:19 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, Greg Kurz
On Wed, 2014-12-10 at 17:50 -0600, Scott Wood wrote:
> On Wed, 2014-12-10 at 13:14 +1100, Michael Ellerman wrote:
> > On Tue, 2014-12-09 at 18:14 -0600, Scott Wood wrote:
> > > What benefit is there to ignoring "scripture" here? Going from an easy
> > > to use command line option to needing to mess around with the dts file
> > > is not a usability improvement. If you want to make it Freescale-only,
> > > fine. If you want to push me to fix the problems with the
> > > implementation, fine.
> >
> > It's easy to use but it doesn't necessarily work.
> >
> > You said in your other mail to Greg "Sometimes it's useful to ensure that the
> > second thread has never run when debugging a problem.".
> >
> > But you don't know that, for all you know your firmware has started the thread
> > and it's busy looping somewhere. Perhaps you guys know that your firmware
> > doesn't do that, but it's still a hack.
>
> I know that our firmware doesn't do that, and I can verify by reading
> the relevant register.
Lucky you :)
> > We end up with cpus in the present map, but we have no idea where they are or
> > what they are doing.
>
> Can we check smt-enabled a little earlier and refrain from marking the
> secondary threads as present if smt is disabled?
We could, and that would make the semantics much saner. But it would actually
be exactly the opposite of what the folks who originally hit this bug want to
happen.
They are using it as a shortcut for cpu hotunplug, ie. they want the threads
asleep in Linux ready to be hotplugged back in.
This is why I wanted to remove it, because those are both valid expectations of
what smt-enabled=off should do, but they are mutually exclusive. Not to mention
that the current code doesn't implement either of those properly.
Anyway for now we should just ignore it on powernv. We can look at doing
something saner in general in future.
cheers
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2 2/4] powerpc: drop smt_enabled_at_boot
2014-12-05 15:13 [PATCH 2 0/4] powerpc: don't mess with SMT at boot time Greg Kurz
2014-12-05 15:14 ` [PATCH 2 1/4] powerpc: drop the ability to tweak SMT mode " Greg Kurz
@ 2014-12-05 15:14 ` Greg Kurz
2014-12-05 15:15 ` [PATCH 2 3/4] powerpc: drop smp_generic_cpu_bootable() Greg Kurz
` (2 subsequent siblings)
4 siblings, 0 replies; 19+ messages in thread
From: Greg Kurz @ 2014-12-05 15:14 UTC (permalink / raw)
To: linuxppc-dev
The paths using the smt_enabled_at_boot variable reveal the following
lifecycle:
initial value: int smt_enabled_at_boot = 1
|
|
check_smt_enabled(): smt_enabled_at_boot = threads_per_core
|
|
setup_tlb_core_data(): if (smt_enabled_at_boot >= 2
|
|
smp_generic_cpu_bootable(): if (!smt_enabled_at_boot
|
if (smt_enabled_at_boot
It appears that smt_enabled_at_boot is just a duplicate of threads_per_core.
Let's drop it.
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/smp.h | 2 --
arch/powerpc/kernel/setup_64.c | 15 +--------------
arch/powerpc/kernel/smp.c | 8 +++-----
3 files changed, 4 insertions(+), 21 deletions(-)
diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
index 5a6614a..fae8cad 100644
--- a/arch/powerpc/include/asm/smp.h
+++ b/arch/powerpc/include/asm/smp.h
@@ -174,8 +174,6 @@ static inline void set_hard_smp_processor_id(int cpu, int phys)
#endif /* !CONFIG_SMP */
#endif /* !CONFIG_PPC64 */
-extern int smt_enabled_at_boot;
-
extern int smp_mpic_probe(void);
extern void smp_mpic_setup_cpu(int cpu);
extern int smp_generic_kick_cpu(int nr);
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 29c1845..ba80480 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -114,7 +114,7 @@ static void setup_tlb_core_data(void)
* or e6500 tablewalk mode, or else TLB handlers
* will be racy and could produce duplicate entries.
*/
- if (smt_enabled_at_boot >= 2 &&
+ if (threads_per_core >= 2 &&
!mmu_has_feature(MMU_FTR_USE_TLBRSRV) &&
book3e_htw_mode != PPC_HTW_E6500) {
/* Should we panic instead? */
@@ -129,18 +129,6 @@ static void setup_tlb_core_data(void)
}
#endif
-#ifdef CONFIG_SMP
-
-static void check_smt_enabled(void)
-{
- /* Default to enabling all threads */
- smt_enabled_at_boot = threads_per_core;
-}
-
-#else
-#define check_smt_enabled()
-#endif /* CONFIG_SMP */
-
/** Fix up paca fields required for the boot cpu */
static void fixup_boot_paca(void)
{
@@ -462,7 +450,6 @@ void __init setup_system(void)
xmon_setup();
smp_setup_cpu_maps();
- check_smt_enabled();
setup_tlb_core_data();
/*
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 8b2d2dc..9577791 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -80,8 +80,6 @@ struct smp_ops_t *smp_ops;
/* Can't be static due to PowerMac hackery */
volatile unsigned int cpu_callin_map[NR_CPUS];
-int smt_enabled_at_boot = 1;
-
static void (*crash_ipi_function_ptr)(struct pt_regs *) = NULL;
/*
@@ -95,10 +93,10 @@ int smp_generic_cpu_bootable(unsigned int nr)
* during boot if the user requests it.
*/
if (system_state == SYSTEM_BOOTING && cpu_has_feature(CPU_FTR_SMT)) {
- if (!smt_enabled_at_boot && cpu_thread_in_core(nr) != 0)
+ if (!threads_per_core && cpu_thread_in_core(nr) != 0)
return 0;
- if (smt_enabled_at_boot
- && cpu_thread_in_core(nr) >= smt_enabled_at_boot)
+ if (threads_per_core
+ && cpu_thread_in_core(nr) >= threads_per_core)
return 0;
}
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 2 3/4] powerpc: drop smp_generic_cpu_bootable()
2014-12-05 15:13 [PATCH 2 0/4] powerpc: don't mess with SMT at boot time Greg Kurz
2014-12-05 15:14 ` [PATCH 2 1/4] powerpc: drop the ability to tweak SMT mode " Greg Kurz
2014-12-05 15:14 ` [PATCH 2 2/4] powerpc: drop smt_enabled_at_boot Greg Kurz
@ 2014-12-05 15:15 ` Greg Kurz
2014-12-05 15:34 ` Greg Kurz
2014-12-05 15:15 ` [PATCH 2 4/4] powerpc: drop the cpu_bootable hook Greg Kurz
2014-12-12 5:22 ` [PATCH 2 0/4] powerpc: don't mess with SMT at boot time Michael Ellerman
4 siblings, 1 reply; 19+ messages in thread
From: Greg Kurz @ 2014-12-05 15:15 UTC (permalink / raw)
To: linuxppc-dev
The following assertions are always true:
- threads_per_core > 0
- cpu & (threads_per_core - 1) < threads_per_core
It means smp_generic_cpu_bootable() always returns true.
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
arch/powerpc/kernel/smp.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 9577791..8d7f114 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -89,17 +89,6 @@ static void (*crash_ipi_function_ptr)(struct pt_regs *) = NULL;
*/
int smp_generic_cpu_bootable(unsigned int nr)
{
- /* Special case - we inhibit secondary thread startup
- * during boot if the user requests it.
- */
- if (system_state == SYSTEM_BOOTING && cpu_has_feature(CPU_FTR_SMT)) {
- if (!threads_per_core && cpu_thread_in_core(nr) != 0)
- return 0;
- if (threads_per_core
- && cpu_thread_in_core(nr) >= threads_per_core)
- return 0;
- }
-
return 1;
}
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 2 3/4] powerpc: drop smp_generic_cpu_bootable()
2014-12-05 15:15 ` [PATCH 2 3/4] powerpc: drop smp_generic_cpu_bootable() Greg Kurz
@ 2014-12-05 15:34 ` Greg Kurz
0 siblings, 0 replies; 19+ messages in thread
From: Greg Kurz @ 2014-12-05 15:34 UTC (permalink / raw)
To: linuxppc-dev
Subject should read "powerpc: drop useless code in smp_generic_cpu_bootable()"
actually...
On Fri, 05 Dec 2014 16:15:12 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> The following assertions are always true:
> - threads_per_core > 0
> - cpu & (threads_per_core - 1) < threads_per_core
>
> It means smp_generic_cpu_bootable() always returns true.
>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
> arch/powerpc/kernel/smp.c | 11 -----------
> 1 file changed, 11 deletions(-)
>
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 9577791..8d7f114 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -89,17 +89,6 @@ static void (*crash_ipi_function_ptr)(struct pt_regs *) = NULL;
> */
> int smp_generic_cpu_bootable(unsigned int nr)
> {
> - /* Special case - we inhibit secondary thread startup
> - * during boot if the user requests it.
> - */
> - if (system_state == SYSTEM_BOOTING && cpu_has_feature(CPU_FTR_SMT)) {
> - if (!threads_per_core && cpu_thread_in_core(nr) != 0)
> - return 0;
> - if (threads_per_core
> - && cpu_thread_in_core(nr) >= threads_per_core)
> - return 0;
> - }
> -
> return 1;
> }
>
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2 4/4] powerpc: drop the cpu_bootable hook
2014-12-05 15:13 [PATCH 2 0/4] powerpc: don't mess with SMT at boot time Greg Kurz
` (2 preceding siblings ...)
2014-12-05 15:15 ` [PATCH 2 3/4] powerpc: drop smp_generic_cpu_bootable() Greg Kurz
@ 2014-12-05 15:15 ` Greg Kurz
2014-12-05 15:42 ` [PATCH] " Greg Kurz
2014-12-12 5:22 ` [PATCH 2 0/4] powerpc: don't mess with SMT at boot time Michael Ellerman
4 siblings, 1 reply; 19+ messages in thread
From: Greg Kurz @ 2014-12-05 15:15 UTC (permalink / raw)
To: linuxppc-dev
All powerpc platforms share the same cpu_bootable hook, which does nothing
but { return 1 }. It is not needed anymore. Let's drop it at last.
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/smp.h | 1 -
arch/powerpc/kernel/smp.c | 3 +--
arch/powerpc/platforms/85xx/smp.c | 1 -
arch/powerpc/platforms/cell/smp.c | 1 -
arch/powerpc/platforms/powernv/smp.c | 1 -
arch/powerpc/platforms/pseries/smp.c | 1 -
6 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
index fae8cad..76edb9c 100644
--- a/arch/powerpc/include/asm/smp.h
+++ b/arch/powerpc/include/asm/smp.h
@@ -50,7 +50,6 @@ struct smp_ops_t {
void (*give_timebase)(void);
int (*cpu_disable)(void);
void (*cpu_die)(unsigned int nr);
- int (*cpu_bootable)(unsigned int nr);
};
extern void smp_send_debugger_break(void);
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 8d7f114..de6d60f 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -489,8 +489,7 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)
cpu_thread_in_subcore(cpu))
return -EBUSY;
- if (smp_ops == NULL ||
- (smp_ops->cpu_bootable && !smp_ops->cpu_bootable(cpu)))
+ if (smp_ops == NULL)
return -EINVAL;
cpu_idle_thread_init(cpu, tidle);
diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
index d7c1e69..3aedc35 100644
--- a/arch/powerpc/platforms/85xx/smp.c
+++ b/arch/powerpc/platforms/85xx/smp.c
@@ -326,7 +326,6 @@ out:
struct smp_ops_t smp_85xx_ops = {
.kick_cpu = smp_85xx_kick_cpu,
- .cpu_bootable = smp_generic_cpu_bootable,
#ifdef CONFIG_HOTPLUG_CPU
.cpu_disable = generic_cpu_disable,
.cpu_die = generic_cpu_die,
diff --git a/arch/powerpc/platforms/cell/smp.c b/arch/powerpc/platforms/cell/smp.c
index c8017a7..09f4eec 100644
--- a/arch/powerpc/platforms/cell/smp.c
+++ b/arch/powerpc/platforms/cell/smp.c
@@ -142,7 +142,6 @@ static struct smp_ops_t bpa_iic_smp_ops = {
.probe = smp_iic_probe,
.kick_cpu = smp_cell_kick_cpu,
.setup_cpu = smp_cell_setup_cpu,
- .cpu_bootable = smp_generic_cpu_bootable,
};
/* This is called very early */
diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
index 4753958..2e8a727 100644
--- a/arch/powerpc/platforms/powernv/smp.c
+++ b/arch/powerpc/platforms/powernv/smp.c
@@ -191,7 +191,6 @@ static struct smp_ops_t pnv_smp_ops = {
.probe = xics_smp_probe,
.kick_cpu = pnv_smp_kick_cpu,
.setup_cpu = pnv_smp_setup_cpu,
- .cpu_bootable = smp_generic_cpu_bootable,
#ifdef CONFIG_HOTPLUG_CPU
.cpu_disable = pnv_smp_cpu_disable,
.cpu_die = generic_cpu_die,
diff --git a/arch/powerpc/platforms/pseries/smp.c b/arch/powerpc/platforms/pseries/smp.c
index a3555b1..505a689 100644
--- a/arch/powerpc/platforms/pseries/smp.c
+++ b/arch/powerpc/platforms/pseries/smp.c
@@ -222,7 +222,6 @@ static struct smp_ops_t pSeries_xics_smp_ops = {
.probe = pSeries_smp_probe,
.kick_cpu = smp_pSeries_kick_cpu,
.setup_cpu = smp_xics_setup_cpu,
- .cpu_bootable = smp_generic_cpu_bootable,
};
/* This is called very early */
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH] powerpc: drop the cpu_bootable hook
2014-12-05 15:15 ` [PATCH 2 4/4] powerpc: drop the cpu_bootable hook Greg Kurz
@ 2014-12-05 15:42 ` Greg Kurz
0 siblings, 0 replies; 19+ messages in thread
From: Greg Kurz @ 2014-12-05 15:42 UTC (permalink / raw)
To: linuxppc-dev
All powerpc platforms share the same cpu_bootable hook, which does nothing
but { return 1 }. It is not needed anymore. Let's drop it at last.
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
v3: drop smp_generic_cpu_bootable() as well
I knew that posting a few minutes before leaving is bad idea on fridays... ;)
arch/powerpc/include/asm/smp.h | 1 -
arch/powerpc/kernel/smp.c | 14 +-------------
arch/powerpc/platforms/85xx/smp.c | 1 -
arch/powerpc/platforms/cell/smp.c | 1 -
arch/powerpc/platforms/powernv/smp.c | 1 -
arch/powerpc/platforms/pseries/smp.c | 1 -
6 files changed, 1 insertion(+), 18 deletions(-)
diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
index fae8cad..76edb9c 100644
--- a/arch/powerpc/include/asm/smp.h
+++ b/arch/powerpc/include/asm/smp.h
@@ -50,7 +50,6 @@ struct smp_ops_t {
void (*give_timebase)(void);
int (*cpu_disable)(void);
void (*cpu_die)(unsigned int nr);
- int (*cpu_bootable)(unsigned int nr);
};
extern void smp_send_debugger_break(void);
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 8d7f114..e8bd931 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -82,17 +82,6 @@ volatile unsigned int cpu_callin_map[NR_CPUS];
static void (*crash_ipi_function_ptr)(struct pt_regs *) = NULL;
-/*
- * Returns 1 if the specified cpu should be brought up during boot.
- * Used to inhibit booting threads if they've been disabled or
- * limited on the command line
- */
-int smp_generic_cpu_bootable(unsigned int nr)
-{
- return 1;
-}
-
-
#ifdef CONFIG_PPC64
int smp_generic_kick_cpu(int nr)
{
@@ -489,8 +478,7 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)
cpu_thread_in_subcore(cpu))
return -EBUSY;
- if (smp_ops == NULL ||
- (smp_ops->cpu_bootable && !smp_ops->cpu_bootable(cpu)))
+ if (smp_ops == NULL)
return -EINVAL;
cpu_idle_thread_init(cpu, tidle);
diff --git a/arch/powerpc/platforms/85xx/smp.c b/arch/powerpc/platforms/85xx/smp.c
index d7c1e69..3aedc35 100644
--- a/arch/powerpc/platforms/85xx/smp.c
+++ b/arch/powerpc/platforms/85xx/smp.c
@@ -326,7 +326,6 @@ out:
struct smp_ops_t smp_85xx_ops = {
.kick_cpu = smp_85xx_kick_cpu,
- .cpu_bootable = smp_generic_cpu_bootable,
#ifdef CONFIG_HOTPLUG_CPU
.cpu_disable = generic_cpu_disable,
.cpu_die = generic_cpu_die,
diff --git a/arch/powerpc/platforms/cell/smp.c b/arch/powerpc/platforms/cell/smp.c
index c8017a7..09f4eec 100644
--- a/arch/powerpc/platforms/cell/smp.c
+++ b/arch/powerpc/platforms/cell/smp.c
@@ -142,7 +142,6 @@ static struct smp_ops_t bpa_iic_smp_ops = {
.probe = smp_iic_probe,
.kick_cpu = smp_cell_kick_cpu,
.setup_cpu = smp_cell_setup_cpu,
- .cpu_bootable = smp_generic_cpu_bootable,
};
/* This is called very early */
diff --git a/arch/powerpc/platforms/powernv/smp.c b/arch/powerpc/platforms/powernv/smp.c
index 4753958..2e8a727 100644
--- a/arch/powerpc/platforms/powernv/smp.c
+++ b/arch/powerpc/platforms/powernv/smp.c
@@ -191,7 +191,6 @@ static struct smp_ops_t pnv_smp_ops = {
.probe = xics_smp_probe,
.kick_cpu = pnv_smp_kick_cpu,
.setup_cpu = pnv_smp_setup_cpu,
- .cpu_bootable = smp_generic_cpu_bootable,
#ifdef CONFIG_HOTPLUG_CPU
.cpu_disable = pnv_smp_cpu_disable,
.cpu_die = generic_cpu_die,
diff --git a/arch/powerpc/platforms/pseries/smp.c b/arch/powerpc/platforms/pseries/smp.c
index a3555b1..505a689 100644
--- a/arch/powerpc/platforms/pseries/smp.c
+++ b/arch/powerpc/platforms/pseries/smp.c
@@ -222,7 +222,6 @@ static struct smp_ops_t pSeries_xics_smp_ops = {
.probe = pSeries_smp_probe,
.kick_cpu = smp_pSeries_kick_cpu,
.setup_cpu = smp_xics_setup_cpu,
- .cpu_bootable = smp_generic_cpu_bootable,
};
/* This is called very early */
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2 0/4] powerpc: don't mess with SMT at boot time
2014-12-05 15:13 [PATCH 2 0/4] powerpc: don't mess with SMT at boot time Greg Kurz
` (3 preceding siblings ...)
2014-12-05 15:15 ` [PATCH 2 4/4] powerpc: drop the cpu_bootable hook Greg Kurz
@ 2014-12-12 5:22 ` Michael Ellerman
4 siblings, 0 replies; 19+ messages in thread
From: Michael Ellerman @ 2014-12-12 5:22 UTC (permalink / raw)
To: Greg Kurz; +Cc: linuxppc-dev
On Fri, 2014-12-05 at 16:13 +0100, Greg Kurz wrote:
> As requested by mpe, this series now covers both the smt-enabled
> kernel parameter and the ibm,smt-enabled property. The cleanup was
> split into 3 separate patches to ease review, but I guess they
> could be folded into a single patch as well.
Sorry to send you down the garden path Greg. It would have been a nice
cleanup, but seems the FSL folks still see value in smt-enabled.
So can you rework it to just ignore smt-enabled on powernv.
That should be as simple as implementing a powernv_cpu_bootable() that always
returns true.
cheers
^ permalink raw reply [flat|nested] 19+ messages in thread