Because ARM's MPAM controls are probed using MMIO, resctrl can't be
initialised until enough CPUs are online to have determined the
system-wide supported num_closid. Arm64 also supports 'late onlined
secondaries', where only a subset of CPUs are online during boot.

These two combine to mean the MPAM driver may not be able to initialise
resctrl until user-space has brought 'enough' CPUs online.

To allow MPAM to initialise resctrl after __init text has been free'd,
remove all the __init markings from resctrl.

The existing __exit markings cause these functions to be removed by the
linker as it has never been possible to build resctrl as a module. MPAM
has an error interrupt which causes the driver to reset and disable
itself. Remove the __exit markings to allow the MPAM driver to tear down
resctrl when an error occurs.

Signed-off-by: James Morse <james.morse@arm.com>
---
If 'late onlined secondaries' is an alien concept, I can add a worked
example to the commit message:
If a system has two L3 caches, but during boot only CPU-0 is online,
then no CPU is able to probe the features of the second L3 cache.
It's not until user-space brings other CPUs online that the MPAM
driver can finally get a glimpse of all the hardware to determine
what properties the system has.
---
 arch/x86/kernel/cpu/resctrl/internal.h | 2 +-
 arch/x86/kernel/cpu/resctrl/monitor.c  | 4 ++--
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 8 ++++----
 include/linux/resctrl.h                | 4 ++--
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 3a3962736061..56218193a8ba 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -457,7 +457,7 @@ void closid_free(int closid);
 int alloc_rmid(u32 closid);
 void free_rmid(u32 closid, u32 rmid);
 int rdt_get_mon_l3_config(struct rdt_resource *r);
-void __exit resctrl_mon_resource_exit(void);
+void resctrl_mon_resource_exit(void);
 bool rdt_cpu_has(int flag);
 void mon_event_count(void *info);
 int rdtgroup_mondata_show(struct seq_file *m, void *arg);
diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
index 8b316d9acc3b..7e6fca138cb7 100644
--- a/arch/x86/kernel/cpu/resctrl/monitor.c
+++ b/arch/x86/kernel/cpu/resctrl/monitor.c
@@ -954,7 +954,7 @@ static int dom_data_init(struct rdt_resource *r)
 	return err;
 }
 
-static void __exit dom_data_exit(struct rdt_resource *r)
+static void dom_data_exit(struct rdt_resource *r)
 {
 	if (!r->mon_capable)
 		return;
@@ -1076,7 +1076,7 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
 	return 0;
 }
 
-void __exit resctrl_mon_resource_exit(void)
+void resctrl_mon_resource_exit(void)
 {
 	struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
 
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 218aebd6387f..1425a33d201d 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2069,7 +2069,7 @@ static struct rftype *rdtgroup_get_rftype_by_name(const char *name)
 	return NULL;
 }
 
-static void __init thread_throttle_mode_init(void)
+static void thread_throttle_mode_init(void)
 {
 	struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_MBA);
 	struct rftype *rft;
@@ -3997,7 +3997,7 @@ static void rdtgroup_destroy_root(void)
 	rdtgroup_default.kn = NULL;
 }
 
-static void __init rdtgroup_setup_default(void)
+static void rdtgroup_setup_default(void)
 {
 	mutex_lock(&rdtgroup_mutex);
 
@@ -4190,7 +4190,7 @@ void resctrl_offline_cpu(unsigned int cpu)
  *
  * Return: 0 on success or -errno
  */
-int __init resctrl_init(void)
+int resctrl_init(void)
 {
 	int ret = 0;
 
@@ -4244,7 +4244,7 @@ int __init resctrl_init(void)
 	return ret;
 }
 
-void __exit resctrl_exit(void)
+void resctrl_exit(void)
 {
 	debugfs_remove_recursive(debugfs_resctrl);
 	unregister_filesystem(&rdt_fs_type);
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index f463fb949677..5da55e58f229 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -393,7 +393,7 @@ void resctrl_arch_reset_rmid_all(struct rdt_resource *r, struct rdt_domain *d);
 extern unsigned int resctrl_rmid_realloc_threshold;
 extern unsigned int resctrl_rmid_realloc_limit;
 
-int __init resctrl_init(void);
-void __exit resctrl_exit(void);
+int resctrl_init(void);
+void resctrl_exit(void);
 
 #endif /* _RESCTRL_H */
-- 
2.39.2

Hi James,

On 3/21/2024 9:51 AM, James Morse wrote:
> Because ARM's MPAM controls are probed using MMIO, resctrl can't be
> initialised until enough CPUs are online to have determined the
> system-wide supported num_closid. Arm64 also supports 'late onlined
> secondaries', where only a subset of CPUs are online during boot.
> 
> These two combine to mean the MPAM driver may not be able to initialise
> resctrl until user-space has brought 'enough' CPUs online.
> 
> To allow MPAM to initialise resctrl after __init text has been free'd,
> remove all the __init markings from resctrl.
> 
> The existing __exit markings cause these functions to be removed by the
> linker as it has never been possible to build resctrl as a module. MPAM
> has an error interrupt which causes the driver to reset and disable
> itself. Remove the __exit markings to allow the MPAM driver to tear down
> resctrl when an error occurs.

Obviously for the reasons you state this code has never been exercised.
Were you able to test this error interrupt flow yet?

Reinette

On Mon, Apr 08, 2024 at 08:32:36PM -0700, Reinette Chatre wrote:
> Hi James,
> 
> On 3/21/2024 9:51 AM, James Morse wrote:
> > Because ARM's MPAM controls are probed using MMIO, resctrl can't be
> > initialised until enough CPUs are online to have determined the
> > system-wide supported num_closid. Arm64 also supports 'late onlined
> > secondaries', where only a subset of CPUs are online during boot.
> > 
> > These two combine to mean the MPAM driver may not be able to initialise
> > resctrl until user-space has brought 'enough' CPUs online.
> > 
> > To allow MPAM to initialise resctrl after __init text has been free'd,
> > remove all the __init markings from resctrl.
> > 
> > The existing __exit markings cause these functions to be removed by the
> > linker as it has never been possible to build resctrl as a module. MPAM
> > has an error interrupt which causes the driver to reset and disable
> > itself. Remove the __exit markings to allow the MPAM driver to tear down
> > resctrl when an error occurs.
> 
> Obviously for the reasons you state this code has never been exercised.
> Were you able to test this error interrupt flow yet?
> 
> Reinette
> 

I think this will have to wait for James to respond.

There is code to tear down resctrl in response to an MPAM error interrupt,
but I don't know how it has been exercised so far (if at all).

https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/tree/drivers/platform/mpam/mpam_devices.c?h=mpam/snapshot/v6.7-rc2#n1940


Cheers
---Dave

Hi Dave, Reinette,

> On Mon, Apr 08, 2024 at 08:32:36PM -0700, Reinette Chatre wrote:
>> Hi James,
>>
>> On 3/21/2024 9:51 AM, James Morse wrote:
>>> Because ARM's MPAM controls are probed using MMIO, resctrl can't be
>>> initialised until enough CPUs are online to have determined the
>>> system-wide supported num_closid. Arm64 also supports 'late onlined
>>> secondaries', where only a subset of CPUs are online during boot.
>>>
>>> These two combine to mean the MPAM driver may not be able to initialise
>>> resctrl until user-space has brought 'enough' CPUs online.
>>>
>>> To allow MPAM to initialise resctrl after __init text has been free'd,
>>> remove all the __init markings from resctrl.
>>>
>>> The existing __exit markings cause these functions to be removed by the
>>> linker as it has never been possible to build resctrl as a module. MPAM
>>> has an error interrupt which causes the driver to reset and disable
>>> itself. Remove the __exit markings to allow the MPAM driver to tear down
>>> resctrl when an error occurs.
>>
>> Obviously for the reasons you state this code has never been exercised.
>> Were you able to test this error interrupt flow yet?
>>
>> Reinette
>>
> 
> I think this will have to wait for James to respond.
> 
> There is code to tear down resctrl in response to an MPAM error interrupt,
> but I don't know how it has been exercised so far (if at all).

We are managed to test the MPAM error interrupt (on the platform that 
supports MPAM interrupts on software errors). For instance programming
more resource control groups (part IDs) than available, and It appears 
to correctly remove the "resctrl" mount point (though mount command 
still shows resctrl on /sys/fs/resctrl type resctrl (rw,relatime)
), but

# mount -t resctrl resctrl /sys/fs/resctrl
mount: /sys/fs/resctrl: mount point does not exist.

Additionally, a question regarding this, Is a complete system restart 
necessary to regain the mount?

Thanks
-Amit

Hi Amit, Reinette,

On 11/04/2024 16:51, Amit Singh Tomar wrote:
>> On Mon, Apr 08, 2024 at 08:32:36PM -0700, Reinette Chatre wrote:
>>> On 3/21/2024 9:51 AM, James Morse wrote:
>>>> Because ARM's MPAM controls are probed using MMIO, resctrl can't be
>>>> initialised until enough CPUs are online to have determined the
>>>> system-wide supported num_closid. Arm64 also supports 'late onlined
>>>> secondaries', where only a subset of CPUs are online during boot.
>>>>
>>>> These two combine to mean the MPAM driver may not be able to initialise
>>>> resctrl until user-space has brought 'enough' CPUs online.
>>>>
>>>> To allow MPAM to initialise resctrl after __init text has been free'd,
>>>> remove all the __init markings from resctrl.
>>>>
>>>> The existing __exit markings cause these functions to be removed by the
>>>> linker as it has never been possible to build resctrl as a module. MPAM
>>>> has an error interrupt which causes the driver to reset and disable
>>>> itself. Remove the __exit markings to allow the MPAM driver to tear down
>>>> resctrl when an error occurs.
>>>
>>> Obviously for the reasons you state this code has never been exercised.
>>> Were you able to test this error interrupt flow yet?

>> I think this will have to wait for James to respond.
>>
>> There is code to tear down resctrl in response to an MPAM error interrupt,
>> but I don't know how it has been exercised so far (if at all).

Previously I saw one or two kernfs structures left behind. (to discover this you had to
leave a shell with its CWD in the filesystem), but it looks like those issues have been
solved.

Dave points out that resctrl_exit() removing the sysfs mount point means the filesystem
can't be umount()ed. Systemd doesn't seem to care today, but might choke on this in the
future.

I think the right thing to do here is get resctrl_exit() to call rdtgroup_destroy_root(),
and drop sysfs_remove_mount_point(). This creates a bit of asymmetry, but if resctrl were
a module the mount-point stuff would be done in module init/exit - only we don't have a
module to unload, so the asymmetry is to be expected. I don't think its worth adding new
__exit text that we know will never be used for the sake of symmetry.

With this change, triggering the interrupt makes all the files under resctrl disappear, I
can then umount() the filesystem, but not re-mount it.


The aim here is for the arch code to be able to say "this is broken, I can't support
resctrl" with minimum changes to the existing code.

There are a couple of vanishingly unlikely corner cases that need tightening up: e.g. the
rmid_ptrs[] array disappears, a syscall could get blocked on the rdtgroup_mutex while the
teardown happens, once it gains the lock it discoverers a surprise NULL pointer.

Fixing these can wait until after the code is moved as these things can't happen on x86.
(patches are in the 'extra's branch of the mpam tree:
https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/log/?h=mpam/snapshot%2bextras/v6.10-rc1
)


> We are managed to test the MPAM error interrupt (on the platform that supports MPAM
> interrupts on software errors). For instance programming
> more resource control groups (part IDs) than available, and It appears to correctly remove
> the "resctrl" mount point (though mount command still shows resctrl on /sys/fs/resctrl
> type resctrl (rw,relatime)
> ), but
> 
> # mount -t resctrl resctrl /sys/fs/resctrl
> mount: /sys/fs/resctrl: mount point does not exist.
> 
> Additionally, a question regarding this, Is a complete system restart necessary to regain
> the mount?

It is - but you are likely to hit the same software bug again. The story here is about
keeping the machine running without penalising the wrong task. I think its acceptable for
programs driving resctrl to go wrong, provided the workload doesn't run with the wrong
configuration.


Thanks,

James

On Thu, Apr 11, 2024 at 09:21:38PM +0530, Amit Singh Tomar wrote:
> Hi Dave, Reinette,
> 
> > On Mon, Apr 08, 2024 at 08:32:36PM -0700, Reinette Chatre wrote:
> > > Hi James,
> > > 
> > > On 3/21/2024 9:51 AM, James Morse wrote:
> > > > Because ARM's MPAM controls are probed using MMIO, resctrl can't be
> > > > initialised until enough CPUs are online to have determined the
> > > > system-wide supported num_closid. Arm64 also supports 'late onlined
> > > > secondaries', where only a subset of CPUs are online during boot.
> > > > 
> > > > These two combine to mean the MPAM driver may not be able to initialise
> > > > resctrl until user-space has brought 'enough' CPUs online.
> > > > 
> > > > To allow MPAM to initialise resctrl after __init text has been free'd,
> > > > remove all the __init markings from resctrl.
> > > > 
> > > > The existing __exit markings cause these functions to be removed by the
> > > > linker as it has never been possible to build resctrl as a module. MPAM
> > > > has an error interrupt which causes the driver to reset and disable
> > > > itself. Remove the __exit markings to allow the MPAM driver to tear down
> > > > resctrl when an error occurs.
> > > 
> > > Obviously for the reasons you state this code has never been exercised.
> > > Were you able to test this error interrupt flow yet?
> > > 
> > > Reinette
> > > 
> > 
> > I think this will have to wait for James to respond.
> > 
> > There is code to tear down resctrl in response to an MPAM error interrupt,
> > but I don't know how it has been exercised so far (if at all).
> 
> We are managed to test the MPAM error interrupt (on the platform that
> supports MPAM interrupts on software errors). For instance programming
> more resource control groups (part IDs) than available, and It appears to
> correctly remove the "resctrl" mount point (though mount command still shows
> resctrl on /sys/fs/resctrl type resctrl (rw,relatime)
> ), but

Thanks for trying this out!

Is it possible to unmount resctrl once the system is in this state?

> # mount -t resctrl resctrl /sys/fs/resctrl
> mount: /sys/fs/resctrl: mount point does not exist.

What if you now try to mount resctrl somewhere else, e.g.:

# mount -t resctrl resctrl /mnt

I'm guessing this _should_ fail if you weren't able to unmount resctrl,
since resctrl seems to forbid multiple mount instances.

I'm not sure what the best behaviour is here.  Leaving resctrl "half-
mounted" might be a good thing: at this point the system is in a semi-
bad state we want to make sure it can't be remounted.  Unregistering the
resctrl filesystem from the fs core feels cleaner if feasible though.

Leaving an impossible unmount operation for init to do during reboot/
shutdown feels unfortunate.

We might have to look at what other filesystems do in this area.

The mount machinery does provide other ways of getting into broken,
impossible situations from userspace, so this doesn't feel like an
entirely new problem.

> 
> Additionally, a question regarding this, Is a complete system restart
> necessary to regain the mount?
> 
> Thanks
> -Amit

I think James will need to comment on this, but I think that yes, it
is probably appropriate to require a reboot.  I think an MPAM error
interrupt should only happen if the software did something wrong, so
it's a bit like hitting a BUG(): we don't promise that everything works
100% properly until the system is restarted.  Misbehaviour should be
contained to MPAM though.

Cheers
---Dave

Hi Dave,

> Is it possible to unmount resctrl once the system is in this state?
No, it can't be unmounted, as there is no mount exist.

>> # mount -t resctrl resctrl /sys/fs/resctrl
>> mount: /sys/fs/resctrl: mount point does not exist.
> 
> What if you now try to mount resctrl somewhere else, e.g.:
> 
> # mount -t resctrl resctrl /mnt
root@localhost:~# mount -t resctrl resctrl /test
mount: /test: unknown filesystem type 'resctrl'.

> 
> I'm guessing this _should_ fail if you weren't able to unmount resctrl,
> since resctrl seems to forbid multiple mount instances.
> 
> I'm not sure what the best behaviour is here.  Leaving resctrl "half-
> mounted" might be a good thing: at this point the system is in a semi-
> bad state we want to make sure it can't be remounted.  Unregistering the
> resctrl filesystem from the fs core feels cleaner if feasible though.
> 
> Leaving an impossible unmount operation for init to do during reboot/
> shutdown feels unfortunate.
> 
> We might have to look at what other filesystems do in this area.
> 
> The mount machinery does provide other ways of getting into broken,
> impossible situations from userspace, so this doesn't feel like an
> entirely new problem.
> 
>>
>> Additionally, a question regarding this, Is a complete system restart
>> necessary to regain the mount?
>>
>> Thanks
>> -Amit
> 
> I think James will need to comment on this, but I think that yes, it
> is probably appropriate to require a reboot.  I think an MPAM error
> interrupt should only happen if the software did something wrong, so
> it's a bit like hitting a BUG(): we don't promise that everything works
> 100% properly until the system is restarted.  Misbehaviour should be
> contained to MPAM though.
> 
if "resctrl" is nonfunctional in this state, then this comment[1] here 
does *not* make sense.

"restore any modified controls to their reset values."

Thanks
-Amit

[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/tree/drivers/platform/mpam/mpam_devices.c?h=mpam/snapshot/v6.7-rc2#n2228

root@localhost:~# mount
tmpfs on /run/user/0 type tmpfs 
(rw,nosuid,nodev,relatime,size=32923772k,nr_inodes=8230943,mode=700)
resctrl on /sys/fs/resctrl type resctrl (rw,relatime)

root@localhost:~# devmem msc_addr 32 0x9999
[  687.096276] mpam: error irq from msc:1 'PARTID_SEL_Range', 
partid:39321, pmg: 0, ris: 0

root@localhost:~# mount
tmpfs on /run/user/0 type tmpfs 
(rw,nosuid,nodev,relatime,size=32923772k,nr_inodes=8230943,mode=700)
resctrl on /sys/fs/resctrl type resctrl (rw,relatime)

root@localhost:~# umount resctrl
umount: /sys/fs/resctrl: no mount point specified.

root@localhost:~# mount
tmpfs on /run/user/0 type tmpfs 
(rw,nosuid,nodev,relatime,size=32923772k,nr_inodes=8230943,mode=700)

root@localhost:~# mount -t resctrl resctrl /test
mount: /test: unknown filesystem type 'resctrl'.

On Tue, Apr 30, 2024 at 12:43:05PM +0530, Amit Singh Tomar wrote:
> Hi Dave,
> 
> > Is it possible to unmount resctrl once the system is in this state?
> No, it can't be unmounted, as there is no mount exist.

I see.


> > > # mount -t resctrl resctrl /sys/fs/resctrl
> > > mount: /sys/fs/resctrl: mount point does not exist.
> > 
> > What if you now try to mount resctrl somewhere else, e.g.:
> > 
> > # mount -t resctrl resctrl /mnt
> root@localhost:~# mount -t resctrl resctrl /test
> mount: /test: unknown filesystem type 'resctrl'.

Oh, right, so the resctrl filesystem gets unregistered in that
case...

> > 
> > I'm guessing this _should_ fail if you weren't able to unmount resctrl,
> > since resctrl seems to forbid multiple mount instances.
> > 
> > I'm not sure what the best behaviour is here.  Leaving resctrl "half-
> > mounted" might be a good thing: at this point the system is in a semi-
> > bad state we want to make sure it can't be remounted.  Unregistering the
> > resctrl filesystem from the fs core feels cleaner if feasible though.
> > 
> > Leaving an impossible unmount operation for init to do during reboot/
> > shutdown feels unfortunate.
> > 
> > We might have to look at what other filesystems do in this area.
> > 
> > The mount machinery does provide other ways of getting into broken,
> > impossible situations from userspace, so this doesn't feel like an
> > entirely new problem.
> > 
> > > 
> > > Additionally, a question regarding this, Is a complete system restart
> > > necessary to regain the mount?
> > > 
> > > Thanks
> > > -Amit
> > 
> > I think James will need to comment on this, but I think that yes, it
> > is probably appropriate to require a reboot.  I think an MPAM error
> > interrupt should only happen if the software did something wrong, so
> > it's a bit like hitting a BUG(): we don't promise that everything works
> > 100% properly until the system is restarted.  Misbehaviour should be
> > contained to MPAM though.
> > 
> if "resctrl" is nonfunctional in this state, then this comment[1] here does
> *not* make sense.
> 
> "restore any modified controls to their reset values."

Can you clarify what you mean here?

I think it makes sense to clean up the MPAM hardware as well as we can
in these situations, even if we can't be certain what went wrong.

[final comments below]

> Thanks
> -Amit
> 
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/tree/drivers/platform/mpam/mpam_devices.c?h=mpam/snapshot/v6.7-rc2#n2228
> 
> root@localhost:~# mount
> tmpfs on /run/user/0 type tmpfs
> (rw,nosuid,nodev,relatime,size=32923772k,nr_inodes=8230943,mode=700)
> resctrl on /sys/fs/resctrl type resctrl (rw,relatime)
> 
> root@localhost:~# devmem msc_addr 32 0x9999
> [  687.096276] mpam: error irq from msc:1 'PARTID_SEL_Range', partid:39321,
> pmg: 0, ris: 0
> 
> root@localhost:~# mount
> tmpfs on /run/user/0 type tmpfs
> (rw,nosuid,nodev,relatime,size=32923772k,nr_inodes=8230943,mode=700)
> resctrl on /sys/fs/resctrl type resctrl (rw,relatime)
> 
> root@localhost:~# umount resctrl
> umount: /sys/fs/resctrl: no mount point specified.
> 
> root@localhost:~# mount
> tmpfs on /run/user/0 type tmpfs
> (rw,nosuid,nodev,relatime,size=32923772k,nr_inodes=8230943,mode=700)
> 
> root@localhost:~# mount -t resctrl resctrl /test
> mount: /test: unknown filesystem type 'resctrl'.


Thanks for trying this out.

I guess the behaviour here might want a bit more thought.

I'm not too keen on us leaving a defective mount in the namespace,
with a nonexistent mount pount.  I'm wondering whether things like
systemd may get confused by this...

Cheers
---Dave

>>> I think James will need to comment on this, but I think that yes, it
>>> is probably appropriate to require a reboot.  I think an MPAM error
>>> interrupt should only happen if the software did something wrong, so
>>> it's a bit like hitting a BUG(): we don't promise that everything works
>>> 100% properly until the system is restarted.  Misbehaviour should be
>>> contained to MPAM though.
>>>
>> if "resctrl" is nonfunctional in this state, then this comment[1] here does
>> *not* make sense.
>>
>> "restore any modified controls to their reset values."
> 
> Can you clarify what you mean here?

What I meant was, What's the rationale behind restoring the modified 
controls, if user is going to restart the system anyways (in order to 
use MPAM again), but later realized that it is needed so that *non* MPAM 
loads (user may still want to run other things even after MPAM error 
interrupt) would not have any adverse effect with modified controls.

Therefore, taking my statement back.

> 
> I think it makes sense to clean up the MPAM hardware as well as we can
> in these situations, even if we can't be certain what went wrong.
> 
> [final comments below]
> 
>> Thanks
>> -Amit
>>
>> [1]: https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_morse_linux.git_tree_drivers_platform_mpam_mpam-5Fdevices.c-3Fh-3Dmpam_snapshot_v6.7-2Drc2-23n2228&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=V_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A-JKU&m=DzR4EYX-356bYvqcrD5mYQBzLmDppMaRaHx6yjN7nRE5GH7nogtw6VtDZchmqb_q&s=SKpVy4sPg3dbFJGfMfUGoo252IHOrHrLfcv5f0sCmm0&e=
>>
>> root@localhost:~# mount
>> tmpfs on /run/user/0 type tmpfs
>> (rw,nosuid,nodev,relatime,size=32923772k,nr_inodes=8230943,mode=700)
>> resctrl on /sys/fs/resctrl type resctrl (rw,relatime)
>>
>> root@localhost:~# devmem msc_addr 32 0x9999
>> [  687.096276] mpam: error irq from msc:1 'PARTID_SEL_Range', partid:39321,
>> pmg: 0, ris: 0
>>
>> root@localhost:~# mount
>> tmpfs on /run/user/0 type tmpfs
>> (rw,nosuid,nodev,relatime,size=32923772k,nr_inodes=8230943,mode=700)
>> resctrl on /sys/fs/resctrl type resctrl (rw,relatime)
>>
>> root@localhost:~# umount resctrl
>> umount: /sys/fs/resctrl: no mount point specified.
>>
>> root@localhost:~# mount
>> tmpfs on /run/user/0 type tmpfs
>> (rw,nosuid,nodev,relatime,size=32923772k,nr_inodes=8230943,mode=700)
>>
>> root@localhost:~# mount -t resctrl resctrl /test
>> mount: /test: unknown filesystem type 'resctrl'.
> 
> 
> Thanks for trying this out.
> 
> I guess the behaviour here might want a bit more thought.
> 
> I'm not too keen on us leaving a defective mount in the namespace,
> with a nonexistent mount pount.  I'm wondering whether things like
> systemd may get confused by this...
> 
> Cheers
> ---Dave

On Wed, May 01, 2024 at 09:51:51PM +0530, Amit Singh Tomar wrote:
> 
> > > > I think James will need to comment on this, but I think that yes, it
> > > > is probably appropriate to require a reboot.  I think an MPAM error
> > > > interrupt should only happen if the software did something wrong, so
> > > > it's a bit like hitting a BUG(): we don't promise that everything works
> > > > 100% properly until the system is restarted.  Misbehaviour should be
> > > > contained to MPAM though.
> > > > 
> > > if "resctrl" is nonfunctional in this state, then this comment[1] here does
> > > *not* make sense.
> > > 
> > > "restore any modified controls to their reset values."
> > 
> > Can you clarify what you mean here?
> 
> What I meant was, What's the rationale behind restoring the modified
> controls, if user is going to restart the system anyways (in order to use
> MPAM again), but later realized that it is needed so that *non* MPAM loads
> (user may still want to run other things even after MPAM error interrupt)
> would not have any adverse effect with modified controls.
> 
> Therefore, taking my statement back.

Ack: we can't force the system to restart without losing data.  Really,
the decision about when and whether to attempt a graceful shutdown or
reboot should be left to userspace.  But until userspace does shut down
the system, we do our best to behave as if the broken part of the system
(MPAM) were not present at all.

[...]

Cheers
---Dave

Hi guys,

On 02/05/2024 16:58, Dave Martin wrote:
> On Wed, May 01, 2024 at 09:51:51PM +0530, Amit Singh Tomar wrote:
>>>>> I think James will need to comment on this, but I think that yes, it
>>>>> is probably appropriate to require a reboot.  I think an MPAM error
>>>>> interrupt should only happen if the software did something wrong, so
>>>>> it's a bit like hitting a BUG(): we don't promise that everything works
>>>>> 100% properly until the system is restarted.  Misbehaviour should be
>>>>> contained to MPAM though.

Indeed - all the reasons for the MPAM error interrupt being triggered indicate a software
bug, so re-mounting resctrl with the same buggy code isn't going to fix anything.


>>>> if "resctrl" is nonfunctional in this state, then this comment[1] here does
>>>> *not* make sense.
>>>>
>>>> "restore any modified controls to their reset values."

The MPAM driver goes on to reset all the MPAM hardware to the best of its ability.
These means everything gets set back to 100%, so its as if MPAM is not implemented.
This is better than throttling the wrong task because an out-of-range PARTID for
${important_task} is using the configuration of ${background_process}...


>>> Can you clarify what you mean here?
>>
>> What I meant was, What's the rationale behind restoring the modified
>> controls, if user is going to restart the system anyways (in order to use
>> MPAM again),  but later realized that it is needed so that *non* MPAM loads>> (user may still want to run other things even after MPAM error interrupt)
>> would not have any adverse effect with modified controls.
>>
>> Therefore, taking my statement back.
> 
> Ack: we can't force the system to restart without losing data.  Really,
> the decision about when and whether to attempt a graceful shutdown or
> reboot should be left to userspace.  But until userspace does shut down
> the system, we do our best to behave as if the broken part of the system
> (MPAM) were not present at all.

Dave's systemd choking on this angle is interesting - I'll go experiment with this.

The alternative here is to delete the __exit text completely as it can't be run, and
instead get MPAM's error interrupt to disable the static-keys and return -EIO for every
call into the arch code.
I didn't do this as its likely to cause extra churn to ensure that every arch helper can
propagate errors back to user-space, and this seemed like a good (re-)use of existing code.

The third option was to not do anything in MPAM, and just print a message to say bad
things might be happening. Given its extra work for hardware to detect the error
conditions, I previously assumed no-one would do this, and hardware would just 'go wrong'
instead... but as someone has built this, it would be good to try and react to it.


Thanks,

James