From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752846AbbKQK6h (ORCPT ); Tue, 17 Nov 2015 05:58:37 -0500 Received: from www262.sakura.ne.jp ([202.181.97.72]:40574 "EHLO www262.sakura.ne.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752283AbbKQK6f (ORCPT ); Tue, 17 Nov 2015 05:58:35 -0500 Subject: Re: [PATCH 2/2] mm: do not loop over ALLOC_NO_WATERMARKS without triggering reclaim To: mhocko@kernel.org, Andrew Morton References: <1447680139-16484-1-git-send-email-mhocko@kernel.org> <1447680139-16484-3-git-send-email-mhocko@kernel.org> Cc: Mel Gorman , David Rientjes , linux-mm@kvack.org, LKML , Michal Hocko From: Tetsuo Handa Message-ID: <564B0841.6030409@I-love.SAKURA.ne.jp> Date: Tue, 17 Nov 2015 19:58:09 +0900 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <1447680139-16484-3-git-send-email-mhocko@kernel.org> Content-Type: text/plain; charset=iso-2022-jp Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Michal Hocko wrote: > __alloc_pages_slowpath is looping over ALLOC_NO_WATERMARKS requests if > __GFP_NOFAIL is requested. This is fragile because we are basically > relying on somebody else to make the reclaim (be it the direct reclaim > or OOM killer) for us. The caller might be holding resources (e.g. > locks) which block other other reclaimers from making any progress for > example. Remove the retry loop and rely on __alloc_pages_slowpath to > invoke all allowed reclaim steps and retry logic. This implies invoking OOM killer, doesn't it? > /* Avoid recursion of direct reclaim */ > - if (current->flags & PF_MEMALLOC) > + if (current->flags & PF_MEMALLOC) { > + /* > + * __GFP_NOFAIL request from this context is rather bizarre > + * because we cannot reclaim anything and only can loop waiting > + * for somebody to do a work for us. > + */ > + if (WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL)) { > + cond_resched(); > + goto retry; I think that this "goto retry;" omits call to out_of_memory() which is allowed for __GFP_NOFAIL allocations. Even if this is what you meant, current thread can be a workqueue, which currently need a short sleep (as with wait_iff_congested() changes), can't it? > + } > goto nopage; > + } > > /* Avoid allocations with no watermarks from looping endlessly */ > if (test_thread_flag(TIF_MEMDIE) && !(gfp_mask & __GFP_NOFAIL)) > Well, is it cond_resched() which should include if (current->flags & PF_WQ_WORKER) schedule_timeout(1); than wait_iff_congested() because not all yield calls use wait_iff_congested() and giving pending workqueue jobs a chance to be processed is anyway preferable? int __sched _cond_resched(void) { if (should_resched(0)) { if ((current->flags & PF_WQ_WORKER) && workqueue_has_pending_jobs()) schedule_timeout(1); else preempt_schedule_common(); return 1; } return 0; }