| Message ID | 53DB6B4F.9080605@arm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 2014-08-01 at 11:26 +0100, Sudeep Holla wrote: > > On 22/07/14 19:01, Olof Johansson wrote: > > This is a major refresh of the multi_v7_defconfig: > > > > - Bring over a bunch of Samsung drivers to make ODROID-U3 and Chromebooks usable > > * Enable big.LITTLE > > * MCPM > [...] > > +CONFIG_BIG_LITTLE=y > > +CONFIG_BL_SWITCHER=y > > IIUC, this will enable switcher code by default. I am not sure if this > is intentional ? E.g.: After this I can have only 2 active cpus instead > of 5 on my Vexpress TC2 platform. > > IMO we can keep this enabled by default in the build, but disabled > by default on boot. TC2 has a big.LITTLE processor and the switcher is the only mainlined way of making any kind of proper use of big.LITTLE, so why not have it enabled by default? > One way to achieve this: > (There's sysfs to re-enable it runtime) The opposite is also true, if you don't want the switcher enabled you can disable it by the same method after boot ;-) > -->8 > diff --git a/arch/arm/common/bL_switcher.c b/arch/arm/common/bL_switcher.c > index 490f3dced749..f4c36e70166a 100644 > --- a/arch/arm/common/bL_switcher.c > +++ b/arch/arm/common/bL_switcher.c > @@ -794,7 +794,7 @@ static int bL_switcher_hotplug_callback(struct > notifier_block *nfb, > return NOTIFY_DONE; > } > > -static bool no_bL_switcher; > +static bool no_bL_switcher = true; This changes the default for everyone, which I guess is fair enough if there is a good reason, but I'm not sure there is.
On Fri, Aug 1, 2014 at 4:01 AM, Jon Medhurst (Tixy) <tixy@linaro.org> wrote: > On Fri, 2014-08-01 at 11:26 +0100, Sudeep Holla wrote: >> >> On 22/07/14 19:01, Olof Johansson wrote: >> > This is a major refresh of the multi_v7_defconfig: >> > >> > - Bring over a bunch of Samsung drivers to make ODROID-U3 and Chromebooks usable >> > * Enable big.LITTLE >> > * MCPM >> [...] > >> > +CONFIG_BIG_LITTLE=y >> > +CONFIG_BL_SWITCHER=y >> >> IIUC, this will enable switcher code by default. I am not sure if this >> is intentional ? E.g.: After this I can have only 2 active cpus instead >> of 5 on my Vexpress TC2 platform. >> >> IMO we can keep this enabled by default in the build, but disabled >> by default on boot. > > TC2 has a big.LITTLE processor and the switcher is the only mainlined > way of making any kind of proper use of big.LITTLE, so why not have it > enabled by default? +1. > >> One way to achieve this: >> (There's sysfs to re-enable it runtime) > > The opposite is also true, if you don't want the switcher enabled you > can disable it by the same method after boot ;-) > >> -->8 >> diff --git a/arch/arm/common/bL_switcher.c b/arch/arm/common/bL_switcher.c >> index 490f3dced749..f4c36e70166a 100644 >> --- a/arch/arm/common/bL_switcher.c >> +++ b/arch/arm/common/bL_switcher.c >> @@ -794,7 +794,7 @@ static int bL_switcher_hotplug_callback(struct >> notifier_block *nfb, >> return NOTIFY_DONE; >> } >> >> -static bool no_bL_switcher; >> +static bool no_bL_switcher = true; > > This changes the default for everyone, which I guess is fair enough if > there is a good reason, but I'm not sure there is. No, I don't think there is. -Olof
On Fri, 2014-08-01 at 12:01 +0100, Jon Medhurst (Tixy) wrote: > > -->8 > > diff --git a/arch/arm/common/bL_switcher.c b/arch/arm/common/bL_switcher.c > > index 490f3dced749..f4c36e70166a 100644 > > --- a/arch/arm/common/bL_switcher.c > > +++ b/arch/arm/common/bL_switcher.c > > @@ -794,7 +794,7 @@ static int bL_switcher_hotplug_callback(struct > > notifier_block *nfb, > > return NOTIFY_DONE; > > } > > > > -static bool no_bL_switcher; > > +static bool no_bL_switcher = true; > > This changes the default for everyone, which I guess is fair enough if > there is a good reason, but I'm not sure there is. I would vote for this change and argue that b.L switcher is "strange enough" (ie. I know I've got 5 processors, so why do I see only 2?) _not_ to have it enabled by default. I'd hope that a user looking for this functionality will know how to turn it on. Paweł
On 01/08/14 12:03, Olof Johansson wrote: > On Fri, Aug 1, 2014 at 4:01 AM, Jon Medhurst (Tixy) <tixy@linaro.org> wrote: >> On Fri, 2014-08-01 at 11:26 +0100, Sudeep Holla wrote: >>> >>> On 22/07/14 19:01, Olof Johansson wrote: >>>> This is a major refresh of the multi_v7_defconfig: >>>> >>>> - Bring over a bunch of Samsung drivers to make ODROID-U3 and Chromebooks usable >>>> * Enable big.LITTLE >>>> * MCPM >>> [...] >> >>>> +CONFIG_BIG_LITTLE=y >>>> +CONFIG_BL_SWITCHER=y >>> >>> IIUC, this will enable switcher code by default. I am not sure if this >>> is intentional ? E.g.: After this I can have only 2 active cpus instead >>> of 5 on my Vexpress TC2 platform. >>> >>> IMO we can keep this enabled by default in the build, but disabled >>> by default on boot. >> >> TC2 has a big.LITTLE processor and the switcher is the only mainlined >> way of making any kind of proper use of big.LITTLE, so why not have it >> enabled by default? > > +1. > >> >>> One way to achieve this: >>> (There's sysfs to re-enable it runtime) >> >> The opposite is also true, if you don't want the switcher enabled you >> can disable it by the same method after boot ;-) >> >>> -->8 >>> diff --git a/arch/arm/common/bL_switcher.c b/arch/arm/common/bL_switcher.c >>> index 490f3dced749..f4c36e70166a 100644 >>> --- a/arch/arm/common/bL_switcher.c >>> +++ b/arch/arm/common/bL_switcher.c >>> @@ -794,7 +794,7 @@ static int bL_switcher_hotplug_callback(struct >>> notifier_block *nfb, >>> return NOTIFY_DONE; >>> } >>> >>> -static bool no_bL_switcher; >>> +static bool no_bL_switcher = true; >> >> This changes the default for everyone, which I guess is fair enough if >> there is a good reason, but I'm not sure there is. > > No, I don't think there is. > It's just that people using TC2 will suddenly see 3 of the 5 CPUs missing. Regards, Sudeep
On Fri, 2014-08-01 at 12:12 +0100, Sudeep Holla wrote: > > On 01/08/14 12:03, Olof Johansson wrote: > > On Fri, Aug 1, 2014 at 4:01 AM, Jon Medhurst (Tixy) <tixy@linaro.org> wrote: > >> On Fri, 2014-08-01 at 11:26 +0100, Sudeep Holla wrote: > >>> > >>> On 22/07/14 19:01, Olof Johansson wrote: > >>>> This is a major refresh of the multi_v7_defconfig: > >>>> > >>>> - Bring over a bunch of Samsung drivers to make ODROID-U3 and Chromebooks usable > >>>> * Enable big.LITTLE > >>>> * MCPM > >>> [...] > >> > >>>> +CONFIG_BIG_LITTLE=y > >>>> +CONFIG_BL_SWITCHER=y > >>> > >>> IIUC, this will enable switcher code by default. I am not sure if this > >>> is intentional ? E.g.: After this I can have only 2 active cpus instead > >>> of 5 on my Vexpress TC2 platform. > >>> > >>> IMO we can keep this enabled by default in the build, but disabled > >>> by default on boot. > >> > >> TC2 has a big.LITTLE processor and the switcher is the only mainlined > >> way of making any kind of proper use of big.LITTLE, so why not have it > >> enabled by default? > > > > +1. > > > >> > >>> One way to achieve this: > >>> (There's sysfs to re-enable it runtime) > >> > >> The opposite is also true, if you don't want the switcher enabled you > >> can disable it by the same method after boot ;-) > >> > >>> -->8 > >>> diff --git a/arch/arm/common/bL_switcher.c b/arch/arm/common/bL_switcher.c > >>> index 490f3dced749..f4c36e70166a 100644 > >>> --- a/arch/arm/common/bL_switcher.c > >>> +++ b/arch/arm/common/bL_switcher.c > >>> @@ -794,7 +794,7 @@ static int bL_switcher_hotplug_callback(struct > >>> notifier_block *nfb, > >>> return NOTIFY_DONE; > >>> } > >>> > >>> -static bool no_bL_switcher; > >>> +static bool no_bL_switcher = true; > >> > >> This changes the default for everyone, which I guess is fair enough if > >> there is a good reason, but I'm not sure there is. > > > > No, I don't think there is. > > > > It's just that people using TC2 will suddenly see 3 of the 5 CPUs missing. Yes, if they we're previously using multi_v7_defconfig (do people working specifically with TC2's use that?) Conversely, with the change in default proposed above, anyone with their own configs enabling the switcher will suddenly see the number of CPUs go from 2 to 5. We also have the situation where we have a config option, which when enabled, doesn't actually do anything unless the user also changes boot arguments or takes measures to enable it after boot. Which seems the wrong way for things to work to me. I believe that if we don't want the switcher enabled in kernels built with multi_v7_defconfig, then it should be done by not adding the config option to multi_v7_defconfig in the first place.
On 01/08/14 12:28, Jon Medhurst (Tixy) wrote: > On Fri, 2014-08-01 at 12:12 +0100, Sudeep Holla wrote: >> >> On 01/08/14 12:03, Olof Johansson wrote: >>> On Fri, Aug 1, 2014 at 4:01 AM, Jon Medhurst (Tixy) <tixy@linaro.org> wrote: >>>> On Fri, 2014-08-01 at 11:26 +0100, Sudeep Holla wrote: [...] >>>> >>>>> One way to achieve this: >>>>> (There's sysfs to re-enable it runtime) >>>> >>>> The opposite is also true, if you don't want the switcher enabled you >>>> can disable it by the same method after boot ;-) >>>> >>>>> -->8 >>>>> diff --git a/arch/arm/common/bL_switcher.c b/arch/arm/common/bL_switcher.c >>>>> index 490f3dced749..f4c36e70166a 100644 >>>>> --- a/arch/arm/common/bL_switcher.c >>>>> +++ b/arch/arm/common/bL_switcher.c >>>>> @@ -794,7 +794,7 @@ static int bL_switcher_hotplug_callback(struct >>>>> notifier_block *nfb, >>>>> return NOTIFY_DONE; >>>>> } >>>>> >>>>> -static bool no_bL_switcher; >>>>> +static bool no_bL_switcher = true; >>>> >>>> This changes the default for everyone, which I guess is fair enough if >>>> there is a good reason, but I'm not sure there is. >>> >>> No, I don't think there is. >>> >> >> It's just that people using TC2 will suddenly see 3 of the 5 CPUs missing. > > Yes, if they we're previously using multi_v7_defconfig (do people > working specifically with TC2's use that?) > I don't, but assumed many might use it. > Conversely, with the change in default proposed above, anyone with their > own configs enabling the switcher will suddenly see the number of CPUs > go from 2 to 5. We also have the situation where we have a config > option, which when enabled, doesn't actually do anything unless the user > also changes boot arguments or takes measures to enable it after boot. > Which seems the wrong way for things to work to me. > OK, makes sense. Just curious how many big.LITTLE platforms have CPUFreq support and integrated with bL switcher. Otherwise we end up switching clusters/cpus using dummy i/f anyways(and probably that's the reason why that config is enabled which I missed to understand initially as I was thinking it's more for development and testing only). If is that's acceptable for those platforms, then it should be fine I believe ? Regards, Sudeep
On Fri, 2014-08-01 at 15:57 +0100, Sudeep Holla wrote: > > On 01/08/14 12:28, Jon Medhurst (Tixy) wrote: > > On Fri, 2014-08-01 at 12:12 +0100, Sudeep Holla wrote: > >> > >> On 01/08/14 12:03, Olof Johansson wrote: > >>> On Fri, Aug 1, 2014 at 4:01 AM, Jon Medhurst (Tixy) <tixy@linaro.org> wrote: > >>>> On Fri, 2014-08-01 at 11:26 +0100, Sudeep Holla wrote: > > [...] > > >>>> > >>>>> One way to achieve this: > >>>>> (There's sysfs to re-enable it runtime) > >>>> > >>>> The opposite is also true, if you don't want the switcher enabled you > >>>> can disable it by the same method after boot ;-) > >>>> > >>>>> -->8 > >>>>> diff --git a/arch/arm/common/bL_switcher.c b/arch/arm/common/bL_switcher.c > >>>>> index 490f3dced749..f4c36e70166a 100644 > >>>>> --- a/arch/arm/common/bL_switcher.c > >>>>> +++ b/arch/arm/common/bL_switcher.c > >>>>> @@ -794,7 +794,7 @@ static int bL_switcher_hotplug_callback(struct > >>>>> notifier_block *nfb, > >>>>> return NOTIFY_DONE; > >>>>> } > >>>>> > >>>>> -static bool no_bL_switcher; > >>>>> +static bool no_bL_switcher = true; > >>>> > >>>> This changes the default for everyone, which I guess is fair enough if > >>>> there is a good reason, but I'm not sure there is. > >>> > >>> No, I don't think there is. > >>> > >> > >> It's just that people using TC2 will suddenly see 3 of the 5 CPUs missing. > > > > Yes, if they we're previously using multi_v7_defconfig (do people > > working specifically with TC2's use that?) > > > > I don't, but assumed many might use it. > > > Conversely, with the change in default proposed above, anyone with their > > own configs enabling the switcher will suddenly see the number of CPUs > > go from 2 to 5. We also have the situation where we have a config > > option, which when enabled, doesn't actually do anything unless the user > > also changes boot arguments or takes measures to enable it after boot. > > Which seems the wrong way for things to work to me. > > > > OK, makes sense. Just curious how many big.LITTLE platforms have CPUFreq > support and integrated with bL switcher. Otherwise we end up switching > clusters/cpus using dummy i/f anyways Hmm, that is a point, there are 3 other big.LITTLE SoC's I can spot in mainline [1], and I wouldn't want to speculate how they would be affected by having the big.LITTLE switcher enabled. [1] exynos5420, exynos5260, r8a7790
On Fri, Aug 01, 2014 at 04:53:19PM +0100, Jon Medhurst (Tixy) wrote: > On Fri, 2014-08-01 at 15:57 +0100, Sudeep Holla wrote: > > > > On 01/08/14 12:28, Jon Medhurst (Tixy) wrote: > > > On Fri, 2014-08-01 at 12:12 +0100, Sudeep Holla wrote: > > >> > > >> On 01/08/14 12:03, Olof Johansson wrote: > > >>> On Fri, Aug 1, 2014 at 4:01 AM, Jon Medhurst (Tixy) <tixy@linaro.org> wrote: > > >>>> On Fri, 2014-08-01 at 11:26 +0100, Sudeep Holla wrote: > > > > [...] > > > > >>>> > > >>>>> One way to achieve this: > > >>>>> (There's sysfs to re-enable it runtime) > > >>>> > > >>>> The opposite is also true, if you don't want the switcher enabled you > > >>>> can disable it by the same method after boot ;-) > > >>>> > > >>>>> -->8 > > >>>>> diff --git a/arch/arm/common/bL_switcher.c b/arch/arm/common/bL_switcher.c > > >>>>> index 490f3dced749..f4c36e70166a 100644 > > >>>>> --- a/arch/arm/common/bL_switcher.c > > >>>>> +++ b/arch/arm/common/bL_switcher.c > > >>>>> @@ -794,7 +794,7 @@ static int bL_switcher_hotplug_callback(struct > > >>>>> notifier_block *nfb, > > >>>>> return NOTIFY_DONE; > > >>>>> } > > >>>>> > > >>>>> -static bool no_bL_switcher; > > >>>>> +static bool no_bL_switcher = true; > > >>>> > > >>>> This changes the default for everyone, which I guess is fair enough if > > >>>> there is a good reason, but I'm not sure there is. > > >>> > > >>> No, I don't think there is. > > >>> > > >> > > >> It's just that people using TC2 will suddenly see 3 of the 5 CPUs missing. > > > > > > Yes, if they we're previously using multi_v7_defconfig (do people > > > working specifically with TC2's use that?) > > > > > > > I don't, but assumed many might use it. > > > > > Conversely, with the change in default proposed above, anyone with their > > > own configs enabling the switcher will suddenly see the number of CPUs > > > go from 2 to 5. We also have the situation where we have a config > > > option, which when enabled, doesn't actually do anything unless the user > > > also changes boot arguments or takes measures to enable it after boot. > > > Which seems the wrong way for things to work to me. > > > > > > > OK, makes sense. Just curious how many big.LITTLE platforms have CPUFreq > > support and integrated with bL switcher. Otherwise we end up switching > > clusters/cpus using dummy i/f anyways > > Hmm, that is a point, there are 3 other big.LITTLE SoC's I can spot in > mainline [1], and I wouldn't want to speculate how they would be > affected by having the big.LITTLE switcher enabled. > > [1] exynos5420, exynos5260, r8a7790 5422/5800 is a fourth SoC, but it's nearly identical to 5420 with a few bugfixes. At the end of the day, b.L switcher has predictable user behavior, while running with assymetric SMP does not. Until the scheduler work has been done, it is significantly better to enable the switcher instead of SMP on these platforms. Once the scheduler work has come further, we can switch over. But not until then. -Olof
On Fri, Aug 01, 2014 at 12:06:13PM +0100, Pawel Moll wrote: > On Fri, 2014-08-01 at 12:01 +0100, Jon Medhurst (Tixy) wrote: > > > -->8 > > > diff --git a/arch/arm/common/bL_switcher.c b/arch/arm/common/bL_switcher.c > > > index 490f3dced749..f4c36e70166a 100644 > > > --- a/arch/arm/common/bL_switcher.c > > > +++ b/arch/arm/common/bL_switcher.c > > > @@ -794,7 +794,7 @@ static int bL_switcher_hotplug_callback(struct > > > notifier_block *nfb, > > > return NOTIFY_DONE; > > > } > > > > > > -static bool no_bL_switcher; > > > +static bool no_bL_switcher = true; > > > > This changes the default for everyone, which I guess is fair enough if > > there is a good reason, but I'm not sure there is. > > I would vote for this change and argue that b.L switcher is "strange > enough" (ie. I know I've got 5 processors, so why do I see only 2?) > _not_ to have it enabled by default. I'd hope that a user looking for > this functionality will know how to turn it on. I strongly disagree. Today nearly all products that ship have a switcher enabled, since the scheduler work has not been done yet upstream. Having a platform that behaves undeterministically because the scheduler can't tell the core types apart and makes bad decisions is better than someone not finding the right number of CPUs under /proc/cpuinfo. And the same goes in the other direction: Any user looking to test out the scheduler work will know how to turn it on. The only other argument I'm willing to have here is if you want to somehow control this per platform and add a hook on vexpress that turns off the switcher by default. The rest of them should have it on. -Olof
Olof Johansson <olof@lixom.net> writes: > On Fri, Aug 1, 2014 at 4:01 AM, Jon Medhurst (Tixy) <tixy@linaro.org> wrote: >> On Fri, 2014-08-01 at 11:26 +0100, Sudeep Holla wrote: >>> >>> On 22/07/14 19:01, Olof Johansson wrote: >>> > This is a major refresh of the multi_v7_defconfig: >>> > >>> > - Bring over a bunch of Samsung drivers to make ODROID-U3 and Chromebooks usable >>> > * Enable big.LITTLE >>> > * MCPM >>> [...] >> >>> > +CONFIG_BIG_LITTLE=y >>> > +CONFIG_BL_SWITCHER=y >>> >>> IIUC, this will enable switcher code by default. I am not sure if this >>> is intentional ? E.g.: After this I can have only 2 active cpus instead >>> of 5 on my Vexpress TC2 platform. >>> >>> IMO we can keep this enabled by default in the build, but disabled >>> by default on boot. >> >> TC2 has a big.LITTLE processor and the switcher is the only mainlined >> way of making any kind of proper use of big.LITTLE, so why not have it >> enabled by default? > > +1. > I disagree. The bL switcher is a stopgap used on products (which have their own defconfigs anyways) but upstream development is focused on HMP (or whatever the current buzzword is for the kernel directly scheduling both big and little cores.) So IMO, for upstream coverage, we should *not* be enabling the switcher but should be letting the scheduler directly manage all CPUs. At least on Exynos, with MCPM support merged, the kernel can boot up all the CPUs and directly manage them. Kevin
Olof Johansson <olof@lixom.net> writes: > On Fri, Aug 01, 2014 at 04:53:19PM +0100, Jon Medhurst (Tixy) wrote: >> On Fri, 2014-08-01 at 15:57 +0100, Sudeep Holla wrote: >> > >> > On 01/08/14 12:28, Jon Medhurst (Tixy) wrote: >> > > On Fri, 2014-08-01 at 12:12 +0100, Sudeep Holla wrote: >> > >> >> > >> On 01/08/14 12:03, Olof Johansson wrote: >> > >>> On Fri, Aug 1, 2014 at 4:01 AM, Jon Medhurst (Tixy) <tixy@linaro.org> wrote: >> > >>>> On Fri, 2014-08-01 at 11:26 +0100, Sudeep Holla wrote: >> > >> > [...] >> > >> > >>>> >> > >>>>> One way to achieve this: >> > >>>>> (There's sysfs to re-enable it runtime) >> > >>>> >> > >>>> The opposite is also true, if you don't want the switcher enabled you >> > >>>> can disable it by the same method after boot ;-) >> > >>>> >> > >>>>> -->8 >> > >>>>> diff --git a/arch/arm/common/bL_switcher.c b/arch/arm/common/bL_switcher.c >> > >>>>> index 490f3dced749..f4c36e70166a 100644 >> > >>>>> --- a/arch/arm/common/bL_switcher.c >> > >>>>> +++ b/arch/arm/common/bL_switcher.c >> > >>>>> @@ -794,7 +794,7 @@ static int bL_switcher_hotplug_callback(struct >> > >>>>> notifier_block *nfb, >> > >>>>> return NOTIFY_DONE; >> > >>>>> } >> > >>>>> >> > >>>>> -static bool no_bL_switcher; >> > >>>>> +static bool no_bL_switcher = true; >> > >>>> >> > >>>> This changes the default for everyone, which I guess is fair enough if >> > >>>> there is a good reason, but I'm not sure there is. >> > >>> >> > >>> No, I don't think there is. >> > >>> >> > >> >> > >> It's just that people using TC2 will suddenly see 3 of the 5 CPUs missing. >> > > >> > > Yes, if they we're previously using multi_v7_defconfig (do people >> > > working specifically with TC2's use that?) >> > > >> > >> > I don't, but assumed many might use it. >> > >> > > Conversely, with the change in default proposed above, anyone with their >> > > own configs enabling the switcher will suddenly see the number of CPUs >> > > go from 2 to 5. We also have the situation where we have a config >> > > option, which when enabled, doesn't actually do anything unless the user >> > > also changes boot arguments or takes measures to enable it after boot. >> > > Which seems the wrong way for things to work to me. >> > > >> > >> > OK, makes sense. Just curious how many big.LITTLE platforms have CPUFreq >> > support and integrated with bL switcher. Otherwise we end up switching >> > clusters/cpus using dummy i/f anyways >> >> Hmm, that is a point, there are 3 other big.LITTLE SoC's I can spot in >> mainline [1], and I wouldn't want to speculate how they would be >> affected by having the big.LITTLE switcher enabled. >> >> [1] exynos5420, exynos5260, r8a7790 > > 5422/5800 is a fourth SoC, but it's nearly identical to 5420 with a few > bugfixes. > > At the end of the day, b.L switcher has predictable user behavior, while > running with assymetric SMP does not. What is unpredictable? Perhaps sub-optimal, but I don't see what's any more unpreditable about it than normal SMP. > Until the scheduler work has been done, > it is significantly better to enable the switcher instead of SMP on these > platforms. > > Once the scheduler work has come further, we can switch over. But not until > then. I think the upstream defconfigs should facilitate the broader testing of the scheduler work by having the swticher disabled. By enabling the switcher (and the corresponding cpufreq switcher driver) by default, we're now actually making one more obstacle for broader testing of the generic scheduling on all cores. Kevin
On Fri, 8 Aug 2014, Kevin Hilman wrote: > Olof Johansson <olof@lixom.net> writes: > > > On Fri, Aug 1, 2014 at 4:01 AM, Jon Medhurst (Tixy) <tixy@linaro.org> wrote: > >> On Fri, 2014-08-01 at 11:26 +0100, Sudeep Holla wrote: > >>> > >>> On 22/07/14 19:01, Olof Johansson wrote: > >>> > This is a major refresh of the multi_v7_defconfig: > >>> > > >>> > - Bring over a bunch of Samsung drivers to make ODROID-U3 and Chromebooks usable > >>> > * Enable big.LITTLE > >>> > * MCPM > >>> [...] > >> > >>> > +CONFIG_BIG_LITTLE=y > >>> > +CONFIG_BL_SWITCHER=y > >>> > >>> IIUC, this will enable switcher code by default. I am not sure if this > >>> is intentional ? E.g.: After this I can have only 2 active cpus instead > >>> of 5 on my Vexpress TC2 platform. > >>> > >>> IMO we can keep this enabled by default in the build, but disabled > >>> by default on boot. > >> > >> TC2 has a big.LITTLE processor and the switcher is the only mainlined > >> way of making any kind of proper use of big.LITTLE, so why not have it > >> enabled by default? > > > > +1. > > > > I disagree. > > The bL switcher is a stopgap used on products (which have their own > defconfigs anyways) but upstream development is focused on HMP (or > whatever the current buzzword is for the kernel directly scheduling both > big and little cores.) > > So IMO, for upstream coverage, we should *not* be enabling the switcher > but should be letting the scheduler directly manage all CPUs. I agree... in principle. In practice the upstream scheduler has no notion of asymmetric processing yet, and probably still for a while to come. Once the scheduler does a semi-descent job at it then the switcher should default to being disabled. > At least on Exynos, with MCPM support merged, the kernel can boot up all > the CPUs and directly manage them. "Managing" them is still somewhat overstated. Yes, the scheduler does _see_ them but it still can't manage them properly. At best you'll get somewhat random system performance, at worst it'll be completely inefficient. In most cases the switcher will give you much better behavior for the time being. Nicolas
On Fri, 8 Aug 2014, Kevin Hilman wrote: > Olof Johansson <olof@lixom.net> writes: > > > On Fri, Aug 01, 2014 at 04:53:19PM +0100, Jon Medhurst (Tixy) wrote: > >> On Fri, 2014-08-01 at 15:57 +0100, Sudeep Holla wrote: > >> > > >> > On 01/08/14 12:28, Jon Medhurst (Tixy) wrote: > >> > > On Fri, 2014-08-01 at 12:12 +0100, Sudeep Holla wrote: > >> > >> > >> > >> On 01/08/14 12:03, Olof Johansson wrote: > >> > >>> On Fri, Aug 1, 2014 at 4:01 AM, Jon Medhurst (Tixy) <tixy@linaro.org> wrote: > >> > >>>> On Fri, 2014-08-01 at 11:26 +0100, Sudeep Holla wrote: > >> > > >> > [...] > >> > > >> > >>>> > >> > >>>>> One way to achieve this: > >> > >>>>> (There's sysfs to re-enable it runtime) > >> > >>>> > >> > >>>> The opposite is also true, if you don't want the switcher enabled you > >> > >>>> can disable it by the same method after boot ;-) > >> > >>>> > >> > >>>>> -->8 > >> > >>>>> diff --git a/arch/arm/common/bL_switcher.c b/arch/arm/common/bL_switcher.c > >> > >>>>> index 490f3dced749..f4c36e70166a 100644 > >> > >>>>> --- a/arch/arm/common/bL_switcher.c > >> > >>>>> +++ b/arch/arm/common/bL_switcher.c > >> > >>>>> @@ -794,7 +794,7 @@ static int bL_switcher_hotplug_callback(struct > >> > >>>>> notifier_block *nfb, > >> > >>>>> return NOTIFY_DONE; > >> > >>>>> } > >> > >>>>> > >> > >>>>> -static bool no_bL_switcher; > >> > >>>>> +static bool no_bL_switcher = true; > >> > >>>> > >> > >>>> This changes the default for everyone, which I guess is fair enough if > >> > >>>> there is a good reason, but I'm not sure there is. > >> > >>> > >> > >>> No, I don't think there is. > >> > >>> > >> > >> > >> > >> It's just that people using TC2 will suddenly see 3 of the 5 CPUs missing. > >> > > > >> > > Yes, if they we're previously using multi_v7_defconfig (do people > >> > > working specifically with TC2's use that?) > >> > > > >> > > >> > I don't, but assumed many might use it. > >> > > >> > > Conversely, with the change in default proposed above, anyone with their > >> > > own configs enabling the switcher will suddenly see the number of CPUs > >> > > go from 2 to 5. We also have the situation where we have a config > >> > > option, which when enabled, doesn't actually do anything unless the user > >> > > also changes boot arguments or takes measures to enable it after boot. > >> > > Which seems the wrong way for things to work to me. > >> > > > >> > > >> > OK, makes sense. Just curious how many big.LITTLE platforms have CPUFreq > >> > support and integrated with bL switcher. Otherwise we end up switching > >> > clusters/cpus using dummy i/f anyways > >> > >> Hmm, that is a point, there are 3 other big.LITTLE SoC's I can spot in > >> mainline [1], and I wouldn't want to speculate how they would be > >> affected by having the big.LITTLE switcher enabled. > >> > >> [1] exynos5420, exynos5260, r8a7790 > > > > 5422/5800 is a fourth SoC, but it's nearly identical to 5420 with a few > > bugfixes. > > > > At the end of the day, b.L switcher has predictable user behavior, while > > running with assymetric SMP does not. > > What is unpredictable? Perhaps sub-optimal, but I don't see what's any > more unpreditable about it than normal SMP. CPUs not being equal, your overall performance will be randomized depending on which CPU a given task ends up being scheduled on. And in turn this is going to skew scheduler profiling and statistics gathering for that task if it is migrated around. Some performance critical tasks might never get the boost from a big CPU while other tasks will needlessly drain your battery by not being run on a small CPU. People will complain and the squirrel across the street will steal your SD cards, etc. > > Until the scheduler work has been done, > > it is significantly better to enable the switcher instead of SMP on these > > platforms. > > > > Once the scheduler work has come further, we can switch over. But not until > > then. > > I think the upstream defconfigs should facilitate the broader testing of > the scheduler work by having the swticher disabled. > > By enabling the switcher (and the corresponding cpufreq switcher driver) > by default, we're now actually making one more obstacle for broader > testing of the generic scheduling on all cores. The generic scheduler currently has nothing really worth testing by the wider audience. Be assured that when it does then the switcher will indeed default to off. Nicolas
On Fri, Aug 8, 2014 at 11:42 PM, Kevin Hilman <khilman@linaro.org> wrote: > Olof Johansson <olof@lixom.net> writes: > >> >> At the end of the day, b.L switcher has predictable user behavior, while >> running with assymetric SMP does not. > > What is unpredictable? Perhaps sub-optimal, but I don't see what's any > more unpreditable about it than normal SMP. > >> Until the scheduler work has been done, >> it is significantly better to enable the switcher instead of SMP on these >> platforms. >> >> Once the scheduler work has come further, we can switch over. But not until >> then. > > I think the upstream defconfigs should facilitate the broader testing of > the scheduler work by having the swticher disabled. I thought the upstream defconfigs were meant to make machines work out of the box. Any development effort can surely tweak the configs to their needs. > By enabling the switcher (and the corresponding cpufreq switcher driver) > by default, we're now actually making one more obstacle for broader > testing of the generic scheduling on all cores. Actually, there might some advantages to turning on the switcher. 1. The performance/power numbers of the b.L switcher is the minimum we need to achieve on a scheduler-driven b.L system. It establishes a baseline. Shame on us working on EAS if the switcher does better. 2. The switcher exercises the cpufreq drivers quite a bit. So any niggles will be ironed out on platforms where it hasn't been well tested. Regards, Amit
On Fri, Aug 8, 2014 at 11:26 AM, Nicolas Pitre <nicolas.pitre@linaro.org> wrote: > On Fri, 8 Aug 2014, Kevin Hilman wrote: [...] >> I think the upstream defconfigs should facilitate the broader testing of >> the scheduler work by having the swticher disabled. >> >> By enabling the switcher (and the corresponding cpufreq switcher driver) >> by default, we're now actually making one more obstacle for broader >> testing of the generic scheduling on all cores. > > The generic scheduler currently has nothing really worth testing by the > wider audience. Be assured that when it does then the switcher will > indeed default to off. I guess my vacation successfully purged my memory. I was thinking the switcher was only a complie-time option, but just realized that it can be disabled at runtime (via /sys/kernel/bL_switcher/active.) With the runtime enable/disable feature of the switcher, I don't have any more (reasonable) objections to it being enabled by default. Kevin
diff --git a/arch/arm/common/bL_switcher.c b/arch/arm/common/bL_switcher.c index 490f3dced749..f4c36e70166a 100644 --- a/arch/arm/common/bL_switcher.c +++ b/arch/arm/common/bL_switcher.c @@ -794,7 +794,7 @@ static int bL_switcher_hotplug_callback(struct notifier_block *nfb, return NOTIFY_DONE; } -static bool no_bL_switcher; +static bool no_bL_switcher = true; core_param(no_bL_switcher, no_bL_switcher, bool, 0644);