Re: [PATCH v5 1/7] powerpc/mm: update ptep_set_access_flag to not do full mm tlb flush

[Balbir Singh <bsingharora@gmail.com>]

On 24/11/16 01:36, Aneesh Kumar K.V wrote:
> Balbir Singh <bsingharora@gmail.com> writes:
> 
>> On 23/11/16 22:53, Aneesh Kumar K.V wrote:
>>> Balbir Singh <bsingharora@gmail.com> writes:
>>>
>>>> On 23/11/16 22:09, Aneesh Kumar K.V wrote:
>>>>> When we are updating pte, we just need to flush the tlb mapping for
>>>>> that pte. Right now we do a full mm flush because we don't track page
>>>>> size. Update the interface to track the page size and use that to
>>>>> do the right tlb flush.
>>>>>
>>>>
>>>> Could you also clarify the scope -- this seems to be _radix_ only.
>>>> The problem statement is not very clear and why doesn't the flush_tlb_page()
>>>> following ptep_set_access_flags() work? What else do we need to do?
>>>
>>> Yes it modifies only radix part.  Don't understand the flush_tlb_page()
>>> part of the comment above. We are modifying the tlbflush that we need to do in the pte update
>>> sequence for DD1. ie, we need to do the flush before we can set the pte
>>> with new value.
>>>
>>> Also in this specific case, we can idealy drop that flush_tlb_page,
>>> because relaxing an access really don't need a tlb flush from generic
>>> architecture point of view. I left it there to make sure, we measure and
>>> get the invalidate path correct before going ahead with that
>>> optimization.
>>>
>>
>> OK.. here is my untested solution. I've only compiled it.
>> It breaks the 64/hash/radix abstractions, but it makes the
>> changes much simpler
>>
>> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> 
> I find the below one more confusing and complicated, spreading the
> details of DD1 around the code. I am not sure what extra i could have
> done to simplify the code. We have done the arch pte updates such that
> most of the update use the pte_update() interface and the one which relax
> the access bits get to ptep_set_access_flag. All pte updated rules are
> contained there. What you did below is that you moved the dd1 sequence
> out to a place where page size is available. What I did in my patch is to
> pass page size around. IMHO it is a matter of style. I also want to pass
> page size around so that we keep huge_pte_update, pte_update,
> ptep_set_access_flags all similar.
> 

Agreed and the reason I did it that way is that after a while we know
the _dd1_ variants need not be supported/maintained at all. It is a
matter of style and I was wondering if we need to change the API
to pass address and page_size as a permanent solution.

Balbir Singh.