Re: [PATCH v6 04/24] x86/resctrl: Move rmid allocation out of mkdir_rdt_prepare()

James Morse
2023-10-05T17:16:28Z

Hi Babu,

On 04/10/2023 19:01, Moger, Babu wrote:
> On 9/14/23 12:21, James Morse wrote:
>> RMID are allocated for each monitor or control group directory, because
>> each of these needs its own RMID. For control groups,
>> rdtgroup_mkdir_ctrl_mon() later goes on to allocate the CLOSID.
>>
>> MPAM's equivalent of RMID is not an independent number, so can't be
>> allocated until the CLOSID is known. An RMID allocation for one CLOSID
>> may fail, whereas another may succeed depending on how many monitor
>> groups a control group has.
>>
>> The RMID allocation needs to move to be after the CLOSID has been
>> allocated.
>>
>> Move the RMID allocation out of mkdir_rdt_prepare() to occur in its caller,
>> after the mkdir_rdt_prepare() call. This allows the RMID allocator to
>> know the CLOSID.

>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 7a7369a323b5..d25cb8c9a20e 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -3189,6 +3189,12 @@ static int mkdir_rdt_prepare_rmid_alloc(struct rdtgroup *rdtgrp)
>> return 0;
>> }
>>
>> +static void mkdir_rdt_prepare_rmid_free(struct rdtgroup *rgrp)
>> +{
>> + if (rdt_mon_capable)
>> + free_rmid(rgrp->mon.rmid);
>> +}
>> +
>> static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
>> const char *name, umode_t mode,
>> enum rdt_group_type rtype, struct rdtgroup **r)
>> @@ -3254,12 +3260,6 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
>> goto out_destroy;
>> }
>>
>> - ret = mkdir_rdt_prepare_rmid_alloc(rdtgrp);
>> - if (ret)
>> - goto out_destroy;
>> -
>> - kernfs_activate(kn);
>
> You should not remove "kernfs_activate(kn); from here (only the last line).
>
> kernfs_create_dir is called in this function.
>
> /* kernfs creates the directory for rdtgrp */
> kn = kernfs_create_dir(parent_kn, name, mode, rdtgrp);
>
>
> There should be matching kernfs_activate.

I think your point is kernfs_activate() should have been called by the time
mkdir_rdt_prepare() returns because it creates other directories. I don't think this
matters because kernfs_activate() is a tree operation. Sure, the control/monitor group
directory isn't visible once mkdir_rdt_prepare() returns, but by the time either of its
two callers return, changes to the directory tree have been activated.

Moving these lines is the to ensure user-space doesn't see the control/monitor group as
existing without the mon_data directory that is created by mkdir_rdt_prepare_rmid_alloc().

>> -
>> /*
>> * The caller unlocks the parent_kn upon success.
>> */
>> @@ -3278,7 +3278,6 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
>> static void mkdir_rdt_prepare_clean(struct rdtgroup *rgrp)
>> {
>> kernfs_remove(rgrp->kn);
>> - free_rmid(rgrp->mon.rmid);
>> rdtgroup_remove(rgrp);
>> }
>>
>> @@ -3300,12 +3299,21 @@ static int rdtgroup_mkdir_mon(struct kernfs_node *parent_kn,
>> prgrp = rdtgrp->mon.parent;
>> rdtgrp->closid = prgrp->closid;
>>
>> + ret = mkdir_rdt_prepare_rmid_alloc(rdtgrp);
>> + if (ret) {
>> + mkdir_rdt_prepare_clean(rdtgrp);
>> + goto out_unlock;
>> + }
>> +
>> + kernfs_activate(rdtgrp->kn);
>
> I dont see the need for this. There is kernfs_activate inside
> mkdir_rdt_prepare_rmid_alloc (mkdir_rdt_prepare_rmid_alloc
> ->mkdir_mondata_all) for all the files created.

> Also mkdir_rdt_prepare already has kernfs_activate for the files it created.

It does, and this makes the mon_data directory visible in the parent control/monitor group
- but that control/monitor group isn't visible until this kernfs_activate(rdtgrp->kn)
makes it visible. The scope of these tree operations is different.

Looking at this again, there is an existing problem with the mon_groups directory not
being visible until after the control/monitor group is visible, worse is that if the
mon_group directory creation fails, the control/monitor group is removed. Chances are
no-one is depending on this.

I do think ultimately these kernfs_activate() calls should be moved to the end of the
syscall helpers that change the directory structure. This would stop things being briefly
visible.

Thanks!

James