Message ID | 20221020074701.84326-1-haifeng.xu@shopee.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4ac7:0:0:0:0:0 with SMTP id y7csp126716wrs; Thu, 20 Oct 2022 01:05:10 -0700 (PDT) X-Google-Smtp-Source: AMsMyM6dWKXj8OLi3NKR2YKhjfe8hagM7AXe9U3eQL3LcFXPETSYvK6Y8zNmHy10wHMgTdZNdnI1 X-Received: by 2002:a17:906:ef8c:b0:78d:4a00:7c7b with SMTP id ze12-20020a170906ef8c00b0078d4a007c7bmr10265402ejb.187.1666253110240; Thu, 20 Oct 2022 01:05:10 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666253110; cv=none; d=google.com; s=arc-20160816; b=fottBTADztl5kbZ5bDJ0GHaXglYE3U3qIjchEZkxNmHuxKZ/n3oZLGqgskgaj2D0yi TWg1kWFPBaElxW4qCXAGOtUhnJ57AFq0EGJztIj3wWv2c46dcuKE86SekBgI5MR1GwuX KxbIlGtxkGgWRjFyEa464LaBhgPcD6vofIn9Uldy713fbqB5/vLGpTFONokw4yMpsYAT 9KNAh9C3lXqA2y4iv1YsJOpVA8xWFC9JYKmaVyANsLyX//TyuAl3HJzJ01NuFH/CYIE1 RCa6jJQRTNkmbMeR1qXWdPQWlKgt/0S9NVSwjM98RQWcTLejz8bYZklenmM7sa65MN2e IIFQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=WEv0DAtYrQBEKc4KQv+MDWqqO4sqebBTQ3kaboRnQNg=; b=rAybRjPRcsjnbSaf0etsZ6HRjPsxs39CudQKLjghLZj7ZE+BeruaNqqBm2HeO+srov 7x0qZk7Ee8oRz/ofdPv7R4ycQwuM+UsV91UpyLcN/K9g5ztjC5t8EKHm99qfs6KB4kxo a0guPL3G0CbNT8cvnYaj4lbLhe5h9ZT7HQ0pKtIT0gzqwbBK7/3HPjfRPq0TBpPE8Lmf 7wyErVgQhVefUsxxryNt0mJGj2FLM2F9tEbVY3m3Kl6jVKNr9Ao+toR/hUP0GozPNyoz MlIZ/+e29Xm6wcQ9MKHgNyoUjwIT9a2/N2WZRCggu3tWT3xORl0uYtU55rJJnQ0DEJGo naJw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@shopee.com header.s=shopee.com header.b=GTQ3+2aB; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=shopee.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id e18-20020a17090658d200b0078e0e8508fbsi18446381ejs.457.2022.10.20.01.04.20; Thu, 20 Oct 2022 01:05:10 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@shopee.com header.s=shopee.com header.b=GTQ3+2aB; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=shopee.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230360AbiJTHrv (ORCPT <rfc822;realc9580@gmail.com> + 99 others); Thu, 20 Oct 2022 03:47:51 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50504 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230307AbiJTHrt (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 20 Oct 2022 03:47:49 -0400 Received: from mail-pj1-x1035.google.com (mail-pj1-x1035.google.com [IPv6:2607:f8b0:4864:20::1035]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id EF07B17579B for <linux-kernel@vger.kernel.org>; Thu, 20 Oct 2022 00:47:47 -0700 (PDT) Received: by mail-pj1-x1035.google.com with SMTP id pq16so19072919pjb.2 for <linux-kernel@vger.kernel.org>; Thu, 20 Oct 2022 00:47:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=shopee.com; s=shopee.com; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=WEv0DAtYrQBEKc4KQv+MDWqqO4sqebBTQ3kaboRnQNg=; b=GTQ3+2aB72+kOTV0y/y2Ncy5TWnk/z4Cj5D2tlFLPIwoT+7QGZOijPWbsL6YlAkTR2 WH6rXobe+/n1Op2T4Bkpy9k87M8Eihx9tndD/7jNX/1bQbEeceTVqzY0GKTOKj5mSIYn yX+uFQLqGCfD2BwOppd9RVzffo/ZpfeFt1SGkK9aFWDcLq0Kh8AtY+WHaxEzRe20mDnG MKjRrA2S3k7IDrGfHDygEhy5zVbNDzzFDKhkI2oBWOeH0zGZs9+Wa4SbJPTHgw7hGfW4 7mgZq9Rt5Gr+HE7ElBc8URsjfh7LITM/q6YvddDawZP1iiwOGdcQnuNR/b5RG5tmqDah /G3w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=WEv0DAtYrQBEKc4KQv+MDWqqO4sqebBTQ3kaboRnQNg=; b=nlO9SuDFDF+FVwgsutrARTOW+04iNDLVItxiq7ebQ8mieduRQdrzD2RydDj/QPAqZ7 ealQUGdlutwvzsLYO9kkGYJnJPmnH952ix6Y+SP/wOFBahVHG9rQhadfKklTtpBW1Mz6 7gW+Zjndz6Bn01VslMtkhHwIKHNKUDZAdcLQf33Vv1hPAxWmykcD3JV8POLUSjPDb08k BKALSU5CIDZTZnDMS+5oNSI3N5zIi6VTN2C36GBucc89bLgrWI/c6sfabX6ijmdk5VCa Dx0fG9LM0JRw8Hh6CokcH4epG0NX5wWDwORd3YYn6Bfe5HoOqEfQwzZK0s0aRKhFyssG wyWg== X-Gm-Message-State: ACrzQf0g05nPqSdhPnRQyy30uEFTsgp+8gKb/RlpRn2G6cWG2nMrROJC OumvWEmNKS+rOjK9yu3kxrIFkw== X-Received: by 2002:a17:903:2286:b0:185:3948:be93 with SMTP id b6-20020a170903228600b001853948be93mr12818567plh.121.1666252067220; Thu, 20 Oct 2022 00:47:47 -0700 (PDT) Received: from ubuntu-haifeng.default.svc.cluster.local ([101.127.248.173]) by smtp.gmail.com with ESMTPSA id q8-20020a170902eb8800b00176a2d23d1asm11939007plg.56.2022.10.20.00.47.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 20 Oct 2022 00:47:46 -0700 (PDT) From: "haifeng.xu" <haifeng.xu@shopee.com> To: tj@kernel.org Cc: lizefan.x@bytedance.com, hannes@cmpxchg.org, cgroups@vger.kernel.org, linux-kernel@vger.kernel.org, "haifeng.xu" <haifeng.xu@shopee.com> Subject: [PATCH] cgroup: Simplify code in css_set_move_task Date: Thu, 20 Oct 2022 07:47:01 +0000 Message-Id: <20221020074701.84326-1-haifeng.xu@shopee.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1747193021307698362?= X-GMAIL-MSGID: =?utf-8?q?1747193021307698362?= |
Series |
cgroup: Simplify code in css_set_move_task
|
|
Commit Message
Haifeng Xu
Oct. 20, 2022, 7:47 a.m. UTC
Check the populated state of css_set in css_set_update_populated
and update the populated state of to_cset after from_cset is updated.
Signed-off-by: haifeng.xu <haifeng.xu@shopee.com>
---
kernel/cgroup/cgroup.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
Comments
Hello. On Thu, Oct 20, 2022 at 07:47:01AM +0000, "haifeng.xu" <haifeng.xu@shopee.com> wrote: > - lockdep_assert_held(&css_set_lock); > + if (!cset || css_set_populated(cset)) > + return; 1) the guard should be css_set_populated() ^ populated (when unsetting the populated trait) 2) I'd keep the lockdep_assert_held() after it anyway. Also the commit message should explain what's the reason to move css_set_populated() after css_set_move_task(). Thanks, Michal
On 2022/10/27 16:05, Michal Koutný wrote: > Hello. > > On Thu, Oct 20, 2022 at 07:47:01AM +0000, "haifeng.xu" <haifeng.xu@shopee.com> wrote: >> - lockdep_assert_held(&css_set_lock); >> + if (!cset || cset is either getting the first task or losing the last(cset)) >> + return; > > 1) the guard should be css_set_populated() ^ populated (when unsetting > the populated trait) > > 2) I'd keep the lockdep_assert_held() after it anyway. > > Also the commit message should explain what's the reason to move > css_set_populated() after css_set_move_task(). > > > Thanks, > Michal Hi, Michal. 1) If calls 'css_set_update_populated' , the cset is either getting the first task or losing the last. There is a need to update the populated state of the cset only when 'css_set_populated' returns false. In other words, the last has been deleted from from_cset and the first hasn't been added to to_cset yet. 2) Thanks for your suggestion. I'll keep 'lockdep_assert_held'. 3) In order to update the populated state of to_cset the same way from_cset does, 'css_set_update_populated' is also invoked during the process of moving a task to to_cset. Thanks, Haifeng
Hello. > 1) If calls 'css_set_update_populated' , the cset is either getting the > first task or losing the last. There is a need to update the populated > state of the cset only when 'css_set_populated' returns false. > In other words, the last has been deleted from from_cset and the first > hasn't been added to to_cset yet. I've likely misread the condition previously. I see how this works now (update happens after "from_cset" but before "to_cset" migration). > 3) In order to update the populated state of to_cset the same way > from_cset does, 'css_set_update_populated' is also invoked during the > process of moving a task to to_cset. As I think more about this in the context of vertical migrations (ancestor<->descendant, such as during controller dis- or enablement), I'm afraid the inverted order would lead to "spurious" emptiness notifications in ancestors (in the case a there is just a single task that migrates parent->child, parent/cgroup.populated would generate and event that'd be nullified by the subsequent population of the child). So I'm not sure the change is worth it. Michal
On 2022/10/31 21:11, Michal Koutný wrote: > Hello. > >> 1) If calls 'css_set_update_populated' , the cset is either getting the >> first task or losing the last. There is a need to update the populated >> state of the cset only when 'css_set_populated' returns false. >> In other words, the last has been deleted from from_cset and the first >> hasn't been added to to_cset yet. > > I've likely misread the condition previously. I see how this works now > (update happens after "from_cset" but before "to_cset" migration). > >> 3) In order to update the populated state of to_cset the same way >> from_cset does, 'css_set_update_populated' is also invoked during the >> process of moving a task to to_cset. > > As I think more about this in the context of vertical migrations > (ancestor<->descendant, such as during controller dis- or enablement), > I'm afraid the inverted order would lead to "spurious" emptiness > notifications in ancestors (in the case a there is just a single task > that migrates parent->child, parent/cgroup.populated would generate and > event that'd be nullified by the subsequent population of the child). Hi, Michal. I understand your worries. Can I just check the populated state of css_set in 'css_set_update_populated' and don't change the order any more? I think it can also streamline 'css_set_move_task' a bit. Thanks, Hiafeng. > > So I'm not sure the change is worth it. > > Michal
On Thu, Nov 03, 2022 at 10:13:22AM +0800, Haifeng Xu wrote: > I understand your worries. Can I just check the populated state of > css_set in 'css_set_update_populated' and don't change the order any > more? I think it can also streamline 'css_set_move_task' a bit. FWIW, I don't see much value in the proposed change. The resulting code isn't better in any noticeable way. Even if the change were straightforward, the value of the patch would seem questionable. There's no point in creating code churns like this. Nothing is improved in any material way while creating completely unnecessary risk for subtle breakages. Thanks.
On 2022/11/3 10:31, Tejun Heo wrote: > On Thu, Nov 03, 2022 at 10:13:22AM +0800, Haifeng Xu wrote: >> I understand your worries. Can I just check the populated state of >> css_set in 'css_set_update_populated' and don't change the order any >> more? I think it can also streamline 'css_set_move_task' a bit. > > FWIW, I don't see much value in the proposed change. The resulting code > isn't better in any noticeable way. Even if the change were straightforward, > the value of the patch would seem questionable. There's no point in creating > code churns like this. Nothing is improved in any material way while > creating completely unnecessary risk for subtle breakages. > > Thanks. > Got it, thanks.
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c index d922773fa90b..6c474be57f91 100644 --- a/kernel/cgroup/cgroup.c +++ b/kernel/cgroup/cgroup.c @@ -861,7 +861,8 @@ static void css_set_update_populated(struct css_set *cset, bool populated) { struct cgrp_cset_link *link; - lockdep_assert_held(&css_set_lock); + if (!cset || css_set_populated(cset)) + return; list_for_each_entry(link, &cset->cgrp_links, cgrp_link) cgroup_update_populated(link->cgrp, populated); @@ -903,16 +904,12 @@ static void css_set_move_task(struct task_struct *task, { lockdep_assert_held(&css_set_lock); - if (to_cset && !css_set_populated(to_cset)) - css_set_update_populated(to_cset, true); - if (from_cset) { WARN_ON_ONCE(list_empty(&task->cg_list)); css_set_skip_task_iters(from_cset, task); list_del_init(&task->cg_list); - if (!css_set_populated(from_cset)) - css_set_update_populated(from_cset, false); + css_set_update_populated(from_cset, false); } else { WARN_ON_ONCE(!list_empty(&task->cg_list)); } @@ -926,6 +923,7 @@ static void css_set_move_task(struct task_struct *task, WARN_ON_ONCE(task->flags & PF_EXITING); cgroup_move_task(task, to_cset); + css_set_update_populated(to_cset, true); list_add_tail(&task->cg_list, use_mg_tasks ? &to_cset->mg_tasks : &to_cset->tasks); }