lkml.org 
[lkml]   [2016]   [Nov]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [HMM v13 16/18] mm/hmm/migrate: new memory migration helper for use with device memory
Date
Jerome Glisse <jglisse@redhat.com> 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

\
 
 \ /
  Last update: 2016-11-20 19:22    [W:2.229 / U:0.456 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site