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 E563AC77B61 for ; Thu, 30 Mar 2023 07:13:57 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 63AF46B0072; Thu, 30 Mar 2023 03:13:57 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5EA816B0074; Thu, 30 Mar 2023 03:13:57 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 48B916B0075; Thu, 30 Mar 2023 03:13:57 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 340426B0072 for ; Thu, 30 Mar 2023 03:13:57 -0400 (EDT) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 02AAB4077E for ; Thu, 30 Mar 2023 07:13:56 +0000 (UTC) X-FDA: 80624700114.06.7AEFD8D Received: from mail-ed1-f49.google.com (mail-ed1-f49.google.com [209.85.208.49]) by imf27.hostedemail.com (Postfix) with ESMTP id 212FD40007 for ; Thu, 30 Mar 2023 07:13:54 +0000 (UTC) Authentication-Results: imf27.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=UvXOXvaK; spf=pass (imf27.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.49 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=1680160435; 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=rIO+xjpPvxFE7p3Fg529YdgqYE0es+Za65fM8K1qjgQ=; b=fcg56y/O4SQ4Xj1z1EKJK2PpJvgO7mB7qAsYJqhuvsRW9VjEJJnWijMuqkZVVXyfuBwh+x JBs5fOqfPYDtn8NfCtNTFlb9yYqSuZV4Ceo8903XvkZliSXR/I5meI0oiKvOxDZcTC6uma KIf/MvyWvOG/wxEoBU+wR4LSN8Ot2Xo= ARC-Authentication-Results: i=1; imf27.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=UvXOXvaK; spf=pass (imf27.hostedemail.com: domain of yosryahmed@google.com designates 209.85.208.49 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=1680160435; a=rsa-sha256; cv=none; b=ZqO68IZGXR0AHQdtXF5kk8sPrfSYEELu7OWE+G/qsJ2VjKAQU+798SkW1/C096xvxTPSpO RJQwKm/VZgoCX6kGfAxoxlplrnOEgir/BZPEoXNkiqK4Hbu54TCtLnehmzVAAtM0MrXxbR G8M22RrPIiBa8cjFDYV0GZ/gzMRTD8o= Received: by mail-ed1-f49.google.com with SMTP id h8so72707308ede.8 for ; Thu, 30 Mar 2023 00:13:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1680160433; 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=rIO+xjpPvxFE7p3Fg529YdgqYE0es+Za65fM8K1qjgQ=; b=UvXOXvaKg5W3pOiKg4eDH9lHl4Zdbajrfr9yCdPGB59p3EbanzUW5AbZ60M0Fo1jLj xgrtU13pG+2HRUXSZV1YHQUF6aAPx8bmMFTsyT+tMN4I6uSd3iTp4e0p5o0dlpChmR1N kgpM/rCb2oFLWV87Mo4Q+i3t95OakU3lMRj4YQ1nysYmA+Tpl5yBca0fvDqMPohFxbDB Y7+D8BKyWgRNceAUWpnfWnDi7HIUgkBAG3eDp8W+4bCHX6Cc8wGgEGVLJTMMQ7fZWTfd 8/Oa9xlswq0vuQl+sJims+8vhoieSnu1v+Z91iZH9bXW/MlL8LF9mWWEI22UEcU5KFZ+ xqbQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680160433; 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=rIO+xjpPvxFE7p3Fg529YdgqYE0es+Za65fM8K1qjgQ=; b=CDJgSrHoNogSZsUyRAO4ut/oKlpFcqHersBR0s4SpZqgIDFCK8xBBdht/8pPny9Qv1 5otHBhTtLaYR5ttpUuyrsGHsk8HnmdwUsXhHkpTC3MxX3AcUPZH1eetBEj9mKO9/vElf 1yny6BOOxC6Jkp4M5J+qDu8+xE/mBsDlZO0rW9ggnwgBa9qMEgq9zR0kd7+haiHx0ZjT x7Fjf5/ebNxX14rW767crK5YLDnvnzT11T0FRdxj2w7StrDeaOwrGfYKWvfkjlJIo+/8 1AuSZ9GYA94J/WWdo9f7+S8TXvByO/+0BV6GW8lpMjzDwv+0zsXLblvV8LaAl3SYBiMy tdkQ== X-Gm-Message-State: AAQBX9fxl5UJHjTEndwMUICOrRcvMgoh7Ce6M1uo26nj0uFjgu1rWiv6 w/e7zHnFsA2/QwyPyYvPn9x+xHJNYXJeQJUATcv+BA== X-Google-Smtp-Source: AKy350YivWZ/HKPgZxm5eZxF7+2qcjmArqrs51gN4d7jnY/jgrQdpgyHe6R7cHRBBvkOSSb0g8UX51iLn7ZYwWx1eHQ= X-Received: by 2002:a17:906:7846:b0:933:1967:a984 with SMTP id p6-20020a170906784600b009331967a984mr10847576ejm.15.1680160433435; Thu, 30 Mar 2023 00:13:53 -0700 (PDT) MIME-Version: 1.0 References: <20230328221644.803272-1-yosryahmed@google.com> <20230328221644.803272-5-yosryahmed@google.com> <20230329192059.2nlme5ubshzdbpg6@google.com> In-Reply-To: From: Yosry Ahmed Date: Thu, 30 Mar 2023 00:13:16 -0700 Message-ID: Subject: Re: [PATCH v2 4/9] cgroup: rstat: add WARN_ON_ONCE() if flushing outside task context To: Michal Hocko Cc: Shakeel Butt , Tejun Heo , Josef Bacik , Jens Axboe , Zefan Li , Johannes Weiner , Roman Gushchin , 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-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 212FD40007 X-Stat-Signature: 1tbjgqe6cotsbuuq7h9i6q1px63megeg X-HE-Tag: 1680160434-310991 X-HE-Meta: U2FsdGVkX1/8msSW1NgR7pbyfLQS2+y9xr/kE8DFPgtifPrq+Rb2cGxfXslCRK2lrAia+GQ0bb/hIXMTaFWOAu0vIfK2Er+5Us2nAWotUa0uyvN1ucOrUk13LZSYqcyyquVY13v7H9Qg355o/kYj/ox5kTkrVKiRhEIKfMPTi4kEQAD8cCCBMDcC+jnd/G+oJsDN0wKqW0slQ/89+Fr+GvfTtkGKqE/aI8jiv5aDgwAxWwoMlnPPKamo6/vOiXbh+7JybZY5+R/GTzW8PWIxnmZEWBOLJuvtOEEbh/pkS0goZZ7IzMkQlK5JVlbuvvcZVOdpwWC2E8JMtW8bt5JNumV5mBKSX/TSJCztIPw/EmoGENxe/lMvHnpON76w+GrV9IiHvLV47zwss4Be+CjtK6o5nhuiOr0Ha2AsU1uXSmo1vPb7XZJKF+3V341AmhWOlGV6gDhe0O4Qy2tgxWoV0IKQbseQ+ZZGbsrxRA5EBwlbS1bcU2OsQgLfCcA1BtjaUJGJd2qOh4794oy4EzKs6umDPdHHOiB7JnYnYJ9E7WXFpggVMf61vS2TRUKQzx3kgxfBMYTnceA/gSEu10jNORgziHaQpBUM5okq+91gjap7clo8FQJ8H1am39Pp7o155ZnpX+udN9ebd3IMHeNoJHCH71/3pcZe2GcKY0VJ+TFodVOLHASAvKEOUBxkAjbZqT2t8SJ9WKQuqeiR5YHRl8eIevUyOxuSPS10Lo0NO5kHWaNg1jHOxfWfdq8QGYgT2+spjvLoECYFSj+CrKCwXKsK+/a8p1p4z0Gt56UnSZNrGh71wOVDC85qyGOd6DOR9ZSkshGVw01UwqEJcvuOzgQHzLerBKZRiYReR/R6t4TiHhoEo2uHBiUyr0niGcJBDDHkQ06Cz5YxvKGSZPdpXvRRgF5S5koTxnwIt6vLcutg3BMBZbsvYo6wPuKH2Cn48Cn/J9Cq8Sy+NwXe9nB SS4v6jsH I1ZZvQ9Z7BN7CnXuTnJkXkkwaw1+kMfTN6YxlYIze8OPDFnnEYe0YInITzmrMeiMOIMb95zP/QLDUCFJTGxurAX4FvNi/sB/Eu7MfbsSLNP2KnfQXnkVqSmyKzCLdgAAHpxuSlzyrZcOf9H/VXmJKZnFmfrFjcstQboUp9FYwOcTd1fNUzvsu1uFn3GD4tRsXnPP6Hs30Mwn9tRFLUkLEWHixddxZQ+CJVApkLQScFakHuTHxWVL+hYy+lObqktO4z0uzlGn+ANiS4UnFxToA08ELLi7NkayRmP/yyzbqdFN764FrJTqwZmlyIo70kTN7ol63cJFYOta8wOBSoXs/Q4XWgMXWZ8mRmfq0gvHfJnPu3zo0a3/Elz1hWpyl0iTEsodAP/wtnSB2wtJqHwl/BTd6JLgoFa2tGU6MZFtPJgwiBRET5O31BxcpwQ== 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 30, 2023 at 12:06=E2=80=AFAM Michal Hocko wro= te: > > On Wed 29-03-23 19:20:59, Shakeel Butt wrote: > > On Wed, Mar 29, 2023 at 11:41:39AM -0700, Yosry Ahmed wrote: > > > On Wed, Mar 29, 2023 at 4:22=E2=80=AFAM Michal Hocko wrote: > > > > > > > > On Tue 28-03-23 22:16:39, Yosry Ahmed wrote: > > > > > rstat flushing is too expensive to perform in irq context. > > > > > The previous patch removed the only context that may invoke an rs= tat > > > > > flush from irq context, add a WARN_ON_ONCE() to detect future > > > > > violations, or those that we are not aware of. > > > > > > > > > > Ideally, we wouldn't flush with irqs disabled either, but we have= one > > > > > context today that does so in mem_cgroup_usage(). Forbid callers = from > > > > > irq context for now, and hopefully we can also forbid callers wit= h irqs > > > > > disabled in the future when we can get rid of this callsite. > > > > > > > > I am sorry to be late to the discussion. I wanted to follow up on > > > > Johannes reply in the previous version but you are too fast ;) > > > > > > > > I do agree that this looks rather arbitrary. You do not explain how= the > > > > warning actually helps. Is the intention to be really verbose to th= e > > > > kernel log when somebody uses this interface from the IRQ context a= nd > > > > get bug reports? What about configurations with panic on warn? Do w= e > > > > really want to crash their systems for something like that? > > > > > > Thanks for taking a look, Michal! > > > > > > The ultimate goal is not to flush in irq context or with irqs > > > disabled, as in some cases it causes irqs to be disabled for a long > > > time, as flushing is an expensive operation. The previous patch in th= e > > > series should have removed the only context that flushes in irq > > > context, and the purpose of the WARN_ON_ONCE() is to catch future use= s > > > or uses that we might have missed. > > > > > > There is still one code path that flushes with irqs disabled (also > > > mem_cgroup_usage()), and we cannot remove this just yet; we need to > > > deprecate usage threshold events for root to do that. So we cannot > > > enforce not flushing with irqs disabled yet. > > > > > > So basically the patch is trying to enforce what we have now, not > > > flushing in irq context, and hopefully at some point we will also be > > > able to enforce not flushing with irqs disabled. > > > > > > If WARN_ON_ONCE() is the wrong tool for this, please let me know. > > > > > > > If I understand Michal's concern, the question is should be start with > > pr_warn_once() instead of WARN_ON_ONCE() and I think yes we should star= t > > with pr_warn_once(). > > Yes, I do not really like the WARN_ON here. It is an overkill. pr_warn > would much less intrusive but potentially incomplete because you won't > know who that offender is. So if you really care about those then you > would need to call dump_stack as well. > > So the real question is. Do we really care so deeply? After all somebody > might be calling this from within a spin lock or irq disabled section > resulting in a similar situation without noticing. There are discussions in [1] about making atomic rstat flush not disable irqs throughout the process, so in that case it would only result in a similar situation if the caller has irq disabled. The only caller that might have irq disabled is the same caller that might be in irq context before this series: mem_cgroup_usage(). On that note, and while I have your attention, I was wondering if we can eliminate the flush call completely from mem_cgroup_usage(), and read the global stats counters for root memcg instead of the root counters. There might be subtle differences, but the root memcg usage isn't super accurate now anyway (e.g. it doesn't include kernel memory). With that removed, no callers to rstat flushing would be from irq context or have irqs disabled. There will only be one atomic flusher (mem_cgroup_wb_stats()), and we can proceed with [1] if it causes a problem. What do you think? [1] https://lore.kernel.org/lkml/CAJD7tkZrp=3D4zWvjE9_010TAG1T_crCbf9P64UzJ= ABspgcrGPKg@mail.gmail.com/ > -- > Michal Hocko > SUSE Labs