Re: [PATCH v7 5/8] x86/resctrl: Unwind the errors inside rdt_enable_ctx()

[Reinette Chatre]

Hi Babu,

On 8/11/2023 1:09 PM, Babu Moger wrote:
> rdt_enable_ctx() enables the features provided during resctrl mount.
> 
> Additions to rdt_enable_ctx() are required to also modify error paths
> of rdt_enable_ctx() callers to ensure correct unwinding if errors
> are encountered after calling rdt_enable_ctx(). This is error prone.
> 
> Introduce rdt_disable_ctx() to refactor the error unwinding of
> rdt_enable_ctx() to simplify future additions.
> 
> Suggested-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   44 ++++++++++++++++++++++++--------
>  1 file changed, 33 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 3010e3a1394d..0805fac04401 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2377,19 +2377,44 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
>  			     struct rdtgroup *prgrp,
>  			     struct kernfs_node **mon_data_kn);
>  
> +static void rdt_disable_ctx(struct rdt_fs_context *ctx)
> +{
> +	if (ctx->enable_cdpl2)
> +		resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
> +
> +	if (ctx->enable_cdpl3)
> +		resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
> +
> +	if (ctx->enable_mba_mbps)
> +		set_mba_sc(false);
> +}

I am not sure if rdt_disable_ctx() should depend on the
fs context (more later).

> +
>  static int rdt_enable_ctx(struct rdt_fs_context *ctx)
>  {
>  	int ret = 0;
>  
> -	if (ctx->enable_cdpl2)
> +	if (ctx->enable_cdpl2) {
>  		ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, true);
> +		if (ret)
> +			goto out_disable;
> +	}
>  
> -	if (!ret && ctx->enable_cdpl3)
> +	if (ctx->enable_cdpl3) {
>  		ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, true);
> +		if (ret)
> +			goto out_disable;
> +	}
>  
> -	if (!ret && ctx->enable_mba_mbps)
> +	if (ctx->enable_mba_mbps) {
>  		ret = set_mba_sc(true);
> +		if (ret)
> +			goto out_disable;
> +	}
> +
> +	return 0;
>  
> +out_disable:
> +	rdt_disable_ctx(ctx);
>  	return ret;

This is not the exit pattern we usually follow. Also note
that it could lead to incorrect behavior if there is an early failure
in this function but all unwinding would end up being done in
rdt_disable_ctx() because error unwinding is done based on whether
a feature _should_ be enabled not whether it was enabled.
Could you please instead have direct error handling by only undoing
what was already done at the time of the error?

>  }
>  
> @@ -2497,13 +2522,13 @@ static int rdt_get_tree(struct fs_context *fc)
>  	}
>  
>  	ret = rdt_enable_ctx(ctx);
> -	if (ret < 0)
> -		goto out_cdp;
> +	if (ret)
> +		goto out;
>  
>  	ret = schemata_list_create();
>  	if (ret) {
>  		schemata_list_destroy();
> -		goto out_mba;
> +		goto out_ctx;
>  	}
>  
>  	closid_init();
> @@ -2562,11 +2587,8 @@ static int rdt_get_tree(struct fs_context *fc)
>  	kernfs_remove(kn_info);
>  out_schemata_free:
>  	schemata_list_destroy();
> -out_mba:
> -	if (ctx->enable_mba_mbps)
> -		set_mba_sc(false);
> -out_cdp:
> -	cdp_disable_all();
> +out_ctx:
> +	rdt_disable_ctx(ctx);
>  out:
>  	rdt_last_cmd_clear();
>  	mutex_unlock(&rdtgroup_mutex);
> 
> 

rdt_enable_ctx() is called in rdt_get_tree() and thus its work should
also be cleaned up in rdt_kill_sb(). Note how the cleanup you replace
here is also duplicated in rdt_kill_sb(), meaning the unwinding continues
to be open coded and patch #7 propagates this. 
Could rdt_kill_sb() not also call rdt_disable_ctx()? This brings me
back to the earlier comment about it depending on the fs context. I
do not know if the context will be valid at this time. I do not
think that the context is required though it could have no parameters
and do cleanup as is done at the moment.

Reinette