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 35878C282C5 for ; Sat, 1 Mar 2025 01:30:16 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B8F9D6B007B; Fri, 28 Feb 2025 20:30:15 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id B3F3D6B0083; Fri, 28 Feb 2025 20:30:15 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9E037280001; Fri, 28 Feb 2025 20:30:15 -0500 (EST) 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 7E14B6B007B for ; Fri, 28 Feb 2025 20:30:15 -0500 (EST) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 16D0BB6C49 for ; Sat, 1 Mar 2025 01:30:15 +0000 (UTC) X-FDA: 83171251590.06.54304F6 Received: from mail-pl1-f177.google.com (mail-pl1-f177.google.com [209.85.214.177]) by imf05.hostedemail.com (Postfix) with ESMTP id EC30F100005 for ; Sat, 1 Mar 2025 01:30:12 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=GvZgjGLv; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf05.hostedemail.com: domain of inwardvessel@gmail.com designates 209.85.214.177 as permitted sender) smtp.mailfrom=inwardvessel@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1740792613; 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=WcnC0wr+7zYlTTCeDV2IWo4JFlYwYpgFbPA87xiSvVg=; b=iado7vBtaXJlnoeSZcfx2sp+Lq4S/lw8sjMpzgRsObiGHB6htJUYkZPnhAtqmE3GX/CMrw H57EC0St9dtxrqxbmSQwHGED+zA65a9fKE0yQFURkKX+f9MHzGMGhDJzgQnfnXwwocKgAJ x8C0BxtJRVnmy3iCqXAzMzkDNhvbmF4= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1740792613; a=rsa-sha256; cv=none; b=RjuBJHvKrxzIR6s3Rv04OGtuiY2agH+LfQuHzVg/lghWoIP1VivBEq28NAVxK+CtP47Z1R e5VB+Soem5zjTLbojJRaiFQpDfjn1jAwa4Efpwdx8LzRnnaTv3RtIKQBIo6x03g+9bGRxJ LeCm3zTSsUHbEEj8o15u+Jcfpm/qauo= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=GvZgjGLv; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf05.hostedemail.com: domain of inwardvessel@gmail.com designates 209.85.214.177 as permitted sender) smtp.mailfrom=inwardvessel@gmail.com Received: by mail-pl1-f177.google.com with SMTP id d9443c01a7336-22374f56453so25807445ad.0 for ; Fri, 28 Feb 2025 17:30:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1740792612; x=1741397412; darn=kvack.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=WcnC0wr+7zYlTTCeDV2IWo4JFlYwYpgFbPA87xiSvVg=; b=GvZgjGLvNraq1XybEDEh1fZwHpqrhQYQPIAriOmJyu2SKpEKCT//hv9uzOuxlYnRt6 1xlHCj3DrYTEfZ2FHz71nm4nbtCJshpAFdkxx5hIvgE0Th8HY/OvAymOUz4CJmXMyCoA RAFZho74apBPAzvm13TOmE6Z1kR7PJ3QaJYpYey2iQ2DpKerxBlvx297lJDJ4CcFEnQG m2AG4vsb+QI30XqI1ZWC71A6SYYQwB4+tBzUm8/74bOGfQ76G40s2WZtQZicu1/I7juR 0ymM+FtVMg2P7YB5AwUvsqjDRTZxa0utwpPJv5aBvvrFWr1QK3equYS1mQynH7hrmzOC UpXA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1740792612; x=1741397412; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=WcnC0wr+7zYlTTCeDV2IWo4JFlYwYpgFbPA87xiSvVg=; b=o9/NjuILkMOkv3vEUFDPAQZfL1LGwNEq5wb+BlVHpPRh49W2wruMmASqEBRlZ+BSTY h7WR9UnmQ/94tGzlNcwzWUf9AIHEirFhn4enK69sY7MS9KBijeI+vBznaaOvEkoyTYUC A+RLRt/Ad7f3Aw17OwElyapBpIbUHq9PepEXnDggema8HkxY92auIDsww38xQP1q5U21 RWSroec+666+CQeudBjKiLgiLZtQG9jw2OJ6NDetxF2rwIq/uJip0HvAu3xbZXpKgAy7 d0AzuM1saiBv+uh2DhFw00g1oSIjZNRIVvpu7dt/18espH/IPaDD+DmmKr8yMEwE86vC nyXA== X-Forwarded-Encrypted: i=1; AJvYcCWGhZCDwWXq4z+c25f9IBBMBwEN/8TCGDKGDM2cGRdiQ9zrVdSh0Upzw9ClP1+gvVEW+GeN/0+Nng==@kvack.org X-Gm-Message-State: AOJu0YyI1Gl/OaPfyEApDe5odpzVmmOJ93vb6YhI4nYhhGh11cuwx9Ht E7m4J4lmh0Ke+fy/x3OFBJjZdJhAFtR8xeIQlPuGGc5WvaNv9kU+ X-Gm-Gg: ASbGncvcr6i3+tjuHf5OtqCvyHB6VQUez2MFexrf2zYqjSWiL+2mUuhaOiQaiy8Csxc PG0xPBEX/La9V/zND2hJXjkR5RJWPH5oKAeg7jqGG6G9uMQp7j45VNH6umHt7/vKL+aS8LI4t4/ 0pGbn2uW+Nhl6BeHE098sj9C93epY4OXhnkNRyLVvMKcL++Tnr9yjEqJmdTGC3NiCVZ9Uys0HdJ bY2gTnsD7HCwZdnBPJbDqOaq40KdU2vG/SsyG3QTMBY32KeObZ8zNINgFkVrKVUiehXibKGVWc0 xB68mKjJ/4ibbN7oo2ksyoqnpipRBnjyqBCQKUeu8VBc0vaFdFz3zK2EXLhnX6lzyoAHUCfg653 oFKd/QhMV+qvUxgE= X-Google-Smtp-Source: AGHT+IFS84nBKQ7qD5FSWHU3UAF4RGLGLUtVzTkLxoE6IYbV54utngWld0V1qE0kZjahmoo9ERDBrQ== X-Received: by 2002:a05:6a00:4b10:b0:731:43ca:5cc6 with SMTP id d2e1a72fcca58-734ac3d3761mr8854564b3a.15.1740792611653; Fri, 28 Feb 2025 17:30:11 -0800 (PST) Received: from [192.168.2.117] (c-67-188-127-15.hsd1.ca.comcast.net. [67.188.127.15]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-73637081ba7sm178941b3a.112.2025.02.28.17.30.09 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 28 Feb 2025 17:30:09 -0800 (PST) Message-ID: <0a551dcc-6a95-46a4-9a60-7e62200e63ef@gmail.com> Date: Fri, 28 Feb 2025 17:30:08 -0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 1/4 v2] cgroup: move cgroup_rstat from cgroup to cgroup_subsys_state To: Yosry Ahmed Cc: tj@kernel.org, shakeel.butt@linux.dev, mhocko@kernel.org, hannes@cmpxchg.org, akpm@linux-foundation.org, linux-mm@kvack.org, cgroups@vger.kernel.org, kernel-team@meta.com References: <20250227215543.49928-1-inwardvessel@gmail.com> <20250227215543.49928-2-inwardvessel@gmail.com> Content-Language: en-US From: JP Kobryn In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: EC30F100005 X-Stat-Signature: w5onnshjxdiwyns4gpbfbkjyqu9dtdpz X-HE-Tag: 1740792612-504073 X-HE-Meta: U2FsdGVkX19BUQF+cqvvpf4wFlTJS8f91LBBc0tiFj8QreJRRZVkQR3l6ahb7BTbeotdyx4oMftJOVgE5uG/A5oE6gbF1vjhqM6KSnhjNCcodVLPm4s5grxOkwLIHIYzKfn0hroDiMQUAZa7zHUTy84RRLI97V3HJu7WkbzZyH0PTrmfhQlW19WcOSsLhP5izm5vPM5gtb799xqQwgWfkWXlINibGdv8IaOz7ubeMujax4gBl1AaLeMc+ijsg/DZJ8T+dH7BzAR0CWeS38rsMFz9+HdmMZRUsxfS+dxOTHCPC4bCpeyXkr7QnWmg5JOF8Vtq6R/qEI9vHg0p3Iku/a4erV94d5rR1e+pCfPS84Lce6EC43+AlHJCV3hj6c/jg2AtohWG4FlrjwQf5qX8V9ceOpbdmF4Y3wFc6mNe5Zpvp/SrgARhXwwB2xXCUwg7iULB/wDtUnCYQjwMmCVjVv/xJE7IHVivXqhn4mKdK/bVV+8QCSgd/Ic6BBJmDfXd34OCv2FCxvc/8pnlGoqLsi1wyrdsucInlOoKMFE/+H0D0tfC4rqVufvwxxk0TKB4v5A/1ebBsbiQum6cEiklkcb04DjzaOnCT5MPnWpTQ5mzLP7FSFj13AtZoYN5l/nU1wEgv0O0E3SaemezmoRq2S4aci/7oqHK+QAcCOqEYmNpSXOIFHImJ0ChxSEOOeuuVbAmpbhdxFr7BtW2hvG4z/gjjoRNQBzgEeO9CyK+MSzbpuK61e04bSEyezKchSwgWGydr5SZiA54kNSTunDduZO4pNaHtGhN8kDSSaIUhgX1Na2P7n7V21zVtZJ5fBWVjLKfXd5h8bsBFvl2FM9mCcvflEmH6Whj4OJpFiC7Oai2QS6iqGPZW4sTbauqTV+KuJzph3iz9vJcRf6VzEBbQGnMO59vSzyhpM9Sn/tjASR4vMTr55NZ2RE3qCmNG9BndPPLZdf0FOLrwBSSUaI UzT+aGPu WX9NcdrcyeOiV4IFX2FYkw2Tqq8Cuoio3yBZVuDFLnfbWWgv7khfjHfqmWGRX3qd4C0cW410SGIdxqBGJwc0l3XoULIfe72TymZiselNeA49bHg5XsA1vsXn+iMEOnwCXiq5yRJ/BBFZ3PmUI4y/CMYNHGWy4Z/s9h2ZsjNlW1dCC3cO85Rfm9G73ZQCuqMgmvEN8+iPV5vGET3uOiAJenk9UVP7s2g+QUKuFikpWe9tGeNVUaI3Fq8uvz/AzcaohSiN9pcUQ8bfDWBHCFupUKNj6hmqxZPHLZh7JZc4ps5Ihga9Yl2vGq2J9ZnvY1WLJFo8zfFB58Kk9Xq1rDmoiMVEHJWuOhkJCvcZF918Hg8TWm4tP/CpJMTTe4SplndfXnkBaf/1A9dZB+iha3DvoQP1Ojzki8qD/TKR1OZ+ZuYfmcJAotDoygpv1no0QKhccW9yK 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: List-Subscribe: List-Unsubscribe: On 2/28/25 5:25 PM, Yosry Ahmed wrote: > On Fri, Feb 28, 2025 at 05:06:23PM -0800, JP Kobryn wrote: > [..] >>> >>>> cgroup_idr_replace(&ss->css_idr, NULL, css->id); >>>> if (ss->css_released) >>> [..] >>>> @@ -6188,6 +6186,9 @@ int __init cgroup_init(void) >>>> css->id = cgroup_idr_alloc(&ss->css_idr, css, 1, 2, >>>> GFP_KERNEL); >>>> BUG_ON(css->id < 0); >>>> + >>>> + if (css->ss && css->ss->css_rstat_flush) >>>> + BUG_ON(cgroup_rstat_init(css)); >>> >>> Why do we need this call here? We already call cgroup_rstat_init() in >>> cgroup_init_subsys(). IIUC for subsystems with ss->early_init, we will >>> have already called cgroup_init_subsys() in cgroup_init_early(). >>> >>> Did I miss something? >> >> Hmmm it's a good question. cgroup_rstat_init() is deferred in the same >> way that cgroup_idr_alloc() is. So for ss with early_init == true, >> cgroup_rstat_init() is not called during cgroup_early_init(). > > Oh I didn't realize that the call here is only when early_init == false. > I think we need a comment to clarify that cgroup_idr_alloc() and > cgroup_rstat_init() are not called in cgroup_init_subsys() when > early_init == true, and hence need to be called in cgroup_init(). > > Or maybe organize the code in a way to make this more obvious (put them > in a helper with a descriptive name? idk). I see what you're getting at. Let me think of something for v3. > >> >> Is it safe to call alloc_percpu() during early boot? If so, the >> deferral can be removed and cgroup_rstat_init() can be called in one >> place. > > I don't think so. cgroup_init_early() is called before > setup_per_cpu_areas(). Cool. Thanks for pointing that out. > >> >>> >>>> } else { >>>> cgroup_init_subsys(ss, false); >>>> } >>> [..] >>>> @@ -300,27 +306,25 @@ static inline void __cgroup_rstat_unlock(struct cgroup *cgrp, int cpu_in_loop) >>>> } >>>> /* see cgroup_rstat_flush() */ >>>> -static void cgroup_rstat_flush_locked(struct cgroup *cgrp) >>>> +static void cgroup_rstat_flush_locked(struct cgroup_subsys_state *css) >>>> __releases(&cgroup_rstat_lock) __acquires(&cgroup_rstat_lock) >>>> { >>>> + struct cgroup *cgrp = css->cgroup; >>>> int cpu; >>>> lockdep_assert_held(&cgroup_rstat_lock); >>>> for_each_possible_cpu(cpu) { >>>> - struct cgroup *pos = cgroup_rstat_updated_list(cgrp, cpu); >>>> + struct cgroup_subsys_state *pos; >>>> + pos = cgroup_rstat_updated_list(css, cpu); >>>> for (; pos; pos = pos->rstat_flush_next) { >>>> - struct cgroup_subsys_state *css; >>>> + if (!pos->ss) >>>> + cgroup_base_stat_flush(pos->cgroup, cpu); >>>> + else >>>> + pos->ss->css_rstat_flush(pos, cpu); >>>> - cgroup_base_stat_flush(pos, cpu); >>>> - bpf_rstat_flush(pos, cgroup_parent(pos), cpu); >>>> - >>>> - rcu_read_lock(); >>>> - list_for_each_entry_rcu(css, &pos->rstat_css_list, >>>> - rstat_css_node) >>>> - css->ss->css_rstat_flush(css, cpu); >>>> - rcu_read_unlock(); >>>> + bpf_rstat_flush(pos->cgroup, cgroup_parent(pos->cgroup), cpu); >>> >>> We should call bpf_rstat_flush() only if (!pos->ss) as well, right? >>> Otherwise we will call BPF rstat flush whenever any subsystem is >>> flushed. >>> >>> I guess it's because BPF can now pass any subsystem to >>> cgroup_rstat_flush(), and we don't keep track. I think it would be >>> better if we do not allow BPF programs to select a css and always make >>> them flush the self css. >>> >>> We can perhaps introduce a bpf_cgroup_rstat_flush() wrapper that takes >>> in a cgroup and passes cgroup->self internally to cgroup_rstat_flush(). >> >> I'm fine with this if others are in agreement. A similar concept was >> done in v1. > > Let's wait for Shakeel to chime in here since he suggested removing this > hook, but I am not sure if he intended to actually do it or not. Better > not to waste effort if this will be gone soon anyway. > >> >>> >>> But if the plan is to remove the bpf_rstat_flush() call here soon then >>> it's probably not worth the hassle. >>> >>> Shakeel (and others), WDYT? >>