On Thu, 6 May 2010 10:46:21 +0100 Mel Gorman <mel@xxxxxxxxx> wrote: > On Thu, May 06, 2010 at 04:38:37PM +0900, KAMEZAWA Hiroyuki wrote: > > On Wed, 5 May 2010 14:14:40 +0100 > > Mel Gorman <mel@xxxxxxxxx> wrote: > > > > > vma_adjust() is updating anon VMA information without locks being taken. > > > In contrast, file-backed mappings use the i_mmap_lock and this lack of > > > locking can result in races with users of rmap_walk such as page migration. > > > vma_address() can return -EFAULT for an address that will soon be valid. > > > For migration, this potentially leaves a dangling migration PTE behind > > > which can later cause a BUG_ON to trigger when the page is faulted in. > > > > > > With the recent anon_vma changes, there can be more than one anon_vma->lock > > > to take in a anon_vma_chain but a second lock cannot be spinned upon in case > > > of deadlock. The rmap walker tries to take locks of different anon_vma's > > > but if the attempt fails, locks are released and the operation is restarted. > > > > > > For vma_adjust(), the locking behaviour prior to the anon_vma is restored > > > so that rmap_walk() can be sure of the integrity of the VMA information and > > > lists when the anon_vma lock is held. With this patch, the vma->anon_vma->lock > > > is taken if > > > > > > a) If there is any overlap with the next VMA due to the adjustment > > > b) If there is a new VMA is being inserted into the address space > > > c) If the start of the VMA is being changed so that the > > > relationship between vm_start and vm_pgoff is preserved > > > for vma_address() > > > > > > Signed-off-by: Mel Gorman <mel@xxxxxxxxx> > > > > I'm sorry I couldn't catch all details but can I make a question ? > > Of course. > > > Why seq_counter is bad finally ? I can't understand why we have > > to lock anon_vma with risks of costs, which is mysterious struct now. > > > > Adding a new to mm_struct is too bad ? > > > > It's not the biggest problem. I'm not totally against this approach but > some of the problems I had were; > > 1. It introduced new locking. anon_vmas would be covered by RCU, > spinlocks and seqlock - each of which is used in different > circumstances. The last patch I posted doesn't drastically > alter the locking. It just says that if you are taking multiple > locks, you must start from the "root" anon_vma. > ok. I just thought a lock-system which we have to find "which lock should I take" is not very good. > 2. I wasn't sure if it was usable by transparent hugepage support. > Andrea? Hmm. > > 3. I had similar concerns about it livelocking like the > trylock-and-retry although it's not terrible. > Agreed. > 4. I couldn't convince myself at the time that it wasn't possible for > someone to manipulate the list while it was being walked and a VMA would be > missed. For example, if fork() was called while rmap_walk was happening, > were we guaranteed to find the VMAs added to the list? I admit I didn't > fully investigate this question at the time as I was still getting to > grips with anon_vma. I can reinvestigate if you think the "lock the root > anon_vma first when taking multiple locks" has a bad cost that is > potentially resolved with seqcounter > If no regressions in measurement, I have no objections. > 5. It added a field to mm_struct. It's the smallest of concerns though. > > Do you think it's a better approach and should be revisited? > > If everyone think seqlock is simple, I think it should be. But it seems you all are going ahead with anon_vma->lock approach. (Basically, it's ok to me if it works. We may be able to make it better in later.) I'll check your V7. Thank you for answering. Regards, -Kame -- 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>