On Wed, May 12, 2010 at 05:08:11PM -0400, Rik van Riel wrote: > On 05/12/2010 05:02 PM, Mel Gorman wrote: > >> This last comment is a bit light. It's actually restoring the lock that >> was taken in 2.6.33 to some extent except we are always taking it now. >> In 2.6.33, it was resricted to >> >> if (vma->anon_vma&& (insert || importer || start != vma->vm_start)) >> anon_vma = vma->anon_vma; >> >> but now it's always. Has it been determined that the locking in 2.6.33 >> was insufficient or are we playing it safe now? > > Playing it safe, mostly. > Sure. I did the same, got the same question from Andrea and more or less gave the same answer :) . I asked again in case you spotted something I didn't. > Another aspect is that, if you look at the if condition above, > the number of cases where we have an anon_vma and do not take > the lock is pretty small. > > Basically only the case where we expand a VMA upward or merge > VMAs in an mprotect. I believe in pretty much all other cases > we end up needing to take the lock. > Looking at the if condition, brk() would appear to be the most important case, right? This would appear to correlate with the reasoning behind that condition in the first place in commit 252c5f94d944487e9f50ece7942b0fbf659c5c31 where sbrk contended on the lock heavily. I can't convince myself 100% but it is possible we will regress on that test case again if the same logic is not applied to the locking. I ran a brk() microbenchmark from aim9 and the results were really bad - 48% regression. I didn't rerun with the old logic to see the results unfortunately and right now I'm on the road. It'll be tomorrow morning before I get the chance. > I am not entirely convinced the old code took the lock in all > of the required cases. > I have vague worries about expand_upwards but otherwise the reasoning seemed solid and even with the new anon_vma code, we are not doing anything fundamentally different in this area. Maybe it's best to play it safe now and always take the lock, but it's worth reconsidering later particularly if this patch gets fingered in some performance-related bisection later. -- Mel Gorman Part-time Phd Student Linux Technology Center University of Limerick IBM Dublin Software Lab -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxxx For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>