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 DD3E1C74A5B for ; Thu, 23 Mar 2023 15:46:39 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 76B586B0072; Thu, 23 Mar 2023 11:46:39 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 71C3E6B0074; Thu, 23 Mar 2023 11:46:39 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5E35B6B0075; Thu, 23 Mar 2023 11:46:39 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 4E7926B0072 for ; Thu, 23 Mar 2023 11:46:39 -0400 (EDT) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 1FC22160724 for ; Thu, 23 Mar 2023 15:46:39 +0000 (UTC) X-FDA: 80600590518.21.A87F3B5 Received: from mail-ed1-f54.google.com (mail-ed1-f54.google.com [209.85.208.54]) by imf02.hostedemail.com (Postfix) with ESMTP id 32E638001F for ; Thu, 23 Mar 2023 15:46:35 +0000 (UTC) Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=NnpqzKnh; spf=pass (imf02.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=1679586396; 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=TIHMsdmfle7NugjLuFYQiNRPSMKIKN1rr5aYwFC8Kug=; b=pdS+f2wdKhpT94P6OsIrD8429tF/nXKPz9ggSaIIwe+H+fJBiAekYzCImdCpDhSA67NPpN TkHva9dww7ahuJ1f1Y1nZ/RdVjjTYhMDnuu2x0D2UH2WOniGtOXeXDz148cE0JaaX3fhyC 2d3+L0PlU2rkw1f7SJHfLOv980x5p1Y= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=NnpqzKnh; spf=pass (imf02.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=1679586396; a=rsa-sha256; cv=none; b=o2ORtjsEYXs5xKGTmptCzQHztvU2NhOGYQBFCNujk0/orZtXEHlXD2sjelvAO9qtGGFzOO sZj6VIaJYahphHbqhSu2m0WPjVlCCP/NbFkQZPQLdf89zXiOOrNgTjBmHKxsNnFpxJNr4P IjbIJl0sCJ/WazXcDdC0oEywi1J/ds4= Received: by mail-ed1-f54.google.com with SMTP id r11so88607992edd.5 for ; Thu, 23 Mar 2023 08:46:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1679586395; 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=TIHMsdmfle7NugjLuFYQiNRPSMKIKN1rr5aYwFC8Kug=; b=NnpqzKnhqFnJAt8X+CKmsaMihz2+x72k0iSwes0Z93hrQCCmnIjykaTSGPeNQ5pFhH Ihrt+7xKrvcJ83OWSpM9YB5TzbUiwKAKedqLBMpOYu7VudORQSNlTrZ/sZVIouVytrmh 8mRFg85soOgbowRyhrnI2fxNMftE7L06F70jdiEbEkgPyZZb1Gx9QEk58YI9QSl27zs1 8nh6biODhrdfptWo85kOkztlCfF4i/Ih+hkxxrH/M9QQ9WDY2JEdbRrI10oZ7tKOl1RJ gthE+6pN8lZAyXxGbsbBwRN8FPh2O5R7OTCAdHVZyHYMghuAo6HWsZtEhTNsg344Cx3N ML3A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679586395; 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=TIHMsdmfle7NugjLuFYQiNRPSMKIKN1rr5aYwFC8Kug=; b=GlGlzIP5YqWhNej5k1JRcPhRaw22tJPJRA3HwsYChqtCJIWSaf+mSL4R8mNUOhsNeu UueR3SKpZQBsH+yUVWevyVe+AqPmkCB4mux6M5f6bmgKZGt4i1aQ63gfEpkv6KwjIjgn eLBegHO2QUKwicbzvN6kUVcSd++3FuolbN9UOUAmdvpKRqwPpAH4vWrpbVd9UNcQqEUh L7Ppk6VQhB+mk7yzYKfCuVCd5bmv1KG/bV1yb5QqWKkB6/Zz0Jn73MbnXU5aWMymkHy6 mPA3wPz3bA1RvIMpX4f6QUzAv16XfG9y1KMc2FkZWNlUtPdEBhXatMFIKpzOIV5M2xrV +nUQ== X-Gm-Message-State: AO0yUKW1HQMi+tAgFppuxdCYZQq2Ludxt/3J53f/k1Smzxz8TJ14Yc4b bnEbCYzroNtmwS9N3tt9CycB2KFWBwCamp4dPbdp7w== X-Google-Smtp-Source: AK7set9qvSPeDiywcKEkQlyr4PFl7U28fHGlCYplhrVevp0yQUPR3vcb0VJD03YLXfnBSRIguQiNqhK55JgXWj1sgmw= X-Received: by 2002:a17:906:34cd:b0:8e5:411d:4d09 with SMTP id h13-20020a17090634cd00b008e5411d4d09mr5178971ejb.15.1679586394636; Thu, 23 Mar 2023 08:46:34 -0700 (PDT) MIME-Version: 1.0 References: <20230323040037.2389095-1-yosryahmed@google.com> <20230323040037.2389095-4-yosryahmed@google.com> <20230323154304.GA739026@cmpxchg.org> In-Reply-To: <20230323154304.GA739026@cmpxchg.org> From: Yosry Ahmed Date: Thu, 23 Mar 2023 08:45:58 -0700 Message-ID: Subject: Re: [RFC PATCH 3/7] cgroup: rstat: remove cgroup_rstat_flush_irqsafe() To: Johannes Weiner Cc: Tejun Heo , Josef Bacik , Jens Axboe , Zefan Li , Michal Hocko , Roman Gushchin , Shakeel Butt , Muchun Song , Andrew Morton , 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: 32E638001F X-Rspam-User: X-Stat-Signature: dzqnj8u3hxituhkiinqwso8gwdoehjpn X-HE-Tag: 1679586395-437364 X-HE-Meta: U2FsdGVkX19NKv0ZrJnMHsYyE0jwYReVtLmZ5qc/CawOdneRkphzgwRF+YCMvHzBO7Npj4vrA0hegw2cB9bOrdCoHe46ZtkXvimlaOOxhGWxccV3F98efUWq8Jg6KDGc7BdTtuyen0WtoZBzZcWWmSiCzbZ0LWRqcfkLGQsK+PdP7f0EMSQLOqISr7FpxRp4W9TmRqm8Z69Ql9b1G47z+3DhlgYKWikHDSmxnVslJZxxIQkWCq0cO6zBzi0nVxyPtEmOco+JufpS8uRCJDqqFDqGTCbmjR+jN/SLkyBJ+0SlJa6CflXWf/cSkCuKw4l32e7BvVOvqRKXe5zBs/DrroU+cUEISvI1UnQmcQK1OTxm7PpC5+T9PzH66YvLCiFCp+W55qrDuttQriYUXH65NEhxOKFlY7XPWWi48DfeZEBcKas9iRkidL9xGgPVDl4s3MinMA84O+ZM+qVhEYDQSSoSYg5tKXLxNaG436ZucRxhUuT3fJxseg022DILecXX7cGJy8a+5549Ai0kZmoVLrF9Rxr3+xR+qoRxyK4gEKcpCzQHVn5DOlFMZrv8etwIclKH+GKZpohYrLBIH+K922nMH/nthFmak5CFsrXbDTkn09i2fKg5vVoyZMSz9QXc1MfDFuUpKUiDmV2pITO9xtwBX3fubhx6x2PKxc1o3qevN6xodoCKqzLJS9yvL2PzSbw5t4zHzYlk0T1kv5ZU/Dhs2laBUgjEOcgzqL2v3SBTVSSGUmRXVuN/d2Xkveq4zpLmxF+wyq/V6x9+H8prRBbfDTh4yo4yBovn4M1sX5sJx1o0WQ33Kd0WL3otG7YcdOkcl9HdhCHnWAQjXVSXiYFRkEdn8lcyQxSeIzgbp/4DJduYNxhjWb7jpvOsNtW3lWE0Q+vYOOSqGpZPmpO9usnwT5/E4OJGGFla+qPbqZPDddBEmIIrK0GLgZAhZUveceBjM0Elm98bV4+xsfC uzkJod3t K1HbUWEOtZKiqBZLy3tip+OTUuJV1bJcgdtjmCtpEoV+MAr2FkfYKleuJr6/k3mx+VZmbAFDJCAebL9My6Vij9SPnYZIePYrkZ7Eg2H/pbzVXcJv/UvvK8WlkxVkrL5voznsOGWGCneh8Of1VttlzuZ9oXCzYhVqZCvY0Np8+baqkwQfWXFTLJTkybDachUnTeBPSARhfpltr04IDaK338uBNNhgEBGqk+aK9XAv5a74B8vlN/Uixt3PgmF06iREN+8tERepIsoRb+00aWUaPpyEPS62gOJG33lDfCkDe4KlCkdHpF5z0EjZjX5SRhqw5Z4lQ3ADoAbrGni5c9N+2vRCtjX0jqLP5SyDq/5sQaut94XHlHlnP1bzvqg== 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, Mar 23, 2023 at 8:43=E2=80=AFAM Johannes Weiner wrote: > > On Thu, Mar 23, 2023 at 04:00:33AM +0000, Yosry Ahmed wrote: > > The naming of cgroup_rstat_flush_irqsafe() can be confusing. > > It can read like "irqsave", which means that it disables > > interrupts throughout, but it actually doesn't. It is just "safe" to > > call from contexts with interrupts disabled. > > > > Furthermore, this is only used today by mem_cgroup_flush_stats(), which > > will be changed by a later patch to optionally allow sleeping. Simplify > > the code and make it easier to reason about by instead passing in an > > explicit @may_sleep argument to cgroup_rstat_flush(), which gets passed > > directly to cgroup_rstat_flush_locked(). > > > > Signed-off-by: Yosry Ahmed > > --- > > block/blk-cgroup.c | 2 +- > > include/linux/cgroup.h | 3 +-- > > kernel/cgroup/cgroup.c | 4 ++-- > > kernel/cgroup/rstat.c | 24 +++++------------------- > > mm/memcontrol.c | 6 +++--- > > 5 files changed, 12 insertions(+), 27 deletions(-) > > > > diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c > > index bd50b55bdb61..3fe313ce5e6b 100644 > > --- a/block/blk-cgroup.c > > +++ b/block/blk-cgroup.c > > @@ -1043,7 +1043,7 @@ static int blkcg_print_stat(struct seq_file *sf, = void *v) > > if (!seq_css(sf)->parent) > > blkcg_fill_root_iostats(); > > else > > - cgroup_rstat_flush(blkcg->css.cgroup); > > + cgroup_rstat_flush(blkcg->css.cgroup, true); > > > > rcu_read_lock(); > > hlist_for_each_entry_rcu(blkg, &blkcg->blkg_list, blkcg_node) { > > diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h > > index 3410aecffdb4..743c345b6a3f 100644 > > --- a/include/linux/cgroup.h > > +++ b/include/linux/cgroup.h > > @@ -691,8 +691,7 @@ static inline void cgroup_path_from_kernfs_id(u64 i= d, char *buf, size_t buflen) > > * cgroup scalable recursive statistics. > > */ > > void cgroup_rstat_updated(struct cgroup *cgrp, int cpu); > > -void cgroup_rstat_flush(struct cgroup *cgrp); > > -void cgroup_rstat_flush_irqsafe(struct cgroup *cgrp); > > +void cgroup_rstat_flush(struct cgroup *cgrp, bool may_sleep); > > void cgroup_rstat_flush_hold(struct cgroup *cgrp); > > void cgroup_rstat_flush_release(void); > > > > diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c > > index 935e8121b21e..58df0fc379f6 100644 > > --- a/kernel/cgroup/cgroup.c > > +++ b/kernel/cgroup/cgroup.c > > @@ -5393,7 +5393,7 @@ static void css_release_work_fn(struct work_struc= t *work) > > if (ss) { > > /* css release path */ > > if (!list_empty(&css->rstat_css_node)) { > > - cgroup_rstat_flush(cgrp); > > + cgroup_rstat_flush(cgrp, true); > > list_del_rcu(&css->rstat_css_node); > > } > > > > @@ -5406,7 +5406,7 @@ static void css_release_work_fn(struct work_struc= t *work) > > /* cgroup release path */ > > TRACE_CGROUP_PATH(release, cgrp); > > > > - cgroup_rstat_flush(cgrp); > > + cgroup_rstat_flush(cgrp, true); > > This is an anti-pattern, please don't do this. Naked bool arguments > are a pain to comprehend at the callsite and an easy vector for bugs. > > cgroup_rstat_flush_atomic() would be a better name for, well, atomic > callsites. Thanks for pointing this out. I will rename it to cgroup_rstat_flush_atomic() in upcoming versions instead. I will also do the same for mem_cgroup_flush_stats() as I introduce a similar boolean argument for it in the following patch. > > > diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c > > index af11e28bd055..fe8690bb1e1c 100644 > > --- a/kernel/cgroup/rstat.c > > +++ b/kernel/cgroup/rstat.c > > @@ -243,30 +243,16 @@ static bool should_skip_flush(void) > > * This function is safe to call from contexts that disable interrupts= , but > > * @may_sleep must be set to false, otherwise the function may block. > > */ > > -__bpf_kfunc void cgroup_rstat_flush(struct cgroup *cgrp) > > +__bpf_kfunc void cgroup_rstat_flush(struct cgroup *cgrp, bool may_slee= p) > > { > > if (should_skip_flush()) > > return; > > > > - might_sleep(); > > - spin_lock(&cgroup_rstat_lock); > > - cgroup_rstat_flush_locked(cgrp, true); > > - spin_unlock(&cgroup_rstat_lock); > > -} > > - > > -/** > > - * cgroup_rstat_flush_irqsafe - irqsafe version of cgroup_rstat_flush(= ) > > - * @cgrp: target cgroup > > - * > > - * This function can be called from any context. > > - */ > > -void cgroup_rstat_flush_irqsafe(struct cgroup *cgrp) > > -{ > > - if (should_skip_flush()) > > - return; > > + if (may_sleep) > > + might_sleep(); > > might_sleep_if() Thanks for pointing this out. I don't think it will be needed if we keep cgroup_rstat_flush_irqsafe() and only rename it to cgroup_rstat_flush_atomic(), but it is useful to know that it exists for future reference.