Adding a test that will test the correctness of snmalloc

[NirjharRoyiitk]

This is a test that tests the correctness of our new allocator snmalloc. This test should run continuosly and should not crash.

The pseudocode is taken from https://fortanix.atlassian.net/wiki/spaces/PLAT/pages/3155820642/Memory+Allocators

We should modify the configurable parameters https://github.com/fortanix/rust-sgx/blob/memory-correctness-test/examples/mem-correctness-test/src/main.rs#L43 and also modify the heap-size parameter in rust-sgx/examples/mem-correctness-test/Cargo.toml file then simply do the following

    cargo run --target x86_64-fortanix-unknown-sgx

[arai-fortanix]

I don't know how Rust vecs interact with the underlying heap allocator, but as a future improvement I think it probably makes sense to exercise resize() on existing vecs to grow and shrink them, in addition to allocating and freeing vecs.

It might also be nice to have command-line arguments or environment variables or some other mechanism for controlling the number of threads and number of iterations without having to recompile the code.

[arai-fortanix]

Is there a reason we're setting debug=false here?

[arai-fortanix]

I have a slight preference for not using 0xdeadbeef as a magic number. This is a popular choice for magic number, so if we're using 3rd party libraries, there's a slightly higher chance of this particular magic number being used by something else, compared to picking some different magic number. Also, 32 bits is pretty small if it's important that we don't encounter the magic number on accident. 64 bits would be better, if that's possible.

Both of these comments are very minor.

[arai-fortanix]

Maybe as a future improvement, it may be a good idea to fill the buffer with pseudo-random data that can be easily checked, rather than by repeating the same bytes over and over. I've worked on memory allocators in the past that had bugs that manifested as data being copied to the wrong location in memory. These bugs were not caught by a test similar to this one that repeated the same magic number repeatedly, because all of the magic numbers were the same. When we changed the test to use pseudo-random (but calculatable) sequences, the test was able to catch the bug.

[arai-fortanix]

Should the comment say "This function selects a random number..." instead of "This buffer selects..."?

[arai-fortanix]

Is there a reason to do this calculation this way instead of just calling get_random_num(1, MAX_BUFFER_CHECKS_PER_THREAD_ITERATION)?

[arai-fortanix]

Use get_random_num(0, array_of_vectors.len() - 1)?

[arai-fortanix]

Does removing the vec from array_of_vectors not cause the vec to be dropped?

[arai-fortanix]

Minor: Maybe this should be named MAX_ALLOWED_USED_HEAP_PERCENTAGE?

[BobbyAtFortanix]

why don't you create an array of tester functions: fn traverse_and_check_buffer(buf: &Vec)
and in the worker thread we just go by the number of elements in the array.
this way we can:

• expand the test variety by adding more operations
• change the test type weight/ratio by adding more functions of the same type
• remove test types, for debug purposes or when they become obsolete, or for whatever other reason
• other fun stuff

[arai-fortanix]

I like this idea (as a future enhancement), but I would add that the usual way to have different weights for different functions is not to duplicate their entries in the table but rather to have an explicit weight associated with each table entry, and then randomly choose an activity taking the weights into account.

[aditijannu]

Would be nice to have an explanation or a one line comment about each of these parameters.

[aditijannu]

Change MAGIC_NUM_TO_CHECK to snake case as it is a dynamic value

[aditijannu]

You can compile this program and address the warnings generated by the rust compiler, I see a lot of unused variables or improvements that can be easily addressed:

warning: unused import: `num_cpus`
  --> src/main.rs:20:5
   |
20 | use num_cpus;
   |     ^^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

warning: unused import: `std::time::Instant`
  --> src/main.rs:26:5
   |
26 | use std::time::Instant;
   |     ^^^^^^^^^^^^^^^^^^

warning: unnecessary parentheses around assigned value
  --> src/main.rs:45:47
   |
45 | const PER_THREAD_PER_BUFFER_MAX_SIZE: usize = (4 * TO_KB);
   |                                               ^         ^
   |
   = note: `#[warn(unused_parens)]` on by default
help: remove these parentheses
   |
45 - const PER_THREAD_PER_BUFFER_MAX_SIZE: usize = (4 * TO_KB);
45 + const PER_THREAD_PER_BUFFER_MAX_SIZE: usize = 4 * TO_KB;
   |

[arai-fortanix]

You can also turn on a setting that makes warning fail the build.

[arai-fortanix]

My intention with better randomization of the fill data was that every element of every vec should get a distinct value. I think that's fine as a future enhancement.

[raoulstrackx]

This and the functions above could be logically grouped in Heap struct. That'd also simplify their names. You could do the same for allocating, freeing and checking allocated chunks.

[raoulstrackx]

You can use a Barrier here

[raoulstrackx]

I think you have the same issue here as in #566: You allocate a Vec assuming each element is a byte, which is not the case

[raoulstrackx]

I liked Dan's comment on magic numbers. You could even calculate a checksum instead

[raoulstrackx]

This is confusing as get_num_processors doesn't return the number of processors on the machine

[NirjharRoyiitk]

As an optimization I will fill up this function with so that it returns number of processors. Currently it returns a hardcoded value

[raoulstrackx]

Yes but if it's hardcoded, there's no need to have this function

[raoulstrackx]

Can the minimum be 1 byte?

[NirjharRoyiitk]

I initially tried with 1 byte but since the vector is of of u64, I kept the minimum size to be 16 bytes

[raoulstrackx]

Yes that type probably comes from how you compare it with a magic number. You could use a checksum instead

[raoulstrackx]

Can you interact with the memory allocator directly? Vec may internally do some optimizations. For example, allocating more than requested. That'd mess up the stress test for the allocator

[NirjharRoyiitk]

I haven't tried interacting with the allocator directly. Do you want me to try this in this PR? @raoulstrackx

[raoulstrackx]

Yes, please

[raoulstrackx]

Pretty much all this code could also be executed on almost all targets the Rust compiler supports, except some statements/functions like these. You could restrict these to only the SGX target with:

#[cfg(target_env = "sgx")]
extern "C" {
    static HEAP_BASE: u64;
    static HEAP_SIZE: usize;
}

That way you can develop and test under Linux, and if that works, try it within an enclave.