Re: [PATCH v6 1/3] x86/resctrl: Add mount option "mba_MBps_event"

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Tony,

On 12/12/2023 12:02 PM, Tony Luck wrote:
> On Tue, Dec 12, 2023 at 09:54:38AM -0800, Reinette Chatre wrote:
>>
>> On 12/8/2023 2:09 PM, Peter Newman wrote:
>>> On Fri, Dec 8, 2023 at 1:57 PM Tony Luck <tony.luck@intel.com> wrote:
>>>>
>>>> On Fri, Dec 08, 2023 at 10:17:08AM -0800, Peter Newman wrote:
>>>>> Hi Tony,
>>>>>
>>>>> On Thu, Dec 7, 2023 at 11:56 AM Tony Luck <tony.luck@intel.com> wrote:
>>>>>> @@ -2715,7 +2723,25 @@ static int rdt_parse_param(struct fs_context *fc, struct fs_parameter *param)
>>>>>>         case Opt_mba_mbps:
>>>>>>                 if (!supports_mba_mbps())
>>>>>>                         return -EINVAL;
>>>>>> -               ctx->enable_mba_mbps = true;
>>>>>> +               if (is_mbm_local_enabled())
>>>>>> +                       ctx->enable_mba_mbps_local = true;
>>>>>> +               else
>>>>>> +                       return -EINVAL;
>>>>>> +               return 0;
>>>>>> +       case Opt_mba_mbps_event:
>>>>>> +               if (!supports_mba_mbps())
>>>>>> +                       return -EINVAL;
>>>>>> +               if (!strcmp("mbm_local_bytes", param->string)) {
>>>>>> +                       if (!is_mbm_local_enabled())
>>>>>> +                               return -EINVAL;
>>>>>> +                       ctx->enable_mba_mbps_local = true;
>>>>>> +               } else if (!strcmp("mbm_total_bytes", param->string)) {
>>>>>> +                       if (!is_mbm_total_enabled())
>>>>>> +                               return -EINVAL;
>>>>>> +                       ctx->enable_mba_mbps_total = true;
>>>>>> +               } else {
>>>>>> +                       return -EINVAL;
>>>>>
>>>>> It looks like if I pass
>>>>> "mba_MBps_event=mbm_total_bytes,mba_MBps_event=mbm_local_bytes" I can
>>>>> set both flags true.
>>>>
>>>> That's going to be confusing. I'll add code to stop the user from
>>>> passing both options.
>>>
>>> Also kind of confusing, after reading the second patch, I realized
>>> "mba_MBps_event=mbm_total_bytes,mba_MBps" also results in both being
>>> set. If you're able to fail the mount operation if both flags somehow
>>> get set, that would address this one too.
>>
>> Are two separate flags required? All existing options within struct rdt_fs_context
>> are of type bool but that does not imply that it is the required type for
>> all. 
> 
> Reinette,
> 
> Maybe a flag and a value?  The structure becomes:
> 
> struct rdt_fs_context {
> 	struct kernfs_fs_context	kfc;
> 	bool				enable_cdpl2;
> 	bool				enable_cdpl3;
> 	bool				enable_mba_mbps;
> 	enum resctrl_event_id		mba_mbps_event;
> 	bool				enable_debug;
> };

A flag and value would work. This brings the implementation close
to the resource properties. Something that is confusing to me with
this change is the inconsistent naming:

struct rdt_fs_context:
	bool			enable_mba_mbps
	enum resctrl event_id	mba_mbps_event

struct resctrl_membw:
	bool			mba_sc
	enum resctrl_event_id	mba_mbps_event


The intention with the above naming is not obvious to me. How are
these intended to be viewed?

One option could be to view these as separately representing user
space (struct rdt_fs_context) and kernel space (struct resctrl_membw).
If this is the case then the following naming may be more intuitive:

struct rdt_fs_context:
	bool			enable_mba_mbps
	enum resctrl event_id	mba_mbps_event

struct resctrl_membw:
	bool			mba_sc
	enum resctrl_event_id	mba_sc_event



> 
> Mount option parsing (including blocking user from setting the options
> multiple times):
> 
> 	case Opt_mba_mbps:
> 		if (!supports_mba_mbps() || ctx->enable_mba_mbps)
> 			return -EINVAL;

I am not familiar with the API but it seems that invalfc() is available
to communicate a more useful message to user space than the default one
shown in changelog of patch #2.

> 		if (is_mbm_local_enabled())
> 			ctx->mba_mbps_event = QOS_L3_MBM_LOCAL_EVENT_ID;
> 		else if (is_mbm_total_enabled())
> 			ctx->mba_mbps_event = QOS_L3_MBM_TOTAL_EVENT_ID;
> 		else
> 			return -EINVAL;
> 		ctx->enable_mba_mbps = true;
> 		return 0;
> 	case Opt_mba_mbps_event:
> 		if (!supports_mba_mbps() || ctx->enable_mba_mbps)
> 			return -EINVAL;
> 		if (!strcmp("mbm_local_bytes", param->string))
> 			ctx->mba_mbps_event = QOS_L3_MBM_LOCAL_EVENT_ID;
> 		else if (!strcmp("mbm_total_bytes", param->string))
> 			ctx->mba_mbps_event = QOS_L3_MBM_TOTAL_EVENT_ID;
> 		else
> 			return -EINVAL;
> 		ctx->enable_mba_mbps = true;
> 		return 0;
> 
> 
> and use of the options to enable the feature:
> 
> 	if (ctx->enable_mba_mbps) {
> 		r->membw.mba_mbps_event = ctx->mba_mbps_event;
> 		ret = set_mba_sc(true);
> 		if (ret)
> 			goto out_cdpl3;
> 	}

Since 0 will not be used for an unset/invalid value I expect mba_mbps_event
will not (cannot) be cleared by rdt_disable_ctx(). If this is the case I think
future changes can be supported by expanding the kerneldoc of struct resctrl_membw
to document that "@mba_mbps_event (or @mba_sc_event?) is invalid if @mba_sc
is false".

Reinette