Re: [PATCH] mremap: Avoid TLB flushing anonymous pages that are not in swap cache

[Nadav Amit <nadav.amit@gmail.com>]

Mel Gorman <mgorman@techsingularity.net> 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.