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;
Message-ID: <8f68ace7-e05b-ad6d-fa74-5ff8e179aec9@intel.com>
Date:   Fri, 14 Jul 2023 14:54:17 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.13.0
Subject: Re: [PATCH v5 7/8] x86/resctrl: Move default control group creation
 during mount
Content-Language: en-US
To:     <babu.moger@amd.com>, <corbet@lwn.net>, <tglx@linutronix.de>,
        <mingo@redhat.com>, <bp@alien8.de>
CC:     <fenghua.yu@intel.com>, <dave.hansen@linux.intel.com>,
        <x86@kernel.org>, <hpa@zytor.com>, <paulmck@kernel.org>,
        <akpm@linux-foundation.org>, <quic_neeraju@quicinc.com>,
        <rdunlap@infradead.org>, <damien.lemoal@opensource.wdc.com>,
        <songmuchun@bytedance.com>, <peterz@infradead.org>,
        <jpoimboe@kernel.org>, <pbonzini@redhat.com>,
        <chang.seok.bae@intel.com>, <pawan.kumar.gupta@linux.intel.com>,
        <jmattson@google.com>, <daniel.sneddon@linux.intel.com>,
        <sandipan.das@amd.com>, <tony.luck@intel.com>,
        <james.morse@arm.com>, <linux-doc@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <bagasdotme@gmail.com>,
        <eranian@google.com>, <christophe.leroy@csgroup.eu>,
        <jarkko@kernel.org>, <adrian.hunter@intel.com>,
        <quic_jiles@quicinc.com>, <peternewman@google.com>
References: <168564586603.527584.10518315376465080920.stgit@bmoger-ubuntu>
 <168564612103.527584.4866621411469438225.stgit@bmoger-ubuntu>
 <9cb1a07b-0e17-d930-263e-5433952cf241@intel.com>
 <c10643f9-ac6b-7912-1fe1-c9446c79b931@amd.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <c10643f9-ac6b-7912-1fe1-c9446c79b931@amd.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?YkRwcysvNDVrdkp3MG5kWWU0N1RvN05RYXJyTDkyZ1RPTlAxSXJ1TStGSjBo?=
 =?utf-8?B?QXBFRXBIdW80RjNUdFRqZkk2bGtWUllPeTRMVmd4UysvN3c5MWIwblBqTS9G?=
 =?utf-8?B?dXZ0Z1d4THgvMkt6anNibW1YZlYva0k1SVhrUjEzN3FMeDZlUFlpL3pNNzc3?=
 =?utf-8?B?Q0pEWE5QVTl5UktuZ1FwVmtVMGxrTDl5Z2dmK25QWDJ1dU9WbzZYSHdLOUZt?=
 =?utf-8?B?bkRnUDV4RnJYRkJJaU4yQTcrdjBnOFRZYnNHSG5xNFlUVDk5VUpiT3ZxdnJk?=
 =?utf-8?B?bll6aVZ5eThIRENPV2VvM2hNUWJKZnl6dXlyL3k1d1Z3d1c2U0xvbDIrMWE2?=
 =?utf-8?B?cEYvL0MzQ01hdGgwbm9QVnU4OVdyQzFyREVsMkhFaDE1OENHMkpma3ZCUldz?=
 =?utf-8?B?Z0VZUjIvRlUvalB3aVZzZlJHNHNTQ25wckNxdEpwTXBuekUrOVZ0anFURzlJ?=
 =?utf-8?B?TnU0UlpUR0U5L3Q5SHFheklhb1VIeUVRY3c2cnBLZTJvcjNST2hXV1N3QUdk?=
 =?utf-8?B?U25QSnJ3ZmZXVUdSQUUrcTlPamNTdlNYK1NQbVZUQnEzSzM4bmFPSElzZFlG?=
 =?utf-8?B?WVZncnVHSWFCd09SYkowNzdQQjU4UUhvQ3l0VzBQR3RHRUFsVE4vaUducVZ0?=
 =?utf-8?B?bDVuVkg2TjFSaEcvR2tucmFOYWtYM0lFOFRaNmYrbEMrdExaT1dpbUN0Z0VQ?=
 =?utf-8?B?YndCVkJvci9GMWFMcFN0RkhwS3ZkNHFOb1R6QVdQc2hFb2J4c3lqeVpEOEhy?=
 =?utf-8?B?Qlo5OTBYRGI5MDNPVldlaForRlRnZW0xWTMwU0JBUkVlR2ZCb1NxbXVXQmZU?=
 =?utf-8?B?Y0U1c3g3cUp4b2VTK2thbmxsc2FwcnlRbGI3aHNLNUZHZUR5NmgzT0UvaUtu?=
 =?utf-8?B?WXlRQlB5MG9ZbitaMTByRWM1dXZxdTYyNGlSZ1EwN1ZkcWRHTUQwUWR5ZlBF?=
 =?utf-8?B?ZWlZV09UOWFBYzQxa2hZMS9VZ2dPMDlqcW9NK2drZ3dzajRyd0ZHQldKTCtu?=
 =?utf-8?B?V1BLMSsxQzFyT2l0T0JENHhLY0VWVFdYS09MYS9KYTRodVJlc1hwVFB0SnpW?=
 =?utf-8?B?SDc2bXpnQzVERkp0VTBkekdabFFveC9JRDB0QzVKcUhwdHQzcWtxNUtlRWhH?=
 =?utf-8?B?T1Bjd2ZMYk1rYlplRkJmRVQ5cGdzd2hvakZ3SEhNYXk1SG96ZzVmeU1aWWxH?=
 =?utf-8?B?aHcrajBORnp1M21KVCtBZ1ZqZjJnMlduSlE5RnRremJXQzBYbHhXdmNwbVRR?=
 =?utf-8?B?bks2aWN0bUlrQ05YUU1BV0JrL2N5ejJrVFN6anNIVjNHbGhBeU5FSnVUU0Y3?=
 =?utf-8?B?RElwcHlEbFpYUlVXZndJWFVJd1JEaS9VYzN0UmJEOVFCZTFLTlZNSUhLbWgx?=
 =?utf-8?B?TTJCSzhLdkdGQTN1dzd3N3FnL3BVR0FyZWRuMlBJdERUbUxmQWRxZDVWS01J?=
 =?utf-8?B?SG8vdlY5Y3ZtWWxYanZkT29mOFNyMWJvRythWk1lNGFkcFliaTJMR2x3bjA2?=
 =?utf-8?B?Zk1vTEdzam1CaXYwanFkajMwa1BWU2owcHJNWTlxQmhGNksvQ0UxcTNDM3ZN?=
 =?utf-8?B?M0pkaXQyQVpaTFAwUGNrTFllNXhZUi9DSExRbTdqVnloV3IzRUFMZ3NMUkdC?=
 =?utf-8?B?R3FDZU14aUIrMU84ODRsMk0zS1pIeDFNd1NQek1rajFXVWNlMHVBT0hCTHVM?=
 =?utf-8?B?SGtyaEZxSitPUnhyYjd0R1N3VTQvTnVkMStNMlJKRS80eXk3OVVwRG1XTUxp?=
 =?utf-8?B?aXhPYnZxNVoyRXVnNExtMXpwWXhWTkdiMUh5Szh4eUdORm1FOGhZMERLcnFx?=
 =?utf-8?B?cmREamRNTk5rbTByMDVlWHFIOE1DZHZJeUJrOUE3R0tjUGMzOXVUYXUrNWJP?=
 =?utf-8?B?bUxUVUUvaGlqV1RXeG5BemVVVzVlM1BieVFQLzltRWV6YmZ2b3NTMFJxbHcw?=
 =?utf-8?B?bVNtTENYWWZDcXMzRC85NGJLQ2JDQklhWGtENzVJOTdzQVFUcTlxeWFzRmJl?=
 =?utf-8?B?WXViZUV5T0drd1k3dmYzeCt2STVPSmtLZGQ0RktvTTlWMXFtbENZdXRCUEZm?=
 =?utf-8?B?dXduMThzcit4NjhsQkFRSUE2QktHdVhUeGRwbHRxdmVIUVpNNElRcllOeFN4?=
 =?utf-8?B?UFB3STVkZVVUNCtsZ3NITHhHWXR4aUpad1hmeG9KQjlaTUtlckFYNTd3SUo0?=
 =?utf-8?B?UWc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 19b1017d-e5f0-43f0-d7f0-08db84b4e142
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jul 2023 21:54:21.2842
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 4M6KS4Rj+Nh68HQGGgE0WSTAf5ou9LtNPY///hO9nDtD1dB49lteHuFUKkMXkDEGfInbcjAMJBhHx0bbnL6tudBZ4HrO/ObIwROcVbs/vK0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR11MB8404
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,
        URIBL_BLOCKED autolearn=ham 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

Hi Babu,

On 7/14/2023 9:26 AM, Moger, Babu wrote:
> Hi Reinette,
> Sorry.. Took a while to respond. I had to recreate the issue to refresh my
> memory.

No problem!

> On 7/7/23 16:46, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 6/1/2023 12:02 PM, Babu Moger wrote:


>>>  	ctx = kzalloc(sizeof(struct rdt_fs_context), GFP_KERNEL);
>>> -	if (!ctx)
>>> +	if (!ctx) {
>>> +		kernfs_destroy_root(rdt_root);
>>>  		return -ENOMEM;
>>> +	}
>>>  
>>>  	ctx->kfc.root = rdt_root;
>>>  	ctx->kfc.magic = RDTGROUP_SUPER_MAGIC;
>>> @@ -2845,6 +2860,9 @@ static void rdt_kill_sb(struct super_block *sb)
>>>  	static_branch_disable_cpuslocked(&rdt_alloc_enable_key);
>>>  	static_branch_disable_cpuslocked(&rdt_mon_enable_key);
>>>  	static_branch_disable_cpuslocked(&rdt_enable_key);
>>> +	/* Remove the default group and cleanup the root */
>>> +	list_del(&rdtgroup_default.rdtgroup_list);
>>> +	kernfs_destroy_root(rdt_root);
>>
>> Why not just add kernfs_remove(rdtgroup_default.kn) to rmdir_all_sub()?
> 
> List rdtgroup_default.rdtgroup_list is added during the mount and had to
> be removed during umount and rdt_root is destroyed here.

I do not think it is required for default resource group management to
be tied with the resctrl files associated with default resource group.

I think rdtgroup_setup_root can be split in two, one for all the
resctrl files that should be done at mount/unmount and one for the
default group init done at __init.

>>>  	kernfs_kill_sb(sb);
>>>  	mutex_unlock(&rdtgroup_mutex);
>>>  	cpus_read_unlock();
>>> @@ -3598,10 +3616,8 @@ static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
>>>  	.show_options	= rdtgroup_show_options,
>>>  };
>>>  
>>> -static int __init rdtgroup_setup_root(void)
>>> +static int rdtgroup_setup_root(void)
>>>  {
>>> -	int ret;
>>> -
>>>  	rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
>>>  				      KERNFS_ROOT_CREATE_DEACTIVATED |
>>>  				      KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
>>> @@ -3618,19 +3634,11 @@ static int __init rdtgroup_setup_root(void)
>>>  
>>>  	list_add(&rdtgroup_default.rdtgroup_list, &rdt_all_groups);
>>>  
>>> -	ret = rdtgroup_add_files(kernfs_root_to_node(rdt_root), RFTYPE_CTRL_BASE);
>>> -	if (ret) {
>>> -		kernfs_destroy_root(rdt_root);
>>> -		goto out;
>>> -	}
>>> -
>>>  	rdtgroup_default.kn = kernfs_root_to_node(rdt_root);
>>> -	kernfs_activate(rdtgroup_default.kn);
>>>  
>>> -out:
>>>  	mutex_unlock(&rdtgroup_mutex);
>>>  
>>> -	return ret;
>>> +	return 0;
>>>  }
>>>  
>>>  static void domain_destroy_mon_state(struct rdt_domain *d)
>>> @@ -3752,13 +3760,9 @@ int __init rdtgroup_init(void)
>>>  	seq_buf_init(&last_cmd_status, last_cmd_status_buf,
>>>  		     sizeof(last_cmd_status_buf));
>>>  
>>> -	ret = rdtgroup_setup_root();
>>> -	if (ret)
>>> -		return ret;
>>> -
>>>  	ret = sysfs_create_mount_point(fs_kobj, "resctrl");
>>>  	if (ret)
>>> -		goto cleanup_root;
>>> +		return ret;
>>>  
>>
>> It is not clear to me why this change is required, could you
>> please elaborate? It seems that all that is needed is for 
>> rdtgroup_add_files() to move to rdt_get_tree() (which you have done)
>> and then an additional call to kernfs_remove() in rmdir_all_sub().
>> I must be missing something, could you please help me understand?
>>
> 
> Yes. I started with that approach. But there are issues with that approach.
> 
> Currently, rdt_root(which is rdtgroup_default.kn) is created during
> rdtgroup_init. At the same time the root files are created. Also, default
> group is added to rdt_all_groups. Basically, the root files and
> rdtgroup_default group is always there even though filesystem is never
> mounted. Also mbm_over and cqm_limbo workqueues are always running even
> though filesystem is not mounted.
> 
> I changed rdtgroup_add_files() to move to rdt_get_tree() and added
> kernfs_remove() in rmdir_all_sub(). This caused problems. The
> kernfs_remove(rdtgroup_default.kn) removes all the reference counts and
> releases the root. When we mount again, we hit this this problem below.
> 
> [  404.558461] ------------[ cut here ]------------
> [  404.563631] WARNING: CPU: 35 PID: 7728 at fs/kernfs/dir.c:522
> kernfs_new_node+0x63/0x70
> 
> 404.778793]  ? __warn+0x81/0x140
> [  404.782535]  ? kernfs_new_node+0x63/0x70
> [  404.787036]  ? report_bug+0x102/0x200
> [  404.791247]  ? handle_bug+0x3f/0x70
> [  404.795269]  ? exc_invalid_op+0x13/0x60
> [  404.799671]  ? asm_exc_invalid_op+0x16/0x20
> [  404.804461]  ? kernfs_new_node+0x63/0x70
> [  404.808954]  ? snprintf+0x49/0x70
> [  404.812762]  __kernfs_create_file+0x30/0xc0
> [  404.817534]  rdtgroup_add_files+0x6c/0x100
> 
> Basically kernel says your rdt_root is not initialized. That is the reason
> I had to move everything to mount time. The rdt_root is created and
> initialized during the mount and also destroyed during the umount.
> And I had to move rdt_enable_key check during rdt_root creation.
> 

ok, thank you for the additional details. I see now how this patch evolved.
I understand how rdt_root needs to be created/destroyed
during mount/unmount. If I understand correctly the changes to
rdt_init_fs_context() was motivated by this line:

	ctx->kfc.root = rdt_root;

... that prompted you to move rdt_root creation there in order to have
it present for this assignment and that prompted the
rdt_enable_key check to follow. Is this correct?

I am concerned about the changes to rdt_init_fs_context() since it further
separates the resctrl file management, it breaks the symmetry of the
key checked and set, and finally these new actions seem unrelated to a function
named "init_fs_context". I looked at other examples and from what I can tell
it is not required that ctx->kfc.root be initialized within
rdt_init_fs_context(). Looks like the value is required by kernfs_get_tree()
that is called from rdt_get_tree(). For comparison I found cgroup_do_get_tree().
Note how cgroup_do_get_tree(), within the .get_tree callback,
initializes kernfs_fs_context.root and then call kernfs_get_tree()? 

It thus looks to me as though things can be simplified significantly
if the kernfs_fs_context.root assignment is moved from rdt_init_fs_context()
to rdt_get_tree(). rdt_get_tree() can then create rdt_root (and add all needed
files), assign it to kernfs_fs_context.root and call kernfs_get_tree().

What do you think?

Reinette