Re: [PATCH 01/11] mm/mremap: introduce more mergeable mremap via MREMAP_RELOCATE_ANON

[David Hildenbrand <david@redhat.com>]

> 
>>
>>> +	/* The above check should imply these. */
>>> +	VM_WARN_ON_ONCE(folio_mapcount(folio) > folio_nr_pages(folio));
>>> +	VM_WARN_ON_ONCE(!PageAnonExclusive(folio_page(folio, 0)));
>>
>> This can trigger in one nasty case, where we can lose the PAE bit during
>> swapin (refault from the swapcache while the folio is under writeback, and
>> the device does not allow for modifying the data while under writeback).
> 
> Ugh god wasn't aware of that. So maybe drop this second one?

Yes.

> 
>>
>>> +
>>> +	/*
>>> +	 * A pinned folio implies that it will be used for a duration longer
>>> +	 * than that over which the mmap_lock is held, meaning that another part
>>> +	 * of the kernel may be making use of this folio.
>>> +	 *
>>> +	 * Since we are about to manipulate index & mapping fields, we cannot
>>> +	 * safely proceed because whatever has pinned this folio may then
>>> +	 * incorrectly assume these do not change.
>>> +	 */
>>> +	if (folio_maybe_dma_pinned(folio))
>>> +		goto out;
>>
>> As discussed, this can race with GUP-fast. SO *maybe* we can just allow for
>> moving these.
> 
> I'm guessing you mean as discussed below? :P Or in the cover letter I've not
> read yet? :P

The latter .. IIRC :P It was late ...

> 
> Yeah, to be honest you shouldn't be fiddling with index, mapping anyway except
> via rmap logic.
> 
> I will audit access of these fields just to be safe.
> 

[...]

>>> +
>>> +	state.ptep = ptep_start;
>>> +	for (; !pte_done(&state); pte_next(&state, nr_pages)) {
>>> +		pte_t pte = ptep_get(state.ptep);
>>> +
>>> +		if (pte_none(pte) || !pte_present(pte)) {
>>> +			nr_pages = 1;
>>
>> What if we have
>>
>> (a) A migration entry (possibly we might fail migration and simply remap the
>> original folio)
>>
>> (b) A swap entry with a folio in the swapcache that we can refault.
>>
>> I don't think we can simply skip these ...
> 
> Good point... will investigate these cases.

migration entries are really nasty ... probably have to wait for the 
migration entry to become a present pte again.

swap entries ... we could lookup any folio in the swapcache and adjust that.

> 
>>
>>> +			continue;
>>> +		}
>>> +
>>> +		nr_pages = relocate_anon_pte(pmc, &state, undo);
>>> +		if (!nr_pages) {
>>> +			ret = false;
>>> +			goto out;
>>> +		}
>>> +	}
>>> +
>>> +	ret = true;
>>> +out:
>>> +	pte_unmap_unlock(ptep_start, state.ptl);
>>> +	return ret;
>>> +}
>>> +
>>> +static bool __relocate_anon_folios(struct pagetable_move_control *pmc, bool undo)
>>> +{
>>> +	pud_t *pudp;
>>> +	pmd_t *pmdp;
>>> +	unsigned long extent;
>>> +	struct mm_struct *mm = current->mm;
>>> +
>>> +	if (!pmc->len_in)
>>> +		return true;
>>> +
>>> +	for (; !pmc_done(pmc); pmc_next(pmc, extent)) {
>>> +		pmd_t pmd;
>>> +		pud_t pud;
>>> +
>>> +		extent = get_extent(NORMAL_PUD, pmc);
>>> +
>>> +		pudp = get_old_pud(mm, pmc->old_addr);
>>> +		if (!pudp)
>>> +			continue;
>>> +		pud = pudp_get(pudp);
>>> +
>>> +		if (pud_trans_huge(pud) || pud_devmap(pud))
>>> +			return false;
>>
>> We don't support PUD-size THP, why to we have to fail here?
> 
> This is just to be in line with other 'magical future where we have PUD THP'
> stuff in mremap.c.
> 
> A later commit that permits huge folio support actually lets us support these...
> 
>>
>>> +
>>> +		extent = get_extent(NORMAL_PMD, pmc);
>>> +		pmdp = get_old_pmd(mm, pmc->old_addr);
>>> +		if (!pmdp)
>>> +			continue;
>>> +		pmd = pmdp_get(pmdp);
>>> +
>>> +		if (is_swap_pmd(pmd) || pmd_trans_huge(pmd) ||
>>> +		    pmd_devmap(pmd))
>>> +			return false;
>>
>> Okay, this case could likely be handled later (present anon folio or
>> migration entry; everything else, we can skip).
> 
> Hmm, but how? the PMD cannot be traversed in this case?
> 
> 'Present' migration entry? Migration entries are non-present right? :) Or is it
> different at PMD?

"present anon folio" or "migration entry" :)

So that latter meant a PMD migration entry (that is non-present)

[...]

>>>    	pmc.new = new_vma;
>>> +	if (relocate_anon) {
>>> +		lock_new_anon_vma(new_vma);
>>> +		pmc.relocate_locked = new_vma;
>>> +
>>> +		if (!relocate_anon_folios(&pmc, /* undo= */false)) {
>>> +			unsigned long start = new_vma->vm_start;
>>> +			unsigned long size = new_vma->vm_end - start;
>>> +
>>> +			/* Undo if fails. */
>>> +			relocate_anon_folios(&pmc, /* undo= */true);
>>
>> You'd assume this cannot fail, but I think it can: imagine concurrent
>> GUP-fast ...
> 
> Well if we change the racey code to ignore DMA pinned we should be ok right?

We completely block migration/swapout, or could they happen 
concurrently? I assume you'd block them already using the rmap locks in 
write mode.

> 
>>
>> I really wish we can find a way to not require the fallback.
> 
> Yeah the fallback is horrible but we really do need it. See the page table move
> fallback code for nightmares also :)
> 
> We could also alternatively:
> 
> - Have some kind of anon_vma fragmentation where some folios in range reference
>    a different anon_vma that we link to the original VMA (quite possibly very
>    broken though).
> 
> - Keep a track of folios somehow and separate them from the page table walk (but
>    then we risk races)
> 
> - Have some way of telling the kernel that such a situation exists with a new
>    object that can be pointed to by folio->mapping, that the rmap code recognise,
>    like essentially an 'anon_vma migration entry' which can fail.
> 
> I already considered combining this operation with the page table move
> operation, but the locking gets horrible and the undo is categorically much
> worse and I'm not sure it's actually workable.

Yeah, I have to further think about that. :(

-- 
Cheers,

David / dhildenb