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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1DCFBC76196 for ; Fri, 7 Apr 2023 01:03:39 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A24716B0072; Thu, 6 Apr 2023 21:03:38 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 9D4786B0074; Thu, 6 Apr 2023 21:03:38 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 89D556B0075; Thu, 6 Apr 2023 21:03:38 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 7C3AC6B0072 for ; Thu, 6 Apr 2023 21:03:38 -0400 (EDT) Received: from smtpin30.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 55A531A11D8 for ; Fri, 7 Apr 2023 01:03:38 +0000 (UTC) X-FDA: 80652797316.30.CE1B3B9 Received: from mail-ej1-f47.google.com (mail-ej1-f47.google.com [209.85.218.47]) by imf26.hostedemail.com (Postfix) with ESMTP id 7D413140013 for ; Fri, 7 Apr 2023 01:03:36 +0000 (UTC) Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=Gi1Y36e4; spf=pass (imf26.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.47 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1680829416; a=rsa-sha256; cv=none; b=2rY2272FgjIBTa4UJp8BpbVrUmAXVh89arlmSdAN4vuooqBYciNORhWwTPffHSwljdsU5d 0Zw2pEqBttLRR6VswSlIGweAXPWOKRJTdQ5lRlZQPy7BjlSLbOvgyMqPfzAvbB70vI4kUC DL5CDSYNDXdeQMgZpRZpRk7heMhdgOo= ARC-Authentication-Results: i=1; imf26.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=Gi1Y36e4; spf=pass (imf26.hostedemail.com: domain of yosryahmed@google.com designates 209.85.218.47 as permitted sender) smtp.mailfrom=yosryahmed@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1680829416; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=I1XvlUCiR72ZgTzOM/og5L0+VnHEni2vL4kpkj08keI=; b=TEKfKytTDT8qhvFZECjlijlWXBsu7Jk8XqwvFjFWrZmv3z/bRuLQ4mLSfD2sfPzdSd/hXN sbt6pE7c6PRJgENLXwUK+0D+AidrhyS9E6w3It/5XjhroFbH8RjoJkWh9jJc37NTW6cJSA j/l+xaQAzB5eVieWa581YGOB0Crojng= Received: by mail-ej1-f47.google.com with SMTP id n21so5304339ejz.4 for ; Thu, 06 Apr 2023 18:03:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1680829415; x=1683421415; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=I1XvlUCiR72ZgTzOM/og5L0+VnHEni2vL4kpkj08keI=; b=Gi1Y36e4Jc0UALptyl/LTUaKrFpOmFC+j8X9vJU+aUHaJ3V/nAFb/UcxcPBuAejvaH /DykQnS/vNIUGvAj/vpicf1rCmjLItBkP+bLF+dGVfixHnNW+5CvjeDuW4eo191qm+tD /Nq+T7LDKnMVZlYaOt6vDiKYbS+KZVpU3p3A8de2y3YKNEYQKgxNxXTk0yrPUZn575XF mnbdm2DvXhWi2hHHHAf/oVSgR8l2Df2pc7SE6ASCqpyOOXjWD43ob5Yvini26m7y0KFt weAJHGxy1ugwudrZ9aw5LDrjquxkqb8elbQYiLtsZ+UmZCKrfLf6fl9xA0VwSR5qwu3S lr/w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680829415; x=1683421415; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=I1XvlUCiR72ZgTzOM/og5L0+VnHEni2vL4kpkj08keI=; b=C3qjWox540HJq9oxxvOwZbTM6bJ8PKckf25Ese6EXZrXsXTfvwzYzNCQ2BGDShcqQH cXtVkfjBDA3Ju7Su6kTFxR2KSPaV0rPap30wrCBgxNV8UB6wUkqjRx3fqruMythZIZDe KR1YvOhI1aLTc21t8tk+DHMf9ESQHdjpV0YcTQ71qBQ4c/k1+iBY2DoCNbXhjC+YVI8g R9eZk8WPTxkSMBqOvmNhvtr4u9ohKY4X6Aw2UT3l4sxLAMRlTetRsgVLng3wI2y8pLNX 5qnmEmqsS1cOjSPOm0/KkRcm8NWpZRUK6fYAVCF7w+jGl/L2wVdqsxAQDpms63wIqTY6 S0+A== X-Gm-Message-State: AAQBX9fA9D8I+LZ707S82+SpzzFlES3naRGDnzKse/x5cckUdhOPeSZj /pJJOAX2rMu5TBUGtRGHvJTBEn5Xj6qaiXFFXXuhLg== X-Google-Smtp-Source: AKy350bKwpPCq7aUCh6NiZcMFFssax3P1vh2/SYajMVgSOKC2SUmSEJRZckrVOXLSYAQz7B816irIxnvUuw5f1vCMFM= X-Received: by 2002:a17:906:d976:b0:931:2bcd:ee00 with SMTP id rp22-20020a170906d97600b009312bcdee00mr332600ejb.15.1680829414816; Thu, 06 Apr 2023 18:03:34 -0700 (PDT) MIME-Version: 1.0 References: <20230405185427.1246289-1-yosryahmed@google.com> <20230405185427.1246289-3-yosryahmed@google.com> In-Reply-To: From: Yosry Ahmed Date: Thu, 6 Apr 2023 18:02:58 -0700 Message-ID: Subject: Re: [PATCH v5 2/2] mm: vmscan: refactor reclaim_state helpers To: Peter Xu Cc: Andrew Morton , Alexander Viro , "Darrick J. Wong" , Christoph Lameter , David Rientjes , Joonsoo Kim , Vlastimil Babka , Roman Gushchin , Hyeonggon Yoo <42.hyeyoo@gmail.com>, "Matthew Wilcox (Oracle)" , Miaohe Lin , David Hildenbrand , Johannes Weiner , NeilBrown , Shakeel Butt , Michal Hocko , Yu Zhao , Dave Chinner , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-xfs@vger.kernel.org, linux-mm@kvack.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspam-User: X-Rspamd-Queue-Id: 7D413140013 X-Rspamd-Server: rspam01 X-Stat-Signature: uo8cf3m3x7jrrhb6azfbfesn55eucmjn X-HE-Tag: 1680829416-167178 X-HE-Meta: U2FsdGVkX191z1igSJZ4x6NWEs3MXhUUfIY4uaSWHyqB7Xc51zQ5qSgMnw3JpzZozbw01QfoTcJI1F6Fh68RxNn2yXaYqJ0cCx/HX+nLBwnmkYjE8s3xowD3aaXGyV2MZk7246m+I1UM+UnEk1fv0iLqVI8ku/9tk6uL01Wmj/3g5O/RUAjYkshVr8ZDJE0sQcpNyOLiqHwG6V5yr3uOqExdryt95o3Gu5iM4jxI+pYXpP5fDY1Amg5oX4TqoUlZvNOqSNfrUe9qN5Q75GVG0O0Y5+e352hFj2/lZKRjvl1QblzEP59o65SgQsiRnF/8KHFAV2gdYqiJEmQxk3aO4217sS+1ysA2YFgNnAXStaxo2ycvVZOVzHjP9dWAN8rA4+41bNfGLut6AkvDga3FfrjTT4GcwavsuqXwHUA7rmaGrL3Dw6CPioiYEVfo07BxdarIScWNPMUO3LdH0S6L1aAcq+1l6RU62aTfMniKBBpMlv/r5l8MRD0Rd1X3E2d+SuAcBjA1JCn09b9UH7COcz//b4uZlilCvwhKQYRm0qVg7NzuAe+EXPyrKpoLKJMv81A3m81z9Q4hV4C0M01wwgcD6f8Xuaab+5RzNK3bX6xoZMDW3g6dqQSklU0jjtREqF+fLb8U5mr5bIdQwSWVkqrhiBax4uLx5R8sAxhWrKOddi9Xjahz62gFiy7GwR4TJoKiigRlVOsolV3c062Yvrtp3u1uFht1l30/wZBo1ID1rQEGLptUOBR+iN9dXwtNyP1Xqw1MokokXs6Lc75owLoWFaIbb7q9r/KL4wGOx4q0jFySJjJuJVJuhiqHrgUIuQ9BhOjdkfDqo77jg2KmtpIqhSu1wX4beSbF9XLwElInVekYzeBgx6q2WrGMPmdQ1nGdVFRWH6DC0zrszGymrHm2CFxsAlblAZtyxDQtZdH4XeAsR2GLhyRxSQsIaz9rUqMpsWpDKfaMu+j9UFf qTXFly4U 7eZt+vcbVzpK7M9QAyXwGnbutNFqIVs0Bf8D0WSt2viN4I4b25VWj9/wA0OP6+ijy02IHJGZdUZdxUHt/2+uxGWZN0EnIW67XoFTSL61zlAQlrDt655QlkkYkkndq9arGBhXUHPZGkAhZk4yopJq98oHIjSCFbGCylVXO6sPy74qCl8LyXX0jXwRVkIf5FAFK/rumoW1v7YeBKeBFjFyoLIX+bAUSZV5A44sIqMQYJ/3r/3rGKH5OAG+68ZAH6E9LWLoxklKVx8U6AJvpy3lv18zwP2Xj5wh3YKLBfqXH13RsrgHR7wzrMVMqOhmcMPoWlMBywTMERVIQMo5zGk3mzZUNBu6xAet2XBjiHsADw+5qBVF/WcbuwDfQZY6OOPDX6IG9 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 Thu, Apr 6, 2023 at 1:45=E2=80=AFPM Peter Xu wrote: > > Hi, Yosry, > > On Wed, Apr 05, 2023 at 06:54:27PM +0000, Yosry Ahmed wrote: > > [...] > > > diff --git a/mm/vmscan.c b/mm/vmscan.c > > index c82bd89f90364..049e39202e6ce 100644 > > --- a/mm/vmscan.c > > +++ b/mm/vmscan.c > > @@ -188,18 +188,6 @@ struct scan_control { > > */ > > int vm_swappiness =3D 60; > > > > -static void set_task_reclaim_state(struct task_struct *task, > > - struct reclaim_state *rs) > > -{ > > - /* Check for an overwrite */ > > - WARN_ON_ONCE(rs && task->reclaim_state); > > - > > - /* Check for the nulling of an already-nulled member */ > > - WARN_ON_ONCE(!rs && !task->reclaim_state); > > - > > - task->reclaim_state =3D rs; > > -} > > - > > LIST_HEAD(shrinker_list); > > DECLARE_RWSEM(shrinker_rwsem); > > > > @@ -511,6 +499,59 @@ static bool writeback_throttling_sane(struct scan_= control *sc) > > } > > #endif > > > > +static void set_task_reclaim_state(struct task_struct *task, > > + struct reclaim_state *rs) > > +{ > > + /* Check for an overwrite */ > > + WARN_ON_ONCE(rs && task->reclaim_state); > > + > > + /* Check for the nulling of an already-nulled member */ > > + WARN_ON_ONCE(!rs && !task->reclaim_state); > > + > > + task->reclaim_state =3D rs; > > +} > > Nit: I just think such movement not necessary while it loses the "git > blame" information easily. > > Instead of moving this here without major benefit, why not just define > flush_reclaim_state() right after previous set_task_reclaim_state()? An earlier version did that, but we would have to add a forward declaration of global_reclaim() (or cgroup_reclaim()), as they are defined after the previous position of set_task_reclaim_state(). > > > + > > +/* > > + * flush_reclaim_state(): add pages reclaimed outside of LRU-based rec= laim to > > + * scan_control->nr_reclaimed. > > + */ > > +static void flush_reclaim_state(struct scan_control *sc, > > + struct reclaim_state *rs) > > +{ > > + /* > > + * Currently, reclaim_state->reclaimed includes three types of pa= ges > > + * freed outside of vmscan: > > + * (1) Slab pages. > > + * (2) Clean file pages from pruned inodes. > > + * (3) XFS freed buffer pages. > > + * > > + * For all of these cases, we have no way of finding out whether = these > > + * pages were related to the memcg under reclaim. For example, a = freed > > + * slab page could have had only a single object charged to the m= emcg > > + * under reclaim. Also, populated inodes are not on shrinker LRUs > > + * anymore except on highmem systems. > > + * > > + * Instead of over-reporting the reclaimed pages in a memcg recla= im, > > + * only count such pages in global reclaim. This prevents unneces= sary > > + * retries during memcg charging and false positive from proactiv= e > > + * reclaim (memory.reclaim). > > + * > > + * For uncommon cases were the freed pages were actually signific= antly > > + * charged to the memcg under reclaim, and we end up under-report= ing, it > > + * should be fine. The freed pages will be uncharged anyway, even= if > > + * they are not reported properly, and we will be able to make fo= rward > > + * progress in charging (which is usually in a retry loop). > > + * > > + * We can go one step further, and report the uncharged objcg pag= es in > > + * memcg reclaim, to make reporting more accurate and reduce > > + * under-reporting, but it's probably not worth the complexity fo= r now. > > + */ > > + if (rs && global_reclaim(sc)) { > > + sc->nr_reclaimed +=3D rs->reclaimed; > > + rs->reclaimed =3D 0; > > + } > > +} > > + > > static long xchg_nr_deferred(struct shrinker *shrinker, > > struct shrink_control *sc) > > { > > @@ -5346,10 +5387,7 @@ static int shrink_one(struct lruvec *lruvec, str= uct scan_control *sc) > > vmpressure(sc->gfp_mask, memcg, false, sc->nr_scanned - s= canned, > > sc->nr_reclaimed - reclaimed); > > > > - if (global_reclaim(sc)) { > > - sc->nr_reclaimed +=3D current->reclaim_state->reclaimed_s= lab; > > - current->reclaim_state->reclaimed_slab =3D 0; > > - } > > + flush_reclaim_state(sc, current->reclaim_state); > > > > return success ? MEMCG_LRU_YOUNG : 0; > > } > > @@ -6474,10 +6512,7 @@ static void shrink_node(pg_data_t *pgdat, struct= scan_control *sc) > > > > shrink_node_memcgs(pgdat, sc); > > > > - if (reclaim_state && global_reclaim(sc)) { > > - sc->nr_reclaimed +=3D reclaim_state->reclaimed_slab; > > - reclaim_state->reclaimed_slab =3D 0; > > - } > > + flush_reclaim_state(sc, reclaim_state); > > IIUC reclaim_state here still points to current->reclaim_state. Could it > change at all? > > Is it cleaner to make flush_reclaim_state() taking "sc" only if it always > references current->reclaim_state? Good point. I think it's always current->reclaim_state. I think we can make flush_reclaim_state() only take "sc" as an argument, and remove the "reclaim_state" local variable in shrink_node() completely. > > > > > /* Record the subtree's reclaim efficiency */ > > if (!sc->proactive) > > -- > > 2.40.0.348.gf938b09366-goog > > > > -- > Peter Xu >