On Thu 23-08-18 22:48:22, Tetsuo Handa wrote:

On 2018/08/23 20:59, Michal Hocko wrote:

quoted

On Thu 23-08-18 20:30:48, Tetsuo Handa wrote:

quoted

Commit 93065ac753e44438 ("mm, oom: distinguish blockable mode for mmu
notifiers") added "continue;" without calling tlb_finish_mmu(). I don't
know whether tlb_flush_pending imbalance causes problems other than
extra cost, but at least it looks strange.

tlb_flush_pending has mm scope and it would confuse
mm_tlb_flush_pending. At least ptep_clear_flush could get confused and
flush unnecessarily for prot_none entries AFAICS. Other paths shouldn't
trigger for oom victims. Even ptep_clear_flush is unlikely to happen.
So nothing really earth shattering but I do agree that it looks weird
and should be fixed.

OK. But what is the reason we call tlb_gather_mmu() before
mmu_notifier_invalidate_range_start_nonblock() ?
I want that the fix explains why we can't do

-			tlb_gather_mmu(&tlb, mm, start, end);
 			if (mmu_notifier_invalidate_range_start_nonblock(mm, start, end)) {
 				ret = false;
 				continue;
 			}
+			tlb_gather_mmu(&tlb, mm, start, end);

This should be indeed doable because mmu notifiers have no way to know
about tlb_gather. I have no idea why we used to have tlb_gather_mmu like
that before. Most probably a C&P from munmap path where it didn't make
any difference either. A quick check shows that tlb_flush_pending is the
only mm scope thing and none of the notifiers really depend on it.

I would be calmer if both paths were in sync in that regards. So I think
it would be better to go with your previous version first. Maybe it
makes sense to switch the order but I do not really see a huge win for
doing so.
-- 
Michal Hocko
SUSE Labs