On 6 December 2012 11:27, Liviu Dudau Liviu.Dudau@arm.com wrote:
On Thu, Dec 06, 2012 at 09:32:11AM +0000, Viresh Kumar wrote:
Hi Vincent,
On 6 December 2012 14:50, Vincent Guittot vincent.guittot@linaro.org wrote:
It's look like you have disabled packing small task in your v13 with 4a29297
It looks like you are referring to an older version of this branch as the commit id's don't match. Can you please check that again?
In addition, I don't fully understand why you absolutely want to revert 39b0e77. In the worst case, it will not fully solve what it should have solved but it doesn't add any regression.
I don't want to, but Liviu and Morten do :) @Liviu/Morten: Your comments
Hi Vincent,
I was just a messenger here, but this is my understanding of where things are:
- Morten has reviewed your patch and had provided comments, mainly pointing towards the fact that the guarantees that the code is trying to provide are not met.
- You have replied/promised to review and update the patch (is that correct?)
- Nothing has happened since in this area (is that true?)
both are true
To us, looking at the code (mainly the while loop in the is_buddy_busy() function), is seem that you are trying to guarantee that the runnable_avg_sum and the runnable_avg_period that you read from the runqueue have not been updated before deciding if that cpu is busy. But there is no reason* why another thread could not update the runnable_avg_sum and then get preempted before updating the runnable_avg_period, which means you will still use the wrong values when doing the "greater than" check in the return statement.
No, I don't want to ensure that values have not been updated before I use them but that both values are coherent
The other point is that even if your code is "broken" (i.e. you read stale values that still get you out of the while loop) the code works but your estimate of a CPU being busy is off by .... 10% ? In other words, having that while loop in is_buddy_busy() does not change the behaviour of the rest of the code. So the objection is on having the while loop and trying to claim guarantees that are not met. Why not removing the loop?
So if it doesn't change the behavior why removing it ? It make the maintenance of patches series more complex.
Hope I'm making sense.
- my understanding of the scheduler code is rather incomplete, so I'm
expecting to be proven wrong here.
Best regards, Liviu
According to my test, sched-pack-small-tasks-v1-arm is enough if you want to use packing small task on tc2
I didn't get this one. This is what i have included in my tree.
-- viresh
--
| I would like to | | fix the world, | | but they're not | | giving me the | \ source code! /
¯\_(ツ)_/¯