Re: [BUG] mlocked page counter mismatch

Lee Schermerhorn
2009-01-29T14:44:39Z

On Thu, 2009-01-29 at 21:35 +0900, KOSAKI Motohiro wrote:
> Hi
>
> > I think I see it. In try_to_unmap_anon(), called from try_to_munlock(),
> > we have:
> >
> > list_for_each_entry(vma, &anon_vma->head, anon_vma_node) {
> > if (MLOCK_PAGES && unlikely(unlock)) {
> > if (!((vma->vm_flags & VM_LOCKED) &&
> > !!! should be '||' ? ^^
> > page_mapped_in_vma(page, vma)))
> > continue; /* must visit all unlocked vmas */
> > ret = SWAP_MLOCK; /* saw at least one mlocked vma */
> > } else {
> > ret = try_to_unmap_one(page, vma, migration);
> > if (ret == SWAP_FAIL || !page_mapped(page))
> > break;
> > }
> > if (ret == SWAP_MLOCK) {
> > mlocked = try_to_mlock_page(page, vma);
> > if (mlocked)
> > break; /* stop if actually mlocked page */
> > }
> > }
> >
> > or that clause [under if (MLOCK_PAGES && unlikely(unlock))]
> > might be clearer as:
> >
> > if ((vma->vm_flags & VM_LOCKED) && page_mapped_in_vma(page, vma))
> > ret = SWAP_MLOCK; /* saw at least one mlocked vma */
> > else
> > continue; /* must visit all unlocked vmas */
> >
> > Do you agree?
>
> Hmmm.
> I don't think so.
>
> > if (!((vma->vm_flags & VM_LOCKED) &&
> > page_mapped_in_vma(page, vma)))
> > continue; /* must visit all unlocked vmas */
>
> is already equivalent to
>
> > if ((vma->vm_flags & VM_LOCKED) && page_mapped_in_vma(page, vma))
> > ret = SWAP_MLOCK; /* saw at least one mlocked vma */
> > else
> >
> continue; /* must visit all unlocked vmas */

Hmmm, I should know not to try to read code when I'm that sleepy. Had
myself convinced that the condition was wrong...

>
>
> > And, I wonder if we need a similar check for
> > page_mapped_in_vma(page, vma) up in try_to_unmap_one()?
>
> because page_mapped_in_vma() can return 0 if vma is anon vma only.

by "vma is anon vma only", do you mean that the vma being tested--e.g.,
that we found to be VM_LOCKED--no longer has the page mapped in it's
page tables? That is it's purpose--to detect this condition. IIRC, Rik
added it during testing of the mlock patches when we discovered we were
mlocking pages because

>
> In the other word,
> struct adress_space (for file) gurantee that unrelated vma doesn't chained.

right. that's why we don't have the page_mapped_in_vma() check in
try_to_unmap_file().

> but struct anon_vma (for anon) doesn't gurantee that unrelated vma
> doesn't chained.

Well, they're not exactly "unrelated"--vmas attached to an anon_vma are
from the same "family". Any pages that haven't been COWed will still be
mapped into multiple mm's.

My question last night about try_to_unmap_one() wasn't really related to
the mlock statistics glitch. Sorry I wasn't more clear about this. I
was wondering about the case where shrink_page_list was trying to unmap
a page whose vma was on an anon_vma list with other VM_LOCKED vmas that
didn't actually map the page. However, in the early morning light, I
see that the call to page_check_address() handles this.

----------
Anyway, our original responses to this report crossed in the mail. You
said you'd handle it. So, in the meantime, I'm looking at the
mmap()/vm_merge()/mlock_vma_pages_range() issue reported yesterday.

Regards,
Lee