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 0D29FC46CD2 for ; Tue, 2 Jan 2024 17:43:55 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 941E06B02AD; Tue, 2 Jan 2024 12:43:55 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 918836B02AE; Tue, 2 Jan 2024 12:43:55 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7E03D6B02AF; Tue, 2 Jan 2024 12:43:55 -0500 (EST) 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 6FF796B02AD for ; Tue, 2 Jan 2024 12:43:55 -0500 (EST) Received: from smtpin03.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 3D3451C1096 for ; Tue, 2 Jan 2024 17:43:55 +0000 (UTC) X-FDA: 81635094030.03.7F45697 Received: from mail-oi1-f180.google.com (mail-oi1-f180.google.com [209.85.167.180]) by imf03.hostedemail.com (Postfix) with ESMTP id 74AAB2000D for ; Tue, 2 Jan 2024 17:43:53 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=IRY6J3MZ; spf=pass (imf03.hostedemail.com: domain of schatzberg.dan@gmail.com designates 209.85.167.180 as permitted sender) smtp.mailfrom=schatzberg.dan@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1704217433; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=oFAXfoSqedernbiWEjd/3knzaFCGGb+zuLCr9vWqgk4=; b=b9HnmfJox4gV/jSmhlpOWg6lJ5qS4h3Pd/688RTFKERu0440pSyTazuuOwkesIciAFvCQx gZt2B6z3+fC8j5WfK5B71WTs4ZYqC2isJzj4d0lgYVxle3wHBwsgfQLzBLCu+hWg+OVMnH 3rhoJkAxg/Ezs//oTdF7aTCUjbrONGc= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1704217433; a=rsa-sha256; cv=none; b=ClOJNGEqJuv3jvfhcKe8/hiCuitDwxAHK8DN33djxy2anV0l39uXSe1gO8j5B37RIebjTW Udu6/Fnp5EriktOCR5QTrzryxYkC5eqG7UPmiNZDIn8GviQ1EIAAsiFtrfxqXVv4ZbsAZF uwN4V595paV/yyWVZMDcNC+OIkWr41Y= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=IRY6J3MZ; spf=pass (imf03.hostedemail.com: domain of schatzberg.dan@gmail.com designates 209.85.167.180 as permitted sender) smtp.mailfrom=schatzberg.dan@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-oi1-f180.google.com with SMTP id 5614622812f47-3bba50cd318so6678646b6e.0 for ; Tue, 02 Jan 2024 09:43:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1704217432; x=1704822232; darn=kvack.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=oFAXfoSqedernbiWEjd/3knzaFCGGb+zuLCr9vWqgk4=; b=IRY6J3MZlybo28l8CzHjO36wshe1c1Z8ksmAgUKyN3XjkFhRHYZ1i0WOLXbVR7v03L s+qfrYhy5w/6lFbaWFyI8eNrfH+gVN6xyw5j6g79fCvG607hIDwcmRMO4kCEDaFreAky NDpEdWiudYE/Fro0xC9plbz9ut6SZpcrnLhUgmQiyK0yQEyqPryjPRjZlZPMCc1FLSLD 7+LRC00YN7DXvQFD1M+BI0VDItW03rbvxDMAwOHeCpnPA2v0ZacIzEqaDGCxtl2R56NP EDijH84KCaKZXcJ9WbyAy9TRiexP1CY5ckCVAdZwdxWHrojeeTDWrLzM9Wk9CwhIjbZA rCWw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1704217432; x=1704822232; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=oFAXfoSqedernbiWEjd/3knzaFCGGb+zuLCr9vWqgk4=; b=ez4pts8BCbnFMJfukESSOdb8HxQgjeAbSvYfbEszSQVyRsnANMbVPYuzn4zSqPEm3j AhWopsyLqY+L3uQ6+L1s5/CunmOjze3f9eYta8lhvMx6w9mKpnb0hFVn/cQqikKJpLSE r6O7dd6v4ES6ayfnGImM7zC6VzBOI9FZ9LpRR7bXT01TuhnxxLkVUpqSiFKN8XlftvWk hU1NyowZobp9Fc3ujNhMWzz60vJ0HEEpMvjbUMEKrjoOnyLaq9S2YennIBsihXyWKIF0 QgG4xc81PfP6Cka6OEgaPYv4s6/QDCrT8iHuMrTp0hoQ6ECvfh5V+7kmB8KuDeyI6H37 bBPA== X-Gm-Message-State: AOJu0YyeP/lb7MgqFfZJGeGSYDZ/SjIwptOt9zxBN1t+jm12G9eppUcE UGbJlfcgpzd7ylbRql5KKLU= X-Google-Smtp-Source: AGHT+IHl/019xddMgz9dzwdw//SCPvoCIFqs7YXU8Oot7eKHFac5tM8u9C/Ua4cEkhMMQP3o/7gPlg== X-Received: by 2002:a05:6808:3c4d:b0:3bb:cdd5:17c4 with SMTP id gl13-20020a0568083c4d00b003bbcdd517c4mr15552274oib.83.1704217432358; Tue, 02 Jan 2024 09:43:52 -0800 (PST) Received: from dschatzberg-fedora-PF3DHTBV ([2620:10d:c091:500::7:7ce3]) by smtp.gmail.com with ESMTPSA id dm6-20020ad44e26000000b0067f6ec98ae9sm10239578qvb.32.2024.01.02.09.43.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 02 Jan 2024 09:43:51 -0800 (PST) Date: Tue, 2 Jan 2024 12:43:49 -0500 From: Dan Schatzberg To: Michal Hocko Cc: Johannes Weiner , Roman Gushchin , Yosry Ahmed , Huan Yang , linux-kernel@vger.kernel.org, cgroups@vger.kernel.org, linux-mm@kvack.org, Tejun Heo , Zefan Li , Jonathan Corbet , Shakeel Butt , Muchun Song , Andrew Morton , Kefeng Wang , SeongJae Park , "Vishal Moola (Oracle)" , Nhat Pham , Yue Zhao Subject: Re: [PATCH v5 2/2] mm: add swapiness= arg to memory.reclaim Message-ID: References: <20231220152653.3273778-1-schatzberg.dan@gmail.com> <20231220152653.3273778-3-schatzberg.dan@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Queue-Id: 74AAB2000D X-Rspam-User: X-Rspamd-Server: rspam11 X-Stat-Signature: ajzu8t78fcwu1iec79hf9quwykinfwer X-HE-Tag: 1704217433-696809 X-HE-Meta: U2FsdGVkX192NDr4sbrmBRbrSRzAwki/37r0gDkPYxWdBvDBuw0X0ZzS++2BzdGlzj9nQF+UKpJdQ3tEszvADEcb6TX0vYvqrHdVHhnoHH8tnzBLzQGi9uXV/7glWYgsv2zZGNrxsx1Y21rk1urLshkqf+eNqtOsLzlGiDH4PmmqiTLAp3CeyF7sAFAKw67Y9mN7iIeOx30jJoFoy4c83YfV+dQVbuQIl9kK+Mfp7c4ikQkncbk5xtaNRfnvM0GLRbgqBaWRmtukfIG/G1LfAPuzObeCJ5D/u4Fu4H3JD06Wy5A4a3YuTr6hCD7Bi1tNTwCZk8ZRHXoucGluZgRNCD31LAin7Zag5OVmQzcm/F8LmzekkQ23fu+R8mwEhO/9LxRMf/eREs9A4YukWX1Wg/V7yiHat/MUnPrAap+gK1btGUAWTq0KqG/AVwVy6+Q7gl2TUzu1P7YjrIu0aHBlmE6MnXCc+ryWHikZ8wkuQUP1K3DEfsaeGr/YOgwvWlWrjPkxQOexWIAEDwmS2YtThX3Lfm2ycqr0rI1xRnMzpL6Vy5xzoPJyor8GJ7Sih2NVL6xiOM9OJ3WSWj1UISt2eup6n/qdrjq8JZvT/byzzXgWFzd8FSCuLDV/HzcivcnwOU5Mfhc4LLpKscNhZwws2SHDgEySNKuNJu/7ifGJ6kQ2dcFKYKDQ81Fkyqsj5lBeJihCPiPGXxVBgTEBiLvo3BLDQ2TOFllLZQPpSQrOd2276qj4bC0qKQrpZ6h1D01LDLkL9iSD52MlVQNmGLCxpb1G1elPZMDBtKu53ORfEuOt45m/BsWojf4SpsdnoS6XUcGp+fVr4OohjNjRRQuBRfJt9niDs0RSlKMfWk/ObWnU0AeZLceeH4QSo0s/BhKhYCbBOoRbdmbzkSPWP6+TlurHYJeHhfP7TBQswPkBX18O7yF9shWXdoTFyW14a98dxHRK09TtKEK9f07JgeI 0+b+JnuE nfio7+2RgZETSZK5+je6VfgNBAkinoc8odJ6hkT8HBoPPVALAAxa+PWlp2/AzUn+dDgOmK5VoQlKijEH8c+GhilUEajAz2PoV/ftMzeiXHfaPy8tnAj/hC5XUhXpCvgtPopiQhVeJU1RLywlA2opXkgrsUAB+WSIE2LQVgAMYkvcVTeaAv+dEjQ5lzvOemDKkDF/l+U/icwNSbRQBUMv2CSX02OjZf5ixLwyhtW8ITedAUixEkFWSY8K3jjv52RJd5AsXmrbUUygJ1yObtihW1RRXIM6gSSN3ijLMhVS1Kb3u/iXbwZLOoEcw6oTUI9HWyUi1gdDm8PAcGJMEHJDwl4OCh4IXAAJ8WSoMnM/BXv19kFyp4cGtp7LBkoOKlYKhKB6t6DYCjFbUWJ1VOJY064xDOSTRu0Yp/6J8cq2Aamvi9v/BK3OyqIe+tIL6LdwGgDqmnrcNMB315zkH56VzL/R7myxkRUCLYo4T9wjyiuEGA+MKVQxhqUXA83kmed73dX5QMyF1usGyQMa5z6VJJh/4c/y1iXt9/QwHQcLmVogKKTqS1hLWlOSb9YkuZmepXGh1TJCq5/cdk/RSRRJLbHtqq8xr1IsVD7MMRrgc5fCXegyLlQp77ii+BP5HqW+gn//rNkdBJAs76cRutHsibCYXTZvr+oXAOwB40jf02NWxj2+/ktl2sIUCfA== 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: List-Subscribe: List-Unsubscribe: On Thu, Dec 21, 2023 at 10:29:59AM +0100, Michal Hocko wrote: > On Wed 20-12-23 07:26:51, Dan Schatzberg wrote: > > ... > > LGTM > Acked-by: Michal Hocko > Just one minor thing. It would be really great to prevent from potential > incorrect use of mem_cgroup_swappiness. This should be internal function > to memcg. Now, having scan_control internal to vmscan.c makes that > harder and moving it out to swap.h or internal.h sounds overreaching. > > We could do this at least to reduce those mistakes. I can make it a > proper patch if this seems reasonable or you can fold it into your patch > directly. > --- > > diff --git a/mm/vmscan.c b/mm/vmscan.c > index f98dff23b758..5f3a182e9515 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -92,8 +92,10 @@ struct scan_control { > unsigned long anon_cost; > unsigned long file_cost; > > - /* Swappiness value for reclaim. NULL will fall back to per-memcg/global value */ > +#ifdef CONFIG_MEMCG > + /* Swappiness value for reclaim. Always use sc_swappiness()! */ > int *swappiness; > +#endif > > /* Can active folios be deactivated as part of reclaim? */ > #define DEACTIVATE_ANON 1 > @@ -230,6 +232,13 @@ static bool writeback_throttling_sane(struct scan_control *sc) > #endif > return false; > } > + > +static int sc_swappiness(struct scan_control *sc, struct mem_cgroup *memcg) > +{ > + if (sc->swappiness) > + return *sc->swappiness; > + return mem_cgroup_swappiness(memcg); > +} > #else > static bool cgroup_reclaim(struct scan_control *sc) > { > @@ -245,6 +254,10 @@ static bool writeback_throttling_sane(struct scan_control *sc) > { > return true; > } > +static int sc_swappiness(struct scan_control *sc, struct mem_cgroup *memcg) > +{ > + return READ_ONCE(vm_swappiness); > +} > #endif > > static void set_task_reclaim_state(struct task_struct *task, > @@ -2330,8 +2343,7 @@ static void get_scan_count(struct lruvec *lruvec, struct scan_control *sc, > struct pglist_data *pgdat = lruvec_pgdat(lruvec); > struct mem_cgroup *memcg = lruvec_memcg(lruvec); > unsigned long anon_cost, file_cost, total_cost; > - int swappiness = sc->swappiness ? > - *sc->swappiness : mem_cgroup_swappiness(memcg); > + int swappiness = sc_swappiness(sc, memcg); > u64 fraction[ANON_AND_FILE]; > u64 denominator = 0; /* gcc */ > enum scan_balance scan_balance; > @@ -2612,10 +2624,7 @@ static int get_swappiness(struct lruvec *lruvec, struct scan_control *sc) > mem_cgroup_get_nr_swap_pages(memcg) < MIN_LRU_BATCH) > return 0; > > - if (sc->swappiness) > - return *sc->swappiness; > - > - return mem_cgroup_swappiness(memcg); > + return sc_swappiness(sc, memcg); > } > > static int get_nr_gens(struct lruvec *lruvec, int type) > -- > Michal Hocko > SUSE Labs Thanks for the review Michal and sorry for the delayed response. Your patch looks reasonable to me but I'm a bit unclear about the need for #ifdef - mem_cgroup_swappiness already works correctly regardless of CONFIG_MEMCG or not - why not make sc->swappiness and sc_swappiness() unconditional? Happy to roll that into the next version of my patch.