Messages in this thread | | | Date | Thu, 29 Jan 2009 21:35:36 +0900 | Subject | Re: [BUG] mlocked page counter mismatch | From | KOSAKI Motohiro <> |
| |
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.
| |