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=-10.4 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,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 5E2E4C433DB for ; Thu, 11 Feb 2021 18:52:25 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 92A8864D99 for ; Thu, 11 Feb 2021 18:52:24 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 92A8864D99 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 1825F6B014F; Thu, 11 Feb 2021 13:52:24 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 10B8B6B0150; Thu, 11 Feb 2021 13:52:24 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F15FB6B0151; Thu, 11 Feb 2021 13:52:23 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0228.hostedemail.com [216.40.44.228]) by kanga.kvack.org (Postfix) with ESMTP id D76476B014F for ; Thu, 11 Feb 2021 13:52:23 -0500 (EST) Received: from smtpin12.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 9435F2493 for ; Thu, 11 Feb 2021 18:52:23 +0000 (UTC) X-FDA: 77806882566.12.cord58_40047b52761a Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin12.hostedemail.com (Postfix) with ESMTP id 74A871802DA50 for ; Thu, 11 Feb 2021 18:52:23 +0000 (UTC) X-HE-Tag: cord58_40047b52761a X-Filterd-Recvd-Size: 5024 Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by imf23.hostedemail.com (Postfix) with ESMTP for ; Thu, 11 Feb 2021 18:52:22 +0000 (UTC) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 4F3B9AE3D; Thu, 11 Feb 2021 18:52:21 +0000 (UTC) To: Yang Shi Cc: Roman Gushchin , Kirill Tkhai , Shakeel Butt , Dave Chinner , Johannes Weiner , Michal Hocko , Andrew Morton , Linux MM , Linux FS-devel Mailing List , Linux Kernel Mailing List References: <20210209174646.1310591-1-shy828301@gmail.com> <20210209174646.1310591-13-shy828301@gmail.com> From: Vlastimil Babka Subject: Re: [v7 PATCH 12/12] mm: vmscan: shrink deferred objects proportional to priority Message-ID: Date: Thu, 11 Feb 2021 19:52:20 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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: On 2/11/21 6:29 PM, Yang Shi wrote: > On Thu, Feb 11, 2021 at 5:10 AM Vlastimil Babka wrote: >> > trace_mm_shrink_slab_start(shrinker, shrinkctl, nr, >> > freeable, delta, total_scan, priori= ty); >> > @@ -737,10 +708,9 @@ static unsigned long do_shrink_slab(struct shri= nk_control *shrinkctl, >> > cond_resched(); >> > } >> > >> > - if (next_deferred >=3D scanned) >> > - next_deferred -=3D scanned; >> > - else >> > - next_deferred =3D 0; >> > + next_deferred =3D max_t(long, (nr - scanned), 0) + total_scan; >> >> And here's the bias I think. Suppose we scanned 0 due to e.g. GFP_NOFS= . We count >> as newly deferred both the "delta" part of total_scan, which is fine, = but also >> the "nr >> priority" part, where we failed to our share of the "reduce >> nr_deferred" work, but I don't think it means we should also increase >> nr_deferred by that amount of failed work. >=20 > Here "nr" is the saved deferred work since the last scan, "scanned" is > the scanned work in this round, total_scan is the *unscanned" work > which is actually "total_scan - scanned" (total_scan is decreased by > scanned in each loop). So, the logic is "decrease any scanned work > from deferred then add newly unscanned work to deferred". IIUC this is > what "deferred" means even before this patch. Hm I thought the logic was "increase by any new work (delta) that wasn't = done, decrease by old deferred work that was done now". My examples with scanne= d =3D 0 and scanned =3D total_work (total_work before subtracting scanned from it= ) should demonstrate that the logic is different with your patch. >> OTOH if we succeed and scan exactly the whole goal, we are subtracting= from >> nr_deferred both the "nr >> priority" part, which is correct, but also= delta, >> which was new work, not deferred one, so that's incorrect IMHO as well= . >=20 > I don't think so. The deferred comes from new work, why not dec new > work from deferred? >=20 > And, the old code did: >=20 > if (next_deferred >=3D scanned) > next_deferred -=3D scanned; > else > next_deferred =3D 0; >=20 > IIUC, it also decreases the new work (the scanned includes both last > deferred and new delata). Yes, but in the old code, next_deferred starts as nr =3D count_nr_deferred()... total_scan =3D nr; delta =3D ... // something based on freeable total_scan +=3D delta; next_deferred =3D total_scan; // in the common case total_scan >=3D 0 ... and that's "total_scan" before "scanned" is subtracted from it, so it includes the new_work ("delta"), so then it's OK to do "next_deferred -=3D= scanned"; I still think your formula is (unintentionally) changing the logic. You c= an also look at it from different angle, it's effectively (without the max_t() pa= rt) "nr - scanned + total_scan" where total_scan is actually "total_scan - scanne= d" as you point your yourself. So "scanned" is subtracted twice? That can't be = correct... >> So the calculation should probably be something like this? >> >> next_deferred =3D max_t(long, nr + delta - scanned, 0); >> >> Thanks, >> Vlastimil >> >> > + next_deferred =3D min(next_deferred, (2 * freeable)); >> > + >> > /* >> > * move the unused scan count back into the shrinker in a >> > * manner that handles concurrent updates. >> > >> >=20