From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760632AbdEVRc1 (ORCPT ); Mon, 22 May 2017 13:32:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59652 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760192AbdEVRcX (ORCPT ); Mon, 22 May 2017 13:32:23 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com E31E530C426 Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=longman@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com E31E530C426 Subject: Re: [RFC PATCH v2 11/17] cgroup: Implement new thread mode semantics To: Tejun Heo References: <1494855256-12558-1-git-send-email-longman@redhat.com> <1494855256-12558-12-git-send-email-longman@redhat.com> <20170519202624.GA15279@wtj.duckdns.org> Cc: Li Zefan , Johannes Weiner , Peter Zijlstra , Ingo Molnar , cgroups@vger.kernel.org, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, linux-mm@kvack.org, kernel-team@fb.com, pjt@google.com, luto@amacapital.net, efault@gmx.de From: Waiman Long Organization: Red Hat Message-ID: <71ce8f1c-3d92-64f2-085d-8900b8576d25@redhat.com> Date: Mon, 22 May 2017 13:32:20 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Mon, 22 May 2017 17:32:23 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/22/2017 01:13 PM, Waiman Long wrote: > On 05/19/2017 04:26 PM, Tejun Heo wrote: >>> @@ -2982,22 +3010,48 @@ static int cgroup_enable_threaded(struct cgroup *cgrp) >>> LIST_HEAD(csets); >>> struct cgrp_cset_link *link; >>> struct css_set *cset, *cset_next; >>> + struct cgroup *child; >>> int ret; >>> + u16 ss_mask; >>> >>> lockdep_assert_held(&cgroup_mutex); >>> >>> /* noop if already threaded */ >>> - if (cgrp->proc_cgrp) >>> + if (cgroup_is_threaded(cgrp)) >>> return 0; >>> >>> - /* allow only if there are neither children or enabled controllers */ >>> - if (css_has_online_children(&cgrp->self) || cgrp->subtree_control) >>> + /* >>> + * Allow only if it is not the root and there are: >>> + * 1) no children, >>> + * 2) no non-threaded controllers are enabled, and >>> + * 3) no attached tasks. >>> + * >>> + * With no attached tasks, it is assumed that no css_sets will be >>> + * linked to the current cgroup. This may not be true if some dead >>> + * css_sets linger around due to task_struct leakage, for example. >>> + */ >> It doesn't look like the code is actually making this (incorrect) >> assumption. I suppose the comment is from before >> cgroup_is_populated() was added? > Yes, it is a bug. I should have checked the tasks_count instead of using > cgroup_is_populated. Thanks for catching that. Sorry, I would like to take it back. I think cgroup_is_populated() will be set if there is any task attached to the cgroup. So I think it is doing the right thing with regard to (3). Cheers, Longman