| Message ID | CANVkX9FzAwgPKRrnFgoN74yo5_pRJ_by4G8tfPJxh3pLgSaSrg@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/02/2016 01:49 PM, James Nutaro wrote: It looks like you are a first-time contributor. First, welcome to the community. What I say below may sound like a lot, but it's meant to help you be more successful in your contribution, not to scare you off. Subject line is too long. I'd suggest something like: qqq: New clock synchronization module > This patch adds an interface via UNIX shared memory for pacing the > execution of QEMU to match an external simulation clock. The aim is to > allow QEMU to be used as a module inside of a larger simulation system. This paragraph would be fine in the commit message proper. > > The body of the patch is below. Whereas this paragraph isn't needed (and even if it was, would fit better after a '---' separator). > > Signed-off-by: Jim Nutaro (nutaro@gmail.com) Invalid format. Git expects emails to be <like@this>, not (like@this). > >>From 0e0d352be2f91ed2df3526bc55753acfa97509e9 Mon Sep 17 00:00:00 2001 > From: "James J. Nutaro" <nutarojj@ornl.gov> > Date: Mon, 9 Nov 2015 12:36:06 -0500 > Subject: [PATCH] QQQ is a module for pacing the rate of advance of the 'git am' doesn't like your message. $ git am -3 *clock.eml Applying: Added code for synchronzing the internal simulation clock with an external simulation clock fatal: corrupt patch at line 24 Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Added code for synchronzing the internal simulation clock with an external simulation clock The copy of the patch that failed is found in: /home/eblake/qemu/.git/rebase-apply/patch Rather than pasting your patch inline, it's better to use 'git send-email' to post the patch directly. More hints at: http://wiki.qemu.org/Contribute/SubmitAPatch A good exercise for first submission: send the patch to yourself, and run 'git am' on the resulting mail from your Inbox, to prove that you've got 'git send-email' working correctly. After that passes, then send to the list. > computer clock in reference to an external simulation clock. The basic > approach used here is adapted from QBox from Green Socs (hence the name, Q? > Q? Q? - qqq!). Its intended use is to incorporate a complete software stack > into constructive simulation models. The solution proposed here assumes > that > devices (NIC cards, serial ports, and other low level hardware) will be Please wrap your commit message at 60-70 characters (remember that 'git log' will display your message with further indentation, and we still want to read it without scrolling on an 80-column window). Also, the commit message MUST start with a single-line summary, then a blank line, before you dive into a wall of text. > modeled within QEMU and connected to via a UNIX socket or some other > appropriate mechanism. Other pieces of equipment (e.g., electric motors, > communication networks, quad coptors, robotic arms, ...) would be modeled s/coptor/copter/ > in > some other simulation tool. Hence, I assume that the problem of exchanging > data can be solved using existing mechanisms within QEMU and that the only > remaining problem is time synchronization. This module addresses the time > synchronization problem. > > There is an example of how to use this module in the > adevs simulation package at http://sourceforge.net/projects/adevs/ > > The mode of operation is as follows: > > The simulator creates a shared memory of size sizeof(int) that will be > used to exchange time advance information and writes a negative value In native endianness? > to that shared memory segment. Then the simulator forks a process > to execute qemu. Qemu attaches to this shared memory segment using > a key that includes the PID of the forked child (see qqq.c for the > expected shared memory key). Why not copy it here, instead of making us hunt for it? > > Qemu waits for a positive value to appear in the shared memory. Meanwhile > the simulator writes a positive value to indicate the maximum time > advance that it allows to qemu. When qemu receives this positive value > it schedules a timer event for time advance microseconds in the future. > A value of zero indicates that qemu should exit. > > Now the simulator spins on the shared memory value waiting for it to become > negative. When qemu executes the scheduled timer event, qemu writes a > negative value indicating the actual time advanced. At this time, the > simulator > is able to run again and this cycle is repeated until the simulation ends. Should this protocol be documented in a new file in the docs/ directory? > > Authors: > James Nutaro <nutarojj@ornl.gov> git already attributes you as an author; more important is the Signed-off-by designation. > --- > Makefile.target | 2 +- > cpus.c | 9 ++++- > include/qemu/thread.h | 8 ++++ > qqq.c | 101 > +++++++++++++++++++++++++++++++++++++++++++++++ > qqq.h | 67 +++++++++++++++++++++++++++++++ > util/qemu-thread-posix.c | 14 +++++++ > vl.c | 5 +++ > 7 files changed, 204 insertions(+), 2 deletions(-) > create mode 100644 qqq.c > create mode 100644 qqq.h > > @@ -998,7 +1000,12 @@ static void qemu_tcg_wait_io_event(CPUState *cpu) > /* Start accounting real time to the virtual clock if the CPUs > are idle. */ > qemu_clock_warp(QEMU_CLOCK_VIRTUAL); > - qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex); > + /* If qqq is running then this wait must timeout so that the > + * simulation does not become stuck when the cpu idles */ > + if (qqq_enabled()) Alignment looks off; may be a factor of how you pasted the mail. > + qemu_cond_wait_timeout_ns(cpu->halt_cond, &qemu_global_mutex, > 10000); And the fact that my mailer splits your single source line across two lines of reply is an example of where pasting your patch inline broke it. Also, you'll want to run your patch through scripts/checkpatch.pl; we require {} around all if/else code, even when there is only a single statement body. > +++ b/include/qemu/thread.h > @@ -36,6 +36,14 @@ void qemu_cond_destroy(QemuCond *cond); > void qemu_cond_signal(QemuCond *cond); > void qemu_cond_broadcast(QemuCond *cond); > void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex); > +/* This defaults to qemu_cond_wait() on Windows */ > +#ifndef _WIN32 > +void qemu_cond_wait_timeout_ns(QemuCond *cond, QemuMutex *mutex, int64_t > timeout_ns); > +#else > +void qemu_cond_wait_timeout_ns(QemuCond *cond, QemuMutex *mutex, int64_t > timeout_ns) { > + qemu_cond_wait(cond,mutex); Space after comma. > +++ b/qqq.c > @@ -0,0 +1,101 @@ > + > +/* This is a Linux only feature */ > + > +#ifndef _WIN32 This is a file with no copyright header. Are you okay with the implicit default of GPLv2+? Even if you are, explicitly stating it is wiser. > +#include "qemu/main-loop.h" > +#include "qemu/timer.h" This file should #include "qemu/osdep.h" first, before any other header. > + > +/* Shared memory data */ > +static void* shm = NULL; > +static int64_t t = 0; > +static QEMUTimer* sync_timer = NULL; > + > +static void write_mem_value(int val) > +{ > + /* I AM ASSUMING THE MEMORY WRITE WILL BE ATOMIC AND WILL > + * NOT BE CACHED */ MUST WE SHOUT? > + (*((volatile int*)shm)) = val; void* and int* are not necessarily the same size. Your type-punning may backfire. > + > +static void schedule_next_event(void) > +{ > + int time_advance; > + /* Get the time advance allowed by the simulator. This is provided > + * as a positive value that is written by the simulator to the > + * shared memory location. */ > + while ((time_advance = read_mem_value()) < 0); Isn't this going to cause 100% CPU usage while waiting for the other end of the shared memory to change things? Are there any smarter IPC mechanisms we could use that aren't quite so resource intensive? > + /* A zero time advance is an instruction to shutdown */ > + if (time_advance == 0) > + { > + munmap(shm,sizeof(int)); Space after comma. > + exit(0); munmap() before exit() is wasted effort except under a lint debug situation; the exit is going to clean up memory anyway. > + } > + /* Schedule the next syncronization point */ s/syncronization/synchronization/ > + timer_mod(sync_timer,t+time_advance); Spacing. Again, scripts/checkpatch.pl will catch most of these. I'll quit pointing it out. > +} > + > +static void sync_func(void* data) > +{ > + /* Report the actual elapsed time. The value written will be > + * -(elapsed time), and the simulator will recognized the negative > + * value as a signal that the write has been done. */ > + int64_t tnow = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL); > + int usecs = tnow-t; > + write_mem_value((usecs > 0) ? -usecs : -1); How is an error distinguishable from usecs == 1? Does it matter? > + /* Update our time of last event */ > + t = tnow; > + /* Schedule the next event */ > + schedule_next_event(); > +} > + > +bool qqq_enabled(void) { return (shm != NULL); } Split this into a multi-line definition. No need for () around a simple return statement; you could also shorten it to 'return !!shm;' > + > +void setup_qqq(void) > +{ > + char shm_key[100]; > + /* Attach to the shared memory region */ > + sprintf(shm_key,"/qemu_%d",getpid()); sprintf() into a fixed-size buffer is dangerous. It happens to work here, but better would be using g_strdup_printf and not even worrying about the buffer size. > + errno = 0; > + int fd = shm_open(shm_key,O_RDWR,S_IRWXU); No need to pre-set errno; shm_open() is well-behaved (unlike strtol(), where the pre-set is mandatory). > + if (fd == -1) > + { > + perror("Could not open shared memory, qqq not enabled"); Don't use perror() (throughout this patch). Instead, consider making this function take Error **errp, and use error_setg_errno() here; then the caller can decide what to do on failure (abort, ignore, or report it). > + return; > + } > + shm = mmap(NULL,sizeof(int),PROT_READ|PROT_WRITE,MAP_SHARED,fd,0); > + if (shm == NULL) > + { > + perror("Could not map shared memory, qqq not enabled"); Indentation is inconsistent with the rest of the project; it should be four spaces, not two. > + shm_unlink(shm_key); > + return; Leaks fd. > + } > + /* Done with the shared memory file handle */ > + close(fd); > + shm_unlink(shm_key); > + /* Start the timer to ensure time warps advance the clock */ > + sync_timer = timer_new_us(QEMU_CLOCK_VIRTUAL,sync_func,NULL); > + /* Get the time advance that is requested by the simulation */ > + schedule_next_event(); > +} > + > +#endif > +#ifdef _WIN32 > + > +/* Windows functions that do nothing */ > +static inline bool qqq_enabled(void) { return false; } > +static inline void setup_qqq(void){} Better would be to report an error than silently ignore the option. > + > +#else > + > +/* The Linux implementation */ > + > +#include "qemu/main-loop.h" > +#include "qemu/timer.h" > +#include "qemu/thread.h" > +#include "sysemu/cpus.h" > +#include <stdio.h> > +#include <errno.h> > +#include <sys/mman.h> > +#include <sys/stat.h> > +#include <sys/types.h> > +#include <fcntl.h> > +#include <assert.h> > +#include <unistd.h> > +#include <pthread.h> Some of these includes are not necessary if you follow recommended practice and have all .c files #include "qemu/osdeps.h" first. > +++ b/util/qemu-thread-posix.c > @@ -134,6 +134,20 @@ void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex) > error_exit(err, __func__); > } > > +void qemu_cond_wait_timeout_ns(QemuCond *cond, QemuMutex *mutex, int64_t > timeout_ns) > +{ > + static const long ns_sec = 1000000000LL; You're trying to shove a long long into a long, which doesn't necessarily work on 32-bit platforms. Except that the value you are storing fits inside a 32-bit int, so do you even need the LL suffix? > +++ b/vl.c > @@ -125,6 +125,8 @@ int main(int argc, char **argv) > #include "sysemu/replay.h" > #include "qapi/qmp/qerror.h" > > +#include "qqq.h" > + > #define MAX_VIRTIO_CONSOLES 1 > #define MAX_SCLP_CONSOLES 1 > > @@ -4402,6 +4404,9 @@ int main(int argc, char **argv, char **envp) > /* spice needs the timers to be initialized by this point */ > qemu_spice_init(); > #endif > + /* try to setup the qqq interface for syncing advance of the virtual > clock > + * with an external simulator */ > + setup_qqq(); What, unconditional setup instead of a command line option to control when to use it?
diff --git a/Makefile.target b/Makefile.target index 962d004..8956afe 100644 --- a/Makefile.target +++ b/Makefile.target @@ -131,7 +131,7 @@ endif #CONFIG_BSD_USER ######################################################### # System emulator target ifdef CONFIG_SOFTMMU -obj-y += arch_init.o cpus.o monitor.o gdbstub.o balloon.o ioport.o numa.o +obj-y += arch_init.o qqq.o cpus.o monitor.o gdbstub.o balloon.o ioport.o numa.o obj-y += qtest.o bootdevice.o obj-y += hw/ obj-$(CONFIG_KVM) += kvm-all.o diff --git a/cpus.c b/cpus.c index 877bd70..b040285 100644 --- a/cpus.c +++ b/cpus.c @@ -44,6 +44,8 @@ #include "hw/nmi.h" #include "sysemu/replay.h" +#include "qqq.h" + #ifndef _WIN32 #include "qemu/compatfd.h" #endif @@ -998,7 +1000,12 @@ static void qemu_tcg_wait_io_event(CPUState *cpu) /* Start accounting real time to the virtual clock if the CPUs are idle. */ qemu_clock_warp(QEMU_CLOCK_VIRTUAL); - qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex); + /* If qqq is running then this wait must timeout so that the + * simulation does not become stuck when the cpu idles */ + if (qqq_enabled()) + qemu_cond_wait_timeout_ns(cpu->halt_cond, &qemu_global_mutex, 10000); + else + qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex); } while (iothread_requesting_mutex) { diff --git a/include/qemu/thread.h b/include/qemu/thread.h index 5114ec8..c0f6009 100644 --- a/include/qemu/thread.h +++ b/include/qemu/thread.h @@ -36,6 +36,14 @@ void qemu_cond_destroy(QemuCond *cond); void qemu_cond_signal(QemuCond *cond); void qemu_cond_broadcast(QemuCond *cond); void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex); +/* This defaults to qemu_cond_wait() on Windows */ +#ifndef _WIN32 +void qemu_cond_wait_timeout_ns(QemuCond *cond, QemuMutex *mutex, int64_t timeout_ns); +#else +void qemu_cond_wait_timeout_ns(QemuCond *cond, QemuMutex *mutex, int64_t timeout_ns) { + qemu_cond_wait(cond,mutex); +} +#endif void qemu_sem_init(QemuSemaphore *sem, int init); void qemu_sem_post(QemuSemaphore *sem); diff --git a/qqq.c b/qqq.c new file mode 100644 index 0000000..5c2a786 --- /dev/null +++ b/qqq.c @@ -0,0 +1,101 @@ + +/* This is a Linux only feature */ + +#ifndef _WIN32 + +#include "qemu/main-loop.h" +#include "qemu/timer.h" +#include "qemu/thread.h" +#include "sysemu/cpus.h" +#include <stdio.h> +#include <errno.h> +#include <sys/mman.h> +#include <sys/stat.h> +#include <sys/types.h> +#include <fcntl.h> +#include <assert.h> +#include <unistd.h> +#include <pthread.h> +#include "qqq.h" + +/* Shared memory data */ +static void* shm = NULL; +static int64_t t = 0; +static QEMUTimer* sync_timer = NULL; + +static void write_mem_value(int val) +{ + /* I AM ASSUMING THE MEMORY WRITE WILL BE ATOMIC AND WILL + * NOT BE CACHED */ + (*((volatile int*)shm)) = val; +} + +static int read_mem_value(void) +{ + /* I AM ASSUMING THE MEMORY READ WILL BE ATOMIC AND WILL + * NOT BE CACHED */ + return (*((volatile int*)shm)); +} + +static void schedule_next_event(void) +{ + int time_advance; + /* Get the time advance allowed by the simulator. This is provided + * as a positive value that is written by the simulator to the + * shared memory location. */ + while ((time_advance = read_mem_value()) < 0); + /* A zero time advance is an instruction to shutdown */ + if (time_advance == 0) + { + munmap(shm,sizeof(int)); + exit(0); + } + /* Schedule the next syncronization point */ + timer_mod(sync_timer,t+time_advance); +} + +static void sync_func(void* data) +{ + /* Report the actual elapsed time. The value written will be + * -(elapsed time), and the simulator will recognized the negative + * value as a signal that the write has been done. */ + int64_t tnow = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL); + int usecs = tnow-t; + write_mem_value((usecs > 0) ? -usecs : -1); + /* Update our time of last event */ + t = tnow; + /* Schedule the next event */ + schedule_next_event(); +} + +bool qqq_enabled(void) { return (shm != NULL); } + +void setup_qqq(void) +{ + char shm_key[100]; + /* Attach to the shared memory region */ + sprintf(shm_key,"/qemu_%d",getpid()); + errno = 0; + int fd = shm_open(shm_key,O_RDWR,S_IRWXU); + if (fd == -1) + { + perror("Could not open shared memory, qqq not enabled"); + return; + } + shm = mmap(NULL,sizeof(int),PROT_READ|PROT_WRITE,MAP_SHARED,fd,0); + if (shm == NULL) + { + perror("Could not map shared memory, qqq not enabled"); + shm_unlink(shm_key); + return; + } + /* Done with the shared memory file handle */ + close(fd); + shm_unlink(shm_key); + /* Start the timer to ensure time warps advance the clock */ + sync_timer = timer_new_us(QEMU_CLOCK_VIRTUAL,sync_func,NULL); + /* Get the time advance that is requested by the simulation */ + schedule_next_event(); +} + +#endif diff --git a/qqq.h b/qqq.h new file mode 100644 index 0000000..687a9f3 --- /dev/null +++ b/qqq.h @@ -0,0 +1,67 @@ +/* + * A module for pacing the rate of advance of the computer clock + * in reference to an external simulation clock. The basic approach + * used here is adapted from QBox from Green Socs (hence the name, Q? + * Q? Q? - qqq!). The basic mode of operation is as follows: + * + * The simulator creates a shared memory of size sizeof(int) that will be + * used to exchange time advance information, writes a negative value + * to that shared memory segment, and then forks a process + * to execute qemu. Qemu attaches to this shared memory segment using + * a key that includes the PID of the forked child (see qqq.c for the + * expected shared memory key). + * + * Qemu waits for a positive to appear in the shared memory. Meanwhile + * the simulator writes a positive value to indicate the maximum time + * advance that it allows to qemu. When qemu receives this positive value + * it schedules a timer event for time advance microseconds in the future. + * + * Now the simulator spins on the shared memory value waiting for it to become + * negative. When qemu executes the scheduled timer event, qemu writes a + * negative value indicating the actual time advanced. At this time, the simulator + * is able to run again and this cycle is repeated until the simulation ends. + * + * Authors: + * James Nutaro <nutarojj@ornl.gov> + * + * This work is licensed under the terms of the GNU GPL, version 2. See + * the COPYING file in the top-level directory. + * + */ +#ifndef QQQ_H +#define QQQ_H + +/* This is a Linux only feature */ + +#ifdef _WIN32 + +/* Windows functions that do nothing */ +static inline bool qqq_enabled(void) { return false; } +static inline void setup_qqq(void){} + +#else + +/* The Linux implementation */ + +#include "qemu/main-loop.h" +#include "qemu/timer.h" +#include "qemu/thread.h" +#include "sysemu/cpus.h" +#include <stdio.h> +#include <errno.h> +#include <sys/mman.h> +#include <sys/stat.h> +#include <sys/types.h> +#include <fcntl.h> +#include <assert.h> +#include <unistd.h> +#include <pthread.h> + +void setup_qqq(void); +/* Returns true if qqq is enabled and false otherwise. + * It will be enabled only if setup_qqq() is able to + * attach to the shared memory segment. */ +bool qqq_enabled(void); + +#endif +#endif diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c index dbd8094..c6396c4 100644 --- a/util/qemu-thread-posix.c +++ b/util/qemu-thread-posix.c @@ -134,6 +134,20 @@ void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex) error_exit(err, __func__); } +void qemu_cond_wait_timeout_ns(QemuCond *cond, QemuMutex *mutex, int64_t timeout_ns) +{ + static const long ns_sec = 1000000000LL; + struct timeval tv; + struct timespec ts; + int err; + gettimeofday(&tv, NULL); + ts.tv_sec = tv.tv_sec + (timeout_ns + tv.tv_usec*1000) / ns_sec; + ts.tv_nsec = (timeout_ns + tv.tv_usec*1000) % ns_sec; + err = pthread_cond_timedwait(&cond->cond, &mutex->lock, &ts); + if (err != 0 && err != ETIMEDOUT) + error_exit(err, __func__); +} + void qemu_sem_init(QemuSemaphore *sem, int init) { int rc; diff --git a/vl.c b/vl.c index 21e8876..8a2facc 100644 --- a/vl.c +++ b/vl.c @@ -125,6 +125,8 @@ int main(int argc, char **argv) #include "sysemu/replay.h" #include "qapi/qmp/qerror.h" +#include "qqq.h" + #define MAX_VIRTIO_CONSOLES 1 #define MAX_SCLP_CONSOLES 1 @@ -4402,6 +4404,9 @@ int main(int argc, char **argv, char **envp) /* spice needs the timers to be initialized by this point */ qemu_spice_init(); #endif + /* try to setup the qqq interface for syncing advance of the virtual clock + * with an external simulator */ + setup_qqq(); cpu_ticks_init();