Atomic instructions lr/sc do not work on RISC-V

[xobs]

Description

Atomic code allows for synchronization between units of execution. On RISC-V, this can include processors ("harts") and threads. RISC-V follows the "reserve-and-update" method of atomic operations as opposed to the x86 approach of "cmpxchg". That is, the lr instruction places a Reservation, then the sc instruction updates that value if and only if the reservation is still valid.

tlib contains several functions that have the following pattern:

void reserve_address(struct CPUState *env, target_phys_addr_t address)
{
    if (env->atomic_memory_state->number_of_registered_cpus == 1) {
        // if there is just one cpu, return ok status
        return;
    }

    reserve_address_inner(cpu, address, 0);
}

That is, the reservation mechanism is completely turned off when there is only one CPU.

This is incorrect. Reservations should still be present even on single-core systems. This is because a context switch in the middle of the "lr" and "sc" instructions will result in an incorrect state.

Expected behaviour

The following Rust program should work correctly:

use core::sync::atomic::{AtomicUsize, Ordering::SeqCst};
use std::thread;

const LOOPS_PER_THREAD: usize = 2_000_000_000;
const THREAD_COUNT: usize = 2;

const LOCKED: usize = 76;
const UNLOCKED: usize = 1326;

static TEST_ATOM: AtomicUsize = AtomicUsize::new(UNLOCKED);

fn atom_test(_id: usize, count: usize) {
    for _idx in 0..count {
        while TEST_ATOM
            .compare_exchange(UNLOCKED, LOCKED, SeqCst, SeqCst)
            .is_err()
        {
            thread::yield_now();
        }
        assert_eq!(
            TEST_ATOM.swap(UNLOCKED, SeqCst),
            LOCKED,
            "Thread {} failed after {} tries",
            _id,
            _idx
        );
    }
}

fn main() {
    println!("Starting atom test...");
    let mut joiners = vec![];

    for tid in 0..THREAD_COUNT {
        joiners.push(thread::spawn(move || atom_test(tid, LOOPS_PER_THREAD)));
    }
    for j in joiners {
        j.join().unwrap();
    }

    println!("Done!");
}

How to reproduce?

I'm not sure how best to reproduce. I can provide a machine and a binary file. The above code is from a test I put together using the standard Xous libstd code.

In theory this is a C translation of the code above, but I can't get it to reproduce with an existing Linux rv32 image:

#include <stdio.h>
#include <stdatomic.h>
#include <pthread.h>
#include <stdlib.h>

#include <sched.h>

#define LOOPS_PER_THREAD 2000000000
#define THREAD_COUNT 2

static unsigned int LOCKED = 76;
static unsigned int UNLOCKED = 1326;

static atomic_uint TEST_ATOM;

void *atom_test(void *arg)
{
    size_t id = (size_t)arg;
    for (size_t idx = 0; idx < LOOPS_PER_THREAD; ++idx)
    {
        while (!atomic_compare_exchange_weak(&TEST_ATOM, &UNLOCKED, LOCKED))
        {
            sched_yield(); // // pthread_yield(); doesn't seem to be defined
        }

        if ((idx % 10000000) == 0)
        {
            printf("Thread %zu completed %zu loops\n", id, idx);
        }

        int prev_value = atomic_exchange(&TEST_ATOM, UNLOCKED);
        if (prev_value != LOCKED)
        {
            fprintf(stderr, "Thread %zu failed after %zu tries\n", id, idx);
            exit(1);
        }
    }
    return NULL;
}

int main()
{
    printf("Starting atom test...\n");
    pthread_t threads[THREAD_COUNT];
    atomic_init(&TEST_ATOM, UNLOCKED);

    for (size_t tid = 0; tid < THREAD_COUNT; ++tid)
    {
        if (pthread_create(&threads[tid], NULL, atom_test, (void *)tid))
        {
            fprintf(stderr, "Error creating thread %zu\n", tid);
            return 1;
        }
    }

    for (int j = 0; j < THREAD_COUNT; ++j)
    {
        if (pthread_join(threads[j], NULL))
        {
            fprintf(stderr, "Error joining thread %d\n", j);
            return 1;
        }
    }

    printf("Done!\n");
    return 0;
}

Environment

Please, provide the following information:

• OS: Linux and Windows
• Renode version (with commit SHA): a76dac0aecdd7b5354218867ad93fb655901abd3

Additional information

The following patch appears to fix the issue, though more testing will be required:

diff --git a/atomic.c b/atomic.c
index 667a39b..40d9c60 100644
--- a/atomic.c
+++ b/atomic.c
@@ -222,20 +222,20 @@ void clear_global_memory_lock(struct CPUState *env)
 // ! this function should be called when holding the mutex !
 void reserve_address(struct CPUState *env, target_phys_addr_t address)
 {
-    if (env->atomic_memory_state->number_of_registered_cpus == 1) {
-        // if there is just one cpu, return ok status
-        return;
-    }
+    // if (env->atomic_memory_state->number_of_registered_cpus == 1) {
+    //     // if there is just one cpu, return ok status
+    //     return;
+    // }

     reserve_address_inner(cpu, address, 0);
 }

 uint32_t check_address_reservation(struct CPUState *env, target_phys_addr_t address)
 {
-    if (env->atomic_memory_state->number_of_registered_cpus == 1) {
-        // if there is just one cpu, return ok status
-        return 0;
-    }
+    // if (env->atomic_memory_state->number_of_registered_cpus == 1) {
+    //     // if there is just one cpu, return ok status
+    //     return 0;
+    // }

     return check_address_reservation_always(cpu, address);
 }
@@ -246,10 +246,10 @@ void register_address_access(struct CPUState *env, target_phys_addr_t address)
         // no atomic_memory_state so no registration needed
         return;
     }
-    if (env->atomic_memory_state->number_of_registered_cpus == 1) {
-        // this is not need when we have only one cpu
-        return;
-    }
+    // if (env->atomic_memory_state->number_of_registered_cpus == 1) {
+    //     // this is not need when we have only one cpu
+    //     return;
+    // }

     ensure_locked_by_me(env);

@@ -264,10 +264,10 @@ void register_address_access(struct CPUState *env, target_phys_addr_t address)

 void cancel_reservation(struct CPUState *env)
 {
-    if (env->atomic_memory_state->number_of_registered_cpus == 1) {
-        // this is not need when we have only one cpu
-        return;
-    }
+    // if (env->atomic_memory_state->number_of_registered_cpus == 1) {
+    //     // this is not need when we have only one cpu
+    //     return;
+    // }

     cancel_reservation_always(env);
 }

Do you plan to address this issue and file a PR?

Unclear. It seems like more things should break if this was the case, but maybe nobody else is running multi-threaded code on RISC-V under Renode?

[mateusz-holenko]

Hi @xobs, thanks for reporting the issue. https://github.com/renode/renode/commit/0f7375c212b680d41f94a295adc4f5e0f7760351 should address the problem, could you double check it works now fine for you?

[PiotrZierhoffer]

As discussed, closing