From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751908AbdH3Rt0 (ORCPT ); Wed, 30 Aug 2017 13:49:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47408 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751777AbdH3RtG (ORCPT ); Wed, 30 Aug 2017 13:49:06 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 78B91C0587E5 Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=aarcange@redhat.com Date: Wed, 30 Aug 2017 19:49:04 +0200 From: Andrea Arcangeli To: Michal Hocko Cc: Andrew Morton , linux-mm@kvack.org, LKML , Michal Hocko Subject: Re: [RFC PATCH] mm, oom_reaper: skip mm structs with mmu notifiers Message-ID: <20170830174904.GF13559@redhat.com> References: <20170830084600.17491-1-mhocko@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170830084600.17491-1-mhocko@kernel.org> User-Agent: Mutt/1.8.3 (2017-05-23) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Wed, 30 Aug 2017 17:49:06 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Michal, On Wed, Aug 30, 2017 at 10:46:00AM +0200, Michal Hocko wrote: > + * TODO: we really want to get rid of this ugly hack and make sure that > + * notifiers cannot block for unbounded amount of time and add > + * mmu_notifier_invalidate_range_{start,end} around unmap_page_range KVM already should be ok in that respect. However the major reason to prefer mmu_notifier_invalidate_range_start/end is those can block and schedule waiting for stuff happening behind the PCI bus easily. So I'm not sure if the TODO is good idea to keep. > + */ > + if (mm_has_notifiers(mm)) { > + schedule_timeout_idle(HZ); Why the schedule_timeout? What's the difference with the OOM reaper going to sleep again in the main loop instead? > + goto unlock_oom; > + } mm_has_notifiers stops changing after obtaining the mmap_sem for reading. See the do_mmu_notifier_register. So it's better to put the mm_has_notifiers check immediately after the below: > if (!down_read_trylock(&mm->mmap_sem)) { > ret = false; > trace_skip_task_reaping(tsk->pid); If we succeed taking the mmap_sem for reading then we read a stable value out of mm_has_notifiers and be sure it won't be set from under us. Otherwise the patch looks fine including the incremental comment about why the mmu_notifier_invalidate_range in MMU gather wasn't enough. Thanks! Andrea