Re: [PATCH v7 24/24] x86/resctrl: Introduce interface to modify assignment states of the groups

[Reinette Chatre]

Hi Babu,

On 10/4/24 12:36 PM, Moger, Babu wrote:
> On 10/4/2024 11:52 AM, Reinette Chatre wrote:
>> On 10/4/24 9:38 AM, Moger, Babu wrote:
>>> On 10/3/2024 9:17 PM, Reinette Chatre wrote:
>>>> On 10/3/24 6:11 PM, Moger, Babu wrote:
>>>>> On 10/2/2024 1:19 PM, Reinette Chatre wrote:
>>>>>> On 9/27/24 10:47 AM, Moger, Babu wrote:
>>>>>>> On 9/19/2024 12:59 PM, Reinette Chatre wrote:
>>>>>>>> On 9/4/24 3:21 PM, Babu Moger wrote:
>>>>>>
>>>>>>>>> v7: Simplified the parsing (strsep(&token, "//") in rdtgroup_mbm_assign_control_write().
>>>>>>>>>         Added mutex lock in rdtgroup_mbm_assign_control_write() while processing.
>>>>>>>>>         Renamed rdtgroup_find_grp to rdtgroup_find_grp_by_name.
>>>>>>>>>         Fixed rdtgroup_str_to_mon_state to return error for invalid flags.
>>>>>>>>>         Simplified the calls rdtgroup_assign_cntr by merging few functions earlier.
>>>>>>>>>         Removed ABMC reference in FS code.
>>>>>>>>>         Reinette commented about handling the combination of flags like 'lt_' and '_lt'.
>>>>>>>>>         Not sure if we need to change the behaviour here. Processed them sequencially right now.
>>>>>>>>>         Users have the liberty to pass the flags. Restricting it might be a problem later.
>>>>>>>>
>>>>>>>> Could you please give an example of what problem may be encountered later? An assignment
>>>>>>>> like "domain=_lt" seems like a contradiction to me since user space essentially asks
>>>>>>>> for "None of the MBM events" as well as "MBM total event" and "MBM local event".
>>>>>>>
>>>>>>> I agree it is contradiction. But user is the one who decides to do that. I think we should allow it. Also, there is some value to it as well.
>>>>>>>
>>>>>>> "domain=_lt" This will also reset the counters if the total and local events are assigned earlier this action.
>>>>>>
>>>>>> The last sentence is not clear to me. Could you please elaborate what
>>>>>> you mean with "are assigned earlier this action"?
>>>>>>
>>>>>
>>>>> I think I confused you here. "domain=_lt" is equivalent to "domain=lt".  My reasoning is handling all the combination in the code adds code complexity and leave it the user what he wants to do.
>>>>
>>>> hmmm ... and how about "domain=lt_"? Do you think this should also be equivalent to
>>>> "domain=lt" or perhaps an expectation that counters should be assigned to the two events
>>>> and then immediately unassigned?
>>>
>>> Yes. "domain=lt_" should be "domain=lt".
>>>
>>>>
>>>> Giving user such flexibility may be interpreted as the assignment seen as acting
>>>> sequentially through the flags provided. Ideally the interface should behave in
>>>> a predictable way if the goal is to provide flexibility to the user.
>>>>
>>>
>>> My only concern is adding the check now and reverting it back later.
>>> Basically process the flags sequentially and don't differentiate between the flags. I feel it fits the predictable behavior. No?
>>
>> This is the point I was trying to make. If flags are processed sequentially then it would be
>> predictable behavior and if that is documented expectation then that should be ok. The problem
>> that I want to highlight is that if predictable sequential processing is the goal then
>> "domain=_lt" cannot be interpreted the same as "domain="lt_". When flags in "domain=lt_"
>> are processed sequentially then final state should be "domain=_", no?
> 
> Yes. that is correct.
>>
>> If sequential processing is done then "domain=_lt" means "unassign all counters followed
>> by assign of counter to local MBM monitoring, followed by assign of counter to total MBM
>> monitoring". Similarly, "domain=lt_" means "assign a counter to local MBM monitoring, then
>> assign a counter to total MBM monitoring, then unassign all counters".
> 
> Yes. That is correct.
>>
>> If this sequential processing is the goal then the implementation would still need to be
>> adapted. Consider, for example, "domain=lt" ... with sequential processing the user
>> indicates/expects that "local MBM monitoring" has priority if there is only one counter
>> available, but the current implementation does not process it sequentially and would end up
>> assigning counter to "total MBM monitoring" first.
> 
> Sure. Lets accommodate the sequential processing. Process the  flags
> in the order it is provided. I need to make few changes to
> rdtgroup_process_flags() to address it. Hopefully, it can be done
> without much complexity. Thanks

I doubt that the implementation would be complex but it may take some effort for it
to be efficient ... taking actions that involve changing kernel and HW state for each
flag as it is encountered vs. parsing all flags and changing kernel and HW state once.

The risk is that a simple request like "domain=lt" may take twice as long when
doing sequential processing. When users provide flags like "domain=_lt" to take advantage
of sequential processing then there may be an argument like "user gets what is being asked
for" when things are slower, but I am not sure the same can be true for a user
that just wants to run "domain=lt".

To me it seems simpler to require that "_" always appears by itself and that
any flags set by the user using "=" are combined during parsing so that they can be
acted on in a single flow. If indeed users want to do something sequentially
like "unassign all flags and then assign local MBM" then instead of "domain=_l"
I think "domain=_;domain=l" could be used?

Reinette