On Thu, Dec 06, 2012 at 02:06:30PM +0000, Amit Kucheria wrote:
On Thu, Dec 6, 2012 at 5:36 PM, Liviu Dudau <Liviu.Dudau@arm.commailto:Liviu.Dudau@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.Dudau@arm.commailto: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.orgmailto: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.
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.
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.
Even if that is the case, I'm afraid I don't quite like the way this was done. IMHO, you shouldn't just revert bits of another author's patches that you don't agree with.
Hi Amit,
We didn't reverted bits, we have just asked that one of the patches from the series not to be included into the final pull request as it was controversial. It did not change the functionality of the series.
If there are issues regarding the the patches from Vincent, I'd do the following things in order of priority:
- Prove to him that the race exists, preferably with a reproducible test case
We did. Morten and Olivier sent emails to Vincent.
- Give him a chance to convince you otherwise
We did. Initial comments were sent about a month ago.
- Share test results that show bad things happen as a result of some code
N/A. We are claiming that the code works to the same outcome on both cases. It's just that the guarantees that the code is trying to make don't hold water.
- Ask _him_ to separate out that bit from the original patch so you can only pick the bits you like
We are not the ones picking up the bits. It was Viresh. And we have told Viresh that we have comments on the one patch in the series that make it unsuitable for that release. One suggestion that we made was to revert that particular patch. Now Viresh was free to choose a different path to reach the outcome, like asking Vincent to split the patch out of his series. But I guess it was too late in the game for that and it was more expedient to do the revert.
I haven't seen this happen. All I've seen is one side claim it can happen and the other claim that it can't. *shrug*
Not seeing the whole picture then?
Viresh, as an experienced maintainer, I hope you see the value of this approach rather than just pull in the tree.
I realise we're all under pressure here. So let's take a deep breath, step back and do it the right way.
I'm really deploring the fact that we are turning this into a big issue when it is not. We are only nitpicking. If you want to go ahead and include the code just because it make patch management easier, go ahead. Our comments were on the technical side.
Regards, Liviu
/Amit