Re: [BUG] mlocked page counter mismatch

KOSAKI Motohiro
2009-01-29T12:35:38Z

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 */

> 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.

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