From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.2 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4A3BFC433E2 for ; Wed, 16 Sep 2020 11:13:56 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id BD78F21974 for ; Wed, 16 Sep 2020 11:13:55 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="KpU44mYI" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org BD78F21974 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 02F106B0003; Wed, 16 Sep 2020 07:13:55 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id F218A6B0055; Wed, 16 Sep 2020 07:13:54 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E0F3E6B006C; Wed, 16 Sep 2020 07:13:54 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0087.hostedemail.com [216.40.44.87]) by kanga.kvack.org (Postfix) with ESMTP id CB5456B0003 for ; Wed, 16 Sep 2020 07:13:54 -0400 (EDT) Received: from smtpin25.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 837A42C07 for ; Wed, 16 Sep 2020 11:13:54 +0000 (UTC) X-FDA: 77268664788.25.bead97_171555b27119 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin25.hostedemail.com (Postfix) with ESMTP id 4B3B91804E3A1 for ; Wed, 16 Sep 2020 11:13:54 +0000 (UTC) X-HE-Tag: bead97_171555b27119 X-Filterd-Recvd-Size: 8747 Received: from mail-lf1-f67.google.com (mail-lf1-f67.google.com [209.85.167.67]) by imf41.hostedemail.com (Postfix) with ESMTP for ; Wed, 16 Sep 2020 11:13:53 +0000 (UTC) Received: by mail-lf1-f67.google.com with SMTP id d15so6496091lfq.11 for ; Wed, 16 Sep 2020 04:13:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=ulziYxRdZ0MQknNtI0gQrRWTyPDclBN7Ur2ES5F9lNc=; b=KpU44mYIdAvb8RdogyVQ7DXO10QcNpOebBNw5QgfSPhlLov9Bb8PGGhKRmObeh5Xfr ONVX+Y9HphhlxZTmlHMcKIxcx+I8f+Ebw0dLsIEFZapF4PRPpz6OzQvQmTrwAt5YBIZa hgzGvDK3NNOgBH1bF7nyWYmv6ZBK0w3GDlPeHhPwVP/osdWZCwEZ1A89zGCSBIe2ncSg oTYergGC46K/dUQGGRKHnF5ONYNx8w5LFqt5pRXy2XQvq4EWxVPI1aXdjnHVh7YeDUBH zSu45y1C0z5zv+lB4QTzWJXTu1WlJMW8QOzijl9rzGLBPsfaqM9jAesx+vWOftx6mJRh dW8Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=ulziYxRdZ0MQknNtI0gQrRWTyPDclBN7Ur2ES5F9lNc=; b=bpAihppeKiRyGrl6MD5Zk8HU6UKCxvMq4IyqRjTb0wAaZYd2inOI0RhwSil58PnHDh FcGJEiBxHBGLGIa4MnU6RBx4buZhPd+0vj3vhG7I1wmLqBMZN1zj32cIRvJfzPjVBSY2 KAUfMedTzqMpqD9WEJXspZo7J2taPMGIsMlsrghmqK3HQzk1S7wIktav6bE7b0+rje3k gUPK/tJbzN1aUcYj0vjkM+/VAX7ixYz0pz9SojIGWmvT4U2126JGp5ba1SuUZSx749ed CucNNLwKk6xc9ohKmPiL46EJmAPLdC5LIYrFYN7yEu4SQTWV+mVlOBuSIXvTA2kVPx4i HK5Q== X-Gm-Message-State: AOAM531YlCqD1ysxuJFZL7U4VOaBpYhyxvr1GSSL80rVXqC36w79nED0 2D7bPlLmpfYv1CaoK3H8sfU= X-Google-Smtp-Source: ABdhPJwGojRxukMZdzJAf8ylMPIj5ve/oi4386fEdCSgACX1u9DsH2GLqaG/EBkOEJlQ4KcXGGw3lg== X-Received: by 2002:a19:8142:: with SMTP id c63mr7488186lfd.175.1600254832412; Wed, 16 Sep 2020 04:13:52 -0700 (PDT) Received: from [192.168.8.100] (188.147.112.12.nat.umts.dynamic.t-mobile.pl. [188.147.112.12]) by smtp.gmail.com with ESMTPSA id 140sm4528272lfj.146.2020.09.16.04.13.51 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 16 Sep 2020 04:13:51 -0700 (PDT) Subject: Re: [RFC PATCH] mm/page_alloc.c: micro-optimization reduce oom critical section size To: Michal Hocko Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, akpm@linux-foundation.org References: <20200914100654.21746-1-mateusznosek0@gmail.com> <20200914142233.GT16999@dhcp22.suse.cz> <20200915140406.GE3736@dhcp22.suse.cz> From: Mateusz Nosek Message-ID: <8a02fa51-c2b8-e1ec-66b3-1b00954b7040@gmail.com> Date: Wed, 16 Sep 2020 13:13:51 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <20200915140406.GE3736@dhcp22.suse.cz> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 4B3B91804E3A1 X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam04 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Hello, Thank you for your comments. I will modify the patch where necessary and resend v2, but first I will make 100% sure about the lack of synchronization problem, that might potentially be there as you mentioned in previous mail , and try to check some numbers for my support. Sincerely yours, Mateusz Nosek On 9/15/2020 4:04 PM, Michal Hocko wrote: > On Tue 15-09-20 15:09:59, Mateusz Nosek wrote: >> >> >> On 9/14/2020 4:22 PM, Michal Hocko wrote: >>> On Mon 14-09-20 12:06:54, mateusznosek0@gmail.com wrote: >>>> From: Mateusz Nosek >>>> >>>> Most operations from '__alloc_pages_may_oom' do not require oom_mutex hold. >>>> Exception is 'out_of_memory'. The patch refactors '__alloc_pages_may_oom' >>>> to reduce critical section size and improve overall system performance. >>> >>> This is a real slow path. What is the point of optimizing it? Do you >>> have any numbers? >>> >> >> I agree that as this is the slow path, then the hard, complicated >> optimizations are not recommended. In my humble opinion introduced patch is >> not complex and does not decrease code readability or maintainability. In a >> nutshell I see no drawbacks of applying it. > > This is clearly a matter of taste. I do not see a good reason to apply > it TBH. It is a claimed optimization without any numbers to back that > claim. It is also a tricky area so I am usually very careful to touch > this code without a strong reason. Others might feel differently of > course. > > [...] > > Anyway, I have only now looked closer at the patch... > >>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >>>> index b9bd75cacf02..b07f950a5825 100644 >>>> --- a/mm/page_alloc.c >>>> +++ b/mm/page_alloc.c >>>> @@ -3935,18 +3935,7 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order, >>>> .order = order, >>>> }; >>>> struct page *page; >>>> - >>>> - *did_some_progress = 0; >>>> - >>>> - /* >>>> - * Acquire the oom lock. If that fails, somebody else is >>>> - * making progress for us. >>>> - */ >>>> - if (!mutex_trylock(&oom_lock)) { >>>> - *did_some_progress = 1; >>>> - schedule_timeout_uninterruptible(1); >>>> - return NULL; >>>> - } >>>> + bool success; >>>> /* >>>> * Go through the zonelist yet one more time, keep very high watermark >>>> @@ -3959,14 +3948,17 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order, >>>> ~__GFP_DIRECT_RECLAIM, order, >>>> ALLOC_WMARK_HIGH|ALLOC_CPUSET, ac); >>>> if (page) >>>> - goto out; >>>> + return page; >>>> + >>>> + /* Check if somebody else is making progress for us. */ >>>> + *did_some_progress = mutex_is_locked(&oom_lock); > > This is not only quite ugly but wrong as well. In general checking for a > lock state is racy unless the lock is taken somewhere up the call chain. > > In this particular case it wouldn't be a big deal because an additional > retry (did_some_progress = 1) is not really critical. It would likely be > nicer to be deterministic here and not retry on all the early bailouts > regardless of the lock state. > >>>> /* Coredumps can quickly deplete all memory reserves */ >>>> if (current->flags & PF_DUMPCORE) >>>> - goto out; >>>> + return NULL; >>>> /* The OOM killer will not help higher order allocs */ >>>> if (order > PAGE_ALLOC_COSTLY_ORDER) >>>> - goto out; >>>> + return NULL; >>>> /* >>>> * We have already exhausted all our reclaim opportunities without any >>>> * success so it is time to admit defeat. We will skip the OOM killer >>>> @@ -3976,12 +3968,12 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order, >>>> * The OOM killer may not free memory on a specific node. >>>> */ >>>> if (gfp_mask & (__GFP_RETRY_MAYFAIL | __GFP_THISNODE)) >>>> - goto out; >>>> + return NULL; >>>> /* The OOM killer does not needlessly kill tasks for lowmem */ >>>> if (ac->highest_zoneidx < ZONE_NORMAL) >>>> - goto out; >>>> + return NULL; >>>> if (pm_suspended_storage()) >>>> - goto out; >>>> + return NULL; >>>> /* >>>> * XXX: GFP_NOFS allocations should rather fail than rely on >>>> * other request to make a forward progress. >>>> @@ -3992,8 +3984,20 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order, >>>> * failures more gracefully we should just bail out here. >>>> */ >>>> + /* >>>> + * Acquire the oom lock. If that fails, somebody else is >>>> + * making progress for us. >>>> + */ >>>> + if (!mutex_trylock(&oom_lock)) { >>>> + *did_some_progress = 1; >>>> + schedule_timeout_uninterruptible(1); >>>> + return NULL; >>>> + } >>>> + success = out_of_memory(&oc); >>>> + mutex_unlock(&oom_lock); >>>> + >>>> /* Exhausted what can be done so it's blame time */ >>>> - if (out_of_memory(&oc) || WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL)) { >>>> + if (success || WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL)) { >>>> *did_some_progress = 1; >>>> /* >>>> @@ -4004,8 +4008,7 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order, >>>> page = __alloc_pages_cpuset_fallback(gfp_mask, order, >>>> ALLOC_NO_WATERMARKS, ac); >>>> } >>>> -out: >>>> - mutex_unlock(&oom_lock); >>>> + >>>> return page; >>>> } >>>> -- >>>> 2.20.1 >>>> >>> >> Sincerely yours, >> Mateusz Nosek >