Re: [PATCH 1/2] mm,migration: Prevent rmap_walk_[anon|ksm] seeingthe wrong VMA information

[KAMEZAWA Hiroyuki]

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/