Re: [Qemu-devel] [PATCH 2/3] cputlb: serialize tlb updates with env->tlb_lock

[Emilio G. Cota]

On Wed, Oct 03, 2018 at 17:52:32 +0200, Paolo Bonzini wrote:
> On 03/10/2018 17:48, Emilio G. Cota wrote:
> >> it's probably best to do all atomic_set instead of just the memberwise 
> >> copy.
> > Atomics aren't necessary here, as long as the copy is protected by the
> > lock. This allows other vCPUs to see a consistent view of the data (since
> > they always acquire the TLB lock), and since copy_tlb is only called
> > by the vCPU that owns the TLB, regular reads from this vCPU will always
> > see consistent data.
> 
> For reads I agree, but you may actually get a torn read if the writer
> doesn't use atomic_set.

But you cannot get a torn read if all reads that don't hold the lock
are coming from the same thread that performed the write.

> That's because in order to avoid UB all reads or writes that are
> concurrent with another write must be atomic, and the write must be
> atomic too.

Agreed, and that's why there's a patch adding atomic_read to all
.addr_write reads not protected by the lock.

> The lock does prevent write-write concurrent accesses, but
> not read-write, so the write must be atomic here.

But here we have no concurrent read-write, because the non-owner
thread always holds the lock, and the owner thread doesn't need
to synchronize with itself.

I'm appending a small example that might help make my point;
.addr_write corresponds to .written_by_all, and the other
fields correspond to .written_by_owner.

Thanks,

                E.

---
/*
 * baz.c
 * gcc -O3 -fsanitize=thread -o baz baz.c
 */
#include <pthread.h>
#include <stdlib.h>
#include <string.h>
#include <stdio.h>

#define atomic_read(ptr)   __atomic_load_n(ptr, __ATOMIC_RELAXED)
#define atomic_set(ptr, i) __atomic_store_n(ptr, i, __ATOMIC_RELAXED)

#define N_ITER 1000000

struct entry {
        int written_by_all;
        int written_by_owner;
};

static struct entry entry;
static pthread_mutex_t lock;

void *owner_thread(void *arg)
{
        int i;

        for (i = 0; i < N_ITER; i++) {
                if (i % 2) {
                        volatile int a __attribute__((unused));
                        volatile int b __attribute__((unused));

                        a = atomic_read(&entry.written_by_all);
                        b = entry.written_by_owner;
                } else {
                        pthread_mutex_lock(&lock);
                        entry.written_by_all++;
                        entry.written_by_owner++;
                        pthread_mutex_unlock(&lock);
                }
        }
        return NULL;
}

void *other_thread(void *arg)
{
        int i;

        for (i = 0; i < N_ITER; i++) {
                pthread_mutex_lock(&lock);
                atomic_set(&entry.written_by_all, entry.written_by_all + 1);
                pthread_mutex_unlock(&lock);
        }
        return NULL;
}

int main(int argc, char *argv[])
{
        pthread_t threads[2];
        int i;

        pthread_mutex_init(&lock, NULL);
        if (pthread_create(&threads[0], NULL, owner_thread, NULL)) {
                exit(1);
        }
        if (pthread_create(&threads[1], NULL, other_thread, NULL)) {
                exit(1);
        }
        for (i = 0; i < 2; i++) {
                pthread_join(threads[i], NULL);
        }
        return 0;
}