linux-kernel - RE: [PATCH v8 6/7] x86/resctrl: Add write option to "mba_MBps

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <SJ1PR11MB60839C385657E16FA5B84381FC592@SJ1PR11MB6083.namprd11.prod.outlook.com>
Date: Tue, 12 Nov 2024 23:57:51 +0000
From: "Luck, Tony" <tony.luck@...el.com>
To: "Chatre, Reinette" <reinette.chatre@...el.com>, "Yu, Fenghua"
	<fenghua.yu@...el.com>, Peter Newman <peternewman@...gle.com>, "Jonathan
 Corbet" <corbet@....net>, Shuah Khan <skhan@...uxfoundation.org>,
	"x86@...nel.org" <x86@...nel.org>
CC: James Morse <james.morse@....com>, Jamie Iles <quic_jiles@...cinc.com>,
	Babu Moger <babu.moger@....com>, Randy Dunlap <rdunlap@...radead.org>,
	"Shaopeng Tan (Fujitsu)" <tan.shaopeng@...itsu.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"patches@...ts.linux.dev" <patches@...ts.linux.dev>
Subject: RE: [PATCH v8 6/7] x86/resctrl: Add write option to "mba_MBps_event"
 file

> >>>> +   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.

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

-Tony