Re: [PATCH v4 08/10] fs/resctrl: Change last_cmd_status custom during input parsing

[Luck, Tony]

On Wed, Apr 29, 2026 at 08:06:38AM -0700, Reinette Chatre wrote:
> A pattern of usage of last_cmd_status was introduced during its enabling in
> commit
> 
> c377dcfbee80 ("x86/intel_rdt: Add diagnostics when writing the schemata file")
> 
> and since copied throughout resctrl to result in the following custom:
> 
> 	..._write()
> 	{
> 		/* Early parsing of input, exit on failure. */
> 
> 		/* Obtain rdtgroup_mutex */
> 		rdt_last_cmd_clear(); /* Clear last_cmd_status buffer */
> 
> 		/*
> 		 * Act on user command, failures result in detail
> 		 * error message in last_cmd_status buffer via
> 		 * rdt_last_cmd_puts()/rdt_last_cmd_printf().
> 		 */
> 
> 		/* Release rdtgroup_mutex */
> 	}
> 
> If resctrl exits with failure during early parsing of input there are two
> possible scenarios:
>  - The last_cmd_status buffer is empty and a user's read of
>    info/last_cmd_status returns "ok".
>  - The last_cmd_status buffer contains details from an earlier ...write()
>    failure and a user's read of info/last_cmd_status returns this outdated
>    error description.
> 
> Writing to a resctrl file is considered a "resctrl command" and the
> resctrl documentation states the following about the last_cmd_status file:
>    "If the command failed, it will provide more information that can be
>     conveyed in the error returns from file operations."
> Neither of the current scenarios is correct behavior.
> 
> Move early input parsing to be done with rdtgroup_mutex held after the
> last_cmd_status buffer is cleared. Let info/last_cmd_status be accurate
> when an error is encountered during parsing of user command.
> 
> Signed-off-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> Reviewed-by: Ben Horgan <ben.horgan@xxxxxxx>
> ---
> Changes since v1:
> - Add Ben's RB tag.
> ---
>  fs/resctrl/ctrlmondata.c | 54 ++++++++++++++++----------
>  fs/resctrl/monitor.c     | 60 +++++++++++++++++------------
>  fs/resctrl/rdtgroup.c    | 82 +++++++++++++++++++++++-----------------
>  3 files changed, 117 insertions(+), 79 deletions(-)
> 
> diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
> index 8a43efe67a95..5d5e844429df 100644
> --- a/fs/resctrl/ctrlmondata.c
> +++ b/fs/resctrl/ctrlmondata.c
> @@ -312,11 +312,6 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
>  	char *tok, *resname;
>  	int ret = 0;
>  
> -	/* Valid input requires a trailing newline */
> -	if (nbytes == 0 || buf[nbytes - 1] != '\n')
> -		return -EINVAL;
> -	buf[nbytes - 1] = '\0';
> -
>  	rdtgrp = rdtgroup_kn_lock_live(of->kn);
>  	if (!rdtgrp) {
>  		rdtgroup_kn_unlock(of->kn);
> @@ -324,6 +319,15 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
>  	}
>  	rdt_last_cmd_clear();
>  
> +	/* Valid input requires a trailing newline */
> +	if (nbytes == 0 || buf[nbytes - 1] != '\n') {
> +		rdt_last_cmd_puts("Invalid input\n");

This is obviously a big improvement over existing failure to provide a
message (or leaving previous stale message). Do you want to go the extra
mile and disambiguate all these "Invalid input" messages?  E.g. here:

		rdt_last_cmd_puts("schemata: Invalid input\n");

It's good to have a grep-able pattern to diagnostics.

All the other patches in this series look great. You can add my

Reviewed-by: Tony Luck <tony.luck@xxxxxxxxx>

to them all.

-Tony