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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9D50FC6FD18 for ; Tue, 28 Mar 2023 19:00:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229631AbjC1TAj (ORCPT ); Tue, 28 Mar 2023 15:00:39 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58594 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229479AbjC1TAi (ORCPT ); Tue, 28 Mar 2023 15:00:38 -0400 Received: from mail-ed1-x529.google.com (mail-ed1-x529.google.com [IPv6:2a00:1450:4864:20::529]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8493A272B for ; Tue, 28 Mar 2023 12:00:37 -0700 (PDT) Received: by mail-ed1-x529.google.com with SMTP id x3so53711218edb.10 for ; Tue, 28 Mar 2023 12:00:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1680030036; 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=VaFE8MJ/rJnWu49G6qaf7Zeu9SjJjHyr+HaKIIGZlo8=; b=hGNPwQWTEYe3CZmGYHi60X2aSfRWtVK8WiJ7nu3mjdJk4YmjuiqYe9pQ+CDJlPYyxd /pi9trbtJtSOm7MifwcGnSBImrwX+6Z42snV0nbBlJ7UXJ/eKh48Yn/EV7NYVDSmmbPt Rb0l9k4E3FCIRsq0+yQqNxV2ey483qI2f6DnbBWaNsDKB3buuC+B9cupFif5N8kLGalN EqIH5K8IIPJWWoiCFHfQlnBM7LhKzgagHTbCa9LC7C/ObuzCHw9amKjqLxPdo/JKx3RO c+XsEE64L2GpM8WO4QZD+clco6JbOEINCGSlyDLrWKVTxZp91kvGAVcL2J04SbpNs5mz w6JQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680030036; 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=VaFE8MJ/rJnWu49G6qaf7Zeu9SjJjHyr+HaKIIGZlo8=; b=K0NWU0UJqXfil6WFqVvENOrzcXTJaAKFburjDD6A5kqCM6W2nsQVC6rU5rJUsiJJ4U u9Oi1eMpPFMK9SRgoBX52u5dhzI0DvQHva7oH5NgZp2egVDzg0qvaqgy9l8t51Gr3jot sN4wEFVJRA1xBT49b+2vbWJywCXxFq9WXyfH5yvtHqsNjT00RnhoGf+62X6IKaOMmUzU meESZH6uRa7//0Vyl2uJoUmWVWRlopKV+nOCqUDk431FhmVoZAsiCXdNVsdJDPq5VJNM 3mPJNtKKEgDr8sgIPukS6ZukA0VbTAFX5HaAROhr45pAJYxEnRBsoHVvoG6Cx0H2Vwjv x7dg== X-Gm-Message-State: AAQBX9eCGW4M7FAAv74aOusyqStMgVruEbIPcVmQvJHmtRC0HYbTkoYu zXr0KxcDm5TBgt0lBW15J+5KVQG8smPnathFSuDlkA== X-Google-Smtp-Source: AKy350Yhk/i0+JPviKO6ga1I/DH2EtAh9hdlzqBienFUiCmmezrhPlteMpnkr+v35C7I9/6s8VzBjh8YFxr470tp00Q= X-Received: by 2002:a50:d756:0:b0:4fc:e5c:902 with SMTP id i22-20020a50d756000000b004fc0e5c0902mr8274473edj.8.1680030035893; Tue, 28 Mar 2023 12:00:35 -0700 (PDT) MIME-Version: 1.0 References: <20230328061638.203420-1-yosryahmed@google.com> <20230328061638.203420-5-yosryahmed@google.com> In-Reply-To: From: Yosry Ahmed Date: Tue, 28 Mar 2023 11:59:59 -0700 Message-ID: Subject: Re: [PATCH v1 4/9] cgroup: rstat: add WARN_ON_ONCE() if flushing outside task context 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 Precedence: bulk List-ID: X-Mailing-List: linux-block@vger.kernel.org On Tue, Mar 28, 2023 at 10:49=E2=80=AFAM Johannes Weiner wrote: > > On Tue, Mar 28, 2023 at 06:16:33AM +0000, Yosry Ahmed wrote: > > rstat flushing is too expensive to perform in irq context. > > The previous patch removed the only context that may invoke an rstat > > flush from irq context, add a WARN_ON_ONCE() to detect future > > violations, or those that we are not aware of. > > > > Signed-off-by: Yosry Ahmed > > --- > > kernel/cgroup/rstat.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c > > index d3252b0416b6..c2571939139f 100644 > > --- a/kernel/cgroup/rstat.c > > +++ b/kernel/cgroup/rstat.c > > @@ -176,6 +176,8 @@ static void cgroup_rstat_flush_locked(struct cgroup= *cgrp, bool may_sleep) > > { > > int cpu; > > > > + /* rstat flushing is too expensive for irq context */ > > + WARN_ON_ONCE(!in_task()); > > lockdep_assert_held(&cgroup_rstat_lock); > > This seems a bit arbitrary. Why is an irq caller forbidden, but an > irq-disabled, non-preemptible section caller is allowed? The latency > impact on the system would be the same, right? Thanks for taking a look. So in the first patch series the initial purpose was to make sure cgroup_rstat_lock was never acquired in an irq context, so that we can stop disabling irqs while holding it. Tejun disagreed with this approach though. We currently have one caller that calls flushing with irqs disabled (mem_cgroup_usage()) -- so we cannot forbid such callers (yet), but I thought we can at least forbid callers from irq context now (or catch those that we are not aware of), and then maybe forbid irqs_disabled() contexts as well we can get rid of that callsite. WDYT? From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yosry Ahmed Subject: Re: [PATCH v1 4/9] cgroup: rstat: add WARN_ON_ONCE() if flushing outside task context Date: Tue, 28 Mar 2023 11:59:59 -0700 Message-ID: References: <20230328061638.203420-1-yosryahmed@google.com> <20230328061638.203420-5-yosryahmed@google.com> Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1680030036; 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=VaFE8MJ/rJnWu49G6qaf7Zeu9SjJjHyr+HaKIIGZlo8=; b=hGNPwQWTEYe3CZmGYHi60X2aSfRWtVK8WiJ7nu3mjdJk4YmjuiqYe9pQ+CDJlPYyxd /pi9trbtJtSOm7MifwcGnSBImrwX+6Z42snV0nbBlJ7UXJ/eKh48Yn/EV7NYVDSmmbPt Rb0l9k4E3FCIRsq0+yQqNxV2ey483qI2f6DnbBWaNsDKB3buuC+B9cupFif5N8kLGalN EqIH5K8IIPJWWoiCFHfQlnBM7LhKzgagHTbCa9LC7C/ObuzCHw9amKjqLxPdo/JKx3RO c+XsEE64L2GpM8WO4QZD+clco6JbOEINCGSlyDLrWKVTxZp91kvGAVcL2J04SbpNs5mz w6JQ== In-Reply-To: List-ID: Content-Type: text/plain; charset="windows-1252" 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 On Tue, Mar 28, 2023 at 10:49=E2=80=AFAM Johannes Weiner wrote: > > On Tue, Mar 28, 2023 at 06:16:33AM +0000, Yosry Ahmed wrote: > > rstat flushing is too expensive to perform in irq context. > > The previous patch removed the only context that may invoke an rstat > > flush from irq context, add a WARN_ON_ONCE() to detect future > > violations, or those that we are not aware of. > > > > Signed-off-by: Yosry Ahmed > > --- > > kernel/cgroup/rstat.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c > > index d3252b0416b6..c2571939139f 100644 > > --- a/kernel/cgroup/rstat.c > > +++ b/kernel/cgroup/rstat.c > > @@ -176,6 +176,8 @@ static void cgroup_rstat_flush_locked(struct cgroup= *cgrp, bool may_sleep) > > { > > int cpu; > > > > + /* rstat flushing is too expensive for irq context */ > > + WARN_ON_ONCE(!in_task()); > > lockdep_assert_held(&cgroup_rstat_lock); > > This seems a bit arbitrary. Why is an irq caller forbidden, but an > irq-disabled, non-preemptible section caller is allowed? The latency > impact on the system would be the same, right? Thanks for taking a look. So in the first patch series the initial purpose was to make sure cgroup_rstat_lock was never acquired in an irq context, so that we can stop disabling irqs while holding it. Tejun disagreed with this approach though. We currently have one caller that calls flushing with irqs disabled (mem_cgroup_usage()) -- so we cannot forbid such callers (yet), but I thought we can at least forbid callers from irq context now (or catch those that we are not aware of), and then maybe forbid irqs_disabled() contexts as well we can get rid of that callsite. WDYT?