diff mbox series

[03/18] x86/resctrl: Create helper for RMID allocation and mondata dir creation

Message ID 20221021131204.5581-4-james.morse@arm.com
State New
Headers
Series x86/resctrl: monitored closid+rmid together, separate arch/fs locking |

Commit Message

James Morse Oct. 21, 2022, 1:11 p.m. UTC 
  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.

To make a subsequent change that does this easier to read, move the RMID
allocation and mondata dir creation to a helper.

Signed-off-by: James Morse <james.morse@arm.com>
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 42 +++++++++++++++++---------
 1 file changed, 27 insertions(+), 15 deletions(-)

Comments

Fenghua Yu Jan. 6, 2023, 3:23 a.m. UTC | #1 
Hi, James,

> James Morse <james.morse@arm.com> writes:
> 
> 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.
> 
> To make a subsequent change that does this easier to read, move the RMID
> allocation and mondata dir creation to a helper.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 42 +++++++++++++++++---------
>  1 file changed, 27 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 9ce4746778f4..841294ad6263 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2868,6 +2868,30 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp)
>  	return 0;
>  }
> 
> +static int mkdir_rdt_prepare_rmid_alloc(struct rdtgroup *rdtgrp) {
> +	int ret;
> +
> +	if (!rdt_mon_capable)
> +		return 0;
> +
> +	ret = alloc_rmid();
> +	if (ret < 0) {
> +		rdt_last_cmd_puts("Out of RMIDs\n");
> +		return ret;
> +	}
> +	rdtgrp->mon.rmid = ret;
> +
> +	ret = mkdir_mondata_all(rdtgrp->kn, rdtgrp, &rdtgrp-
> >mon.mon_data_kn);
> +	if (ret) {
> +		rdt_last_cmd_puts("kernfs subdir error\n");
> +		free_rmid(rdtgrp->closid, rdtgrp->mon.rmid);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
>  			     const char *name, umode_t mode,
>  			     enum rdt_group_type rtype, struct rdtgroup **r)
> @@ -2933,20 +2957,10 @@ static int mkdir_rdt_prepare(struct kernfs_node
> *parent_kn,
>  		goto out_destroy;
>  	}
> 
> -	if (rdt_mon_capable) {
> -		ret = alloc_rmid();
> -		if (ret < 0) {
> -			rdt_last_cmd_puts("Out of RMIDs\n");
> -			goto out_destroy;
> -		}
> -		rdtgrp->mon.rmid = ret;
> +	ret = mkdir_rdt_prepare_rmid_alloc(rdtgrp);
> +	if (ret)
> +		goto out_destroy;
> 
> -		ret = mkdir_mondata_all(kn, rdtgrp, &rdtgrp-
> >mon.mon_data_kn);
> -		if (ret) {
> -			rdt_last_cmd_puts("kernfs subdir error\n");
> -			goto out_idfree;
> -		}
> -	}
>  	kernfs_activate(kn);
> 
>  	/*
> @@ -2954,8 +2968,6 @@ static int mkdir_rdt_prepare(struct kernfs_node
> *parent_kn,
>  	 */
>  	return 0;
> 
> -out_idfree:
> -	free_rmid(rdtgrp->closid, rdtgrp->mon.rmid);
>  out_destroy:
>  	kernfs_put(rdtgrp->kn);
>  	kernfs_remove(rdtgrp->kn);

Why not free allocated rmid? Rmid leak without freed.

Thanks.

-Fenghua
James Morse Jan. 10, 2023, 5:57 p.m. UTC | #2 
Hi Fenghua,

On 06/01/2023 03:23, Yu, Fenghua wrote:
>> James Morse <james.morse@arm.com> writes:
>>
>> 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.
>>
>> To make a subsequent change that does this easier to read, move the RMID
>> allocation and mondata dir creation to a helper.

>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 9ce4746778f4..841294ad6263 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -2868,6 +2868,30 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp)
>>  	return 0;
>>  }
>>
>> +static int mkdir_rdt_prepare_rmid_alloc(struct rdtgroup *rdtgrp) {
>> +	int ret;
>> +
>> +	if (!rdt_mon_capable)
>> +		return 0;
>> +
>> +	ret = alloc_rmid();
>> +	if (ret < 0) {
>> +		rdt_last_cmd_puts("Out of RMIDs\n");
>> +		return ret;
>> +	}
>> +	rdtgrp->mon.rmid = ret;
>> +
>> +	ret = mkdir_mondata_all(rdtgrp->kn, rdtgrp, &rdtgrp-
>>> mon.mon_data_kn);
>> +	if (ret) {
>> +		rdt_last_cmd_puts("kernfs subdir error\n");


>> +		free_rmid(rdtgrp->closid, rdtgrp->mon.rmid);

                ^^^^^^^^^

>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
>>  			     const char *name, umode_t mode,
>>  			     enum rdt_group_type rtype, struct rdtgroup **r)
>> @@ -2933,20 +2957,10 @@ static int mkdir_rdt_prepare(struct kernfs_node
>> *parent_kn,
>>  		goto out_destroy;
>>  	}
>>
>> -	if (rdt_mon_capable) {
>> -		ret = alloc_rmid();
>> -		if (ret < 0) {
>> -			rdt_last_cmd_puts("Out of RMIDs\n");
>> -			goto out_destroy;
>> -		}
>> -		rdtgrp->mon.rmid = ret;
>> +	ret = mkdir_rdt_prepare_rmid_alloc(rdtgrp);
>> +	if (ret)
>> +		goto out_destroy;
>>
>> -		ret = mkdir_mondata_all(kn, rdtgrp, &rdtgrp-
>>> mon.mon_data_kn);
>> -		if (ret) {
>> -			rdt_last_cmd_puts("kernfs subdir error\n");
>> -			goto out_idfree;
>> -		}
>> -	}
>>  	kernfs_activate(kn);
>>
>>  	/*
>> @@ -2954,8 +2968,6 @@ static int mkdir_rdt_prepare(struct kernfs_node
>> *parent_kn,
>>  	 */
>>  	return 0;
>>
>> -out_idfree:
>> -	free_rmid(rdtgrp->closid, rdtgrp->mon.rmid);
>>  out_destroy:
>>  	kernfs_put(rdtgrp->kn);
>>  	kernfs_remove(rdtgrp->kn);

> Why not free allocated rmid? Rmid leak without freed.

It's just moved into the helper.

This free_rmid() is here because this function used to allocate the rmid and the mondata
as separate steps. Now a helper does that in one go - and cleans up after itself.

If mkdir_rdt_prepare_rmid_alloc() fails, it didn't allocate an rmid .. if it secretly did,
it free()s it itself, see the ^-lighted code above.


Thanks,

James
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 9ce4746778f4..841294ad6263 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2868,6 +2868,30 @@  static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp)
 	return 0;
 }
 
+static int mkdir_rdt_prepare_rmid_alloc(struct rdtgroup *rdtgrp)
+{
+	int ret;
+
+	if (!rdt_mon_capable)
+		return 0;
+
+	ret = alloc_rmid();
+	if (ret < 0) {
+		rdt_last_cmd_puts("Out of RMIDs\n");
+		return ret;
+	}
+	rdtgrp->mon.rmid = ret;
+
+	ret = mkdir_mondata_all(rdtgrp->kn, rdtgrp, &rdtgrp->mon.mon_data_kn);
+	if (ret) {
+		rdt_last_cmd_puts("kernfs subdir error\n");
+		free_rmid(rdtgrp->closid, rdtgrp->mon.rmid);
+		return ret;
+	}
+
+	return 0;
+}
+
 static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
 			     const char *name, umode_t mode,
 			     enum rdt_group_type rtype, struct rdtgroup **r)
@@ -2933,20 +2957,10 @@  static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
 		goto out_destroy;
 	}
 
-	if (rdt_mon_capable) {
-		ret = alloc_rmid();
-		if (ret < 0) {
-			rdt_last_cmd_puts("Out of RMIDs\n");
-			goto out_destroy;
-		}
-		rdtgrp->mon.rmid = ret;
+	ret = mkdir_rdt_prepare_rmid_alloc(rdtgrp);
+	if (ret)
+		goto out_destroy;
 
-		ret = mkdir_mondata_all(kn, rdtgrp, &rdtgrp->mon.mon_data_kn);
-		if (ret) {
-			rdt_last_cmd_puts("kernfs subdir error\n");
-			goto out_idfree;
-		}
-	}
 	kernfs_activate(kn);
 
 	/*
@@ -2954,8 +2968,6 @@  static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
 	 */
 	return 0;
 
-out_idfree:
-	free_rmid(rdtgrp->closid, rdtgrp->mon.rmid);
 out_destroy:
 	kernfs_put(rdtgrp->kn);
 	kernfs_remove(rdtgrp->kn);