Re: [PATCH] mm, oom: Always call tlb_finish_mmu().

[Michal Hocko <mhocko@suse.com>]

On Thu 23-08-18 22:48:22, Tetsuo Handa wrote:
> On 2018/08/23 20:59, Michal Hocko wrote:
> > On Thu 23-08-18 20:30:48, Tetsuo Handa wrote:
> >> 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