Re: [PATCH v8 6/7] x86/resctrl: Add write option to "mba_MBps_event" file

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

Hi Tony,

On 11/12/24 3:57 PM, Luck, Tony wrote:
>>>>>> +   if (!strcmp(buf, "mbm_local_bytes")) {
>>>>>> +           if (is_mbm_local_enabled())
>>>>>> +                   rdtgrp->mba_mbps_event = QOS_L3_MBM_LOCAL_EVENT_ID;
>>>>>> +           else
>>>>>> +                   ret = -ENXIO;
>>>>>> +   } else if (!strcmp(buf, "mbm_total_bytes")) {
>>>>>> +           if (is_mbm_total_enabled())
>>>>>> +                   rdtgrp->mba_mbps_event = QOS_L3_MBM_TOTAL_EVENT_ID;
>>>>>
>>>>>
>>>>> User may think each time toggling the local/total event will effect MBA.
>>>>> And they may create usage case like frequently changing the events to
>>>>> maintain/adjust both total and local within bw boundary.
>>>>>
>>>>> But toggling mba_mbps_event faster than 1sec doesn't have any effect on
>>>>> MBA SC because MBA SC is called every one second.
>>>>>
>>>>> Maybe need to add a ratelimit of 1 second on calling this function? And
>>>>> adding info in the document that toggling speed should be slower than 1
>>>>> second?
>>>>
>>>> The limit would need to be per ctrl_mon group, not on calls to this function.
>>>> It's perfectly ok to switch multiple groups in a short interval.
>>>
>>> Agree.
>>>
>>>>
>>>> I'm not sure how to rate limit here. I could add a delay so that the write()
>>>> call blocks until enough time passes before making the change. But
>>>> what should I do if a user submits more writes to the file? Queue them
>>>> all and apply at one second intervals?
>>>
>>> Maybe define "mba_mbps_last_time" in rdtgroup. Then
>>>
>>> if (time_before(jiffies, rdtgrp->mba_mbps_last_time + HZ) {
>>>     rdt_last_cmd_printf("Too fast (>1/s) mba_MBps event change)\n");
>>>         rdtgroup_kn_unlock(of->kn);
>>>     return -EAGAIN;
>>> }
>>> rdtgrp->mba_mbps_last_time = jiffies;
>>
>> This seems like enforcing an unnecessary limitation. For example, this would mean
>> that user space needs to wait for a second to undo a change to the monitoring event
>> in case there was a mistake. This seems like an unnecessary restriction to me.
>>
>> I am also afraid that there may be some corner cases where a write to the file and
>> the actual run of the overflow handler  (on every domain) may not be exactly HZ apart.
>>
>> Bandwidth allocation remains to be adjusted in either direction with at most the bandwidth
>> granularity. A user attempting to toggle bandwidth event cannot expecting large
>> changes in allocated bandwidth even if the events differ significantly.
>>
>> Surely we can explore more if we learn about a specific use case.
> 
> Note that the kernel generally doesn't prevent the user from doing inane things
> that do not cause damage.  E.g. a user can already abuse the legacy memory
> percentage bandwidth controls with
> 
> while :
> do
> 	echo "MB:0=100;1=10" > schemata
> 	sleep 0.1
> 	echo "MB:0=10;1=100" > schemata
> 	sleep 0.1
> done
> 
> Similar abuse of the percentage mode is also possible.

Good point.

At least with the percentage modes the MBA allocation is promptly written to
hardware.

Related to issue at hand the user is also already able to quickly switch the MBps
value via quick successive writes to the schemata and those changes are already
invisible to the software controller that only consumes user's MBps once per second.

>>>> Maybe it would be better to just to add some additional text to the
>>>> documentation pointing out that resctrl only checks bandwidth once
>>>> per second to make throttling adjustments. So changes to the event
>>>> will only have effect after some seconds have passed?
>>>
>>>
>>> Add additional text would be great.
>>
>>
>> Agreed.
> 
> We hadn't previously documented the rate at which mba_sc measured and adjusted
> the memory bandwidth allocation controls. Once per second is currently an implementation
> detail that in theory could be changed in the future.
> 
> I'm reluctant to carve that value into the stone of resctrl.rst, But without a specific
> value "don't change the values too rapidly" is meaningless.

I agree that one second should not be part of documentation, a higher level generic "interval"
can be used instead. But ... since you pointed out that writing to schemata also has this
caveat any documentation should ideally address that also. With existing documentation seemingly
sufficient in that regard I'll also be ok with keeping it as high level with this new event
support.

Reinette