Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1361728 > unrolled thread

Re: [PATCH v12 21/29] HMM: mm add helper to update page table when migrating memory back v2.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v12 21/29] HMM: mm add helper to update page table when migrating memory back v2. "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2016-03-21 12:30 +0100
    Re: [PATCH v12 21/29] HMM: mm add helper to update page table when  migrating memory back v2. Jerome Glisse <j.glisse@gmail.com> - 2016-03-21 13:10 +0100
      Re: [PATCH v12 21/29] HMM: mm add helper to update page table when migrating memory back v2. "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> - 2016-03-21 14:50 +0100
        Re: [PATCH v12 21/29] HMM: mm add helper to update page table when  migrating memory back v2. Jerome Glisse <j.glisse@gmail.com> - 2016-03-21 15:40 +0100

#1361728 — Re: [PATCH v12 21/29] HMM: mm add helper to update page table when migrating memory back v2.

Jérôme Glisse <jglisse@redhat.com> writes:

> [ text/plain ]
> To migrate memory back we first need to lock HMM special CPU page
> table entry so we know no one else might try to migrate those entry
> back. Helper also allocate new page where data will be copied back
> from the device. Then we can proceed with the device DMA operation.
>
> Once DMA is done we can update again the CPU page table to point to
> the new page that holds the content copied back from device memory.
>
> Note that we do not need to invalidate the range are we are only
> modifying non present CPU page table entry.
>
> Changed since v1:
>   - Save memcg against which each page is precharge as it might
>     change along the way.
>
> Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
> ---
>  include/linux/mm.h |  12 +++
>  mm/memory.c        | 257 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 269 insertions(+)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index c5c062e..1cd060f 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2392,6 +2392,18 @@ static inline void hmm_mm_init(struct mm_struct *mm)
>  {
>  	mm->hmm = NULL;
>  }
> +
> +int mm_hmm_migrate_back(struct mm_struct *mm,
> +			struct vm_area_struct *vma,
> +			pte_t *new_pte,
> +			unsigned long start,
> +			unsigned long end);
> +void mm_hmm_migrate_back_cleanup(struct mm_struct *mm,
> +				 struct vm_area_struct *vma,
> +				 pte_t *new_pte,
> +				 dma_addr_t *hmm_pte,
> +				 unsigned long start,
> +				 unsigned long end);
>  #else /* !CONFIG_HMM */
>  static inline void hmm_mm_init(struct mm_struct *mm)
>  {
> diff --git a/mm/memory.c b/mm/memory.c
> index 3cb3653..d917911a 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3513,6 +3513,263 @@ int handle_mm_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>  }
>  EXPORT_SYMBOL_GPL(handle_mm_fault);
>  
> +
> +#ifdef CONFIG_HMM
> +/* mm_hmm_migrate_back() - lock HMM CPU page table entry and allocate new page.
> + *
> + * @mm: The mm struct.
> + * @vma: The vm area struct the range is in.
> + * @new_pte: Array of new CPU page table entry value.
> + * @start: Start address of the range (inclusive).
> + * @end: End address of the range (exclusive).
> + *
> + * This function will lock HMM page table entry and allocate new page for entry
> + * it successfully locked.
> + */


Can you add more comments around this ?

> +int mm_hmm_migrate_back(struct mm_struct *mm,
> +			struct vm_area_struct *vma,
> +			pte_t *new_pte,
> +			unsigned long start,
> +			unsigned long end)
> +{
> +	pte_t hmm_entry = swp_entry_to_pte(make_hmm_entry_locked());
> +	unsigned long addr, i;
> +	int ret = 0;
> +
> +	VM_BUG_ON(vma->vm_ops || (vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP)));
> +
> +	if (unlikely(anon_vma_prepare(vma)))
> +		return -ENOMEM;
> +
> +	start &= PAGE_MASK;
> +	end = PAGE_ALIGN(end);
> +	memset(new_pte, 0, sizeof(pte_t) * ((end - start) >> PAGE_SHIFT));
> +
> +	for (addr = start; addr < end;) {
> +		unsigned long cstart, next;
> +		spinlock_t *ptl;
> +		pgd_t *pgdp;
> +		pud_t *pudp;
> +		pmd_t *pmdp;
> +		pte_t *ptep;
> +
> +		pgdp = pgd_offset(mm, addr);
> +		pudp = pud_offset(pgdp, addr);
> +		/*
> +		 * Some other thread might already have migrated back the entry
> +		 * and freed the page table. Unlikely thought.
> +		 */
> +		if (unlikely(!pudp)) {
> +			addr = min((addr + PUD_SIZE) & PUD_MASK, end);
> +			continue;
> +		}
> +		pmdp = pmd_offset(pudp, addr);
> +		if (unlikely(!pmdp || pmd_bad(*pmdp) || pmd_none(*pmdp) ||
> +			     pmd_trans_huge(*pmdp))) {
> +			addr = min((addr + PMD_SIZE) & PMD_MASK, end);
> +			continue;
> +		}
> +		ptep = pte_offset_map_lock(mm, pmdp, addr, &ptl);
> +		for (cstart = addr, i = (addr - start) >> PAGE_SHIFT,
> +		     next = min((addr + PMD_SIZE) & PMD_MASK, end);
> +		     addr < next; addr += PAGE_SIZE, ptep++, i++) {
> +			swp_entry_t entry;
> +
> +			entry = pte_to_swp_entry(*ptep);
> +			if (pte_none(*ptep) || pte_present(*ptep) ||
> +			    !is_hmm_entry(entry) ||
> +			    is_hmm_entry_locked(entry))
> +				continue;
> +
> +			set_pte_at(mm, addr, ptep, hmm_entry);
> +			new_pte[i] = pte_mkspecial(pfn_pte(my_zero_pfn(addr),
> +						   vma->vm_page_prot));
> +		}
> +		pte_unmap_unlock(ptep - 1, ptl);


I guess this is fixing all the ptes in the cpu page table mapping a pmd
entry. But then what is below ?


> +
> +		for (addr = cstart, i = (addr - start) >> PAGE_SHIFT;
> +		     addr < next; addr += PAGE_SIZE, i++) {

Your use of vairable addr with multiple loops updating then is also
making it complex. We should definitely add more comments here. I guess
we are going through the same range we iterated above here.

> +			struct mem_cgroup *memcg;
> +			struct page *page;
> +
> +			if (!pte_present(new_pte[i]))
> +				continue;

What is that checking for ?. We set that using pte_mkspecial above ?

> +
> +			page = alloc_zeroed_user_highpage_movable(vma, addr);
> +			if (!page) {
> +				ret = -ENOMEM;
> +				break;
> +			}
> +			__SetPageUptodate(page);
> +			if (mem_cgroup_try_charge(page, mm, GFP_KERNEL,
> +						  &memcg)) {
> +				page_cache_release(page);
> +				ret = -ENOMEM;
> +				break;
> +			}
> +			/*
> +			 * We can safely reuse the s_mem/mapping field of page
> +			 * struct to store the memcg as the page is only seen
> +			 * by HMM at this point and we can clear it before it
> +			 * is public see mm_hmm_migrate_back_cleanup().
> +			 */
> +			page->s_mem = memcg;
> +			new_pte[i] = mk_pte(page, vma->vm_page_prot);
> +			if (vma->vm_flags & VM_WRITE) {
> +				new_pte[i] = pte_mkdirty(new_pte[i]);
> +				new_pte[i] = pte_mkwrite(new_pte[i]);
> +			}

Why mark it dirty if vm_flags is VM_WRITE ?

> +		}
> +
> +		if (!ret)
> +			continue;
> +
> +		hmm_entry = swp_entry_to_pte(make_hmm_entry());
> +		ptep = pte_offset_map_lock(mm, pmdp, addr, &ptl);


Again we loop through the same range ?

> +		for (addr = cstart, i = (addr - start) >> PAGE_SHIFT;
> +		     addr < next; addr += PAGE_SIZE, ptep++, i++) {
> +			unsigned long pfn = pte_pfn(new_pte[i]);
> +
> +			if (!pte_present(new_pte[i]) || !is_zero_pfn(pfn))
> +				continue;


What is that checking for ?
> +
> +			set_pte_at(mm, addr, ptep, hmm_entry);
> +			pte_clear(mm, addr, &new_pte[i]);

what is that pte_clear for ?. Handling of new_pte needs more code comments.

> +		}
> +		pte_unmap_unlock(ptep - 1, ptl);
> +		break;
> +	}
> +	return ret;
> +}
> +EXPORT_SYMBOL(mm_hmm_migrate_back);
> +


-aneesh

[toc] | [next] | [standalone]

#1361753 — Re: [PATCH v12 21/29] HMM: mm add helper to update page table when migrating memory back v2.

On Mon, Mar 21, 2016 at 04:57:32PM +0530, Aneesh Kumar K.V wrote:
> Jérôme Glisse <jglisse@redhat.com> writes:

[...]

> > +
> > +#ifdef CONFIG_HMM
> > +/* mm_hmm_migrate_back() - lock HMM CPU page table entry and allocate new page.
> > + *
> > + * @mm: The mm struct.
> > + * @vma: The vm area struct the range is in.
> > + * @new_pte: Array of new CPU page table entry value.
> > + * @start: Start address of the range (inclusive).
> > + * @end: End address of the range (exclusive).
> > + *
> > + * This function will lock HMM page table entry and allocate new page for entry
> > + * it successfully locked.
> > + */
> 
> 
> Can you add more comments around this ?

I should describe the process a bit more i guess. It is multi-step, first we update
CPU page table with special HMM "lock" entry, this is to exclude concurrent migration
happening on same page. Once we have "locked" the CPU page table entry we allocate
the proper number of pages. Then we schedule the dma from the GPU to this pages and
once it is done we update the CPU page table to point to this pages. This is why we
are going over the page table so many times. This should answer most of your questions
below but i still provide answer for each of them.

> 
> > +int mm_hmm_migrate_back(struct mm_struct *mm,
> > +			struct vm_area_struct *vma,
> > +			pte_t *new_pte,
> > +			unsigned long start,
> > +			unsigned long end)
> > +{
> > +	pte_t hmm_entry = swp_entry_to_pte(make_hmm_entry_locked());
> > +	unsigned long addr, i;
> > +	int ret = 0;
> > +
> > +	VM_BUG_ON(vma->vm_ops || (vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP)));
> > +
> > +	if (unlikely(anon_vma_prepare(vma)))
> > +		return -ENOMEM;
> > +
> > +	start &= PAGE_MASK;
> > +	end = PAGE_ALIGN(end);
> > +	memset(new_pte, 0, sizeof(pte_t) * ((end - start) >> PAGE_SHIFT));
> > +
> > +	for (addr = start; addr < end;) {
> > +		unsigned long cstart, next;
> > +		spinlock_t *ptl;
> > +		pgd_t *pgdp;
> > +		pud_t *pudp;
> > +		pmd_t *pmdp;
> > +		pte_t *ptep;
> > +
> > +		pgdp = pgd_offset(mm, addr);
> > +		pudp = pud_offset(pgdp, addr);
> > +		/*
> > +		 * Some other thread might already have migrated back the entry
> > +		 * and freed the page table. Unlikely thought.
> > +		 */
> > +		if (unlikely(!pudp)) {
> > +			addr = min((addr + PUD_SIZE) & PUD_MASK, end);
> > +			continue;
> > +		}
> > +		pmdp = pmd_offset(pudp, addr);
> > +		if (unlikely(!pmdp || pmd_bad(*pmdp) || pmd_none(*pmdp) ||
> > +			     pmd_trans_huge(*pmdp))) {
> > +			addr = min((addr + PMD_SIZE) & PMD_MASK, end);
> > +			continue;
> > +		}
> > +		ptep = pte_offset_map_lock(mm, pmdp, addr, &ptl);
> > +		for (cstart = addr, i = (addr - start) >> PAGE_SHIFT,
> > +		     next = min((addr + PMD_SIZE) & PMD_MASK, end);
> > +		     addr < next; addr += PAGE_SIZE, ptep++, i++) {
> > +			swp_entry_t entry;
> > +
> > +			entry = pte_to_swp_entry(*ptep);
> > +			if (pte_none(*ptep) || pte_present(*ptep) ||
> > +			    !is_hmm_entry(entry) ||
> > +			    is_hmm_entry_locked(entry))
> > +				continue;
> > +
> > +			set_pte_at(mm, addr, ptep, hmm_entry);
> > +			new_pte[i] = pte_mkspecial(pfn_pte(my_zero_pfn(addr),
> > +						   vma->vm_page_prot));
> > +		}
> > +		pte_unmap_unlock(ptep - 1, ptl);
> 
> 
> I guess this is fixing all the ptes in the cpu page table mapping a pmd
> entry. But then what is below ?

Because we are dealing with special swap entry we know we can not have huge pages.
So we only care about HMM special swap entry. We record entry we want to migrate
in the new_pte array. The loop above is under pmd spin lock, the loop below does
memory allocation and we do not want to hold any spin lock while doing allocation.

> 
> > +
> > +		for (addr = cstart, i = (addr - start) >> PAGE_SHIFT;
> > +		     addr < next; addr += PAGE_SIZE, i++) {
> 
> Your use of vairable addr with multiple loops updating then is also
> making it complex. We should definitely add more comments here. I guess
> we are going through the same range we iterated above here.

Correct we are going over the exact same range, i am keeping the addr only
for alloc_zeroed_user_highpage_movable() purpose.

> 
> > +			struct mem_cgroup *memcg;
> > +			struct page *page;
> > +
> > +			if (!pte_present(new_pte[i]))
> > +				continue;
> 
> What is that checking for ?. We set that using pte_mkspecial above ?

Not all entry in the range might match the criteria (ie special unlocked HMM swap
entry). We want to allocate pages only for entry that match the criteria.

> 
> > +
> > +			page = alloc_zeroed_user_highpage_movable(vma, addr);
> > +			if (!page) {
> > +				ret = -ENOMEM;
> > +				break;
> > +			}
> > +			__SetPageUptodate(page);
> > +			if (mem_cgroup_try_charge(page, mm, GFP_KERNEL,
> > +						  &memcg)) {
> > +				page_cache_release(page);
> > +				ret = -ENOMEM;
> > +				break;
> > +			}
> > +			/*
> > +			 * We can safely reuse the s_mem/mapping field of page
> > +			 * struct to store the memcg as the page is only seen
> > +			 * by HMM at this point and we can clear it before it
> > +			 * is public see mm_hmm_migrate_back_cleanup().
> > +			 */
> > +			page->s_mem = memcg;
> > +			new_pte[i] = mk_pte(page, vma->vm_page_prot);
> > +			if (vma->vm_flags & VM_WRITE) {
> > +				new_pte[i] = pte_mkdirty(new_pte[i]);
> > +				new_pte[i] = pte_mkwrite(new_pte[i]);
> > +			}
> 
> Why mark it dirty if vm_flags is VM_WRITE ?

It is a left over of some debuging i was doing, i missed it.

> 
> > +		}
> > +
> > +		if (!ret)
> > +			continue;
> > +
> > +		hmm_entry = swp_entry_to_pte(make_hmm_entry());
> > +		ptep = pte_offset_map_lock(mm, pmdp, addr, &ptl);
> 
> 
> Again we loop through the same range ?

Yes but this is the out of memory code path here, ie we have to split the migration
into several pass. So what happen here is we clear the new_pte array for entry we
failed to allocate a page for.

> 
> > +		for (addr = cstart, i = (addr - start) >> PAGE_SHIFT;
> > +		     addr < next; addr += PAGE_SIZE, ptep++, i++) {
> > +			unsigned long pfn = pte_pfn(new_pte[i]);
> > +
> > +			if (!pte_present(new_pte[i]) || !is_zero_pfn(pfn))
> > +				continue;
> 
> 
> What is that checking for ?

If new_pte entry is not present then it is not something we want to migrate. If it
is present but does not point to zero pfn then it is an entry for which we allocated
a page so we want to keep it.

> > +
> > +			set_pte_at(mm, addr, ptep, hmm_entry);
> > +			pte_clear(mm, addr, &new_pte[i]);
> 
> what is that pte_clear for ?. Handling of new_pte needs more code comments.
> 

Entry for which we failed to allocate memory we clear the special HMM swap entry
as well as the new_pte entry so that migration code knows it does not have to do
anything here.

Hopes this clarify this code.

Cheers,
Jérôme

[toc] | [prev] | [next] | [standalone]

#1361861

Jerome Glisse <j.glisse@gmail.com> writes:

> [ text/plain ]
> On Mon, Mar 21, 2016 at 04:57:32PM +0530, Aneesh Kumar K.V wrote:
>> Jérôme Glisse <jglisse@redhat.com> writes:
>
> [...]
>
>> > +
>> > +#ifdef CONFIG_HMM
>> > +/* mm_hmm_migrate_back() - lock HMM CPU page table entry and allocate new page.
>> > + *
>> > + * @mm: The mm struct.
>> > + * @vma: The vm area struct the range is in.
>> > + * @new_pte: Array of new CPU page table entry value.
>> > + * @start: Start address of the range (inclusive).
>> > + * @end: End address of the range (exclusive).
>> > + *
>> > + * This function will lock HMM page table entry and allocate new page for entry
>> > + * it successfully locked.
>> > + */
>> 
>> 
>> Can you add more comments around this ?
>
> I should describe the process a bit more i guess. It is multi-step, first we update
> CPU page table with special HMM "lock" entry, this is to exclude concurrent migration
> happening on same page. Once we have "locked" the CPU page table entry we allocate
> the proper number of pages. Then we schedule the dma from the GPU to this pages and
> once it is done we update the CPU page table to point to this pages. This is why we
> are going over the page table so many times. This should answer most of your questions
> below but i still provide answer for each of them.
>
>> 
>> > +int mm_hmm_migrate_back(struct mm_struct *mm,
>> > +			struct vm_area_struct *vma,
>> > +			pte_t *new_pte,
>> > +			unsigned long start,
>> > +			unsigned long end)
>> > +{
>> > +	pte_t hmm_entry = swp_entry_to_pte(make_hmm_entry_locked());
>> > +	unsigned long addr, i;
>> > +	int ret = 0;
>> > +
>> > +	VM_BUG_ON(vma->vm_ops || (vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP)));
>> > +
>> > +	if (unlikely(anon_vma_prepare(vma)))
>> > +		return -ENOMEM;
>> > +
>> > +	start &= PAGE_MASK;
>> > +	end = PAGE_ALIGN(end);
>> > +	memset(new_pte, 0, sizeof(pte_t) * ((end - start) >> PAGE_SHIFT));
>> > +
>> > +	for (addr = start; addr < end;) {
>> > +		unsigned long cstart, next;
>> > +		spinlock_t *ptl;
>> > +		pgd_t *pgdp;
>> > +		pud_t *pudp;
>> > +		pmd_t *pmdp;
>> > +		pte_t *ptep;
>> > +
>> > +		pgdp = pgd_offset(mm, addr);
>> > +		pudp = pud_offset(pgdp, addr);
>> > +		/*
>> > +		 * Some other thread might already have migrated back the entry
>> > +		 * and freed the page table. Unlikely thought.
>> > +		 */
>> > +		if (unlikely(!pudp)) {
>> > +			addr = min((addr + PUD_SIZE) & PUD_MASK, end);
>> > +			continue;
>> > +		}
>> > +		pmdp = pmd_offset(pudp, addr);
>> > +		if (unlikely(!pmdp || pmd_bad(*pmdp) || pmd_none(*pmdp) ||
>> > +			     pmd_trans_huge(*pmdp))) {
>> > +			addr = min((addr + PMD_SIZE) & PMD_MASK, end);
>> > +			continue;
>> > +		}
>> > +		ptep = pte_offset_map_lock(mm, pmdp, addr, &ptl);
>> > +		for (cstart = addr, i = (addr - start) >> PAGE_SHIFT,
>> > +		     next = min((addr + PMD_SIZE) & PMD_MASK, end);
>> > +		     addr < next; addr += PAGE_SIZE, ptep++, i++) {
>> > +			swp_entry_t entry;
>> > +
>> > +			entry = pte_to_swp_entry(*ptep);
>> > +			if (pte_none(*ptep) || pte_present(*ptep) ||
>> > +			    !is_hmm_entry(entry) ||
>> > +			    is_hmm_entry_locked(entry))
>> > +				continue;
>> > +
>> > +			set_pte_at(mm, addr, ptep, hmm_entry);
>> > +			new_pte[i] = pte_mkspecial(pfn_pte(my_zero_pfn(addr),
>> > +						   vma->vm_page_prot));
>> > +		}
>> > +		pte_unmap_unlock(ptep - 1, ptl);
>> 
>> 
>> I guess this is fixing all the ptes in the cpu page table mapping a pmd
>> entry. But then what is below ?
>
> Because we are dealing with special swap entry we know we can not have huge pages.
> So we only care about HMM special swap entry. We record entry we want to migrate
> in the new_pte array. The loop above is under pmd spin lock, the loop below does
> memory allocation and we do not want to hold any spin lock while doing allocation.
>

Can this go as code comment ?

>> 
>> > +
>> > +		for (addr = cstart, i = (addr - start) >> PAGE_SHIFT;
>> > +		     addr < next; addr += PAGE_SIZE, i++) {
>> 
>> Your use of vairable addr with multiple loops updating then is also
>> making it complex. We should definitely add more comments here. I guess
>> we are going through the same range we iterated above here.
>
> Correct we are going over the exact same range, i am keeping the addr only
> for alloc_zeroed_user_highpage_movable() purpose.
>

Can we use a different variable name there ?

>> 
>> > +			struct mem_cgroup *memcg;
>> > +			struct page *page;
>> > +
>> > +			if (!pte_present(new_pte[i]))
>> > +				continue;
>> 
>> What is that checking for ?. We set that using pte_mkspecial above ?
>
> Not all entry in the range might match the criteria (ie special unlocked HMM swap
> entry). We want to allocate pages only for entry that match the criteria.
>

Since we did in the beginning, 
	memset(new_pte, 0, sizeof(pte_t) * ((end - start) >> PAGE_SHIFT));

we should not find present bit set ? using present there is confusing,
may be pte_none(). Also with comments around explaining the details ?

>> 
>> > +
>> > +			page = alloc_zeroed_user_highpage_movable(vma, addr);
>> > +			if (!page) {
>> > +				ret = -ENOMEM;
>> > +				break;
>> > +			}
>> > +			__SetPageUptodate(page);
>> > +			if (mem_cgroup_try_charge(page, mm, GFP_KERNEL,
>> > +						  &memcg)) {
>> > +				page_cache_release(page);
>> > +				ret = -ENOMEM;
>> > +				break;
>> > +			}
>> > +			/*
>> > +			 * We can safely reuse the s_mem/mapping field of page
>> > +			 * struct to store the memcg as the page is only seen
>> > +			 * by HMM at this point and we can clear it before it
>> > +			 * is public see mm_hmm_migrate_back_cleanup().
>> > +			 */
>> > +			page->s_mem = memcg;
>> > +			new_pte[i] = mk_pte(page, vma->vm_page_prot);
>> > +			if (vma->vm_flags & VM_WRITE) {
>> > +				new_pte[i] = pte_mkdirty(new_pte[i]);
>> > +				new_pte[i] = pte_mkwrite(new_pte[i]);
>> > +			}
>> 
>> Why mark it dirty if vm_flags is VM_WRITE ?
>
> It is a left over of some debuging i was doing, i missed it.
>
>> 
>> > +		}
>> > +
>> > +		if (!ret)
>> > +			continue;
>> > +
>> > +		hmm_entry = swp_entry_to_pte(make_hmm_entry());
>> > +		ptep = pte_offset_map_lock(mm, pmdp, addr, &ptl);
>> 
>> 
>> Again we loop through the same range ?
>
> Yes but this is the out of memory code path here, ie we have to split the migration
> into several pass. So what happen here is we clear the new_pte array for entry we
> failed to allocate a page for.
>
>> 
>> > +		for (addr = cstart, i = (addr - start) >> PAGE_SHIFT;
>> > +		     addr < next; addr += PAGE_SIZE, ptep++, i++) {
>> > +			unsigned long pfn = pte_pfn(new_pte[i]);
>> > +
>> > +			if (!pte_present(new_pte[i]) || !is_zero_pfn(pfn))
>> > +				continue;
>> 



So here we are using the fact that we had set new pte using zero pfn in
the firs loop and hence if we find a present new_pte with zero pfn, it implies we
failed to allocate a page for that ?

>> 
>> What is that checking for ?
>
> If new_pte entry is not present then it is not something we want to migrate. If it
> is present but does not point to zero pfn then it is an entry for which we allocated
> a page so we want to keep it.
>
>> > +
>> > +			set_pte_at(mm, addr, ptep, hmm_entry);
>> > +			pte_clear(mm, addr, &new_pte[i]);
>> 
>> what is that pte_clear for ?. Handling of new_pte needs more code comments.
>> 
>
> Entry for which we failed to allocate memory we clear the special HMM swap entry
> as well as the new_pte entry so that migration code knows it does not have to do
> anything here.
>

So that pte_clear is not expecting to do any sort of tlb flushes etc ? The
idea is to put new_pte = 0 ?.  

Can we do all those conditionals without using pte bits ? A check like
pte_present, is_zero_pfn etc confuse the reader. Instead can
we do

if (pte_state[i] == SKIP_LOOP_FIRST)

if (pte_state[i] == SKIP_LOOP_SECOND)

I understand that we want to return new_pte array with valid pages, so
may be the above will make code complex, but atleast code should have
more comments explaining each step

-aneesh

[toc] | [prev] | [next] | [standalone]

#1361893 — Re: [PATCH v12 21/29] HMM: mm add helper to update page table when migrating memory back v2.

On Mon, Mar 21, 2016 at 07:18:41PM +0530, Aneesh Kumar K.V wrote:
> Jerome Glisse <j.glisse@gmail.com> writes:
> > [ text/plain ]
> > On Mon, Mar 21, 2016 at 04:57:32PM +0530, Aneesh Kumar K.V wrote:
> >> Jérôme Glisse <jglisse@redhat.com> writes:

[...]

> >> > +		ptep = pte_offset_map_lock(mm, pmdp, addr, &ptl);
> >> > +		for (cstart = addr, i = (addr - start) >> PAGE_SHIFT,
> >> > +		     next = min((addr + PMD_SIZE) & PMD_MASK, end);
> >> > +		     addr < next; addr += PAGE_SIZE, ptep++, i++) {
> >> > +			swp_entry_t entry;
> >> > +
> >> > +			entry = pte_to_swp_entry(*ptep);
> >> > +			if (pte_none(*ptep) || pte_present(*ptep) ||
> >> > +			    !is_hmm_entry(entry) ||
> >> > +			    is_hmm_entry_locked(entry))
> >> > +				continue;
> >> > +
> >> > +			set_pte_at(mm, addr, ptep, hmm_entry);
> >> > +			new_pte[i] = pte_mkspecial(pfn_pte(my_zero_pfn(addr),
> >> > +						   vma->vm_page_prot));
> >> > +		}
> >> > +		pte_unmap_unlock(ptep - 1, ptl);
> >> 
> >> 
> >> I guess this is fixing all the ptes in the cpu page table mapping a pmd
> >> entry. But then what is below ?
> >
> > Because we are dealing with special swap entry we know we can not have huge pages.
> > So we only care about HMM special swap entry. We record entry we want to migrate
> > in the new_pte array. The loop above is under pmd spin lock, the loop below does
> > memory allocation and we do not want to hold any spin lock while doing allocation.
> >
> 
> Can this go as code comment ?

Yes of course, i should have added more comment in first place.


> >> > +
> >> > +		for (addr = cstart, i = (addr - start) >> PAGE_SHIFT;
> >> > +		     addr < next; addr += PAGE_SIZE, i++) {
> >> 
> >> Your use of vairable addr with multiple loops updating then is also
> >> making it complex. We should definitely add more comments here. I guess
> >> we are going through the same range we iterated above here.
> >
> > Correct we are going over the exact same range, i am keeping the addr only
> > for alloc_zeroed_user_highpage_movable() purpose.
> >
> 
> Can we use a different variable name there ?

If you have suggestion for name ? I am just lacking imagination but i can use
a different name like vaddr.


> >> > +			struct mem_cgroup *memcg;
> >> > +			struct page *page;
> >> > +
> >> > +			if (!pte_present(new_pte[i]))
> >> > +				continue;
> >> 
> >> What is that checking for ?. We set that using pte_mkspecial above ?
> >
> > Not all entry in the range might match the criteria (ie special unlocked HMM swap
> > entry). We want to allocate pages only for entry that match the criteria.
> >
> 
> Since we did in the beginning, 
> 	memset(new_pte, 0, sizeof(pte_t) * ((end - start) >> PAGE_SHIFT));
> 
> we should not find present bit set ? using present there is confusing,
> may be pte_none(). Also with comments around explaining the details ?

Yes pte_none() will works too, i will use that and add comments.


> >> > +			page = alloc_zeroed_user_highpage_movable(vma, addr);
> >> > +			if (!page) {
> >> > +				ret = -ENOMEM;
> >> > +				break;
> >> > +			}
> >> > +			__SetPageUptodate(page);
> >> > +			if (mem_cgroup_try_charge(page, mm, GFP_KERNEL,
> >> > +						  &memcg)) {
> >> > +				page_cache_release(page);
> >> > +				ret = -ENOMEM;
> >> > +				break;
> >> > +			}
> >> > +			/*
> >> > +			 * We can safely reuse the s_mem/mapping field of page
> >> > +			 * struct to store the memcg as the page is only seen
> >> > +			 * by HMM at this point and we can clear it before it
> >> > +			 * is public see mm_hmm_migrate_back_cleanup().
> >> > +			 */
> >> > +			page->s_mem = memcg;
> >> > +			new_pte[i] = mk_pte(page, vma->vm_page_prot);
> >> > +			if (vma->vm_flags & VM_WRITE) {
> >> > +				new_pte[i] = pte_mkdirty(new_pte[i]);
> >> > +				new_pte[i] = pte_mkwrite(new_pte[i]);
> >> > +			}
> >> 
> >> Why mark it dirty if vm_flags is VM_WRITE ?
> >
> > It is a left over of some debuging i was doing, i missed it.

I actually remember why i set the dirty bit, i wanted to change the driver
API to have driver clear the dirty bit if they did not write instead on
relying on them to set it if they did. I thought it was a safer to cope with
potentialy buggy driver. I might update patchset to do that.

[...]

> >> > +		for (addr = cstart, i = (addr - start) >> PAGE_SHIFT;
> >> > +		     addr < next; addr += PAGE_SIZE, ptep++, i++) {
> >> > +			unsigned long pfn = pte_pfn(new_pte[i]);
> >> > +
> >> > +			if (!pte_present(new_pte[i]) || !is_zero_pfn(pfn))
> >> > +				continue;
> >> 
> 
> So here we are using the fact that we had set new pte using zero pfn in
> the firs loop and hence if we find a present new_pte with zero pfn, it implies we
> failed to allocate a page for that ?

Yes that's correct. I could use a another pte flag instead on relying on zero
pfn.

[...]

> >> > +
> >> > +			set_pte_at(mm, addr, ptep, hmm_entry);
> >> > +			pte_clear(mm, addr, &new_pte[i]);
> >> 
> >> what is that pte_clear for ?. Handling of new_pte needs more code comments.
> >> 
> >
> > Entry for which we failed to allocate memory we clear the special HMM swap entry
> > as well as the new_pte entry so that migration code knows it does not have to do
> > anything here.
> >
> 
> So that pte_clear is not expecting to do any sort of tlb flushes etc ? The
> idea is to put new_pte = 0 ?.  

Correct, no tlb flushing needed, new_pte is a private array use only during
migration and never expose to outside world. I will change to new_pte[i] = 0
instead.


> 
> Can we do all those conditionals without using pte bits ? A check like
> pte_present, is_zero_pfn etc confuse the reader. Instead can
> we do
> 
> if (pte_state[i] == SKIP_LOOP_FIRST)
> 
> if (pte_state[i] == SKIP_LOOP_SECOND)
> 
> I understand that we want to return new_pte array with valid pages, so
> may be the above will make code complex, but atleast code should have
> more comments explaining each step

Well another point of new_pte is that we can directly use the new_pte
value to update the CPU page table in the final migration step. But i
can define some HMM_PTE_MIGRATE, HMM_PTE_RESTORE as alias of existing pte
flag and they will be clear along the way depending on outcomes of each
step.

Jérôme

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web