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 EB5C2C6FD1C for ; Thu, 23 Mar 2023 16:10:04 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 731406B0074; Thu, 23 Mar 2023 12:10:04 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 6E1366B0078; Thu, 23 Mar 2023 12:10:04 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5A9446B007B; Thu, 23 Mar 2023 12:10:04 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 4A2D86B0074 for ; Thu, 23 Mar 2023 12:10:04 -0400 (EDT) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 099A612042D for ; Thu, 23 Mar 2023 16:10:04 +0000 (UTC) X-FDA: 80600649528.17.71A804F Received: from mail-yw1-f182.google.com (mail-yw1-f182.google.com [209.85.128.182]) by imf19.hostedemail.com (Postfix) with ESMTP id 29FAF1A0009 for ; Thu, 23 Mar 2023 16:10:01 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=k5dpDN1D; spf=pass (imf19.hostedemail.com: domain of shakeelb@google.com designates 209.85.128.182 as permitted sender) smtp.mailfrom=shakeelb@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1679587802; a=rsa-sha256; cv=none; b=fx0Tq1dXEGkJ7Da7mOiF/uhBW0W139a4ynF+q10V2NoJI0e1fab8/O09OXcSn1QWEJ6VgC J0e9Jv2fLyjr83zaie6PAmtITfJQme+vCEqRCf/Vk/bhfTQX+Rj45rhEI6+++lYP4A5nDX lvB/CgsqcdgZxVm3O+UcBOgZIup2TYQ= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=k5dpDN1D; spf=pass (imf19.hostedemail.com: domain of shakeelb@google.com designates 209.85.128.182 as permitted sender) smtp.mailfrom=shakeelb@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=1679587802; 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=BgZMEf1PHFMinoIbPEomXcwtNzt8eUFNPvTXJsCn0P8=; b=rPs6rBdDve862+VeiDWQysWsvp2XQlA0xez7zmVb0PU34Rqi6a1MWXjo8x+U23i+bv5mFj Jz2EzTGytvl2WED7DFDeVN6y10uIqLY90pTuWcjynusJCOarlwI3IGAY/NWMG8k3VDSte4 QcdyFCwQYip+59dYoBmPXrULDGX84Yc= Received: by mail-yw1-f182.google.com with SMTP id 00721157ae682-5445009c26bso403096327b3.8 for ; Thu, 23 Mar 2023 09:10:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1679587801; 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=BgZMEf1PHFMinoIbPEomXcwtNzt8eUFNPvTXJsCn0P8=; b=k5dpDN1DHcGjTlwbOvMKLMRzGlnzZn29V1PdhQ9swzOTalJXOVLnQWZ41YtSQ6iUrk 3tYVVzf3TXbBDU3Fht0rRrJX5GujygwDq4CznsLvQvc3iwlzWZ7RjrlJRpn6zm+LX6Is wg3xRL0pVLcK4/WUOQciLucr4KsYng/0RiN3fJUrGmHZTbbYLdyI+Mxa6fi3ob6cRFNq 8SpTkAvVF4xMY6kgvEFxHg+HMcX5zbBQrAgc5RP417XZQXgpltBfUeyhzDW4y/NM+4Qd L5xz9jZ+0DhO8BpAwodd5of1IQ/G/KeTFSQm8YwjDJLsC/QfhlZPdbgvRtQ21H8E1ZYK ce/w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679587801; 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=BgZMEf1PHFMinoIbPEomXcwtNzt8eUFNPvTXJsCn0P8=; b=nvjcqypchpWod9sjtj148et/RCG5pKOa110fgwpANE9TsBxpop1xWa2OwTOTrVUsd2 SE5czQSYJCodPSSx8ppf/lyZF58PLos7I9EdExRdDscci4h1fa6icWWDcQBHQI2Es87e Yd4wle4m9fBw0zykRRvy0jCuEJKhgNOV8RiwWXFeZCE7QNLZf7da5Ag35TixplaOOXrt gwuv7m9Xr7lVKvlsDFWkSAA7o1boilf+nAhcPiXOEEbWQdg/Wphsg+qhJXy1vzS4jIP1 O7NmxtvjQywZ+8w6NhIz1YDuMOTyJT35O7l9oi6AhJQU7s+3L0TO7mrL9J36P9PWJlmE IJoA== X-Gm-Message-State: AAQBX9e1q96TtnlmFD8sd+DlWenH21cDhwsEsI3O6uc1gWG5TG91d/Rh SGx2TRyiZogUl/q3kFGqgwwIO8wA2KsVerBcCqjqug== X-Google-Smtp-Source: AKy350YABS+oeu6VgfSzXUY1rl2LsGxM5mQomi4AwvPmARWb+QR5iqh2XFdsgMJ1XB5supf5A5Kgty3nOBMi86V4FKQ= X-Received: by 2002:a81:ae1c:0:b0:52e:e095:d840 with SMTP id m28-20020a81ae1c000000b0052ee095d840mr2159466ywh.0.1679587801172; Thu, 23 Mar 2023 09:10:01 -0700 (PDT) MIME-Version: 1.0 References: <20230323040037.2389095-1-yosryahmed@google.com> <20230323040037.2389095-2-yosryahmed@google.com> In-Reply-To: From: Shakeel Butt Date: Thu, 23 Mar 2023 09:09:49 -0700 Message-ID: Subject: Re: [RFC PATCH 1/7] cgroup: rstat: only disable interrupts for the percpu lock To: Yosry Ahmed Cc: Tejun Heo , Josef Bacik , Jens Axboe , Zefan Li , Johannes Weiner , Michal Hocko , Roman Gushchin , 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-Rspam-User: X-Rspamd-Queue-Id: 29FAF1A0009 X-Rspamd-Server: rspam01 X-Stat-Signature: 6f49u75996fpspw8hoa78t6byn5c5rgm X-HE-Tag: 1679587801-234499 X-HE-Meta: U2FsdGVkX1/UIvKrsfubpTKfrOOz/7xprOglU1TBUplrZEmdnTKdqRNHlCvID3WaY6T1JwBamY6ZQt7hfK11tnFCIry2CFHZTgp1obuGA2j1AvzGEMUX8XymjfBTrW2ffJzzFbajULJX6zaCFKoxjwHZcwT7Qbx0DRTQS63RviciknFAEIiz6su96xfdUq8YTe0sr0oub2yktLtXPGb4mAcVAcuV9p0RL3ktJbbn/vxXIG7HD6FdeWxXzeQv1WUosJlr402AHRF7yQtvsvmUQCgRqJupJPuDVdZRqGl3nQ/KrT86uyixhA31AZ/6DKjSUPLNK0IdjdCmy7cBrobYABTwCBEmwxHhPp4y8+zVt4L+r8BBizi71KkC9VSqWgNy4mnXG7e+k9FgV5iDf2ck06vq0n1zHp3xNapALt6rCg2nK/XYlAemeNGvqkGvb7eFkscwdKS4UeBMsj8HqlkcY78cr1kk8eMT92ELEjkS8Qj3jZnjd/4l6hk22WvN7WsNuscS6LLm5toLgu/9+HcoJQfKb7ZecumYF2WdO+MgPPrQUcFOYicLhJ7kUleZeb+zi4mPDtE4ZyBXDQx9yUY9i83OH04erKD3ImBSsPkFoW3nRWw2nDTDtYP1h0msT3gzpuhqoPvmAD0cAIwQlcKya3zANObE4VzsZPk/I4dTq1zEUG+0eWHDQwyy0ExatbWivLmkRRCNGhE4Xf9kQ1qg1sO6CY8bFFx3vVcyfDrCQkyOqv5wAlaGGEIv116R9sW8Vas2XTKY+3pkvjlM0qtZlQghttREwGFqozZvpif421GAhFzaRRCORweRbSyD2ora2jlW1pD9SZVOYyc42oFu+bwWBKHDQEevbOeX8oNKfw6ZfRktM0NB18MqeuRyATM2NZXDKX2Onl3WD2zdUExZ8NJeI20CB4ejRLTQRdh9YTSEks62kp40cucnmA6YBhKwGZx7utvdu7+72h4UfqE ogmhYqkl fSo0GVA92bX+1Fu0P/34mJWrGCFvqsX8TkbVjuXUHipfQHm1F9rjapFZjBn2ZwSRPRil7EOeR7GSnJA4eGTyBQ0WanJpnFH1XeUlSbIWy849D9B5YraQQybEUhJ+UetTG9XkQo1hz6VPlclVfnodpKcZ7KcP/RP8fRsahC/Wh1wvc5/k14OOA2EEB/EMQTpLnB+wL8jWxHMZ1NZimF/DZf/TgZr+CruJXkWVZY/ZyVa4Fr139AIppodrI4Z+9vCsL924v/Yb9ARUvx0lHH1/givXsznpAkKY1imd/ofvaTFgaWIxFEJDT1ZCs6g== 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:46=E2=80=AFAM Shakeel Butt = wrote: > > On Thu, Mar 23, 2023 at 8:43=E2=80=AFAM Yosry Ahmed wrote: > > > > On Thu, Mar 23, 2023 at 8:40=E2=80=AFAM Shakeel Butt wrote: > > > > > > On Thu, Mar 23, 2023 at 6:36=E2=80=AFAM Yosry Ahmed wrote: > > > > > > > [...] > > > > > > > > > > > > > 2. Are we really calling rstat flush in irq context? > > > > > > > > > > > > I think it is possible through the charge/uncharge path: > > > > > > memcg_check_events()->mem_cgroup_threshold()->mem_cgroup_usage(= ). I > > > > > > added the protection against flushing in an interrupt context f= or > > > > > > future callers as well, as it may cause a deadlock if we don't = disable > > > > > > interrupts when acquiring cgroup_rstat_lock. > > > > > > > > > > > > > 3. The mem_cgroup_flush_stats() call in mem_cgroup_usage() is= only > > > > > > > done for root memcg. Why is mem_cgroup_threshold() interested= in root > > > > > > > memcg usage? Why not ignore root memcg in mem_cgroup_threshol= d() ? > > > > > > > > > > > > I am not sure, but the code looks like event notifications may = be set > > > > > > up on root memcg, which is why we need to check thresholds. > > > > > > > > > > This is something we should deprecate as root memcg's usage is il= l defined. > > > > > > > > Right, but I think this would be orthogonal to this patch series. > > > > > > > > > > I don't think we can make cgroup_rstat_lock a non-irq-disabling lock > > > without either breaking a link between mem_cgroup_threshold and > > > cgroup_rstat_lock or make mem_cgroup_threshold work without disabling > > > irqs. > > > > > > So, this patch can not be applied before either of those two tasks ar= e > > > done (and we may find more such scenarios). > > > > > > Could you elaborate why? > > > > My understanding is that with an in_task() check to make sure we only > > acquire cgroup_rstat_lock from non-irq context it should be fine to > > acquire cgroup_rstat_lock without disabling interrupts. > > From mem_cgroup_threshold() code path, cgroup_rstat_lock will be taken > with irq disabled while other code paths will take cgroup_rstat_lock > with irq enabled. This is a potential deadlock hazard unless > cgroup_rstat_lock is always taken with irq disabled. Oh you are making sure it is not taken in the irq context through should_skip_flush(). Hmm seems like a hack. Normally it is recommended to actually remove all such users instead of silently ignoring/bypassing the functionality. So, how about removing mem_cgroup_flush_stats() from mem_cgroup_usage(). It will break the known chain which is taking cgroup_rstat_lock with irq disabled and you can add WARN_ON_ONCE(!in_task()).