Add dynamic replication to kernel NR #338
Conversation
| use crate::nrproc::NrProcess; | ||
| use crate::process::MAX_PROCESSES; | ||
|
|
||
| /// Types of shared structures the client can request | ||
| #[derive(Debug, Eq, PartialEq, PartialOrd, Clone, Copy)] | ||
| #[repr(u8)] | ||
| pub enum ShmemStructure { | ||
| // TODO(dynrep): remove NrProcLogs/NrLog add NodeReplicated<Process> and |
There was a problem hiding this comment.
I think we should remove this comment?
| @@ -67,7 +67,6 @@ pub(crate) fn schedule() -> ! { | |||
| // There is no process but we're the "main" thread, | |||
| // aggressively try and advance the replica | |||
| let start = rawtime::Instant::now(); | |||
| crate::nrproc::advance_all(); | |||
| crate::arch::advance_fs_replica(); | |||
There was a problem hiding this comment.
Right now cnrfs isn't using dynamic replication, yes? Is that why advance_fs_replica() remains?
If so, I think that's just fine for now - no urgent need to port to cnrfs for the benchmarks we're looking at.
I'm asking because while the advance_fs_replica() logic remains, the tlb work queues remain large/complicated (#290). So it might be nice to document that implementing this for cnrfs would fix that issue :)
| @@ -31,6 +31,7 @@ fn s04_userspace_multicore() { | |||
| .user_feature("test-scheduler-smp") | |||
| .build(); | |||
| let cmdline = RunnerArgs::new_with_build("userspace-smp", &build) | |||
| .nodes(num_cores / 16) | |||
There was a problem hiding this comment.
Does this no longer work with full cores? It's unclear to me how 16 plays into this test.
| } else { | ||
| cmdline = cmdline.nodes(machine.max_numa_nodes()); | ||
| cmdline = cmdline.nodes(std::cmp::max(machine.max_cores() / 16, machine.max_numa_nodes())); |
There was a problem hiding this comment.
Are we assuming max 16 cores per numa node? skylake4x has 24, so this may not be a solid assumption?
| @@ -834,7 +834,7 @@ fn s10_leveldb_benchmark() { | |||
| } | |||
|
|
|||
| #[test] | |||
| fn s10_memcached_benchmark_internal() { | |||
| fn s10_xmemcached_benchmark_internal() { | |||
| @@ -35,7 +35,7 @@ fn maponly_bencher(cores: usize) { | |||
|
|
|||
| // see process.rs the heap split up by core from slots 1..128, so we start from there | |||
| @@ -512,15 +531,12 @@ fn _start(argc: isize, _argv: *const *const u8) -> isize { | |||
| crate::nrproc::register_thread_with_process_replicas(); | |||
There was a problem hiding this comment.
Previously, the controller initialized the log but didn't have a replica of it's own, so the total number of process replicas was equal to the number of clients. Reading the code, I believe this is still true, but I wanted to confirm.
| @@ -20,6 +20,7 @@ pub(crate) struct ShmemAlloc { | |||
| } | |||
|
|
|||
| impl ShmemAlloc { | |||
| #[allow(dead_code)] | |||
There was a problem hiding this comment.
Do the logs use allocators at all? Can this be deleted entirely?
| } | ||
| process_logs | ||
| }; | ||
| // We want to allocate the logs in controller shared memory |
There was a problem hiding this comment.
I think comment is outdated. We are allocating below from local memory below (local_shmem_affinity())
| pcm.set_mem_affinity(mid_to_shmem_affinity(r)).expect("Can't change affinity"); | ||
| } | ||
| AffinityChange::Revert(_orig) => { | ||
| pcm.set_mem_affinity(local_shmem_affinity()).expect("Can't set affinity") |
There was a problem hiding this comment.
local_shmem_affinity() returns a value relative to where it's run. Also - the memory allocator may have been non-shared originally. So we don't want to automatically revert to shmem regardless. I think we need to capture the actual previous value to do this correctly.
Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
Signed-off-by: Gerd Zellweger <mail@gerdzellweger.com>
gz
force-pushed
the
new-replica-code
branch
from
fe70c07 to
ad53903
Compare
No description provided.