Re: Re: [PATCH v10 12/14] x86/sgx: Turn on per-cgroup EPC reclamation

From: "Haitao Huang" <haitao.huang@linux.intel.com>
To: "Michal Koutný" <mkoutny@suse.com>,
	"Haitao Huang" <haitao.huang@linux.intel.com>
Cc: "hpa@zytor.com" <hpa@zytor.com>,
	"tim.c.chen@linux.intel.com" <tim.c.chen@linux.intel.com>,
	"linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
	"x86@kernel.org" <x86@kernel.org>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	"jarkko@kernel.org" <jarkko@kernel.org>,
	"cgroups@vger.kernel.org" <cgroups@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"Mehta, Sohil" <sohil.mehta@intel.com>,
	"tj@kernel.org" <tj@kernel.org>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"bp@alien8.de" <bp@alien8.de>, "Huang, Kai" <kai.huang@intel.com>,
	"mikko.ylinen@linux.intel.com" <mikko.ylinen@linux.intel.com>,
	"seanjc@google.com" <seanjc@google.com>,
	"anakrish@microsoft.com" <anakrish@microsoft.com>,
	"Zhang, Bo" <zhanb@microsoft.com>,
	"kristen@linux.intel.com" <kristen@linux.intel.com>,
	"yangjie@microsoft.com" <yangjie@microsoft.com>,
	"Li, Zhiquan1" <zhiquan1.li@intel.com>,
	"chrisyan@microsoft.com" <chrisyan@microsoft.com>
Subject: Re: Re: [PATCH v10 12/14] x86/sgx: Turn on per-cgroup EPC reclamation
Date: Wed, 10 Apr 2024 13:28:06 -0500	[thread overview]
Message-ID: <op.2l0yw4vnwjvjmi@hhuan26-mobl.amr.corp.intel.com> (raw)
In-Reply-To: <op.2lyv641jwjvjmi@hhuan26-mobl.amr.corp.intel.com>

On Tue, 09 Apr 2024 10:34:06 -0500, Haitao Huang  
<haitao.huang@linux.intel.com> wrote:

> On Tue, 09 Apr 2024 04:03:22 -0500, Michal Koutný <mkoutny@suse.com>  
> wrote:
>
>> On Mon, Apr 08, 2024 at 11:23:21PM -0500, Haitao Huang  
>> <haitao.huang@linux.intel.com> wrote:
>>> It's always non-NULL based on testing.
>>>
>>> It's hard for me to say definitely by reading the code. But IIUC
>>> cgroup_disable command-line only blocks operations in /sys/fs/cgroup  
>>> so user
>>> space can't set up controllers and config limits, etc., for the  
>>> diasabled
>>> ones. Each task->cgroups would still have a non-NULL pointer to the  
>>> static
>>> root object for each cgroup that is enabled by KConfig, so
>>> get_current_misc_cg() thus  sgx_get_current_cg() should not return NULL
>>> regardless 'cgroup_disable=misc'.
>>>
>>> Maybe @Michal or @tj can confirm?
>>
>> The current implementation creates root css object (see cgroup_init(),
>> cgroup_ssid_enabled() check is after cgroup_init_subsys()).
>> I.e. it will look like all tasks are members of root cgroup wrt given
>> controller permanently and controller attribute files won't exist.
>>
>> (It is up to the controller implementation to do further optimization
>> based on the boot-time disablement (e.g. see uses of
>> mem_cgroup_disabled()). Not sure if this is useful for misc controller.)
>>
>> As for the WARN_ON(1), taking example from memcg -- NULL is best
>> synonymous with root. It's a judgement call which of the values to store
>> and when to intepret it.
>>
>> HTH,
>> Michal
> Thanks for the info.
>
> The way I see it, misc does not have special handling like memcg so  
> every task at least belong to the root(default) group even if it's  
> disabled by command line parameter. So we would not get NULL from  
> get_current_misc_cg(). I think I'll keep the WARN_ON_ONCE for now as a  
> reminder in case misc do have custom support for disabling in future.
>
Actually I think it makes more sense just add some comments instead of  
WARN.
That's what I did in v11 now.

Thanks
Haitao