On 6 December 2012 14:46, Liviu Dudau <liviu.du...@arm.com> wrote: > On Thu, Dec 06, 2012 at 12:25:21PM +0000, Vincent Guittot wrote: >> On 6 December 2012 13:06, Liviu Dudau <liviu.du...@arm.com> wrote: >> > On Thu, Dec 06, 2012 at 11:50:15AM +0000, Vincent Guittot wrote: >> >> On 6 December 2012 11:27, Liviu Dudau <liviu.du...@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.guit...@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. >> > >> > But that to me and others seems to be false. I've suggested a way that the >> > values might not be coherent and yet your while loop will exit. >> >> Your example is not possible because these values are only updated by >> the cpu which owns the rq and no thread can preempt this sequence. > > So, you're saying that they are going to be coherent anyway? Because the > sequence > that updates the values cannot be preempted, that's how I read your > statement. In > that case, why do you need the while loop (and the whole patch) ?
Liviu, I'm just saying that the use case above can't happen because the fields are not updated by thread but by scheduler and it can't be pre-empted by a thread while updating them. We can have the situation where cpu A updates its fields and cpu B reads them simultaneously. This is the use case that i'm trying to address with this patch. Regards, Vincent > >> >> > >> >> >> >> > 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. >> > >> > Because we don't like adding code that doesn't work as claimed and then >> > justify >> > its presence by arguing that even if it's broken it doesn't change >> > behaviour. >> > >> > Why having that code in the first place? >> > >> > All we are suggesting here is that the while loop is broken in its >> > assumptions >> > and that you get the same behaviour if you remove it. If you agree then >> > push >> > a patch that removes the while loop and we are going to be likely in favor >> > of the new patch being included. If you really want to have the values >> > coherent >> > then you need a better mechanism for ensuring that. >> > >> > Best regards, >> > Liviu >> > >> >> >> >> > >> >> > 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! / > --------------- > ¯\_(ツ)_/¯ > _______________________________________________ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
