kernel: configure MPU only if switching to a different process

[alphan]

Pull Request Overview

Currently, the scheduler configures the MPU every time when it switches
to a process. This change aims to improve syscall latency by caching the
last running process and configuring the MPU only when switching to a
different process.

See also: #1730.

Testing Strategy

I ran a simple libtock-rs application for measuring syscall latency with
and without this change on opentitan on verilator. This change reduces the
latency of a command syscall by 2751 1711 cycles (the exact number of cycles
saved obviously depends on the actual implementation, e.g. would probably
be less after #1821).

Also ran make allcheck.

TODO or Help Wanted

I couldn't find any doc files or tests that should be updated. Since
this is my first real Tock PR, it would be great if you could point me
in the right direction if I missed anything.

Documentation Updated

• Updated the relevant files in /docs, or no updates are required.

Formatting

• Ran make formatall.

[ppannuto]

Welcome! And thanks for the PR!

I don't think there are any docs or tests that need updating for this. Did your testing include running multiple apps as well?

---

Generally, I think this is a good change. However, I'm concerned there may be some extra considerations where we may need to identify the MPU configuration as "tainted". Specifically:

• if there are additional grant allocations, then the deny region may need to grow (though, this is likely not an immediate issue as the allowed region is generally minimal)
• if there was a change in permitted region, i.e. the application called brk or an IPC allow* then these need to be recalculated
• Maybe others I'm not thinking of?

Basically, I think this is a good common-case optimization, but I think we also need a way for something to "poison" the MPU configuration to force it to be recalculated.

*though that second case may actually be fine since in the current architecture as it's always a different process that is allowing a region, so we'll go through regular process switch

[bradjc]

We would need to update the kernel/src/platform/mpu.rs documentation, particularly to clarify that the MPU implementation must be setup to only enforce in the userspace privilege level.

[alphan]

Did your testing include running multiple apps as well?

Yes, I also tried this with multiple apps and didn't notice any issues.

Generally, I think this is a good change. However, I'm concerned there may be some extra considerations where we may need to identify the MPU configuration as "tainted". Specifically:

• if there are additional grant allocations, then the deny region may need to grow (though, this is likely not an immediate issue as the allowed region is generally minimal)
• if there was a change in permitted region, i.e. the application called brk or an IPC allow* then these need to be recalculated
• Maybe others I'm not thinking of?

Basically, I think this is a good common-case optimization, but I think we also need a way for something to "poison" the MPU configuration to force it to be recalculated.

*though that second case may actually be fine since in the current architecture as it's always a different process that is allowing a region, so we'll go through regular process switch

Thank you for catching this! I actually looked at various places that can change the MPU config but missed at least one case where MPU config can be modified and not applied immediately, e.g. sbrk and brk eventually call configure_mpu() but alloc doesn't. Since cache invalidation is tricky, does it make sense to modify MPU and MPUConfig to keep track of the taintedness of MPUConfig at a higher level?

[ppannuto]

Since cache invalidation is tricky, does it make sense to modify MPU and MPUConfig to keep track of the taintedness of MPUConfig at a higher level?

Yeah, I think that's a great idea. I believe the mpu configuration structure is already well-equipped for this as all of its state is private and can only be modified by getters and setters; hooking the latter to mark the MPU configuration as dirty, and then perhaps updating configure_mpu to know what process it's configured for, pushing the hot-path logic into the MPU itself seems like a good design to me.

In that case, I actually imagine we could remove the configure_mpus that are sprinkled around, instead architecting things such that whenever a process is returned to the MPU layer is consulted first, updating itself if necessary.

[alphan]

• Reverted changes in kernel/src/sched.rs.
• Added app_id parameter to configure_mpu.
• Implemented the idea captured in this PR as a risc-v-only optimization in arch/rv32i/src/pmp.rs.
• Rebased on master.
• Also made disable_mpu a no-op for a more accurate latency measurement (moved its contents to PMPConfig::clear_mpu_registers, called from configure_mpu to stay close to current behavior). It looks like we don't need this function at all but I am deferring this change to Possible RISC-V PMP improvements #1860, which I created to keep track of the things that I noticed while working on this.

[ppannuto]

This looks good to me!

I added the implementation for ARM as well. It's a bit lighter weight there as ARM already largely did this by cacheing the region calculation result. However, this is still a case where we can trade 8 memory writes for 1 memory compare on the hot path, so likely worth it.

---

Re #1654: This adds a feature from the upcoming Option API. It's trivial to re-implement ourselves if the manages to become the last blocking feature, so I think there's little harm in using the official API for now.

[alphan]

@ppannuto, thank you for reviewing this change and your comments. I have a question regarding the changes in arch/cortex-m3/src/mpu.rs. Shouldn't hardware_is_configured_for be defined in MPU (to track the state of the hw)? Also, don't we need a is_dirty field in CortexMConfig?

[ppannuto]

Hmm.., you're right we need some form of structure that is the global to the MPU hardware. The MPU configuration is stored per process (here), so doing dirty tracking in the MpuConfig type won't be sufficient for resetting the MPU when we change processes.

I don't think the CortexMConfig needs an explicit dirty flag. Instead, the cell indicates what application the hardware "is configured for"; whenever the MPU configuration is changed, that cell is cleared to indicate that the MPU is not configured correctly for any application, which acts similarly to a separate dirty signal.

[alphan]

The MPU struct that I was referring to is the one defined here. IIUC, this should track the state of the hardware, i.e. hardware_is_configured_for, while CortexMConfig tracks if it is dirty or not.

[alphan]

@ppannuto, 121b33f LGTM, thanks! I also pushed a commit to fix the way last_configured_for is used in configure_mpu.

[ppannuto]

This all looks good to me; looking forward to the rest of the #1860 series.

[alphan]

Reverted changes in disable_mpu to address @alistair23's comments. We should see the expected latency improvements when do_process stops clearing PMP registers every time it returns to kernel (#1873).

[alphan]

Thank you for reviewing this @bradjc and @silvestrst!

Added comments to address @silvestrst's comments.

[ppannuto]

bors r+

[bradjc]

big queue i guess?