Re: [Qemu-devel] [PATCH] Added code for synchronzing the internal simulation clock with an external simulation clock

[Eric Blake]

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 (address@hidden)

Invalid format.  Git expects emails to be <address@hidden>, not 
(address@hidden).

> 
>>From 0e0d352be2f91ed2df3526bc55753acfa97509e9 Mon Sep 17 00:00:00 2001
> From: "James J. Nutaro" <address@hidden>
> 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 <address@hidden>

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?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature