[PATCH 1/4] kvm tools: Use correct value for user signal base

[PATCH 1/4] kvm tools: Use correct value for user signal base

[Sasha Levin]

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/include/kvm/kvm.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/kvm/include/kvm/kvm.h b/tools/kvm/include/kvm/kvm.h
index f951f2d..6a17362 100644
--- a/tools/kvm/include/kvm/kvm.h
+++ b/tools/kvm/include/kvm/kvm.h
@@ -11,7 +11,7 @@
 #define KVM_32BIT_GAP_SIZE	(512 << 20)
 #define KVM_32BIT_GAP_START	((1ULL << 32) - KVM_32BIT_GAP_SIZE)
 
-#define SIGKVMEXIT		(SIGUSR1 + 2)
+#define SIGKVMEXIT		(SIGRTMIN + 0)
 
 struct kvm {
 	int			sys_fd;		/* For system ioctls(), i.e. /dev/kvm */
-- 
1.7.5.rc3

[PATCH 2/4] kvm tools: Allow pausing guests

[Sasha Levin]

Allow pausing and unpausing guests running on the host.
Pausing a guest means that none of the VCPU threads are running
KVM_RUN until they are unpaused.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/include/kvm/kvm-cpu.h |    1 +
 tools/kvm/include/kvm/kvm.h     |    4 +++
 tools/kvm/kvm-cpu.c             |   18 +++++++++++----
 tools/kvm/kvm-run.c             |    4 +-
 tools/kvm/kvm.c                 |   45 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 65 insertions(+), 7 deletions(-)

diff --git a/tools/kvm/include/kvm/kvm-cpu.h b/tools/kvm/include/kvm/kvm-cpu.h
index b2b6fce..4d99246 100644
--- a/tools/kvm/include/kvm/kvm-cpu.h
+++ b/tools/kvm/include/kvm/kvm-cpu.h
@@ -23,6 +23,7 @@ struct kvm_cpu {
 	struct kvm_msrs		*msrs;		/* dynamically allocated */
 
 	u8			is_running;
+	u8			paused;
 };
 
 struct kvm_cpu *kvm_cpu__init(struct kvm *kvm, unsigned long cpu_id);
diff --git a/tools/kvm/include/kvm/kvm.h b/tools/kvm/include/kvm/kvm.h
index 6a17362..d22a849 100644
--- a/tools/kvm/include/kvm/kvm.h
+++ b/tools/kvm/include/kvm/kvm.h
@@ -12,6 +12,7 @@
 #define KVM_32BIT_GAP_START	((1ULL << 32) - KVM_32BIT_GAP_SIZE)
 
 #define SIGKVMEXIT		(SIGRTMIN + 0)
+#define SIGKVMPAUSE		(SIGRTMIN + 1)
 
 struct kvm {
 	int			sys_fd;		/* For system ioctls(), i.e. /dev/kvm */
@@ -50,6 +51,9 @@ bool kvm__emulate_io(struct kvm *kvm, u16 port, void *data, int direction, int s
 bool kvm__emulate_mmio(struct kvm *kvm, u64 phys_addr, u8 *data, u32 len, u8 is_write);
 bool kvm__register_mmio(u64 phys_addr, u64 phys_addr_len, void (*kvm_mmio_callback_fn)(u64 addr, u8 *data, u32 len, u8 is_write));
 bool kvm__deregister_mmio(u64 phys_addr);
+void kvm__pause(void);
+void kvm__continue(void);
+void kvm__notify_paused(void);
 
 /*
  * Debugging
diff --git a/tools/kvm/kvm-cpu.c b/tools/kvm/kvm-cpu.c
index de0591f..81f479f 100644
--- a/tools/kvm/kvm-cpu.c
+++ b/tools/kvm/kvm-cpu.c
@@ -383,11 +383,15 @@ void kvm_cpu__run(struct kvm_cpu *vcpu)
 		die_perror("KVM_RUN failed");
 }
 
-static void kvm_cpu_exit_handler(int signum)
+static void kvm_cpu_signal_handler(int signum)
 {
-	if (current_kvm_cpu->is_running) {
-		current_kvm_cpu->is_running = false;
-		pthread_kill(pthread_self(), SIGKVMEXIT);
+	if (signum == SIGKVMEXIT) {
+		if (current_kvm_cpu->is_running) {
+			current_kvm_cpu->is_running = false;
+			pthread_kill(pthread_self(), SIGKVMEXIT);
+		}
+	} else if (signum == SIGKVMPAUSE) {
+		current_kvm_cpu->paused = 1;
 	}
 }
 
@@ -400,12 +404,16 @@ int kvm_cpu__start(struct kvm_cpu *cpu)
 
 	pthread_sigmask(SIG_BLOCK, &sigset, NULL);
 
-	signal(SIGKVMEXIT, kvm_cpu_exit_handler);
+	signal(SIGKVMEXIT, kvm_cpu_signal_handler);
+	signal(SIGKVMPAUSE, kvm_cpu_signal_handler);
 
 	kvm_cpu__setup_cpuid(cpu);
 	kvm_cpu__reset_vcpu(cpu);
 
 	for (;;) {
+		if (cpu->paused)
+			kvm__notify_paused();
+
 		kvm_cpu__run(cpu);
 
 		switch (cpu->kvm_run->exit_reason) {
diff --git a/tools/kvm/kvm-run.c b/tools/kvm/kvm-run.c
index 48b8e70..761ac0d 100644
--- a/tools/kvm/kvm-run.c
+++ b/tools/kvm/kvm-run.c
@@ -47,8 +47,8 @@
 #define MIN_RAM_SIZE_MB		(64ULL)
 #define MIN_RAM_SIZE_BYTE	(MIN_RAM_SIZE_MB << MB_SHIFT)
 
-static struct kvm *kvm;
-static struct kvm_cpu *kvm_cpus[KVM_NR_CPUS];
+struct kvm *kvm;
+struct kvm_cpu *kvm_cpus[KVM_NR_CPUS];
 __thread struct kvm_cpu *current_kvm_cpu;
 
 static u64 ram_size;
diff --git a/tools/kvm/kvm.c b/tools/kvm/kvm.c
index 1d756e0..1e22566 100644
--- a/tools/kvm/kvm.c
+++ b/tools/kvm/kvm.c
@@ -6,6 +6,8 @@
 #include "kvm/interrupt.h"
 #include "kvm/mptable.h"
 #include "kvm/util.h"
+#include "kvm/mutex.h"
+#include "kvm/kvm-cpu.h"
 
 #include <linux/kvm.h>
 
@@ -25,6 +27,7 @@
 #include <stdio.h>
 #include <fcntl.h>
 #include <time.h>
+#include <sys/eventfd.h>
 
 #define DEFINE_KVM_EXIT_REASON(reason) [reason] = #reason
 
@@ -68,6 +71,10 @@ struct {
 	{ DEFINE_KVM_EXT(KVM_CAP_EXT_CPUID) },
 };
 
+extern struct kvm_cpu *kvm_cpus[KVM_NR_CPUS];
+static int pause_event;
+static DEFINE_MUTEX(pause_lock);
+
 static bool kvm__supports_extension(struct kvm *kvm, unsigned int extension)
 {
 	int ret;
@@ -575,3 +582,41 @@ void kvm__dump_mem(struct kvm *kvm, unsigned long addr, unsigned long size)
 				  p[n + 4], p[n + 5], p[n + 6], p[n + 7]);
 	}
 }
+
+void kvm__pause(void)
+{
+	int i, paused_vcpus = 0;
+
+	mutex_lock(&pause_lock);
+
+	pause_event = eventfd(0, 0);
+	if (pause_event < 0)
+		die("Failed creating pause notification event");
+	for (i = 0; i < kvm.nrcpus; i++)
+		pthread_kill(kvm_cpus[i]->thread, SIGKVMPAUSE);
+
+	while (paused_vcpus < kvm.nrcpus) {
+		u64 cur_read;
+
+		if (read(pause_event, &cur_read, sizeof(cur_read)) < 0)
+			die("Failed reading pause event");
+		paused_vcpus += cur_read;
+	}
+	close(pause_event);
+}
+
+void kvm__continue(void)
+{
+	mutex_unlock(&pause_lock);
+}
+
+void kvm__notify_paused(void)
+{
+	u64 p = 1;
+
+	if (write(pause_event, &p, sizeof(p)) < 0)
+		die("Failed notifying of paused VCPU.");
+
+	mutex_lock(&pause_lock);
+	mutex_unlock(&pause_lock);
+}
-- 
1.7.5.rc3

[PATCH 3/4] kvm tools: Add a brlock

[Sasha Levin]

brlock is a lock which is very cheap for reads, but very expensive
for writes.
This lock will be used when updates are very rare and reads are
common.
This lock is currently implemented by stopping the guest while
performing the updates. We assume that the only threads which
read from the locked data are VCPU threads, and the only writer
isn't a VCPU thread.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/include/kvm/brlock.h |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
 create mode 100644 tools/kvm/include/kvm/brlock.h

diff --git a/tools/kvm/include/kvm/brlock.h b/tools/kvm/include/kvm/brlock.h
new file mode 100644
index 0000000..f192071
--- /dev/null
+++ b/tools/kvm/include/kvm/brlock.h
@@ -0,0 +1,12 @@
+#ifndef KVM__BRLOCK_H
+#define KVM__BRLOCK_H
+
+#include "kvm/kvm.h"
+#include "kvm/barrier.h"
+
+#define br_read_lock()		mb()
+#define br_read_unlock()	mb()
+
+#define br_write_lock()		kvm__pause()
+#define br_write_unlock()	kvm__continue()
+#endif
-- 
1.7.5.rc3

[PATCH 4/4] kvm tools: Use brlock in MMIO and IOPORT

[Sasha Levin]

Use brlock to protect mmio and ioport modules and make them
update-safe.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/ioport.c |   10 +++++++++-
 tools/kvm/mmio.c   |   17 +++++++++++++++--
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/tools/kvm/ioport.c b/tools/kvm/ioport.c
index d0a1aa8..e00fb59 100644
--- a/tools/kvm/ioport.c
+++ b/tools/kvm/ioport.c
@@ -2,7 +2,7 @@
 
 #include "kvm/kvm.h"
 #include "kvm/util.h"
-
+#include "kvm/brlock.h"
 #include "kvm/rbtree-interval.h"
 #include "kvm/mutex.h"
 
@@ -84,6 +84,7 @@ u16 ioport__register(u16 port, struct ioport_operations *ops, int count, void *p
 {
 	struct ioport *entry;
 
+	br_write_lock();
 	if (port == IOPORT_EMPTY)
 		port = ioport__find_free_port();
 
@@ -105,6 +106,8 @@ u16 ioport__register(u16 port, struct ioport_operations *ops, int count, void *p
 
 	ioport_insert(&ioport_tree, entry);
 
+	br_write_unlock();
+
 	return port;
 }
 
@@ -127,6 +130,7 @@ bool kvm__emulate_io(struct kvm *kvm, u16 port, void *data, int direction, int s
 	bool ret = false;
 	struct ioport *entry;
 
+	br_read_lock();
 	entry = ioport_search(&ioport_tree, port);
 	if (!entry)
 		goto error;
@@ -141,11 +145,15 @@ bool kvm__emulate_io(struct kvm *kvm, u16 port, void *data, int direction, int s
 			ret = ops->io_out(entry, kvm, port, data, size, count);
 	}
 
+	br_read_unlock();
+
 	if (!ret)
 		goto error;
 
 	return true;
 error:
+	br_read_unlock();
+
 	if (ioport_debug)
 		ioport_error(port, data, direction, size, count);
 
diff --git a/tools/kvm/mmio.c b/tools/kvm/mmio.c
index ef986bf..a57abeb 100644
--- a/tools/kvm/mmio.c
+++ b/tools/kvm/mmio.c
@@ -1,5 +1,6 @@
 #include "kvm/kvm.h"
 #include "kvm/rbtree-interval.h"
+#include "kvm/brlock.h"
 
 #include <stdio.h>
 #include <stdlib.h>
@@ -55,41 +56,53 @@ static const char *to_direction(u8 is_write)
 bool kvm__register_mmio(u64 phys_addr, u64 phys_addr_len, void (*kvm_mmio_callback_fn)(u64 addr, u8 *data, u32 len, u8 is_write))
 {
 	struct mmio_mapping *mmio;
+	int ret;
 
 	mmio = malloc(sizeof(*mmio));
 	if (mmio == NULL)
 		return false;
 
+	br_write_lock();
 	*mmio = (struct mmio_mapping) {
 		.node = RB_INT_INIT(phys_addr, phys_addr + phys_addr_len),
 		.kvm_mmio_callback_fn = kvm_mmio_callback_fn,
 	};
 
-	return mmio_insert(&mmio_tree, mmio);
+	ret = mmio_insert(&mmio_tree, mmio);
+	br_write_unlock();
+
+	return ret;
 }
 
 bool kvm__deregister_mmio(u64 phys_addr)
 {
 	struct mmio_mapping *mmio;
 
+	br_write_lock();
 	mmio = mmio_search_single(&mmio_tree, phys_addr);
 	if (mmio == NULL)
 		return false;
 
 	rb_int_erase(&mmio_tree, &mmio->node);
+	br_write_unlock();
+
 	free(mmio);
 	return true;
 }
 
 bool kvm__emulate_mmio(struct kvm *kvm, u64 phys_addr, u8 *data, u32 len, u8 is_write)
 {
-	struct mmio_mapping *mmio = mmio_search(&mmio_tree, phys_addr, len);
+	struct mmio_mapping *mmio;
+
+	br_read_lock();
+	mmio = mmio_search(&mmio_tree, phys_addr, len);
 
 	if (mmio)
 		mmio->kvm_mmio_callback_fn(phys_addr, data, len, is_write);
 	else
 		fprintf(stderr, "Warning: Ignoring MMIO %s at %016llx (length %u)\n",
 			to_direction(is_write), phys_addr, len);
+	br_read_unlock();
 
 	return true;
 }
-- 
1.7.5.rc3

Re: [PATCH 2/4] kvm tools: Allow pausing guests

[Sasha Levin]

I mixed up files, this patch misses:

	/* See if the guest is running */
	if (!kvm_cpus[i] || kvm_cpus[i]->thread == 0)
		return;

In the beginning of kvm__pause() and kvm__continue(). I'll send an
updated patch.

On Sun, 2011-05-29 at 20:32 +0300, Sasha Levin wrote:
> Allow pausing and unpausing guests running on the host.
> Pausing a guest means that none of the VCPU threads are running
> KVM_RUN until they are unpaused.
> 
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---
>  tools/kvm/include/kvm/kvm-cpu.h |    1 +
>  tools/kvm/include/kvm/kvm.h     |    4 +++
>  tools/kvm/kvm-cpu.c             |   18 +++++++++++----
>  tools/kvm/kvm-run.c             |    4 +-
>  tools/kvm/kvm.c                 |   45 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 65 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/kvm/include/kvm/kvm-cpu.h b/tools/kvm/include/kvm/kvm-cpu.h
> index b2b6fce..4d99246 100644
> --- a/tools/kvm/include/kvm/kvm-cpu.h
> +++ b/tools/kvm/include/kvm/kvm-cpu.h
> @@ -23,6 +23,7 @@ struct kvm_cpu {
>  	struct kvm_msrs		*msrs;		/* dynamically allocated */
>  
>  	u8			is_running;
> +	u8			paused;
>  };
>  
>  struct kvm_cpu *kvm_cpu__init(struct kvm *kvm, unsigned long cpu_id);
> diff --git a/tools/kvm/include/kvm/kvm.h b/tools/kvm/include/kvm/kvm.h
> index 6a17362..d22a849 100644
> --- a/tools/kvm/include/kvm/kvm.h
> +++ b/tools/kvm/include/kvm/kvm.h
> @@ -12,6 +12,7 @@
>  #define KVM_32BIT_GAP_START	((1ULL << 32) - KVM_32BIT_GAP_SIZE)
>  
>  #define SIGKVMEXIT		(SIGRTMIN + 0)
> +#define SIGKVMPAUSE		(SIGRTMIN + 1)
>  
>  struct kvm {
>  	int			sys_fd;		/* For system ioctls(), i.e. /dev/kvm */
> @@ -50,6 +51,9 @@ bool kvm__emulate_io(struct kvm *kvm, u16 port, void *data, int direction, int s
>  bool kvm__emulate_mmio(struct kvm *kvm, u64 phys_addr, u8 *data, u32 len, u8 is_write);
>  bool kvm__register_mmio(u64 phys_addr, u64 phys_addr_len, void (*kvm_mmio_callback_fn)(u64 addr, u8 *data, u32 len, u8 is_write));
>  bool kvm__deregister_mmio(u64 phys_addr);
> +void kvm__pause(void);
> +void kvm__continue(void);
> +void kvm__notify_paused(void);
>  
>  /*
>   * Debugging
> diff --git a/tools/kvm/kvm-cpu.c b/tools/kvm/kvm-cpu.c
> index de0591f..81f479f 100644
> --- a/tools/kvm/kvm-cpu.c
> +++ b/tools/kvm/kvm-cpu.c
> @@ -383,11 +383,15 @@ void kvm_cpu__run(struct kvm_cpu *vcpu)
>  		die_perror("KVM_RUN failed");
>  }
>  
> -static void kvm_cpu_exit_handler(int signum)
> +static void kvm_cpu_signal_handler(int signum)
>  {
> -	if (current_kvm_cpu->is_running) {
> -		current_kvm_cpu->is_running = false;
> -		pthread_kill(pthread_self(), SIGKVMEXIT);
> +	if (signum == SIGKVMEXIT) {
> +		if (current_kvm_cpu->is_running) {
> +			current_kvm_cpu->is_running = false;
> +			pthread_kill(pthread_self(), SIGKVMEXIT);
> +		}
> +	} else if (signum == SIGKVMPAUSE) {
> +		current_kvm_cpu->paused = 1;
>  	}
>  }
>  
> @@ -400,12 +404,16 @@ int kvm_cpu__start(struct kvm_cpu *cpu)
>  
>  	pthread_sigmask(SIG_BLOCK, &sigset, NULL);
>  
> -	signal(SIGKVMEXIT, kvm_cpu_exit_handler);
> +	signal(SIGKVMEXIT, kvm_cpu_signal_handler);
> +	signal(SIGKVMPAUSE, kvm_cpu_signal_handler);
>  
>  	kvm_cpu__setup_cpuid(cpu);
>  	kvm_cpu__reset_vcpu(cpu);
>  
>  	for (;;) {
> +		if (cpu->paused)
> +			kvm__notify_paused();
> +
>  		kvm_cpu__run(cpu);
>  
>  		switch (cpu->kvm_run->exit_reason) {
> diff --git a/tools/kvm/kvm-run.c b/tools/kvm/kvm-run.c
> index 48b8e70..761ac0d 100644
> --- a/tools/kvm/kvm-run.c
> +++ b/tools/kvm/kvm-run.c
> @@ -47,8 +47,8 @@
>  #define MIN_RAM_SIZE_MB		(64ULL)
>  #define MIN_RAM_SIZE_BYTE	(MIN_RAM_SIZE_MB << MB_SHIFT)
>  
> -static struct kvm *kvm;
> -static struct kvm_cpu *kvm_cpus[KVM_NR_CPUS];
> +struct kvm *kvm;
> +struct kvm_cpu *kvm_cpus[KVM_NR_CPUS];
>  __thread struct kvm_cpu *current_kvm_cpu;
>  
>  static u64 ram_size;
> diff --git a/tools/kvm/kvm.c b/tools/kvm/kvm.c
> index 1d756e0..1e22566 100644
> --- a/tools/kvm/kvm.c
> +++ b/tools/kvm/kvm.c
> @@ -6,6 +6,8 @@
>  #include "kvm/interrupt.h"
>  #include "kvm/mptable.h"
>  #include "kvm/util.h"
> +#include "kvm/mutex.h"
> +#include "kvm/kvm-cpu.h"
>  
>  #include <linux/kvm.h>
>  
> @@ -25,6 +27,7 @@
>  #include <stdio.h>
>  #include <fcntl.h>
>  #include <time.h>
> +#include <sys/eventfd.h>
>  
>  #define DEFINE_KVM_EXIT_REASON(reason) [reason] = #reason
>  
> @@ -68,6 +71,10 @@ struct {
>  	{ DEFINE_KVM_EXT(KVM_CAP_EXT_CPUID) },
>  };
>  
> +extern struct kvm_cpu *kvm_cpus[KVM_NR_CPUS];
> +static int pause_event;
> +static DEFINE_MUTEX(pause_lock);
> +
>  static bool kvm__supports_extension(struct kvm *kvm, unsigned int extension)
>  {
>  	int ret;
> @@ -575,3 +582,41 @@ void kvm__dump_mem(struct kvm *kvm, unsigned long addr, unsigned long size)
>  				  p[n + 4], p[n + 5], p[n + 6], p[n + 7]);
>  	}
>  }
> +
> +void kvm__pause(void)
> +{
> +	int i, paused_vcpus = 0;
> +
> +	mutex_lock(&pause_lock);
> +
> +	pause_event = eventfd(0, 0);
> +	if (pause_event < 0)
> +		die("Failed creating pause notification event");
> +	for (i = 0; i < kvm.nrcpus; i++)
> +		pthread_kill(kvm_cpus[i]->thread, SIGKVMPAUSE);
> +
> +	while (paused_vcpus < kvm.nrcpus) {
> +		u64 cur_read;
> +
> +		if (read(pause_event, &cur_read, sizeof(cur_read)) < 0)
> +			die("Failed reading pause event");
> +		paused_vcpus += cur_read;
> +	}
> +	close(pause_event);
> +}
> +
> +void kvm__continue(void)
> +{
> +	mutex_unlock(&pause_lock);
> +}
> +
> +void kvm__notify_paused(void)
> +{
> +	u64 p = 1;
> +
> +	if (write(pause_event, &p, sizeof(p)) < 0)
> +		die("Failed notifying of paused VCPU.");
> +
> +	mutex_lock(&pause_lock);
> +	mutex_unlock(&pause_lock);
> +}

-- 

Sasha.

Re: [PATCH 3/4] kvm tools: Add a brlock

[Ingo Molnar]

* Sasha Levin <levinsasha928@gmail.com> wrote:

> +++ b/tools/kvm/include/kvm/brlock.h
> @@ -0,0 +1,12 @@
> +#ifndef KVM__BRLOCK_H
> +#define KVM__BRLOCK_H
> +
> +#include "kvm/kvm.h"
> +#include "kvm/barrier.h"
> +
> +#define br_read_lock()		mb()
> +#define br_read_unlock()	mb()

These only need to be compiler barrier()s AFAICS, because the 'pause' 
op will signal back to the requestor thread - which whole operation 
is a barrier to begin with.

> +#define br_write_lock()		kvm__pause()
> +#define br_write_unlock()	kvm__continue()
> +#endif

Btw., it might make sense to add a comment to this header file, 
explaining what a 'big reader lock' is :-)

Thanks,

	Ingo

Re: [PATCH 3/4] kvm tools: Add a brlock

[Sasha Levin]

On Sun, 2011-05-29 at 20:47 +0200, Ingo Molnar wrote:
> * Sasha Levin <levinsasha928@gmail.com> wrote:
> 
> > +++ b/tools/kvm/include/kvm/brlock.h
> > @@ -0,0 +1,12 @@
> > +#ifndef KVM__BRLOCK_H
> > +#define KVM__BRLOCK_H
> > +
> > +#include "kvm/kvm.h"
> > +#include "kvm/barrier.h"
> > +
> > +#define br_read_lock()		mb()
> > +#define br_read_unlock()	mb()
> 
> These only need to be compiler barrier()s AFAICS, because the 'pause' 
> op will signal back to the requestor thread - which whole operation 
> is a barrier to begin with.

I'm wondering why we need a barrier here at all. In this brlock
implementation the readers are waiting on a mutex in their main loop -
right before a call to KVM_RUN. They can't get anywhere near a
br_read_lock() once br_write_lock() has completed.

> > +#define br_write_lock()		kvm__pause()
> > +#define br_write_unlock()	kvm__continue()
> > +#endif
> 
> Btw., it might make sense to add a comment to this header file, 
> explaining what a 'big reader lock' is :-)

I'll put the commit message into the header, should be enough?

-- 

Sasha.

Re: [PATCH 3/4] kvm tools: Add a brlock

[Ingo Molnar]

* Sasha Levin <levinsasha928@gmail.com> wrote:

> On Sun, 2011-05-29 at 20:47 +0200, Ingo Molnar wrote:
> > * Sasha Levin <levinsasha928@gmail.com> wrote:
> > 
> > > +++ b/tools/kvm/include/kvm/brlock.h
> > > @@ -0,0 +1,12 @@
> > > +#ifndef KVM__BRLOCK_H
> > > +#define KVM__BRLOCK_H
> > > +
> > > +#include "kvm/kvm.h"
> > > +#include "kvm/barrier.h"
> > > +
> > > +#define br_read_lock()		mb()
> > > +#define br_read_unlock()	mb()
> > 
> > These only need to be compiler barrier()s AFAICS, because the 'pause' 
> > op will signal back to the requestor thread - which whole operation 
> > is a barrier to begin with.
> 
> I'm wondering why we need a barrier here at all. In this brlock 
> implementation the readers are waiting on a mutex in their main 
> loop - right before a call to KVM_RUN. They can't get anywhere near 
> a br_read_lock() once br_write_lock() has completed.

Yes - and i alluded to that in one of my previous mails - but i think 
we should do a barrier() just to make sure people use them ;-)

We don't want huge sections of code assuming readonly data 
structures. We should probably also add a debug variant that switches 
this all to rwlocks: that way the correctness of the critical 
sections can be tested.

5 years down the line we do not want to end up with another 'BKL' 
kind of situation.

> > > +#define br_write_lock()		kvm__pause()
> > > +#define br_write_unlock()	kvm__continue()
> > > +#endif
> > 
> > Btw., it might make sense to add a comment to this header file, 
> > explaining what a 'big reader lock' is :-)
> 
> I'll put the commit message into the header, should be enough?

Yeah, that should be more than enough!

Thanks,

	Ingo