[04/18] x86/resctrl: Move rmid allocation out of mkdir_rdt_prepare()
Commit Message
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 are 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.
Signed-off-by: James Morse <james.morse@arm.com>
---
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 29 +++++++++++++++++++-------
1 file changed, 22 insertions(+), 7 deletions(-)
Comments
Hi James,
> 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 are 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.
>
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 29
> +++++++++++++++++++-------
> 1 file changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 841294ad6263..c67083a8a5f5 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2892,6 +2892,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->closid, 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)
> @@ -2957,10 +2963,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);
>
> /*
> @@ -2981,7 +2983,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->closid, rgrp->mon.rmid);
> rdtgroup_remove(rgrp);
> }
>
> @@ -3003,12 +3004,19 @@ 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;
> + }
> +
> /*
> * Add the rdtgrp to the list of rdtgrps the parent
> * ctrl_mon group has to track.
> */
> list_add_tail(&rdtgrp->mon.crdtgrp_list, &prgrp->mon.crdtgrp_list);
>
> +out_unlock:
> rdtgroup_kn_unlock(parent_kn);
Is there any case that a lock is not unlocked here?
Best regards,
Shaopeng Tan
> return ret;
> }
> @@ -3039,10 +3047,15 @@ static int rdtgroup_mkdir_ctrl_mon(struct
> kernfs_node *parent_kn,
> ret = 0;
>
> rdtgrp->closid = closid;
> - ret = rdtgroup_init_alloc(rdtgrp);
> - if (ret < 0)
> +
> + ret = mkdir_rdt_prepare_rmid_alloc(rdtgrp);
> + if (ret)
> goto out_id_free;
>
> + ret = rdtgroup_init_alloc(rdtgrp);
> + if (ret < 0)
> + goto out_rmid_free;
> +
> list_add(&rdtgrp->rdtgroup_list, &rdt_all_groups);
>
> if (rdt_mon_capable) {
> @@ -3061,6 +3074,8 @@ static int rdtgroup_mkdir_ctrl_mon(struct
> kernfs_node *parent_kn,
>
> out_del_list:
> list_del(&rdtgrp->rdtgroup_list);
> +out_rmid_free:
> + mkdir_rdt_prepare_rmid_free(rdtgrp);
> out_id_free:
> closid_free(closid);
> out_common_fail:
> --
> 2.30.2
Hello!
On 10/11/2022 10:50, Shaopeng Tan (Fujitsu) 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 are 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 841294ad6263..c67083a8a5f5 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -3003,12 +3004,19 @@ 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;
>> + }
>> +
>> /*
>> * Add the rdtgrp to the list of rdtgrps the parent
>> * ctrl_mon group has to track.
>> */
>> list_add_tail(&rdtgrp->mon.crdtgrp_list, &prgrp->mon.crdtgrp_list);
>>
>> +out_unlock:
>> rdtgroup_kn_unlock(parent_kn);
> Is there any case that a lock is not unlocked here?
If you've found an example - please share!
The unlock is necessary because mkdir_rdt_prepare() calls rdtgroup_kn_lock_live(),
but expects its caller to do the unlock.
This patch allows the rmid allocation to fail separately to mkdir_rdt_prepare(), so
rdtgroup_mkdir_mon() needs to ensure the unlock() happens when it returns an error, this
work was previously done by 'out_unlock' in mkdir_rdt_prepare().
I hope this helps,
Thanks,
James
@@ -2892,6 +2892,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->closid, 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)
@@ -2957,10 +2963,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);
/*
@@ -2981,7 +2983,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->closid, rgrp->mon.rmid);
rdtgroup_remove(rgrp);
}
@@ -3003,12 +3004,19 @@ 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;
+ }
+
/*
* Add the rdtgrp to the list of rdtgrps the parent
* ctrl_mon group has to track.
*/
list_add_tail(&rdtgrp->mon.crdtgrp_list, &prgrp->mon.crdtgrp_list);
+out_unlock:
rdtgroup_kn_unlock(parent_kn);
return ret;
}
@@ -3039,10 +3047,15 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn,
ret = 0;
rdtgrp->closid = closid;
- ret = rdtgroup_init_alloc(rdtgrp);
- if (ret < 0)
+
+ ret = mkdir_rdt_prepare_rmid_alloc(rdtgrp);
+ if (ret)
goto out_id_free;
+ ret = rdtgroup_init_alloc(rdtgrp);
+ if (ret < 0)
+ goto out_rmid_free;
+
list_add(&rdtgrp->rdtgroup_list, &rdt_all_groups);
if (rdt_mon_capable) {
@@ -3061,6 +3074,8 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn,
out_del_list:
list_del(&rdtgrp->rdtgroup_list);
+out_rmid_free:
+ mkdir_rdt_prepare_rmid_free(rdtgrp);
out_id_free:
closid_free(closid);
out_common_fail: