Compile-time configuration is messy and precludes multi-platform kernels so remove it. Task packing should instead be tuned at run-time, e.g. in user-side boot scripts, with something like:
echo 1 > /sys/kernel/hmp/packing_enable echo 650 > /sys/kernel/hmp/packing_limit
Signed-off-by: Jon Medhurst tixy@linaro.org ---
Mark and Alex, please don't apply this directly. Assuming that this change doesn't provoke any dissent, it can be pulled from the usual place... git://git.linaro.org/arm/big.LITTLE/mp.git for-lsk
kernel/sched/fair.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 43857fe..6610622 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -3676,15 +3676,8 @@ unsigned int hmp_next_up_threshold = 4096; unsigned int hmp_next_down_threshold = 4096;
#ifdef CONFIG_SCHED_HMP_LITTLE_PACKING -#ifndef CONFIG_ARCH_VEXPRESS_TC2 unsigned int hmp_packing_enabled = 1; unsigned int hmp_full_threshold = (NICE_0_LOAD * 9) / 8; -#else -/* TC2 has a sharp consumption curve @ around 800Mhz, so - we aim to spread the load around that frequency. */ -unsigned int hmp_packing_enabled; -unsigned int hmp_full_threshold = 650; /* 80% of the 800Mhz freq * NICE_0_LOAD */ -#endif #endif
static unsigned int hmp_up_migration(int cpu, int *target_cpu, struct sched_entity *se);
Wouldn't it have been better to just revert the patch that added the platform specific pre-processing?
What you are doing now is introducing a second config change into LSK...
The revert would have looked like this:
#ifdef CONFIG_SCHED_HMP_LITTLE_PACKING -#ifndef CONFIG_ARCH_VEXPRESS_TC2 unsigned int hmp_packing_enabled = 1; +#ifndef CONFIG_ARCH_VEXPRESS_TC2 unsigned int hmp_full_threshold = (NICE_0_LOAD * 9) / 8; #else /* TC2 has a sharp consumption curve @ around 800Mhz, so we aim to spread the load around that frequency. */ -unsigned int hmp_packing_enabled; unsigned int hmp_full_threshold = 650; /* 80% of the 800Mhz freq * NICE_0_LOAD */ #endif #endif
Mark
-----Original Message----- From: Jon Medhurst (Tixy) [mailto:tixy@linaro.org] Sent: 01 May 2014 14:37 To: Mark Brown; Alex Shi Cc: Linaro Kernel; Chris Redpath; Robin Randhawa; Mark Hambleton Subject: [PATCH] sched: hmp: Remove compile-time configuration of task packing
Compile-time configuration is messy and precludes multi-platform kernels so remove it. Task packing should instead be tuned at run-time, e.g. in user-side boot scripts, with something like:
echo 1 > /sys/kernel/hmp/packing_enable echo 650 > /sys/kernel/hmp/packing_limit
Signed-off-by: Jon Medhurst tixy@linaro.org
Mark and Alex, please don't apply this directly. Assuming that this change doesn't provoke any dissent, it can be pulled from the usual place... git://git.linaro.org/arm/big.LITTLE/mp.git for-lsk
kernel/sched/fair.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index 43857fe..6610622 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -3676,15 +3676,8 @@ unsigned int hmp_next_up_threshold = 4096; unsigned int hmp_next_down_threshold = 4096;
#ifdef CONFIG_SCHED_HMP_LITTLE_PACKING -#ifndef CONFIG_ARCH_VEXPRESS_TC2 unsigned int hmp_packing_enabled = 1; unsigned int hmp_full_threshold = (NICE_0_LOAD * 9) / 8; -#else -/* TC2 has a sharp consumption curve @ around 800Mhz, so
- we aim to spread the load around that frequency. */
-unsigned int hmp_packing_enabled; -unsigned int hmp_full_threshold = 650; /* 80% of the 800Mhz freq * NICE_0_LOAD */ -#endif #endif
static unsigned int hmp_up_migration(int cpu, int *target_cpu, struct sched_entity *se); -- 1.7.10.4
On Thu, 2014-05-01 at 13:41 +0000, Mark Hambleton wrote:
Wouldn't it have been better to just revert the patch that added the platform specific pre-processing?
I confused. The platform specific pre-processing was part of the original commit d8063e7015 (HMP: Implement task packing for small tasks in HMP systems)
This was then modified by commit ba8ed8301f (sched: hmp: Change TC2 packing config to disabled default if present)
What you are doing now is introducing a second config change into LSK...
Only if you compile with TC2.
The revert would have looked like this:
Which just reverts the second commit I mentioned and still leaves TC2 specific pre-processing.
I guess what you're saying is that you also want the default for hmp_packing_enabled to change to 0, so your multi-platform kernels keep the behaviour they had before this proposed patch? But to do that would mean that everyone else who wasn't compiling with TC2 enabled in their kernel now getting a configuration change instead.
Guess that means then that we should just stick with the status quo then?
I personally don't care one way or the other, I'm just a piggy-in-the-middle patch monkey chimera who'd wish people would make up their minds what they want :-)
So if Mark H, Mark B, and Robin can come to a consensus on what the task packing config should be, then tell me...
On Thu, May 01, 2014 at 03:24:18PM +0100, Jon Medhurst (Tixy) wrote:
I personally don't care one way or the other, I'm just a piggy-in-the-middle patch monkey chimera who'd wish people would make up their minds what they want :-)
So if Mark H, Mark B, and Robin can come to a consensus on what the task packing config should be, then tell me...
I have no real opinion on what the best configuration is, I'm assuming this is intended as a per system tunable and that the main point of the change was to demonstrate best practice for doing the tuning better.
- PGP Signed by an unknown key
On Thu, May 01, 2014 at 03:24:18PM +0100, Jon Medhurst (Tixy) wrote:
I personally don't care one way or the other, I'm just a piggy-in-the-middle patch monkey chimera who'd wish people would make
up
their minds what they want :-)
So if Mark H, Mark B, and Robin can come to a consensus on what the task packing config should be, then tell me...
I have no real opinion on what the best configuration is, I'm assuming this is intended as a per system tunable and that the main point of the change was to demonstrate best practice for doing the tuning better.
My problem here is that this patch doesn't just revert the change that turns off task packing, it also reverts the config that most will probably have been building with...
hmp_full_threshold now changes from:
unsigned int hmp_full_threshold = 650; /* 80% of the 800Mhz freq * NICE_0_LOAD */
To:
unsigned int hmp_full_threshold = (NICE_0_LOAD * 9) / 8;
As I suspect most will be building with TC2 enabled.
Yes it is a tunable, and yes it can be set for a platform.. but it is just more messing about when this patch finally gets reverted.
We have already reverted the latest patch, but what would be cleaner is to revert the first patch and then add a second patch to remove the other bit of config and just set the value to the one that is most likely the most common default?
Mark
Hi Mark,
On 01/05/14 15:36, Mark Hambleton wrote:
- PGP Signed by an unknown key
On Thu, May 01, 2014 at 03:24:18PM +0100, Jon Medhurst (Tixy) wrote:
I personally don't care one way or the other, I'm just a piggy-in-the-middle patch monkey chimera who'd wish people would make
up
their minds what they want :-)
So if Mark H, Mark B, and Robin can come to a consensus on what the task packing config should be, then tell me...
I have no real opinion on what the best configuration is, I'm assuming this is intended as a per system tunable and that the main point of the change was to demonstrate best practice for doing the tuning better.
My problem here is that this patch doesn't just revert the change that turns off task packing, it also reverts the config that most will probably have been building with...
hmp_full_threshold now changes from:
unsigned int hmp_full_threshold = 650; /* 80% of the 800Mhz freq * NICE_0_LOAD */
To:
unsigned int hmp_full_threshold = (NICE_0_LOAD * 9) / 8;
As I suspect most will be building with TC2 enabled.
Yes it is a tunable, and yes it can be set for a platform.. but it is just more messing about when this patch finally gets reverted.
It's actually quite tricky to determine what the optimal value is as it depends where the voltage steps in the DVFS curve go. There isn't really a suitable default for all systems - the nice thing about the TC2 specific data was that it showed that you need to think about it and that you might not even want this feature.
On TC2 there are two problems with packing.
Firstly, the A7s use a lot more power when the first voltage change comes in the DVFS curve, as the comment says. This is why we did the threshold change - to keep the A7s below the first voltage step as much as possible.
If you assume that most of the DVFS governors try to keep utilization in the 80% range by changing frequency (mostly, by default) then you can try to keep the load on a packed CPU down this way but it isn't going to guarantee that you don't go over the target frequency. We only decide on packing at wakeup time, once you've decided then you are stuck with the decision until the next load balance or wakeup.
It turns out that it's difficult to test that it's doing the right thing every time as the packing conditions depend on instantaneous scheduler conditions and these are hard to control. Even when you're sure you are doing the right thing, if you've got synchronized test threads all active at the same time then the scheduler's idea of load is *runnable* time so you get an increase in apparent load from packing those threads together.
Secondly, I've never seen the TC2 A7 cluster power down fully in a trace. When you have one A7 on, hence the L2 etc. is on, the overhead of adding more A7s is low assuming the frequency is.
Anyway, the point is we need a default, 650 is as good as anything else for the reasons above. If it doesn't suit a platform, change it in the init script - you were supposed to do that anyway :)
We have already reverted the latest patch, but what would be cleaner is to revert the first patch and then add a second patch to remove the other bit of config and just set the value to the one that is most likely the most common default?
I haven't had any feedback what people are setting this to in order to decide what the most common value would be. All data points gratefully received.
Mark
--Chris
Fair enough, I just didn't particularly want yet another patch mucking around with the config.
A revert of the one that did the damage and then another patch to remove the other stuff (if you can't do 2 reverts) would be much nicer.
Mark
-----Original Message----- From: Chris Redpath [mailto:Chris.Redpath@arm.com] Sent: 01 May 2014 16:01 To: Mark Hambleton; Mark Brown; Jon Medhurst (Tixy) Cc: Linaro Kernel Subject: Re: [PATCH] sched: hmp: Remove compile-time configuration of task packing
Hi Mark,
On 01/05/14 15:36, Mark Hambleton wrote:
- PGP Signed by an unknown key
On Thu, May 01, 2014 at 03:24:18PM +0100, Jon Medhurst (Tixy) wrote:
I personally don't care one way or the other, I'm just a piggy-in-the-middle patch monkey chimera who'd wish people would
make
up
their minds what they want :-)
So if Mark H, Mark B, and Robin can come to a consensus on what the
task
packing config should be, then tell me...
I have no real opinion on what the best configuration is, I'm assuming this is intended as a per system tunable and that the main point of the change was to demonstrate best practice for doing the tuning better.
My problem here is that this patch doesn't just revert the change that turns
off task packing, it also reverts the config that most will probably have been building with...
hmp_full_threshold now changes from:
unsigned int hmp_full_threshold = 650; /* 80% of the 800Mhz freq *
NICE_0_LOAD */
To:
unsigned int hmp_full_threshold = (NICE_0_LOAD * 9) / 8;
As I suspect most will be building with TC2 enabled.
Yes it is a tunable, and yes it can be set for a platform.. but it is just more
messing about when this patch finally gets reverted.
It's actually quite tricky to determine what the optimal value is as it depends where the voltage steps in the DVFS curve go. There isn't really a suitable default for all systems - the nice thing about the TC2 specific data was that it showed that you need to think about it and that you might not even want this feature.
On TC2 there are two problems with packing.
Firstly, the A7s use a lot more power when the first voltage change comes in the DVFS curve, as the comment says. This is why we did the threshold change - to keep the A7s below the first voltage step as much as possible.
If you assume that most of the DVFS governors try to keep utilization in the 80% range by changing frequency (mostly, by default) then you can try to keep the load on a packed CPU down this way but it isn't going to guarantee that you don't go over the target frequency. We only decide on packing at wakeup time, once you've decided then you are stuck with the decision until the next load balance or wakeup.
It turns out that it's difficult to test that it's doing the right thing every time as the packing conditions depend on instantaneous scheduler conditions and these are hard to control. Even when you're sure you are doing the right thing, if you've got synchronized test threads all active at the same time then the scheduler's idea of load is *runnable* time so you get an increase in apparent load from packing those threads together.
Secondly, I've never seen the TC2 A7 cluster power down fully in a trace. When you have one A7 on, hence the L2 etc. is on, the overhead of adding more A7s is low assuming the frequency is.
Anyway, the point is we need a default, 650 is as good as anything else for the reasons above. If it doesn't suit a platform, change it in the init script - you were supposed to do that anyway :)
We have already reverted the latest patch, but what would be cleaner is to
revert the first patch and then add a second patch to remove the other bit of config and just set the value to the one that is most likely the most common default?
I haven't had any feedback what people are setting this to in order to decide what the most common value would be. All data points gratefully received.
Mark
--Chris
On Thu, May 01, 2014 at 01:41:37PM +0000, Mark Hambleton wrote:
Wouldn't it have been better to just revert the patch that added the platform specific pre-processing?
What you are doing now is introducing a second config change into LSK...
Special casing for TC2 was present since the first version of HMP that was merged into the LSK and was included in the base patch that added small task packing (HMP: Implement task packing for small tasks in HMP systems) in that series so we can't just do a straight revert.
It's not ideal to have to change the userspace for TC2 but on the other hand it's probably good that we have the reference systems using best practice since others will doubtless look at what it does when they do their own integration. Equally well it's *possible* someone might be running this stuff with TC2 built into their kernel as a way of getting the feature.
linaro-kernel@lists.linaro.org
-
Chris Redpath -
Jon Medhurst (Tixy) -
Mark Brown -
Mark Hambleton