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

[Michal Hocko <mhocko@suse.com>]

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.

> More worrisome part in that patch is that I don't know whether using
> trylock if blockable == false at entry is really sufficient. For example,
> since __gnttab_unmap_refs_async() from gnttab_unmap_refs_async() from
> gnttab_unmap_refs_sync() from __unmap_grant_pages() from
> unmap_grant_pages() from unmap_if_in_range() from mn_invl_range_start()
> involves schedule_delayed_work() which could be blocked on memory
> allocation under OOM situation, wait_for_completion() from
> gnttab_unmap_refs_sync() might deadlock? I don't know...

Not really sure why this is in the changelog as it is unrelated to the
fix. Anyway let me try to check...

OK, so I've added in_range(map, start, end) check to not go that
direction. But for some reason that check doesn't consider blockable
value. So it looks definitely wrong. I must have screwed up when
rebasing or something. Thanks for catching that up. I will send a fix.

> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  mm/oom_kill.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index b5b25e4..4f431c1 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -522,6 +522,7 @@ bool __oom_reap_task_mm(struct mm_struct *mm)
>  
>  			tlb_gather_mmu(&tlb, mm, start, end);
>  			if (mmu_notifier_invalidate_range_start_nonblock(mm, start, end)) {
> +				tlb_finish_mmu(&tlb, start, end);
>  				ret = false;
>  				continue;
>  			}
> -- 
> 1.8.3.1
> 

-- 
Michal Hocko
SUSE Labs