[ofa-general] Re: [patch 01/10] emm: mm_lock: Lock a process against reclaim

[Jeremy Fitzhardinge]

Andrea Arcangeli wrote:
> On Fri, Apr 04, 2008 at 04:12:42PM -0700, Jeremy Fitzhardinge wrote:
>   
>> I think you can break this if() down a bit:
>>
>> 			if (!(vma->vm_file && vma->vm_file->f_mapping))
>> 				continue;
>>     
>
> It makes no difference at runtime, coding style preferences are quite
> subjective.
>   

Well, overall the formatting of that if statement is very hard to read.  
Separating out the logically distinct pieces in to different ifs at 
least shows the reader that they are distinct.
Aside from that, doing some manual CSE to remove all the casts and 
expose the actual thing you're testing for would help a lot (are the 
casts even necessary?).

>> So this is an O(n^2) algorithm to take the i_mmap_locks from low to high 
>> order?  A comment would be nice.  And O(n^2)?  Ouch.  How often is it 
>> called?
>>     
>
> It's called a single time when the mmu notifier is registered. It's a
> very slow path of course. Any other approach to reduce the complexity
> would require memory allocations and it would require
> mmu_notifier_register to return -ENOMEM failure. It didn't seem worth
> it.
>   

It's per-mm though.  How many processes would need to have notifiers?


>> And is it necessary to mush lock and unlock together?  Unlock ordering 
>> doesn't matter, so you should just be able to have a much simpler loop, no?
>>     
>
> That avoids duplicating .text. Originally they were separated. unlock
> can't be a simpler loop because I didn't reserve vm_flags bitflags to
> do a single O(N) loop for unlock. If you do malloc+fork+munmap two
> vmas will point to the same anon-vma lock, that's why the unlock isn't
> simpler unless I mark what I locked with a vm_flags bitflag.

Well, its definitely going to need more comments then.  I assumed it 
would end up locking everything, so unlocking everything would be 
sufficient.

    J