Re: [PATCH 4/4] MIPS: Sync icache & dcache in set_pte_at

[David Daney <ddaney@caviumnetworks.com>]

On 03/01/2016 09:19 AM, Paul Burton wrote:
> On Tue, Mar 01, 2016 at 09:13:23AM -0800, David Daney wrote:
>> On 02/29/2016 06:37 PM, Paul Burton wrote:
>> [...]
>>> @@ -234,6 +237,22 @@ static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *pt
>>>   }
>>>   #endif
>>>
>>> +static inline void set_pte_at(struct mm_struct *mm, unsigned long addr,
>>> +			      pte_t *ptep, pte_t pteval)
>>> +{
>>> +	extern void __update_cache(unsigned long address, pte_t pte);
>>> +
>>> +	if (!pte_present(pteval))
>>> +		goto cache_sync_done;
>>> +
>>> +	if (pte_present(*ptep) && (pte_pfn(*ptep) == pte_pfn(pteval)))
>>> +		goto cache_sync_done;
>>> +
>>> +	__update_cache(addr, pteval);
>>> +cache_sync_done:
>>> +	set_pte(ptep, pteval);
>>> +}
>>> +
>>
>> This seems crazy.
>
> Perhaps, but also correct...
>
>> I don't think any other architecture does this type of work in set_pte_at().
>
> Yes they do. As mentioned in the commit message see arm, ia64 or powerpc
> for architectures that all do the same sort of thing in set_pte_at.
>
>> Can you look into finding a better way?
>
> Not that I can see.
>
>> What if you ...
>>
>>
>>>   /*
>>>    * (pmds are folded into puds so this doesn't get actually called,
>>>    * but the define is needed for a generic inline function.)
>>> @@ -430,15 +449,12 @@ static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
>>>
>>>   extern void __update_tlb(struct vm_area_struct *vma, unsigned long address,
>>>   	pte_t pte);
>>> -extern void __update_cache(struct vm_area_struct *vma, unsigned long address,
>>> -	pte_t pte);
>>>
>>>   static inline void update_mmu_cache(struct vm_area_struct *vma,
>>>   	unsigned long address, pte_t *ptep)
>>>   {
>>>   	pte_t pte = *ptep;
>>>   	__update_tlb(vma, address, pte);
>>> -	__update_cache(vma, address, pte);
>>
>> ... Reversed the order of these two operations?
>
> It would make no difference. The window for the race exists between
> flush_dcache_page & set_pte_at. update_mmu_cache isn't called until
> later than set_pte_at, so cannot possibly avoid the race. The commit
> message walks through where the race exists - I don't think you've read
> it.


I think the code that calls set_pte_at() should be fixed.

If cache maintenance is needed before modifying the page tables, that is 
explicitly done in the calling code.

In migrate.c (remove_migration_pte, similar in do_swap_page) we have:
    .
    .
    .
	flush_dcache_page(new);
	set_pte_at(mm, addr, ptep, pte);
    .
    .
    .

Similar in huge_memory.c (unfreeze_page_vma, freeze_page_vma, etc.)

The point being, the callers have the knowledge about what is changing 
and should make sure they do the right thing to keep the caches 
consistent.  The job of set_pte_at() is to manipulate the page tables, 
nothing else.