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 E2029C76196 for ; Tue, 28 Mar 2023 18:45:59 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4B8256B0071; Tue, 28 Mar 2023 14:45:59 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 46761900002; Tue, 28 Mar 2023 14:45:59 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 307B26B0074; Tue, 28 Mar 2023 14:45:59 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 1A70D6B0071 for ; Tue, 28 Mar 2023 14:45:59 -0400 (EDT) Received: from smtpin02.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id D236BC08DA for ; Tue, 28 Mar 2023 18:45:58 +0000 (UTC) X-FDA: 80619186396.02.D56A703 Received: from mail-ed1-f54.google.com (mail-ed1-f54.google.com [209.85.208.54]) by imf07.hostedemail.com (Postfix) with ESMTP id EFBF640021 for ; Tue, 28 Mar 2023 18:45:56 +0000 (UTC) Authentication-Results: imf07.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=hybc48zy; spf=pass (imf07.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.54 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=1680029157; 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=WDPpGTUcAigUdKyxJFj5FSviALfqo1wtwcjHPt+uS1s=; b=0yUQGVAk0QNqsDsCTUQTAOdPrSbrA76BgzJ+hTgKg/jso8YaLnrzz9i2l8d93ZeOhObbva LgP4sKYy7aa7Ge8I95wuqSXoBqm8806hXxqMZkpbhBrKTENtNDaOYkVv8JL3eY/JAvE6jE OHfaJygVfx0lXi3GtRMcTob0YBwL9Lk= ARC-Authentication-Results: i=1; imf07.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=hybc48zy; spf=pass (imf07.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.54 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=1680029157; a=rsa-sha256; cv=none; b=gCAiDL9dePRqbNA/RLJ243vRiP9tA6kHMm+t7p0XlYQ06f7OHkAqNCe5Z90x6MiA8lywnE +DJRGoglSsvM/iXh5e0xXXagbHsLsbo+GHpOvxGua7iSIc8eYAHIakH9YFAl6glBCZjqYy 4zACdDBsGvZFhlpwsjlY7tqgGfUHoFY= Received: by mail-ed1-f54.google.com with SMTP id er13so12644507edb.9 for ; Tue, 28 Mar 2023 11:45:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1680029155; 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=WDPpGTUcAigUdKyxJFj5FSviALfqo1wtwcjHPt+uS1s=; b=hybc48zyKPXEpWZviFjaAJ9WSBFVr1MfoVcaRbhXnniFqrgKcZURpL2D66EDfe2Tom 2enWNHJvfeVMeYKAokqAFAXfZky705CgievqgDkFCxw4bPSTAK2Jp0CTqI7/9FVShpJy vR3o+VqgifbVaYRySVqKDxmaMMNXgAKvJzO0kZxGOm+jjAPdvnHtk7pGDo7cExEnhjW/ 0fQi9SbN5MXhL+Y1R6QN/IJLbl9GreFFGWUTl+AvpY5hoTt68q4RTl/M0g27vj36Yw30 3H/+tfUlSanDAmXpy+sxacjiLYAoaymmPVTxzgAnJ/Lsg5sj0Yw2/MQpjUuiaHO+/Ydn R6gw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680029155; 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=WDPpGTUcAigUdKyxJFj5FSviALfqo1wtwcjHPt+uS1s=; b=rKaY7qiFi6xinxeVz2S+sfP0xofcdsX34afoWBrSrnfXkVoasdLGfQFjLY37jilDZ2 JUTQEj9YfRklI5lfBMufJo8I2KGYmjqaA2R9I94Zrnidh0vn965LerFxYiQcbfENwKjC Mhv0kvfSyCWiX67LRwhSp27NrNXbsCaVs5Bem6c4locrXSpgQFXItUV/CN7dqMO/RPuH frRzxDnnPwWZ7TIRS+4Cm76Vn5FpT4FXLATG6BDZeuLgDKaSQYcFubYudNcGzD3X9KoA 3f/hf1EHSZPu2rDllfoC6tNiajSTN38ur7+JYibAJyEEL5/hi5sVc5UaJOUtJtMcmDm2 B8yQ== X-Gm-Message-State: AAQBX9fNXk9HsvLYILS7w3+mhzKTq2p6PDzIxvwKNwY/lRky8Dsio4x0 0j72wi28K2xEUjRjWTKYV6RjbtNCHRkZ0NHxfivjmw== X-Google-Smtp-Source: AKy350Y0H1o8UGlIOfaueUA1+ITobbazVG++kPK+R45KzkjvS4CMUy84hYIf5MOC6prSXxfzvLslyCUom9GAmiX0YTY= X-Received: by 2002:a17:907:7b8a:b0:931:6e39:3d0b with SMTP id ne10-20020a1709077b8a00b009316e393d0bmr8548276ejc.15.1680029155457; Tue, 28 Mar 2023 11:45:55 -0700 (PDT) MIME-Version: 1.0 References: <20230328061638.203420-1-yosryahmed@google.com> <20230328061638.203420-7-yosryahmed@google.com> In-Reply-To: From: Yosry Ahmed Date: Tue, 28 Mar 2023 11:45:19 -0700 Message-ID: Subject: Re: [PATCH v1 6/9] memcg: sleep during flushing stats in safe contexts To: Johannes Weiner Cc: Tejun Heo , Josef Bacik , Jens Axboe , Zefan Li , Michal Hocko , Roman Gushchin , Shakeel Butt , Muchun Song , Andrew Morton , =?UTF-8?Q?Michal_Koutn=C3=BD?= , Vasily Averin , cgroups@vger.kernel.org, linux-block@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, bpf@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam07 X-Rspamd-Queue-Id: EFBF640021 X-Rspam-User: X-Stat-Signature: r6istjzqnn9uasoknjbex7zhiuhrox3n X-HE-Tag: 1680029156-486650 X-HE-Meta: U2FsdGVkX1+loo56X9AYnx4R4cdmUtKPiqzWmabM/XVQ83oRSA4VwhignJ+guJMaQ73gtJB6ZwQsE1id5dTWoOD5kgRDpwpBDlK/fDkNrx9PND0KZ4SI440qMlfj25kSdnUFn6VDNbzioDVeQAzMwUzkRjRjxol2sEJ4p3+PNfexaozo8CJaqi22jjinOiFwW35wVFMgmBWchJdI0EBXEf4s2pRyehvo2oFIHXP+pJvNvJ+xP92hAUj4Q+p5E0Jb9dmOWWgcdykwIMtxOV2qcUO4X8nnIQfE/cy/AKhlJesu7ryPwaTw0KKOpR+Nd+9YFOdyyhH0gw3tKUaBB2bWzjsBzhhA8UjKRsHK6MfkaF/ZOLQwyf16vAQOrK7RVcu3nVZU/IKM/8Qc01iYJfVk8bybKMqfVzU6rcRmRss14vha/LKvXpJyNLHcbPHvwsoSNBhoGxaV03XyLjcK51Ln0vW9502EucjCgb1Q9wP8Pka5uKo1AhfGC7EDiBq45NZPv59rlAum6SSxwG+ghpQnW3cAVEeTJbg1lCh4OoOXDvk4hGEmsS1ckJRl3ZaH01/ln0BD0efytvcR8Bsxmzpj3wGUPbL5jHIRse9XDqVakwRa+ctoIvC4C0QjyuH0rHeOVKjTslI77bGOo/L7fOAOMsWipQ3+kljsRbpwJEd6IoJx49NrpqpYMHxdNOn8ka0AAf6XO5SkycLBBH38vO1qJ81R16qRWh2FsySC4uNZU7uElMfOVhUy0d9xEmCP8pwSKarnYZTsIgarm+9lbWddK3GkJcAmTPS66ds/hhgDG8pmLzHVbl+RKMVKULsEbITIGOCbvqB0Egzsko1BNpTqdeCVGtCJJjI/jpkPqpTAAtd9B5XcvGBTF2tuDurAhioE8fBqfP3Y0nvgcySaUfMVu4+/mVOYTWA+HqLwJ4+P3qautjkFxC2TTXVHJFra/05FAC2lOR+cnxJvI9khjMg GwFWUyuW 4EzLVrwAU7ZmbKIqPBDB7fo/PKX5Npp8VDJ84bwe9zzAWMWNoKbkwoLIf8qQRokKX2XWyfDzvrLWo1xRcsrxhD+RIg8mVL+6rg38kJHPC7hV2UwjTB/Elvkv4hI2S4XKFNYNDb5g9xV3YNinGJ1rsswtFcrIHm9CrRcMRUC/Tj8MPNjIclOJqbJ5B1MgbnbUqaD9x/f/GzyVW77mW8aGh8/MtcZohv7kBfRVsU/b5lJ1sueokQhqPJ667L+B20fhFlzq2q5VcD8phUoixstLUuvCaECYxOqQDga5e9uqFCWP2ZlHpW7Drpociww== 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 Tue, Mar 28, 2023 at 11:35=E2=80=AFAM Johannes Weiner wrote: > > On Tue, Mar 28, 2023 at 06:16:35AM +0000, Yosry Ahmed wrote: > > @@ -642,24 +642,57 @@ static void __mem_cgroup_flush_stats(void) > > * from memcg flushers (e.g. reclaim, refault, etc). > > */ > > if (atomic_xchg(&stats_flush_ongoing, 1)) > > - return; > > + return false; > > > > WRITE_ONCE(flush_next_time, jiffies_64 + 2*FLUSH_TIME); > > - cgroup_rstat_flush_atomic(root_mem_cgroup->css.cgroup); > > + return true; > > +} > > + > > +static void mem_cgroup_post_stats_flush(void) > > +{ > > atomic_set(&stats_flush_threshold, 0); > > atomic_set(&stats_flush_ongoing, 0); > > } > > > > -void mem_cgroup_flush_stats(void) > > +static bool mem_cgroup_should_flush_stats(void) > > { > > - if (atomic_read(&stats_flush_threshold) > num_online_cpus()) > > - __mem_cgroup_flush_stats(); > > + return atomic_read(&stats_flush_threshold) > num_online_cpus(); > > +} > > + > > +/* atomic functions, safe to call from any context */ > > +static void __mem_cgroup_flush_stats_atomic(void) > > +{ > > + if (mem_cgroup_pre_stats_flush()) { > > + cgroup_rstat_flush_atomic(root_mem_cgroup->css.cgroup); > > + mem_cgroup_post_stats_flush(); > > + } > > +} > > I'm afraid I wasn't very nuanced with my complaint about the bool > parameter in the previous version. In this case, when you can do a > common helper for a couple of API functions defined right below it, > and the callers don't spread throughout the codebase, using bools > makes things simpler while still being easily understandable: Looking at your suggestion now, it seems fairly obvious that this is what I should have gone for. Will do that for v2. Thanks! > > static void do_flush_stats(bool may_sleep) > { > if (atomic_xchg(&stats_flush_ongoing, 1)) > return; > > WRITE_ONCE(flush_next_time, jiffies_64 + 2*FLUSH_TIME); > atomic_set(&stats_flush_threshold, 0); > > if (!may_sleep) > cgroup_rstat_flush_atomic(root_mem_cgroup->css.cgroup); > else > cgroup_rstat_flush(root_mem_cgroup->css.cgroup); > > atomic_set(&stats_flush_ongoing, 0); > } > > void mem_cgroup_flush_stats(void) > { > if (atomic_read(&stats_flush_threshold) > num_online_cpus()) > do_flush_stats(true); > } > > void mem_cgroup_flush_stats_atomic(void) > { > if (atomic_read(&stats_flush_threshold) > num_online_cpus()) > do_flush_stats(false); > } > > > void mem_cgroup_flush_stats_ratelimited(void) > > { > > if (time_after64(jiffies_64, READ_ONCE(flush_next_time))) > > - mem_cgroup_flush_stats(); > > + mem_cgroup_flush_stats_atomic(); > > +} > > This should probably be mem_cgroup_flush_stats_atomic_ratelimited(). > > (Whee, kinda long, but that's alright. Very specialized caller...) It should, but the following patch makes it non-atomic anyway, so I thought I wouldn't clutter the diff by renaming it here and then reverting it back in the next patch. There is an argument for maintaining a clean history tho in case the next patch is reverted separately (which is the reason I put it in a separate patch to begin with) -- so perhaps I should rename it here to mem_cgroup_flush_stats_atomic_ratelimited () and back to mem_cgroup_flush_stats_ratelimited() in the next patch, just for consistency? > > Btw, can you guys think of a reason against moving the threshold check > into the common function? It would then apply to the time-limited > flushes as well, but that shouldn't hurt anything. This would make the > code even simpler: I think the point of having the threshold check outside the common function is that the periodic flusher always flushes, regardless of the threshold, to keep rstat flushing from critical contexts as cheap as possible. If you think it's not worth it, I can make that change. It is a separate functional change tho, so maybe in a separate patch. > > static void do_flush_stats(bool may_sleep) > { > if (atomic_read(&stats_flush_threshold) <=3D num_online_cpus()) > return; > > if (atomic_xchg(&stats_flush_ongoing, 1)) > return; > > WRITE_ONCE(flush_next_time, jiffies_64 + 2*FLUSH_TIME); > atomic_set(&stats_flush_threshold, 0); > > if (!may_sleep) > cgroup_rstat_flush_atomic(root_mem_cgroup->css.cgroup); > else > cgroup_rstat_flush(root_mem_cgroup->css.cgroup); > > atomic_set(&stats_flush_ongoing, 0); > } > > void mem_cgroup_flush_stats(void) > { > do_flush_stats(true); > } > > void mem_cgroup_flush_stats_atomic(void) > { > do_flush_stats(false); > } > > void mem_cgroup_flush_stats_atomic_ratelimited(void) > { > if (time_after64(jiffies_64, READ_ONCE(flush_next_time))) > do_flush_stats(false); > } > > > @@ -2845,7 +2845,7 @@ static void prepare_scan_count(pg_data_t *pgdat, = struct scan_control *sc) > > * Flush the memory cgroup stats, so that we read accurate per-me= mcg > > * lruvec stats for heuristics. > > */ > > - mem_cgroup_flush_stats(); > > + mem_cgroup_flush_stats_atomic(); > > I'm thinking this one could be non-atomic as well. It's called fairly > high up in reclaim without any locks held. A later patch does exactly that. I put making the reclaim and refault paths non-atomic in separate patches to easily revert them if we see a regression. Let me know if this is too defensive and if you'd rather have them squashed. Thanks!