Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx> wrote:
> On Tue, Jun 05, 2018 at 12:53:57PM -0700, Nadav Amit wrote:
>> While I do not have a specific reservation regarding the logic, I find the
>> current TLB invalidation scheme hard to follow and inconsistent. I guess
>> should_force_flush() can be extended and used more commonly to make things
>> clearer.
>>
>> To be more specific and to give an example: Can should_force_flush() be used
>> in zap_pte_range() to set the force_flush instead of the current code?
>>
>> if (!PageAnon(page)) {
>> if (pte_dirty(ptent)) {
>> force_flush = 1;
>> ...
>> }
>
> That check is against !PageAnon pages where it's potentially critical
> that the dirty PTE bit be propogated to the page. You could split the
> separate the TLB flush from the dirty page setting but it's not the same
> class of problem and without perf data, it's not clear it's worthwhile.
>
> Note that I also didn't handle the huge page moving because it's already
> naturally batching a larger range with a lower potential factor of TLB
> flushing and has different potential race conditions.
I noticed.
>
> I agree that the TLB handling would benefit from being simplier but it's
> not a simple search/replace job to deal with the different cases that apply.
I understand. It’s not just a matter of performance: having a consistent
implementation can prevent bugs and allow auditing of the invalidation
scheme.
Anyhow, if I find some free time, I’ll give it a shot.
