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

[Moger, Babu]

Hi Reinette,

On 10/4/2024 4:09 PM, Reinette Chatre wrote:
> 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

Ok. Lets go with this approach treating "_" as a special and cannot be 
combined with other flags. Seems simple to implement.

> 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?

Yes. It can be done.
thanks
Babu Moger