Jerome Glisse <jglisse@xxxxxxxxxx> writes:
.....
>> > +
>> > + *pfns = hmm_pfn_from_pfn(pfn) | HMM_PFN_MIGRATE | flags;
>> > + *pfns |= write ? HMM_PFN_WRITE : 0;
>> > + migrate->npages++;
>> > + get_page(page);
>> > +
>> > + if (!trylock_page(page)) {
>> > + set_pte_at(mm, addr, ptep, pte);
>> > + } else {
>> > + pte_t swp_pte;
>> > +
>> > + *pfns |= HMM_PFN_LOCKED;
>> > +
>> > + entry = make_migration_entry(page, write);
>> > + swp_pte = swp_entry_to_pte(entry);
>> > + if (pte_soft_dirty(pte))
>> > + swp_pte = pte_swp_mksoft_dirty(swp_pte);
>> > + set_pte_at(mm, addr, ptep, swp_pte);
>> > +
>> > + page_remove_rmap(page, false);
>> > + put_page(page);
>> > + pages++;
>> > + }
>>
>> If this is an optimization, can we get that as a seperate patch with
>> addtional comments. ? How does take a successful page lock implies it is
>> not a shared mapping ?
>
> It can be a share mapping and that's fine, migration only fail if page is
> pin.
>
In the previous mail you replied above trylock_page() usage is an
optimization for the usual case where the memory is only use in one
process and that no concurrent migration/memory event is happening.
How did we know that it is only in use by one process. I got the part
that if we can lock, and since we lock the page early, it avoid
concurrent migration. But I am not sure about the use by one process
part.
>
>> > + }
>> > +
>> > + arch_leave_lazy_mmu_mode();
>> > + pte_unmap_unlock(ptep - 1, ptl);
>> > +
>> > + /* Only flush the TLB if we actually modified any entries */
>> > + if (pages)
>> > + flush_tlb_range(walk->vma, start, end);
>> > +
>> > + return 0;
>> > +}
>>
>>
>> So without the optimization the above function is suppose to raise the
>> refcount and collect all possible pfns tha we can migrate in the array ?
>
> Yes correct, this function collect all page we can migrate in the range.
>
.....
>
>> > +static void hmm_migrate_lock_and_isolate(struct hmm_migrate *migrate)
>> > +{
>> > + unsigned long addr = migrate->start, i = 0;
>> > + struct mm_struct *mm = migrate->vma->vm_mm;
>> > + struct vm_area_struct *vma = migrate->vma;
>> > + unsigned long restore = 0;
>> > + bool allow_drain = true;
>> > +
>> > + lru_add_drain();
>> > +
>> > +again:
>> > + for (; addr < migrate->end; addr += PAGE_SIZE, i++) {
>> > + struct page *page = hmm_pfn_to_page(migrate->pfns[i]);
>> > +
>> > + if (!page)
>> > + continue;
>> > +
>> > + if (!(migrate->pfns[i] & HMM_PFN_LOCKED)) {
>> > + lock_page(page);
>> > + migrate->pfns[i] |= HMM_PFN_LOCKED;
>> > + }
>>
>> What does taking a page_lock protect against ? Can we document that ?
>
> This usual page migration process like existing code, page lock protect against
> anyone trying to map the page inside another process or at different address. It
> also block few fs operations. I don't think there is a comprehensive list anywhere
> but i can try to make one.
I was comparing it against the trylock_page() usage above. But I guess
documenting the page lock can be another patch.
>
>> > +
>> > + /* ZONE_DEVICE page are not on LRU */
>> > + if (is_zone_device_page(page))
>> > + goto check;
>> > +
>> > + if (!PageLRU(page) && allow_drain) {
>> > + /* Drain CPU's pagevec so page can be isolated */
>> > + lru_add_drain_all();
>> > + allow_drain = false;
>> > + goto again;
>> > + }
>> > +
>> > + if (isolate_lru_page(page)) {
>> > + migrate->pfns[i] &= ~HMM_PFN_MIGRATE;
>> > + migrate->npages--;
>> > + put_page(page);
>> > + restore++;
>> > + } else
>> > + /* Drop the reference we took in collect */
>> > + put_page(page);
>> > +
>> > +check:
>> > + if (!hmm_migrate_page_check(page, 1)) {
>> > + migrate->pfns[i] &= ~HMM_PFN_MIGRATE;
>> > + migrate->npages--;
>> > + restore++;
>> > + }
>> > + }
>> > +
>
.....
>> > + }
>> > + pte_unmap_unlock(ptep - 1, ptl);
>> > +
>> > + addr = restart;
>> > + i = (addr - migrate->start) >> PAGE_SHIFT;
>> > + for (; addr < next && restore; addr += PAGE_SHIFT, i++) {
>> > + page = hmm_pfn_to_page(migrate->pfns[i]);
>> > + if (!page || (migrate->pfns[i] & HMM_PFN_MIGRATE))
>> > + continue;
>> > +
>> > + migrate->pfns[i] = 0;
>> > + unlock_page(page);
>> > + restore--;
>> > +
>> > + if (is_zone_device_page(page)) {
>> > + put_page(page);
>> > + continue;
>> > + }
>> > +
>> > + putback_lru_page(page);
>> > + }
>> > +
>> > + if (!restore)
>> > + break;
>> > + }
>>
>>
>> All the above restore won't be needed if we didn't do that migration
>> entry setup in the first function right ? We just need to drop the
>> refcount for pages that we failed to isolated ? No need to walk the page
>> table etc ?
>
> Well the migration entry setup is important so that no concurrent migration
> can race with each other, the one that set the migration entry first is the
> one that win in respect of migration. Also the CPU page table entry need to
> be clear so that page content is stable and DMA copy does not miss any data
> left over in some cache.
This is the part i am still tryint to understand.
hmm_collect_walk_pmd(), did migration entry setup only in one process
page table. So how can it prevent concurrent migration because one could
initiate a migration using the va/mapping of another process.
Isn't that page lock that is prevent concurrent migration ?
........
>
>> Why are we walking the page table multiple times ? Is it that after
>> alloc_copy the content of migrate->pfns pfn array is now the new pfns ?
>> It is confusing that each of these functions walk one page table
>> multiple times (even when page can be shared). I was expecting us to
>> walk the page table once to collect the pfns/pages and then use that
>> in rest of the calls. Any specific reason you choose to implement it
>> this way ?
>
> Well you need to know the source and destination page, so either i have
> 2 arrays one for source page and one for destination pages and then i do
> not need to walk page table multiple time. But needing 2 arrays might be
> problematic as here we want to migrate reasonable chunk ie few megabyte
> hence there is a need for vmalloc.
>
> My advice to device driver was to pre-allocate this array once (maybe
> preallocate couple of them). If you really prefer avoiding walking the
> CPU page table over and over then i can switch to 2 arrays solutions.
>
Having two array makes it easy to follow the code. But otherwise I guess
documenting the above usage of page table above the function will also
help.
.....
>> IMHO If we can get each of the above functions documented properly it will
>> help with code review. Also if we can avoid that multiple page table
>> walk, it will make it closer to the existing migration logic.
>>
>
> What kind of documentation are you looking for ? I thought the high level overview
> was enough as none of the function do anything out of the ordinary. Do you want
> more inline documation ? Or a more verbose highlevel overview ?
Inline documentation for functions will be useful. Also if you can split
the hmm_collect_walk_pmd() optimization we discussed above into a
separate patch I guess this will be lot easy to follow.
I still haven't understood why we setup that migration entry early and
that too only on one process page table. If we can explain that as a
separate patch may be it will much easy to follow.
-aneesh
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>