Re: [PATCH v4 2/4] mm: Add function to support extra actions on swap in/out

[Khalid Aziz <khalid.aziz@oracle.com>]

On 01/11/2017 09:56 AM, Dave Hansen wrote:
> On 01/11/2017 08:12 AM, Khalid Aziz wrote:
>> +#ifndef set_swp_pte_at
>> +#define set_swp_pte_at(mm, addr, ptep, pte, oldpte)	\
>> +		set_pte_at(mm, addr, ptep, pte)
>> +#endif
>
> BTW, thanks for the *much* improved description of the series.  This is
> way easier to understand.

Your feedback was very helpful in getting me there.

>
> I really don't think this is the interface we want, though.
> set_swp_pte_at() is really doing *two* things:
> 1. Detecting _PAGE_MCD_4V and squirreling the MCD data away at swap-out
> 2. Reading back in the MCD data at swap-on
>
> You're effectively using (!pte_none(pte) && !pte_present(pte)) to
> determine whether you're at swap in or swap out time.  That's goofy, IMNHO.
>
> It isn't obvious from the context, but this hunk is creating a migration
> PTE.  Why is ADI tag manipulation needed?  We're just changing the
> physical address of the underlying memory, but neither the
> application-visible contents nor the tags are changing.

Memory controller associates tags with physical memory. MMU verifies 
these tags against the tag embedded in VA. If physical memory is moved, 
tags must move with it.

>
>> @@ -1539,7 +1539,7 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>>  		swp_pte = swp_entry_to_pte(entry);
>>  		if (pte_soft_dirty(pteval))
>>  			swp_pte = pte_swp_mksoft_dirty(swp_pte);
>> -		set_pte_at(mm, address, pte, swp_pte);
>> +		set_swp_pte_at(mm, address, pte, swp_pte, pteval);
>>  	} else if (PageAnon(page)) {
>>  		swp_entry_t entry = { .val = page_private(page) };
>>  		pte_t swp_pte;
>
> Which means you're down to a single call that does swap-out, and a
> single call that does swap-in.  There's no reason to hide all your code
> behind set_pte_at().
>
> Just add a new arch-specific call that takes the VMA and the swap PTE
> and stores the ADI bit in there, here:
>
>> @@ -1572,7 +1572,7 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>>  		swp_pte = swp_entry_to_pte(entry);
>>  		if (pte_soft_dirty(pteval))
>>  			swp_pte = pte_swp_mksoft_dirty(swp_pte);
>> -		set_pte_at(mm, address, pte, swp_pte);
>> +		set_swp_pte_at(mm, address, pte, swp_pte, pteval);
>>  	} else
>
> and in do_swap_page(), do the opposite with a second, new call.

That does sound simpler. I will give it more thought.

Thanks,
Khalid