Multi region packing #1599

mkeeter · 2024-01-26T20:07:25Z

This PR modifies the build system to let flash use all available MPU regions, resulting in tighter packing and smaller total image size. This shrinks the sidecar-rev-c image by 256 KiB, and the gimlet-f image by 188 KiB.

In other words, tasks go from having exactly 1 flash region to at least 1 flash region; this is mostly plumbing that change from suggest_memory_region_size all the way through the kconfig.

The change does makes task packing trickier, because tasks can be placed in one of two orientations: largest-chunk-first, or largest-chunk-last. I went for a very dumb O(N^2) algorithm that checks every unplaced task and picks the best; we're far from having > 100 tasks, so I'm not worried about bad scaling (famous last words!).

In addition, it updates xtask/src/sizes.rs to print the individual chunks when the -v flag is specified.

I'm expecting CI to get mad, because some kernels may need more flash space to hold the extra regions.

Before

After

cbiffle

This generally looks good. There are a few areas where I think there's an unmentioned assumption about ordering where I'd like to see some docs (or a type that causes the assumption to be explicit).

cbiffle · 2024-01-29T19:46:47Z

build/xtask/src/sizes.rs

@@ -78,7 +79,10 @@ pub fn run(
        }
        let size = toml.kernel.requires[&mem.to_string()];

-        let suggestion = toml.suggest_memory_region_size("kernel", used);
+        let suggestion = toml.suggest_memory_region_size("kernel", used, 1);
+        assert_eq!(suggestion.len(), 1);


So imagine I'm here in the future and this assertion has failed. What should I look at? Asking because I don't totally understand it as a reviewer. (The answer would make a good assert message or comment.)

Added a string to the assertion in 7d2d02b; this detects a failure in suggest_memory_region_size where it has suggested > 1 region for the kernel, which should never happen.

cbiffle · 2024-01-29T19:49:25Z

sys/kern/build.rs

@@ -292,7 +303,7 @@ fn translate_address(
    task_index: usize,
    address: OwnedAddress,
 ) -> u32 {
-    let key = RegionKey::Owned(task_index, address.region_name);
+    let key = RegionKey::Owned(task_index, 0, address.region_name);


Is this right? It seems simpler than I'd expect.

Yes, although it's a little subtle:

Regions are populated in order from the MultiRegionConfig, so region 0 is going to be the first region in memory (at an address given by MultiRegionConfig::base)

Regions are contiguous, so the offset can be taken from the first region's base address

I added a comment to that effect in 122b0a1

cbiffle · 2024-01-29T19:50:56Z

build/xtask/src/dist.rs

@@ -1311,8 +1338,8 @@ fn generate_task_linker_script(

    writeln!(linkscr, "MEMORY\n{{")?;
    for (name, range) in map {


I think this'd be clearer if the name became ranges, since it's now more than one range.

Yup, done in 05e21fb

cbiffle · 2024-01-29T19:51:35Z

build/xtask/src/dist.rs

@@ -1290,7 +1317,7 @@ fn check_task_priorities(toml: &Config) -> Result<()> {

 fn generate_task_linker_script(
    name: &str,
-    map: &BTreeMap<String, Range<u32>>,
+    map: &BTreeMap<String, Vec<Range<u32>>>,


Does the code below assume this vec is sorted? I notice it accesses first and last.

Same as below, fixed with ContiguousRanges (3d128cb)

cbiffle · 2024-01-29T19:52:04Z

build/xtask/src/dist.rs

+    /// A task may have multiple address ranges in the same memory space for
+    /// efficient packing; if this is the case, the addresses will be contiguous
+    /// and each individual range will respect MPU requirements.
+    pub tasks: BTreeMap<String, BTreeMap<String, Vec<Range<u32>>>>,


(I think this has an implicit assumption about vec order as well, it should go in the comment. Or maybe a BTreeSet would be better than a Vec?)

Yup, I added a ContinguousRanges type which enforces that the ranges are contiguous (3d128cb)

cbiffle · 2024-01-29T19:53:11Z

build/xtask/src/dist.rs

                if let Some(r) = tasks[name].max_sizes.get(&mem.to_string()) {
-                    if bytes > *r as u64 {
+                    let total_bytes = bytes.iter().sum::<u64>();
+                    if total_bytes > *r as u64 {


nit: this looks like a lossy conversion but isn't, consider u64::from(*r)

Good catch, done in 3e2421a

cbiffle · 2024-01-29T19:54:27Z

build/xtask/src/dist.rs

@@ -2082,7 +2248,7 @@ pub fn make_kconfig(
    for (i, (name, task)) in toml.tasks.iter().enumerate() {
        let stacksize = task.stacksize.or(toml.stacksize).unwrap();

-        let flash = &task_allocations[name]["flash"];
+        let flash = &task_allocations[name]["flash"][0];


I kinda feel like every case where we're explicitly accessing the first element of the list should get a comment, because each time I stop and go "uh... wait... is this right or making an assumption"

Yup, fixed with ContiguousRanges in 3d128cb

cbiffle · 2024-01-29T20:08:14Z

I asked @mkeeter for time measurements of the potentially N**2 algorithm here -- execution is currently under a millisecond in every case. So, not worth attempting to optimize the complexity, imo.

build/xtask/src/dist.rs

The kernel will reject leases that span multiple regions, so this PR disables smart packing entirely by forcing each task's flash into a single MPU region (just like before #1599). Otherwise, tasks which loan out their flash memory may produce spurious faults.

mkeeter requested a review from cbiffle January 26, 2024 20:07

mkeeter requested review from flihp and labbott as code owners January 26, 2024 20:30

mkeeter force-pushed the multi-region-packing branch 2 times, most recently from 6c77ace to 86e60eb Compare January 29, 2024 16:00

cbiffle reviewed Jan 29, 2024

View reviewed changes

cbiffle approved these changes Jan 30, 2024

View reviewed changes

build/xtask/src/dist.rs Outdated Show resolved Hide resolved

build/xtask/src/dist.rs Outdated Show resolved Hide resolved

mkeeter added 20 commits February 5, 2024 11:30

MULTI-REGION PACKING?!

1af8afd

Add some checks

79ee996

Fix printing

805a568

Fix kernel build to generate multiple regions

0deb765

Fix Sidecar kernel size

ac9fb31

Fix alignment heuristic to penalize forward blocks

e51b9f9

Bump kernel size a little more

0da5823

More optimization and fixing LPC55 to match baseline

abc99f8

One more heuristic

577419b

Revert Cargo.lock to master (??)

f4e1e6c

One more optimization to reduce kernel flash

637ceb2

Fix whitespace and prints

0b7b873

Add comment to kernel size assertion

187c65e

range -> ranges

93997be

Fix not-actually-lossy conversion

ed553cf

Less cryptic keys

ae316da

Use stronger types

cad54f4

Fix polarity

5aa6a99

More tweaks

b35de0f

More alignment management wrangling

f2bebca

mkeeter force-pushed the multi-region-packing branch from 3dc24d8 to f2bebca Compare February 5, 2024 16:30

More assertions

228786b

More minor fixes

2ee162c

mkeeter merged commit eb8e539 into master Feb 5, 2024
83 checks passed

mkeeter deleted the multi-region-packing branch February 5, 2024 21:25

mkeeter mentioned this pull request Mar 20, 2024

Disable smart packing #1671

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi region packing #1599

Multi region packing #1599

mkeeter commented Jan 26, 2024

cbiffle left a comment

cbiffle Jan 29, 2024

mkeeter Jan 29, 2024

cbiffle Jan 29, 2024

mkeeter Jan 29, 2024

cbiffle Jan 29, 2024

mkeeter Jan 29, 2024

cbiffle Jan 29, 2024

mkeeter Jan 29, 2024

cbiffle Jan 29, 2024

mkeeter Jan 29, 2024

cbiffle Jan 29, 2024

mkeeter Jan 29, 2024

cbiffle Jan 29, 2024

mkeeter Jan 29, 2024

cbiffle commented Jan 29, 2024

		@@ -1311,8 +1338,8 @@ fn generate_task_linker_script(

		writeln!(linkscr, "MEMORY\n{{")?;
		for (name, range) in map {

Multi region packing #1599

Multi region packing #1599

Conversation

mkeeter commented Jan 26, 2024

Before

After

cbiffle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbiffle commented Jan 29, 2024