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

["Emilio G. Cota" <cota@braap.org>]

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;
}