From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752745AbdLANcx (ORCPT ); Fri, 1 Dec 2017 08:32:53 -0500 Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:51352 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752379AbdLANcu (ORCPT ); Fri, 1 Dec 2017 08:32:50 -0500 Date: Fri, 1 Dec 2017 13:32:15 +0000 From: Roman Gushchin To: Michal Hocko CC: , Vladimir Davydov , Johannes Weiner , Tetsuo Handa , David Rientjes , Andrew Morton , Tejun Heo , , , , , Subject: Re: [PATCH] mm, oom: simplify alloc_pages_before_oomkill handling Message-ID: <20171201133214.GB7741@castle.DHCP.thefacebook.com> References: <20171130152824.1591-1-guro@fb.com> <20171201091425.ekrpxsmkwcusozua@dhcp22.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20171201091425.ekrpxsmkwcusozua@dhcp22.suse.cz> User-Agent: Mutt/1.9.1 (2017-09-22) X-Originating-IP: [2620:10d:c092:200::1:f2c] X-ClientProxiedBy: LNXP265CA0051.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:5d::15) To SN2PR15MB1088.namprd15.prod.outlook.com (2603:10b6:804:22::10) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: fb5ffcc2-7962-41d9-7f2e-08d538bff81f X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(4534020)(4602075)(4627115)(201703031133081)(201702281549075)(5600026)(4604075)(2017052603286);SRVR:SN2PR15MB1088; X-Microsoft-Exchange-Diagnostics: 1;SN2PR15MB1088;3:m3XKQwyACtD2GfBi60PhAWm8Xn6tF4Hp0qhEFD9YBkXKw1YhihyDYvsTBBGnNw3xh3Okb6ED2plJlKJj17iQiZB/p1Ul3avIXPu+Q+nLwpf8gtelTLuPMp1Emv9/KtWB8H/smAEu3FgQh5RwQGjPfq2nWkJTXAqfxKkge1yfKtCU5CSgU/C9rAryS3LNdN3qFQfKhQAsPKAgAn7IzL9oLcCM6DTyMwlxpQ3PgjMRs/Iby5ycifzFnFq5QE8mM9OX;25:3CcEPmo8pOmnOQUpLyZYY5uc2Rw3PgfRWdcPnk6b/GYgcJ34rUFa4GuIsNM3ISVuN5B9cBMlDlb2rnkv0yibeI7288Qa+YS/y6gij/RtZE8nUrm7A7puspUplkN4JP9aY1pGqRzvhra2Xg349LKeCdpMa59z9fDkTRDCXdfmqTROmhLp9tF1M/ttAYwMvpcp+fNzjZY3BdXx9QCzuqhrqgEGmJhiCAi0/2bGGF9AFEsizypDPAzAlPMNTCGqBlQXBmaOAoQV15QBKdlQyd1yr4PIoV7f1Ewrpl/VpEVvQ40sQYbciqMaANyAwLu0CESYyAk8gNF8iB7P/LLaQlNHHw==;31:CoelJJgRB3gvD0ZyjkoxgB4W74wLizy94BK9LIwv76WVSIvmQ136pGJArheWjyCiGai+BB8SRA1gQQAvspRm1pPj8F12zxFNujNvqgAv9y/B69LYQ5VoYAaOGYOtW9gU+zh90lPnsmseDWlUozh+Zlc6NNPGSlYNlcgUKbOow7HUNRqqqJtZVWRfaKtqzOT70SZ7YoARN1L5fz5QpzRz/fSvDqZ3o4bP58kHNsFyMHE= X-MS-TrafficTypeDiagnostic: SN2PR15MB1088: X-Microsoft-Exchange-Diagnostics: 1;SN2PR15MB1088;20:3iWR87rOoIQj95XkqU4v7ZomlZ4swDao3Ol/wTqC9Fsf37u4E6KE6X78L3zEqiveTQKwXe+f03ZqOh/xMinH/1DXEtts92wBR0nP7K6X+kHGGsGI3uRIpxWOztJsbJH6ahU+CtcumIA7a2a56Lu7X7RbMnwLlBHm5RdbzbcsWHgE09KCo6XGWkdeaHtdbPzbCXT7Bx3SqvMulHq8xWc6Ht1IN9kTTVh7WkWbkA0KZv98+g6jMmxh84UT7O6OhYcTS0ieKfV0wleCwZ1mcvMiM0l894nK3OnbPsyZSEsgsJAlnciAj1RmQTo0oalkBySdlEbD3JW88a7fmEhixYNwmYFcATLJ7s47A3lqEukeE5ZEKhsvmpuo1ddL0OxeAwt+hn5ClrM5QD2cRyI9d8v9GmKsSUQd1fe3xc4KpHQtHFu+Pg+YCeJD1wosQhhnVYTR9f7jU140kxdeGcosVVhITcJd7HBGg6yMKbJdGggQ0DwlkbnNDmxA0/FVqyI+W6/Q;4:i2ZqF0E8ISSS4+IVqDUTnrHC9uNHD45v+DzyYNpmwWsggsH9xz6vGqmpfrJA9IHWTVGOE9Jpn3wXiCqUaV/CInamnxPkN0oA18axqxl43J1ltcJkT1o7fKCCF3EFXJemv/MDnyoqRmBCUtHob3ln7M5n5meYqFcTXc1Ns0npsJNqVC1/ht5Wybk5VH4GU0yPHjlsIj5iT9e9so4A7hLGxzNT1gge+aFVe5gHCkhmpxU3DBtCUEi7Lo+9eooXt7L6Hk6ooFjB+e8qLlTsbdXeAg== X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(11241501159)(6040450)(2401047)(8121501046)(5005006)(93006095)(93001095)(10201501046)(3002001)(3231022)(6041248)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123555025)(20161123558100)(20161123562025)(20161123564025)(20161123560025)(6072148)(201708071742011);SRVR:SN2PR15MB1088;BCL:0;PCL:0;RULEID:(100000803101)(100110400095);SRVR:SN2PR15MB1088; X-Forefront-PRVS: 05087F0C24 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(6009001)(376002)(346002)(39860400002)(366004)(189002)(76104003)(24454002)(199003)(8676002)(81156014)(33656002)(575784001)(86362001)(16586007)(50466002)(101416001)(58126008)(54906003)(316002)(106356001)(105586002)(1076002)(81166006)(23726003)(6116002)(55016002)(83506002)(47776003)(2906002)(7696005)(52116002)(97736004)(76176011)(8936002)(54356011)(478600001)(305945005)(7736002)(189998001)(68736007)(7416002)(53546010)(9686003)(6666003)(2950100002)(6916009)(229853002)(25786009)(6506006)(5660300001)(39060400002)(52396003)(53936002)(6246003)(4326008)(18370500001)(42262002);DIR:OUT;SFP:1102;SCL:1;SRVR:SN2PR15MB1088;H:castle.DHCP.thefacebook.com;FPR:;SPF:None;PTR:InfoNoRecords;MX:3;A:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1;SN2PR15MB1088;23:+730QDK1a73vT7WOc6BjYEUsIogj6hSiNL0JJ7lWR?= =?us-ascii?Q?4508hXOkZW47g7DqHad+da4Km1BwTOoDhe18XFIIXx/K9F8fSBtgN+0g8q/K?= =?us-ascii?Q?amaAU0s5bHce5i69WGRHCHX74t5Cjp1s7WylVrHpVPT2aud3wstXLheldo+o?= =?us-ascii?Q?dM3QhOG5zYWnnzVgrXQNPr2iygAyQnSAZ1PXhf3HSsAjPvHG48c01kVM+mJl?= =?us-ascii?Q?sf6+AxTR4birFmSBb77H8rtjBvysfexzNTlb/1lrRQeZWpjqqqgMPrdaiPYi?= =?us-ascii?Q?FDuKCT19WgsZct9fGIrBNOWntTfx1+5ld72v1BIYqVt4RnOl+bDcmnYRSRf4?= =?us-ascii?Q?pUF1wp1R29gIBz4zF7neWJi5pNhDFzx5Cn+X61seUTjEyXq3AflcLDat01+A?= =?us-ascii?Q?5ml1K2DKFAZdfwOH1OUwmDDfcMZaPJ59DvupQnbPDJulcckXdUGgLD44vBFT?= =?us-ascii?Q?fJi1GH9+kjZ1cxhv9cPS8TeA4t57ejCeziJ8Ct2MCaCItks35W9ej1ryaRUd?= =?us-ascii?Q?U4AjshAr7504GmcKi9/It87UAADBAnAXOG/hAZv9IpiLAdIH9QHHZpfilyVK?= =?us-ascii?Q?7x5zNjKuEQyONXipphVnRBiJOXZJqe4rumBq2B+fYaqXMePuu9lribv5coHG?= =?us-ascii?Q?mAVZ+JFEg2QOcwZSpYGIURkUxzJ2WhdNpQWi9PgCZBUElAiVN5/evNt47fHy?= =?us-ascii?Q?A3QfoIIZEn4KxYUNUOzLXx79y3CsA3dZn3wnPWDDOgykQg1QMjeDKNfUea/I?= =?us-ascii?Q?P9VtGCXYe+tWn5hd2QrrTc7zCuvoZTcWZ3r9bGZC+mexDEOuX7G4xHzEDzVl?= =?us-ascii?Q?6WsJogQzE6rodk71ZXf2V+pFWOCYu1E7sENBy9qVEZYXDM2DyLmSDDQO6t4S?= =?us-ascii?Q?qLPNpLTUFdGZmjCc67JMFi3VhDSxvsLZKm885ght95tZU2UqtTkWsbRmiyqN?= =?us-ascii?Q?uJIZ/CKtpL+nUG5HdOsZyPmTzct2rINsZ0qVnBt3BLDadadT5d0LODULRXJN?= =?us-ascii?Q?x1vX+5bQRe73RejZpetRC+0nfQ1cc3rr8k+Zv5UNrI/FJK3rS4vDTsELjFmN?= =?us-ascii?Q?6bixRw908xH5T21M/atFwbvFGo51516SQ/89txM3Ou39N4djfpr4OHMGk3iO?= =?us-ascii?Q?P+pjfnW5DQfCL2XMJUd6P8E0p/FPNSCCLmgATnCAIx1W0G3oQyPKBGNya79E?= =?us-ascii?Q?O6/3sVu9lyKPaA0A2xWFwjKjQ9h6z4F8A/Eks8/xY36C+rKxq3iQfmAi4wy/?= =?us-ascii?Q?51Of+c25YgRuJklVKHdXArygEoHjMMHkeYZTbDQ2z/Mp7w3ZQFT3dMabeRXw?= =?us-ascii?Q?JFj5YHt98AX7qW4kKHGGFx6ziSA/OzT3hBev9eaScQr?= X-Microsoft-Exchange-Diagnostics: 1;SN2PR15MB1088;6:4faPKSFgSMRzIKFXyRc5+FL1BALp3i07ylXGGMlF/+clztZHVQYG6e8V+N7SgkjmZgSDvCfzvhBEPMXGj6SqlW9mAD31TkfS2XBgtYshwhNUxwmTenLCI7oZfL6H0JMnGaOGCZBWyj5DlddmmvgJp0M6Y4u1FEncPF4y0Kz48wDUADxFfjKl0Lmevl88AZX9JNpxP+QuvgQbZZ1cKIrPUs7wNYJSBAzUW2Z/UC8Ke5U29n7MXYIUS0RZx6Me4bUcOmB4v26ChLjYXhKh2+1YJQ9xkvjoz8LT47/Hg+3vlGJqaZpUWAmscfqSR5d+QCzUbgwtMYFCRbLQ8h9Uabu6Wz20zddl+2StDkp2keDOQns=;5:U93PK4VbvsOqBqmjN4uMgks7cheg3pDURw4gb5LFPdEm6yLNUFHcVDTG5nKF/6YrAtEsvLBiKnafKUtll6H6JEmWujVP6sb5gt9a3DOzThsDTXQQ0nca+pDJrqxQkZ0PQsk5xHQJ3Ro0pLk6+a8bed0/RtLXPbAj+Yv57MKWj90=;24:A9c05eOAAq+CwZV04yaIadXo+1eZ701degN/fDn0givJiGRxgGydFk7+SKEPCzRAAMGHmvsmHkbhjP62mCofOrmhOUSS8f4WnaUVmB5R4g0=;7:baTmkup/fR9AeeDVTIggcfSVeZPeWTKphfF2DNlL4WWkB7wrizEWYdFM2/FunxtxX20CCPDZUwXPZSRb6g9ymPLI+r1hZ7Yf6q6pmdD23NwJHelBSph0J9oSGpjdLb3mwelqn//WRH1Wx/J9VTMIiu2qMoZ2RF0rcoiIdimt/XToyeW11uei1F2dITsOC/zfrRxuidscz1fic9rpvT6Rx72DBA0q4ZZL7DEAeihrYF6teGTlmu+H6mZbkLfKOJ5k SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1;SN2PR15MB1088;20:w4s5r6gFXS3d7kA4ainkFArQ11Zbx1ZmOVWIg4OT951wGVU3gzDzv07aDRL0GIGInZAqo50KXAuki/79ALiIxZJkLifNnLdc354YuDldr91eFhNTMIIK5prLMl/2VPWgHiW2fuyULmuMms3pL3qtX/QXVb+wEvyS4K9UY6JvKRs= X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Dec 2017 13:32:27.8015 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: fb5ffcc2-7962-41d9-7f2e-08d538bff81f X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2 X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN2PR15MB1088 X-OriginatorOrg: fb.com X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-12-01_03:,, signatures=0 X-Proofpoint-Spam-Reason: safe X-FB-Internal: Safe Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, Michal! I totally agree that out_of_memory() function deserves some refactoring. But I think there is an issue with your patch (see below): On Fri, Dec 01, 2017 at 10:14:25AM +0100, Michal Hocko wrote: > Recently added alloc_pages_before_oomkill gained new caller with this > patchset and I think it just grown to deserve a simpler code flow. > What do you think about this on top of the series? > > --- > From f1f6035ea0df65e7619860b013f2fabdda65233e Mon Sep 17 00:00:00 2001 > From: Michal Hocko > Date: Fri, 1 Dec 2017 10:05:25 +0100 > Subject: [PATCH] mm, oom: simplify alloc_pages_before_oomkill handling > > alloc_pages_before_oomkill is the last attempt to allocate memory before > we go and try to kill a process or a memcg. It's success path always has > to properly clean up the oc state (namely victim reference count). Let's > pull this into alloc_pages_before_oomkill directly rather than risk > somebody will forget to do it in future. Also document that we _know_ > alloc_pages_before_oomkill violates proper layering and that is a > pragmatic decision. > > Signed-off-by: Michal Hocko > --- > include/linux/oom.h | 2 +- > mm/oom_kill.c | 21 +++------------------ > mm/page_alloc.c | 24 ++++++++++++++++++++++-- > 3 files changed, 26 insertions(+), 21 deletions(-) > > diff --git a/include/linux/oom.h b/include/linux/oom.h > index 10f495c8454d..7052e0a20e13 100644 > --- a/include/linux/oom.h > +++ b/include/linux/oom.h > @@ -121,7 +121,7 @@ extern void oom_killer_enable(void); > > extern struct task_struct *find_lock_task_mm(struct task_struct *p); > > -extern struct page *alloc_pages_before_oomkill(const struct oom_control *oc); > +extern bool alloc_pages_before_oomkill(struct oom_control *oc); > > extern int oom_evaluate_task(struct task_struct *task, void *arg); > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > index 4678468bae17..5c2cd299757b 100644 > --- a/mm/oom_kill.c > +++ b/mm/oom_kill.c > @@ -1102,8 +1102,7 @@ bool out_of_memory(struct oom_control *oc) > if (!is_memcg_oom(oc) && sysctl_oom_kill_allocating_task && > current->mm && !oom_unkillable_task(current, NULL, oc->nodemask) && > current->signal->oom_score_adj != OOM_SCORE_ADJ_MIN) { > - oc->page = alloc_pages_before_oomkill(oc); > - if (oc->page) > + if (alloc_pages_before_oomkill(oc)) > return true; > get_task_struct(current); > oc->chosen_task = current; > @@ -1112,13 +1111,8 @@ bool out_of_memory(struct oom_control *oc) > } > > if (mem_cgroup_select_oom_victim(oc)) { > - oc->page = alloc_pages_before_oomkill(oc); > - if (oc->page) { > - if (oc->chosen_memcg && > - oc->chosen_memcg != INFLIGHT_VICTIM) > - mem_cgroup_put(oc->chosen_memcg); You're removing chosen_memcg releasing here, but I don't see where you do this instead. And I'm not sure that putting mem_cgroup_put() into alloc_pages_before_oomkill() is a way towards simpler code. I was thinking about a bit larger refactoring: splitting out_of_memory() into the following parts (defined as separate functions): victim selection (per-process, memcg-aware or just allocating task), last allocation attempt, OOM action (kill process, kill memcg, panic). Hopefully it can simplify the things, but I don't have code yet. Thanks!