The functions parse_cbm() and parse_bw() require mode and CLOSID to
validate the Capacity Bit Mask (CBM). It is passed through struct
rdtgroup in rdt_parse_data.

This can be simplified by passing the mode and closid directly, instead of
through the rdtgroup struct. Doing so also facilitates calling parse_cbm()
to verify the CBM within the io_alloc feature, since io_alloc does not
have rdtgroup context.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
v7: Rephrase of changelog.

v6: Changed the subject line to fs/resctrl.

v5: Resolved conflicts due to recent resctrl FS/ARCH code restructure.

v4: New patch to call parse_cbm() directly to avoid code duplication.
---
 fs/resctrl/ctrlmondata.c | 29 +++++++++++++----------------
 fs/resctrl/internal.h    |  6 ++++++
 2 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
index e78828b0408a..5c16557fb7a8 100644
--- a/fs/resctrl/ctrlmondata.c
+++ b/fs/resctrl/ctrlmondata.c
@@ -23,11 +23,6 @@
 
 #include "internal.h"
 
-struct rdt_parse_data {
-	struct rdtgroup		*rdtgrp;
-	char			*buf;
-};
-
 typedef int (ctrlval_parser_t)(struct rdt_parse_data *data,
 			       struct resctrl_schema *s,
 			       struct rdt_ctrl_domain *d);
@@ -77,8 +72,8 @@ static int parse_bw(struct rdt_parse_data *data, struct resctrl_schema *s,
 		    struct rdt_ctrl_domain *d)
 {
 	struct resctrl_staged_config *cfg;
-	u32 closid = data->rdtgrp->closid;
 	struct rdt_resource *r = s->res;
+	u32 closid = data->closid;
 	u32 bw_val;
 
 	cfg = &d->staged_config[s->conf_type];
@@ -156,9 +151,10 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
 static int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
 		     struct rdt_ctrl_domain *d)
 {
-	struct rdtgroup *rdtgrp = data->rdtgrp;
+	enum rdtgrp_mode mode = data->mode;
 	struct resctrl_staged_config *cfg;
 	struct rdt_resource *r = s->res;
+	u32 closid = data->closid;
 	u32 cbm_val;
 
 	cfg = &d->staged_config[s->conf_type];
@@ -171,7 +167,7 @@ static int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
 	 * Cannot set up more than one pseudo-locked region in a cache
 	 * hierarchy.
 	 */
-	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP &&
+	if (mode == RDT_MODE_PSEUDO_LOCKSETUP &&
 	    rdtgroup_pseudo_locked_in_hierarchy(d)) {
 		rdt_last_cmd_puts("Pseudo-locked region in hierarchy\n");
 		return -EINVAL;
@@ -180,9 +176,9 @@ static int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
 	if (!cbm_validate(data->buf, &cbm_val, r))
 		return -EINVAL;
 
-	if ((rdtgrp->mode == RDT_MODE_EXCLUSIVE ||
-	     rdtgrp->mode == RDT_MODE_SHAREABLE) &&
-	    rdtgroup_cbm_overlaps_pseudo_locked(d, cbm_val)) {
+	if ((mode == RDT_MODE_EXCLUSIVE ||
+	     mode == RDT_MODE_SHAREABLE) &&
+	     rdtgroup_cbm_overlaps_pseudo_locked(d, cbm_val)) {
 		rdt_last_cmd_puts("CBM overlaps with pseudo-locked region\n");
 		return -EINVAL;
 	}
@@ -191,14 +187,14 @@ static int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
 	 * The CBM may not overlap with the CBM of another closid if
 	 * either is exclusive.
 	 */
-	if (rdtgroup_cbm_overlaps(s, d, cbm_val, rdtgrp->closid, true)) {
+	if (rdtgroup_cbm_overlaps(s, d, cbm_val, closid, true)) {
 		rdt_last_cmd_puts("Overlaps with exclusive group\n");
 		return -EINVAL;
 	}
 
-	if (rdtgroup_cbm_overlaps(s, d, cbm_val, rdtgrp->closid, false)) {
-		if (rdtgrp->mode == RDT_MODE_EXCLUSIVE ||
-		    rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
+	if (rdtgroup_cbm_overlaps(s, d, cbm_val, closid, false)) {
+		if (mode == RDT_MODE_EXCLUSIVE ||
+		    mode == RDT_MODE_PSEUDO_LOCKSETUP) {
 			rdt_last_cmd_puts("Overlaps with other group\n");
 			return -EINVAL;
 		}
@@ -262,7 +258,8 @@ static int parse_line(char *line, struct resctrl_schema *s,
 	list_for_each_entry(d, &r->ctrl_domains, hdr.list) {
 		if (d->hdr.id == dom_id) {
 			data.buf = dom;
-			data.rdtgrp = rdtgrp;
+			data.closid = rdtgrp->closid;
+			data.mode = rdtgrp->mode;
 			if (parse_ctrlval(&data, s, d))
 				return -EINVAL;
 			if (rdtgrp->mode ==  RDT_MODE_PSEUDO_LOCKSETUP) {
diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
index 3d4a3d7696d0..ab76a1e2e679 100644
--- a/fs/resctrl/internal.h
+++ b/fs/resctrl/internal.h
@@ -201,6 +201,12 @@ struct rdtgroup {
 	struct pseudo_lock_region	*plr;
 };
 
+struct rdt_parse_data {
+	u32			closid;
+	enum rdtgrp_mode	mode;
+	char			*buf;
+};
+
 /* rdtgroup.flags */
 #define	RDT_DELETED		1
 
-- 
2.34.1

Hi Babu,

On 7/10/25 10:16 AM, Babu Moger wrote:
> The functions parse_cbm() and parse_bw() require mode and CLOSID to

No need to say "function" when using (). Also, drop parse_bw(), since it
does not validate CBMs.

> validate the Capacity Bit Mask (CBM). It is passed through struct

"passed through" -> "passed via"?

> rdtgroup in rdt_parse_data.

"rdt_parse_data" -> "struct rdt_parse_data"

> 
> This can be simplified by passing the mode and closid directly, instead of

closid -> CLOSID

> through the rdtgroup struct. Doing so also facilitates calling parse_cbm()
> to verify the CBM within the io_alloc feature, since io_alloc does not
> have rdtgroup context.

Above notes that "simplification" is the primary motivation but I do not think
this change qualifies as a "simplification". How about second paragraph changed
to something like:

	The io_alloc feature also uses CBMs to indicate which portions of
	cache are allocated for I/O traffic. The CBMs are provided by
	user space and need to be validated the same as CBMs provided for
	general (CPU) cache allocation. parse_cbm() cannot be used as-is
	since io_alloc does not have rdtgroup context.

	Pass the mode and CLOSID directly to parse_cbm() via struct rdt_parse_data
	instead of through the rdtgroup struct to facilitate calling parse_cbm() to
	verify the CBM of the io_alloc feature.

(please feel free to improve)

> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---



> ---
>  fs/resctrl/ctrlmondata.c | 29 +++++++++++++----------------
>  fs/resctrl/internal.h    |  6 ++++++
>  2 files changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
> index e78828b0408a..5c16557fb7a8 100644
> --- a/fs/resctrl/ctrlmondata.c
> +++ b/fs/resctrl/ctrlmondata.c
> @@ -23,11 +23,6 @@
>  
>  #include "internal.h"
>  
> -struct rdt_parse_data {
> -	struct rdtgroup		*rdtgrp;
> -	char			*buf;
> -};
> -

This patch is only about replacing rdtgroup with mode and CLOSID, there is no
motivation for relocating the structure declaration. This looks to be a change
needed by following patch but is another change that becomes unnecessary if
the io_alloc code, specifically resctrl_io_alloc_cbm_write() and
resctrl_io_alloc_parse_line() from next patch, are moved to ctrlmondata.c.

...

> @@ -171,7 +167,7 @@ static int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
>  	 * Cannot set up more than one pseudo-locked region in a cache
>  	 * hierarchy.
>  	 */
> -	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP &&
> +	if (mode == RDT_MODE_PSEUDO_LOCKSETUP &&
>  	    rdtgroup_pseudo_locked_in_hierarchy(d)) {
>  		rdt_last_cmd_puts("Pseudo-locked region in hierarchy\n");
>  		return -EINVAL;
> @@ -180,9 +176,9 @@ static int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
>  	if (!cbm_validate(data->buf, &cbm_val, r))
>  		return -EINVAL;
>  
> -	if ((rdtgrp->mode == RDT_MODE_EXCLUSIVE ||
> -	     rdtgrp->mode == RDT_MODE_SHAREABLE) &&
> -	    rdtgroup_cbm_overlaps_pseudo_locked(d, cbm_val)) {
> +	if ((mode == RDT_MODE_EXCLUSIVE ||
> +	     mode == RDT_MODE_SHAREABLE) &&
> +	     rdtgroup_cbm_overlaps_pseudo_locked(d, cbm_val)) {

Please fix alignment.

>  		rdt_last_cmd_puts("CBM overlaps with pseudo-locked region\n");
>  		return -EINVAL;
>  	}

Reinette

Hi Reinette,

On 7/21/25 22:30, Reinette Chatre wrote:
> Hi Babu,
> 
> On 7/10/25 10:16 AM, Babu Moger wrote:
>> The functions parse_cbm() and parse_bw() require mode and CLOSID to
> 
> No need to say "function" when using (). Also, drop parse_bw(), since it
> does not validate CBMs.

Sure.
> 
>> validate the Capacity Bit Mask (CBM). It is passed through struct
> 
> "passed through" -> "passed via"?
> 

Sure.

>> rdtgroup in rdt_parse_data.
> 
> "rdt_parse_data" -> "struct rdt_parse_data"
> 

Sure.

>>
>> This can be simplified by passing the mode and closid directly, instead of
> 
> closid -> CLOSID
> 
Sure,

>> through the rdtgroup struct. Doing so also facilitates calling parse_cbm()
>> to verify the CBM within the io_alloc feature, since io_alloc does not
>> have rdtgroup context.
> 
> Above notes that "simplification" is the primary motivation but I do not think
> this change qualifies as a "simplification". How about second paragraph changed
> to something like:
> 
> 	The io_alloc feature also uses CBMs to indicate which portions of
> 	cache are allocated for I/O traffic. The CBMs are provided by
> 	user space and need to be validated the same as CBMs provided for
> 	general (CPU) cache allocation. parse_cbm() cannot be used as-is
> 	since io_alloc does not have rdtgroup context.
> 
> 	Pass the mode and CLOSID directly to parse_cbm() via struct rdt_parse_data
> 	instead of through the rdtgroup struct to facilitate calling parse_cbm() to
> 	verify the CBM of the io_alloc feature.
> 
> (please feel free to improve)
> 

Looks good.

>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
> 
> 
> 
>> ---
>>  fs/resctrl/ctrlmondata.c | 29 +++++++++++++----------------
>>  fs/resctrl/internal.h    |  6 ++++++
>>  2 files changed, 19 insertions(+), 16 deletions(-)
>>
>> diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
>> index e78828b0408a..5c16557fb7a8 100644
>> --- a/fs/resctrl/ctrlmondata.c
>> +++ b/fs/resctrl/ctrlmondata.c
>> @@ -23,11 +23,6 @@
>>  
>>  #include "internal.h"
>>  
>> -struct rdt_parse_data {
>> -	struct rdtgroup		*rdtgrp;
>> -	char			*buf;
>> -};
>> -
> 
> This patch is only about replacing rdtgroup with mode and CLOSID, there is no
> motivation for relocating the structure declaration. This looks to be a change
> needed by following patch but is another change that becomes unnecessary if
> the io_alloc code, specifically resctrl_io_alloc_cbm_write() and
> resctrl_io_alloc_parse_line() from next patch, are moved to ctrlmondata.c.
> 

Yes. Not required to move now.

> ...
> 
>> @@ -171,7 +167,7 @@ static int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
>>  	 * Cannot set up more than one pseudo-locked region in a cache
>>  	 * hierarchy.
>>  	 */
>> -	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP &&
>> +	if (mode == RDT_MODE_PSEUDO_LOCKSETUP &&
>>  	    rdtgroup_pseudo_locked_in_hierarchy(d)) {
>>  		rdt_last_cmd_puts("Pseudo-locked region in hierarchy\n");
>>  		return -EINVAL;
>> @@ -180,9 +176,9 @@ static int parse_cbm(struct rdt_parse_data *data, struct resctrl_schema *s,
>>  	if (!cbm_validate(data->buf, &cbm_val, r))
>>  		return -EINVAL;
>>  
>> -	if ((rdtgrp->mode == RDT_MODE_EXCLUSIVE ||
>> -	     rdtgrp->mode == RDT_MODE_SHAREABLE) &&
>> -	    rdtgroup_cbm_overlaps_pseudo_locked(d, cbm_val)) {
>> +	if ((mode == RDT_MODE_EXCLUSIVE ||
>> +	     mode == RDT_MODE_SHAREABLE) &&
>> +	     rdtgroup_cbm_overlaps_pseudo_locked(d, cbm_val)) {
> 
> Please fix alignment.

Sure.

> 
>>  		rdt_last_cmd_puts("CBM overlaps with pseudo-locked region\n");
>>  		return -EINVAL;
>>  	}
> 
> Reinette
> 

-- 
Thanks
Babu Moger