Re: Question about atomics

[Warner Losh]

On Tue, Mar 8, 2022 at 9:29 AM Warner Losh <imp@bsdimp.com> wrote:

On Tue, Mar 8, 2022 at 7:26 AM Paolo Bonzini <pbonzini@redhat.com> wrote:

On 3/8/22 15:09, Warner Losh wrote:
>
>     Yes, qatomic_foo is expected to work.  It's what we use across
>     threads, and it is expected to work "in kernel mode", i.e. within cpu_loop().
>
> Even when the writers are done in the context of system calls to the kernel?

Yes.

That said, for the similar syscall in Linux we just forward it to the
kernel (and the kernel obviously can only do an atomic---no
start_exclusive/end_exclusive involved).

OK. It seemed similar to futex, but I didn't know if that worked because

it restricted itself, or because all atomics worked when used from the

kernel :)

So how do you handle multiple writers? I think I've found a way they

can race, but I need a sanity check to make sure I'm understanding

correctly.

FreeBSD's pthread_mutex is shared between the kernel and user land.

So it does a compare and set to take the lock. Uncontested and unheld

locks will mean we've taken the lock and return. Contested locks

are kicked to the kernel to wait. When userland releases the lock

it signals the kernel to wakeup via a system call. The kernel then

does a cas to try to acquire the lock. It either returns with the lock

held, or goes back to sleep. This we have atomics operating both in

the kernel (via standard host atomics) and userland atomics done

via start/end_exclusive. So I'm struggling with how the start/end_exclsuive

interacts with the cas from the kernel. the kernel can modify the value

after tcg has read the old value before it's goes to set it thinking

that it's OK.

Basically. Lock is unlocked, so it has '0' in the owner field, which is

the oldval for the cas operation.

Thread 1 (tcg inside of start_exclusive)

     old = *(uint64_t *)g2h(cs, addr);
     if (old == oldval)

/*                  kernel does the cas here, finds it uncontested and stores it's ownership */

         *(uint64_t *)g2h(cs, addr) = new; /* now the kernel value is overwritten, two threads think they own the lock */

Or am I missing something there?

This doesn't need to necessarily work, but I'm trying to understand if I understand

the race well enough (and if so, I'll need to do something else to implement

these things).

Warner

 

> And if the system call does this w/o using
> the start_exclusive/end_exclusive stuff, is that a problem?

If it does it without start_exclusive/end_exclusive they must use
qatomic_foo().

So this answer is in the context *-user implementing a system call

that's executed as a callout from cpu_loop()? Or does the kernel

have to use the C11 atomics that qatomic_foo() is based on... I'm

thinking the former based on the above, but want to make sure.

 

If it does it with start_exclusive/end_exclusive, they
can even write a compare-and-exchange as

     old = *(uint64_t *)g2h(cs, addr);
     if (old == oldval)
         *(uint64_t *)g2h(cs, addr) = new;

 Nice.

The test program that's seeing corrupted mutex state is just two threads. I've simplified

it a bit (it's an ATF test, and there's a lot of boilerplate that I removed, including validating

the return values). It looks pretty straight forward. Often it will work, sometimes, though

it fails with an internal assertion in what implements pthread_mutex about state that's

not possible w/o the atomics/system calls that implement the pthread_mutex failing to

work.

Warner

P.S. Here's the code for reading purposes... W/o the headers it won't compile and the

ATF stuff at the end comes from elsewhere...

static pthread_mutex_t static_mutex = PTHREAD_MUTEX_INITIALIZER;

static int global_x;

bool thread3_started = false;

static void *
mutex3_threadfunc(void *arg) {
        long count = *(int *)arg;

        thread3_started = true;

        while (count--) {
                pthread_mutex_lock(&static_mutex);
                global_x++;
                pthread_mutex_unlock(&static_mutex);
        }

}

int main(int argc, char **argv) {

        int count, count2;

        pthread_t new;
        void *joinval;

        global_x = 0;

        count = count2 = 1000;

        pthread_mutex_lock(&static_mutex);
        pthread_create(&new, NULL, mutex3_threadfunc, &count2);

        while (!thread3_started) {
                /* Wait for thread 3 to start to increase chance of race */
        }

       pthread_mutex_unlock(&static_mutex);

       while (count--) {
                pthread_mutex_lock(&static_mutex);
                global_x++;
                pthread_mutex_unlock(&static_mutex);
        }

        pthread_join(new, &joinval);

        pthread_mutex_lock(&static_mutex);
        ATF_REQUIRE_EQ_MSG(count, -1, "%d", count);
        ATF_REQUIRE_EQ_MSG((long)joinval, -1, "%ld", (long)joinval);
        ATF_REQUIRE_EQ_MSG(global_x, 1000 * 2, "%d vs %d", globaol_x,
            1000 * 2);

}