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 24210C74A5B for ; Thu, 23 Mar 2023 15:43:10 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A7C366B0072; Thu, 23 Mar 2023 11:43:09 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A2B3C6B0074; Thu, 23 Mar 2023 11:43:09 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8F3736B0075; Thu, 23 Mar 2023 11:43:09 -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 7ED7A6B0072 for ; Thu, 23 Mar 2023 11:43:09 -0400 (EDT) Received: from smtpin30.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id DFD971C5B9F for ; Thu, 23 Mar 2023 15:43:08 +0000 (UTC) X-FDA: 80600581656.30.4CB342F Received: from mail-qt1-f175.google.com (mail-qt1-f175.google.com [209.85.160.175]) by imf09.hostedemail.com (Postfix) with ESMTP id A79E6140012 for ; Thu, 23 Mar 2023 15:43:06 +0000 (UTC) Authentication-Results: imf09.hostedemail.com; dkim=pass header.d=cmpxchg-org.20210112.gappssmtp.com header.s=20210112 header.b=bp0VhDf9; spf=pass (imf09.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.160.175 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org; dmarc=pass (policy=none) header.from=cmpxchg.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1679586186; 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=KS4mI5dSZp2boSr3gZdSRyxPkDeWYs2g1qOZH+GPmsA=; b=FWq1MmVfePN6WNVfRIaPj+uL/PjpTkcLtrb8js/L8LEQGi955/Ul7/QuB5DB135o3wkl16 2jsXCfKf/v/f0YPBEx1NoQCVdt9OG44okdnEJjGunB66BhgwZecJs/Qb5bvKQp6x78aCKG h3WyX3WeeeEo6Qmnwi4cLd1H/QPgkKg= ARC-Authentication-Results: i=1; imf09.hostedemail.com; dkim=pass header.d=cmpxchg-org.20210112.gappssmtp.com header.s=20210112 header.b=bp0VhDf9; spf=pass (imf09.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.160.175 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org; dmarc=pass (policy=none) header.from=cmpxchg.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1679586186; a=rsa-sha256; cv=none; b=cHQEOjXPWr2+eml9feQfy+VuWd5q+JIugqfC4QDqXoiRE38/NkrmzxDk57v7I67zgfZJmu OPUyEFyMrqgmRBP8msQS2UZppGlWvXsarSYiFeOMqdCxpZyX0iLskLvGFFbjQaF0jpLvat 9+UHWC1HkVEESMi0sH9jpHpozh81go4= Received: by mail-qt1-f175.google.com with SMTP id g19so1887539qts.9 for ; Thu, 23 Mar 2023 08:43:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cmpxchg-org.20210112.gappssmtp.com; s=20210112; t=1679586185; 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=KS4mI5dSZp2boSr3gZdSRyxPkDeWYs2g1qOZH+GPmsA=; b=bp0VhDf9ovL1bSdfzbiIO0gMq8zKiy5UCvl56pjEv3yZC2e6XJiTzXreyCquV4fR7j CHbolL2DDZVqDX1QGbzh4DqoneGgloHixlkykX+mwgOhTScpZ0RN3cjWIl8ZnIRo4osu 3vpBiSm7GHsppP3u3BSs0VM9W+ODBBpYeV1d15NTXPA2/ZeKakTDoSKAHSbIWSlDbKrz oxXVdmDMn15JnWqyPCzF7NvgKA4T9gLpEJ+o6JIdPbX+6YAuye9EmaLBGM7Mve+ZMF8s rtDdUu6CG1rlItGycvCEP+Rz+ZDaXImPAx1sAoc8NrTSJasuUcHon2E4DhyrGrKKmGse +0tQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679586185; 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=KS4mI5dSZp2boSr3gZdSRyxPkDeWYs2g1qOZH+GPmsA=; b=M1iiDTgs4RkWXGs/a5SQt1fJ9egkbd5q8h41zAKgVNkOpBSkpUuNAPi+mQVOs7Gfst NToX7ZCCe7IWAj1UJ1BiKnQaMt+jD92A2Ea8O8KXVrx795BpISVBJ52n1XqmuPtpm3m4 mfOBmTbcn7LO428DZ12Ipvp5Hrh48sHNZxhRz4iEQE5ZHosDLRyoWifFw+dnmqdus0hK rILoBpMTXFEiYCVZJItobmU0tZ4nzY0pIFXFaTEz/lf0YuLgv6Z4ww6hh9pVuMoqQG3l KYfFVj8jVMJMeIuYii4sj/sssZFXThBQZVW5M9i+ePKNH6g3wkIiWCkQK/UPDOITgcis OPFQ== X-Gm-Message-State: AO0yUKVdmFFCMRCOpwhqaK4AlrbTzvXKQnmqkha20OCF3uWItz1o5YtJ n4i4yT/OCqlS1AakNWVbtOqmLw== X-Google-Smtp-Source: AK7set9m68HsM4zsbLTLo8lzq2cUjnnMSSCWIrS35xYc1xlQGS6uWQoye655J6lJtXD51iJlGF9bUA== X-Received: by 2002:a05:622a:38b:b0:3e3:8a0b:8c80 with SMTP id j11-20020a05622a038b00b003e38a0b8c80mr9206917qtx.41.1679586185631; Thu, 23 Mar 2023 08:43:05 -0700 (PDT) Received: from localhost ([2620:10d:c091:400::5:62db]) by smtp.gmail.com with ESMTPSA id 6-20020ac85646000000b003e3897d8505sm3329544qtt.54.2023.03.23.08.43.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 23 Mar 2023 08:43:05 -0700 (PDT) Date: Thu, 23 Mar 2023 11:43:04 -0400 From: Johannes Weiner To: Yosry Ahmed 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 Subject: Re: [RFC PATCH 3/7] cgroup: rstat: remove cgroup_rstat_flush_irqsafe() Message-ID: <20230323154304.GA739026@cmpxchg.org> References: <20230323040037.2389095-1-yosryahmed@google.com> <20230323040037.2389095-4-yosryahmed@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230323040037.2389095-4-yosryahmed@google.com> X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: A79E6140012 X-Stat-Signature: u18mioc4j3iooggyttajmggbjkfui33x X-HE-Tag: 1679586186-53249 X-HE-Meta: U2FsdGVkX1+XTvctijOhlPB1XR5hqIkdYYcArlm1/x/uqKLj11mzZq174BbK+iv6zTEvg8DJ/qKyVzFzy+iv8kPL82M31YQUJmltPp5umuDX8N7zjXzdYsTf5Bb31ytDCTUi5iiPl2acVq8572JZBoaq7k65nD72lpflLEh7zLWIpK2q3CrtW835pLQKOvCySEQNKNw6gccrKoxfY19TnpnsBEaMYcnDqpsxMEW0bGE9gqRdGZ/egGDnKgkuTcqa9JxzfmOtv4NfpxuH2xqb4F44erT+dkBMRzIcyLZBAB9Xx+3mJl3vvEBZj4v4Px3a7WllAob/oVSAc+KjDC5J/xF+6KxALR9YTkBT1E3VtZUFEnetlqiQ/nS+XHRMFr1h1RwwKvHJ5XCtccgoq96Sk3tBtmDK/mQyshoLSYoKHejnW29N42xmEJppTKWNvlugVEwqwpI/4N8jRYd+q83jReXtje0evd1J4NGlmEJm4iCQRT+RQjou2dYqQqD4ZIrf5z+idcD4Ve/TQS2p73cJgeWRDjdGO1ClOtx6vI+j0VT4CrX+EZRKJzgVSyAPmT4PABRAiubAR03DoEcIdHCiEGtQvwY2IsAN3UNODysg8E58zqKhzD9iP5Aln32MCTCs0VaPgMyvioZNWL6PRde/hULqs86reQgvYb1YqYUj/ghFiYeBboQT0ke/If5PK7C0c/Uabs2S/Tl+cOW4u+gVWmmTWdKY2+iRPUOclO4Oeu9AkcFLj/+X/aQuJq9eJGqUvLM5UT26Z/5jsAqpoUIGXYPkKHDzxiZY9s2rw+EJRMNrxJeVYC1FcOcz9yhqQge5tBw0MCEOXcdIMnF28j1QTNp3hLIgGCx8cdYs3OblRl8JKGg5hV4u7+VKEMXd6aj4xFo7rqkUfMKG6hSUVIgHFmykiTS9ZMX8UGYQeOAj/LHFMby0OC1ckU2cUlNsWdPGpluB2+qSbhle67twWyM I1HcrLZg uVHlku/ubgy01BEi6yZ7tyHDkQEAA46clUcyCWzAtFrawrybsJOucfEgmUkcgleIzYHWm0ArNfqTjt+R8hZyiydD7+0x1scemQMg9Z+HluwgGy3puBhgcQFUtPJmUJciEeAXu+f81+6Ky4AI76cTLTrjF4yTpkOgfi4OZ5hhGRpU5eSCU67GLEE12Jq/06fxAkmFvtW1y5kMIipNNBivXtGVoskhwmZXVQLcnvnYK3nSF2wueT6NhuluaG25hHTEXYa8vdXmmVO+URRe3Vr3DG+tWw0P5dCOywoBvvU4kvDbH/h/NNLj06dF5l7FkNfskrMDI6Zj7lfBz3p4ojTYoEcTzG0PlFBbXH4Y1iqUKtH1BjSB51qzA1nqqOl/pFyMFkGQ6x7jSoLL/tabaTNyj4tcbYPL91M2WZ/+yOkA/AhBkKeubgzCVma7jPZCnYQtE0dU2aCN36n1oNrUr7rHecV6exJ5JlhCKkiQsqr8EUev6utaTBOU0n8iUn2uU/u8+Th3s 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 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 id, 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_struct *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_struct *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. > 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_sleep) > { > 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()