* [PATCH 0/5] trace-cmd: Add a recorder readable feature for virtio-trace
From: Yoshihiro YUNOMAE @ 2012-08-22 8:42 UTC (permalink / raw)
To: Steven Rostedt
Cc: Herbert Xu, Arnd Bergmann, qemu-devel, Frederic Weisbecker,
linux-kernel, Borislav Petkov, virtualization, Masami Hiramatsu,
Franch Ch. Eigler, Ingo Molnar, Mathieu Desnoyers,
Anthony Liguori, Greg Kroah-Hartman, Amit Shah, yrl.pp-manager.tt
Hi Steven,
The following patch set provides a feature which can read trace data of a guest
using virtio-trace (https://lkml.org/lkml/2012/8/9/210) for a recorder
function of trace-cmd. This patch set depends on the trace-agent running on a
guest in the virtio-trace system.
To translate raw data of a guest to text data on a host, information of debugfs
in the guest is also needed on the host. In other words, the guest's debugfs
must be exposed (mounted) on the host via other serial line (we don't like to
depend on network connection). For this purpose, we'll use DIOD 9pfs server
(http://code.google.com/p/diod/) as below.
***HOW TO USE***
We explain about how to translate raw data to text data on a host using
trace-cmd applied this patch set and virtio-trace.
- Preparation
1. Make FIFO in a host
virtio-trace uses virtio-serial pipe as trace data paths as to the number
of CPUs and a control path, so FIFO (named pipe) should be created as follows:
# mkdir /tmp/virtio-trace/
# mkfifo /tmp/virtio-trace/trace-path-cpu{0,1,2,...,X}.{in,out}
# mkfifo /tmp/virtio-trace/agent-ctl-path.{in,out}
Here, if we assign 1VCPU for a guest, then we set as follows:
trace-path-cpu0.{in.out}
and
agent-ctl-path.{in,out}.
2. Set up of virtio-serial pipe and unix in a host
Add qemu option to use virtio-serial pipe for tracing and unix for debugfs.
##virtio-serial device##
-device virtio-serial-pci,id=virtio-serial0\
##control path##
-chardev pipe,id=charchannel0,path=/tmp/virtio-trace/agent-ctl-path\
-device virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,\
id=channel0,name=agent-ctl-path\
##data path##
-chardev pipe,id=charchannel1,path=/tmp/virtio-trace/trace-path-cpu0\
-device virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel1,\
id=channel1,name=trace-path-cpu0\
##9pfs path##
-device virtio-serial \
-chardev socket,path=/tmp/virtio-trace/trace-9pfs,server,nowait, \
id=trace-9pfs \
-device virtserialport,chardev=trace-9pfs,name=virtioserial
If you manage guests with libvirt, add the following tags to domain XML files.
Then, libvirt passes the same command option to qemu.
<channel type='pipe'>
<source path='/tmp/virtio-trace/agent-ctl-path'/>
<target type='virtio' name='agent-ctl-path'/>
<address type='virtio-serial' controller='0' bus='0' port='0'/>
</channel>
<channel type='pipe'>
<source path='/tmp/virtio-trace/trace-path-cpu0'/>
<target type='virtio' name='trace-path-cpu0'/>
<address type='virtio-serial' controller='0' bus='0' port='1'/>
</channel>
<channel type='unix'>
<source mode='bind' path='/tmp/virtio-trace/trace-9pfs'/>
<target type='virtio' name='trace-9pfs'/>
<address type='virtio-serial' controller='0' bus='0' port='2'/>
</channel>
Here, chardev names are restricted to trace-path-cpu0 and agent-ctl-path. UNIX
domain socket is automatically created on a host.
3. Boot the guest
You can find some chardev in /dev/virtio-ports/ in the guest.
4. Create symbolic link for trace-cmd on the host
# ln -s /tmp/virtio-trace/trace-path-cpu0.out \
/tmp/virtio-tracing/tracing/per_cpu/cpu0/trace_pipe_raw
5. Wait for 9pfs server on the host
# mount -t 9p -o trans=unix,access=any,uname=root, \
aname=/sys/kernel/debug,version=9p2000.L \
/tmp/virtio-trace/trace-9pfs /tmp/virtio-trace/debugfs
6. Run DIOD on the guest
# diod -E -Nn -u 0
7. Connect DIOD to virtio-console on the guest
# socat TCP4:127.0.0.1:564 /dev/virtio-ports/trace-9pfs
- Execution
1. Run trace-agent on the guest
# ./trace-agent
2. Execute trace-cmd on the host
# AGENT_READ_DIR=/tmp/virtio-trace/tracing \
AGENT_CTL=/tmp/virtio-trace/agent-ctl-path.in \
TRACE_DIR=/tmp/virtio-trace/debugfs/tracing \
./trace-cmd record -e "sched:*
3. Translate raw data to text data on the host
# ./trace-cmd report trace.dat
***Just enhancement ideas***
- Support for trace-cmd => done
- Support for 9pfs protocol
- Support for non-blocking mode in QEMU
Thank you,
---
Masami Hiramatsu (2):
trace-cmd: Use tracing directory to count CPUs
trace-cmd: Use TRACE_DIR envrionment variable if defined
Yoshihiro YUNOMAE (3):
trace-cmd: Use polling function
trace-cmd: Add non-blocking option for open() and splice_read()
trace-cmd: Support trace-agent of virtio-trace
trace-cmd.h | 1
trace-record.c | 41 ++++++++++++++++++++
trace-recorder.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++------
trace-util.c | 27 +++++++++++++
4 files changed, 169 insertions(+), 12 deletions(-)
--
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: yoshihiro.yunomae.ez@hitachi.com
^ permalink raw reply
* [PATCH 1/5] trace-cmd: Use TRACE_DIR envrionment variable if defined
From: Yoshihiro YUNOMAE @ 2012-08-22 8:43 UTC (permalink / raw)
To: Steven Rostedt
Cc: Herbert Xu, Arnd Bergmann, qemu-devel, Frederic Weisbecker,
linux-kernel, Borislav Petkov, virtualization, Masami Hiramatsu,
Franch Ch. Eigler, Ingo Molnar, Mathieu Desnoyers,
Anthony Liguori, Greg Kroah-Hartman, Amit Shah, yrl.pp-manager.tt
In-Reply-To: <20120822084251.17293.69086.stgit@ltc189.sdl.hitachi.co.jp>
From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Use TRACE_DIR environment variable for setting
debugfs/tracing directory if defined. This is
for controlling guest(or remote) ftrace.
Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
---
trace-util.c | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/trace-util.c b/trace-util.c
index e128188..d5a3eb4 100644
--- a/trace-util.c
+++ b/trace-util.c
@@ -311,6 +311,15 @@ char *tracecmd_find_tracing_dir(void)
char type[100];
FILE *fp;
+ tracing_dir = getenv("TRACE_DIR");
+ if (tracing_dir) {
+ tracing_dir = strdup(tracing_dir);
+ if (!tracing_dir)
+ die("malloc");
+ warning("Use environmental tracing directory: %s\n", tracing_dir);
+ return tracing_dir;
+ }
+
if ((fp = fopen("/proc/mounts","r")) == NULL) {
warning("Can't open /proc/mounts for read");
return NULL;
^ permalink raw reply related
* [PATCH 2/5] trace-cmd: Use tracing directory to count CPUs
From: Yoshihiro YUNOMAE @ 2012-08-22 8:43 UTC (permalink / raw)
To: Steven Rostedt
Cc: Herbert Xu, Arnd Bergmann, qemu-devel, Frederic Weisbecker,
linux-kernel, Borislav Petkov, virtualization, Masami Hiramatsu,
Franch Ch. Eigler, Ingo Molnar, Mathieu Desnoyers,
Anthony Liguori, Greg Kroah-Hartman, Amit Shah, yrl.pp-manager.tt
In-Reply-To: <20120822084251.17293.69086.stgit@ltc189.sdl.hitachi.co.jp>
From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Count debugfs/tracing/per_cpu/cpu* to determine the
number of CPUs.
Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
---
trace-record.c | 41 +++++++++++++++++++++++++++++++++++++++++
1 files changed, 41 insertions(+), 0 deletions(-)
diff --git a/trace-record.c b/trace-record.c
index 9dc18a9..ed18951 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -1179,6 +1179,41 @@ static void expand_event_list(void)
}
}
+static int count_tracingdir_cpus(void)
+{
+ char *tracing_dir = NULL;
+ char *percpu_dir = NULL;
+ struct dirent **namelist;
+ int count = 0, n;
+
+ /* Count cpus in per_cpu directory */
+ tracing_dir = tracecmd_find_tracing_dir();
+ if (!tracing_dir)
+ return 0;
+ percpu_dir = malloc_or_die(strlen(tracing_dir) + 9);
+ if (!percpu_dir)
+ goto err;
+
+ sprintf(percpu_dir, "%s/per_cpu", tracing_dir);
+
+ n = scandir(percpu_dir, &namelist, NULL, alphasort);
+ if (n > 0) {
+ while (n--) {
+ if (strncmp("cpu", namelist[n]->d_name, 3) == 0)
+ count++;
+ free(namelist[n]);
+ }
+ free(namelist);
+ }
+
+ if (percpu_dir)
+ free(percpu_dir);
+err:
+ if (tracing_dir)
+ free(tracing_dir);
+ return count;
+}
+
static int count_cpus(void)
{
FILE *fp;
@@ -1189,6 +1224,12 @@ static int count_cpus(void)
size_t n;
int r;
+ cpus = count_tracingdir_cpus();
+ if (cpus > 0)
+ return cpus;
+
+ warning("failed to use tracing_dir to determine number of CPUS");
+
cpus = sysconf(_SC_NPROCESSORS_CONF);
if (cpus > 0)
return cpus;
^ permalink raw reply related
* [PATCH 3/5] trace-cmd: Support trace-agent of virtio-trace
From: Yoshihiro YUNOMAE @ 2012-08-22 8:43 UTC (permalink / raw)
To: Steven Rostedt
Cc: Herbert Xu, Arnd Bergmann, qemu-devel, Frederic Weisbecker,
linux-kernel, Borislav Petkov, virtualization, Masami Hiramatsu,
Franch Ch. Eigler, Ingo Molnar, Mathieu Desnoyers,
Anthony Liguori, Greg Kroah-Hartman, Amit Shah, yrl.pp-manager.tt
In-Reply-To: <20120822084251.17293.69086.stgit@ltc189.sdl.hitachi.co.jp>
Add read path and control path to use trace-agent of virtio-trace.
When we use trace-agent, trace-cmd will be used as follows:
# AGENT_READ_DIR=/tmp/virtio-trace/tracing \
AGENT_CTL=/tmp/virtio-trace/agent-ctl-path.in \
TRACING_DIR=/tmp/virtio-trace/debugfs/tracing \
trace-cmd record -e "sched:*"
Here, AGENT_READ_DIR is the path for a reading directory of virtio-trace,
AGENT_CTL is a control path of trace-agent, and TRACING_DIR is a debugfs path
of a guest.
Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
---
trace-cmd.h | 1 +
trace-recorder.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
trace-util.c | 18 +++++++++++++++++
3 files changed, 75 insertions(+), 1 deletions(-)
diff --git a/trace-cmd.h b/trace-cmd.h
index f904dc5..75506ed 100644
--- a/trace-cmd.h
+++ b/trace-cmd.h
@@ -72,6 +72,7 @@ static inline int tracecmd_host_bigendian(void)
}
char *tracecmd_find_tracing_dir(void);
+char *guest_agent_tracing_read_dir(void);
/* --- Opening and Reading the trace.dat file --- */
diff --git a/trace-recorder.c b/trace-recorder.c
index 215affc..3b750e9 100644
--- a/trace-recorder.c
+++ b/trace-recorder.c
@@ -33,6 +33,7 @@
#include <unistd.h>
#include <ctype.h>
#include <errno.h>
+#include <stdbool.h>
#include "trace-cmd.h"
@@ -43,6 +44,8 @@ struct tracecmd_recorder {
int page_size;
int cpu;
int stop;
+ int ctl_fd;
+ bool agent_existing;
};
void tracecmd_free_recorder(struct tracecmd_recorder *recorder)
@@ -59,11 +62,29 @@ void tracecmd_free_recorder(struct tracecmd_recorder *recorder)
free(recorder);
}
+static char *use_trace_agent_dir(char *ctl_path,
+ struct tracecmd_recorder *recorder)
+{
+ ctl_path = strdup(ctl_path);
+ if (!ctl_path)
+ die("malloc");
+ warning("Use environmental control path: %s\n", ctl_path);
+
+ recorder->ctl_fd = open(ctl_path, O_WRONLY);
+ if (recorder->ctl_fd < 0)
+ return NULL;
+
+ recorder->agent_existing = true;
+
+ return guest_agent_tracing_read_dir();
+}
+
struct tracecmd_recorder *tracecmd_create_recorder_fd(int fd, int cpu)
{
struct tracecmd_recorder *recorder;
char *tracing = NULL;
char *path = NULL;
+ char *ctl_path = NULL;
int ret;
recorder = malloc_or_die(sizeof(*recorder));
@@ -76,12 +97,23 @@ struct tracecmd_recorder *tracecmd_create_recorder_fd(int fd, int cpu)
recorder->trace_fd = -1;
recorder->brass[0] = -1;
recorder->brass[1] = -1;
+ recorder->ctl_fd = -1;
+ recorder->agent_existing = false;
recorder->page_size = getpagesize();
recorder->fd = fd;
- tracing = tracecmd_find_tracing_dir();
+ /*
+ * The trace-agent on a guest is controlled to run or stop by a host,
+ * so we need to assign the control path of the trace-agent to use
+ * virtio-trace.
+ */
+ ctl_path = getenv("AGENT_CTL");
+ if (ctl_path)
+ tracing = use_trace_agent_dir(ctl_path, recorder);
+ else
+ tracing = tracecmd_find_tracing_dir();
if (!tracing) {
errno = ENODEV;
goto out_free;
@@ -182,6 +214,24 @@ long tracecmd_flush_recording(struct tracecmd_recorder *recorder)
return total;
}
+static void operation_to_trace_agent(int ctl_fd, bool run_agent)
+{
+ if (run_agent == true)
+ write(ctl_fd, "1", 2);
+ else
+ write(ctl_fd, "0", 2);
+}
+
+static void run_operation_to_trace_agent(int ctl_fd)
+{
+ operation_to_trace_agent(ctl_fd, true);
+}
+
+static void stop_operation_to_trace_agent(int ctl_fd)
+{
+ operation_to_trace_agent(ctl_fd, false);
+}
+
int tracecmd_start_recording(struct tracecmd_recorder *recorder, unsigned long sleep)
{
struct timespec req;
@@ -189,6 +239,9 @@ int tracecmd_start_recording(struct tracecmd_recorder *recorder, unsigned long s
recorder->stop = 0;
+ if (recorder->agent_existing)
+ run_operation_to_trace_agent(recorder->ctl_fd);
+
do {
if (sleep) {
req.tv_sec = sleep / 1000000;
@@ -214,6 +267,8 @@ void tracecmd_stop_recording(struct tracecmd_recorder *recorder)
if (!recorder)
return;
+ if (recorder->agent_existing)
+ stop_operation_to_trace_agent(recorder->ctl_fd);
recorder->stop = 1;
}
diff --git a/trace-util.c b/trace-util.c
index d5a3eb4..ff639be 100644
--- a/trace-util.c
+++ b/trace-util.c
@@ -304,6 +304,24 @@ static int mount_debugfs(void)
return ret;
}
+char *guest_agent_tracing_read_dir(void)
+{
+ char *tracing_read_dir;
+
+ tracing_read_dir = getenv("AGENT_READ_DIR");
+ if (tracing_read_dir) {
+ tracing_read_dir = strdup(tracing_read_dir);
+ if (!tracing_read_dir)
+ die("malloc");
+ warning("Use environmental read directory of trace-agent: %s\n",
+ tracing_read_dir);
+ return tracing_read_dir;
+ }
+
+ warning("AGENT_READ_DIR was not declared");
+ return NULL;
+}
+
char *tracecmd_find_tracing_dir(void)
{
char debugfs[MAX_PATH+1];
^ permalink raw reply related
* [PATCH 4/5] trace-cmd: Add non-blocking option for open() and splice_read()
From: Yoshihiro YUNOMAE @ 2012-08-22 8:43 UTC (permalink / raw)
To: Steven Rostedt
Cc: Herbert Xu, Arnd Bergmann, qemu-devel, Frederic Weisbecker,
linux-kernel, Borislav Petkov, virtualization, Masami Hiramatsu,
Franch Ch. Eigler, Ingo Molnar, Mathieu Desnoyers,
Anthony Liguori, Greg Kroah-Hartman, Amit Shah, yrl.pp-manager.tt
In-Reply-To: <20120822084251.17293.69086.stgit@ltc189.sdl.hitachi.co.jp>
Add non-blocking option for open() and splice_read() for avoiding block to read
trace data of a guest from FIFO.
If SIGINT comes to read/write processes from the parent process in the case
where FIFO as a read I/F is assigned, then reading is normally blocked for
splice_read(). So, we added nonblock option to open() and splice_read().
Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
---
trace-recorder.c | 13 ++++++++-----
1 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/trace-recorder.c b/trace-recorder.c
index 3b750e9..6577fe8 100644
--- a/trace-recorder.c
+++ b/trace-recorder.c
@@ -124,7 +124,7 @@ struct tracecmd_recorder *tracecmd_create_recorder_fd(int fd, int cpu)
goto out_free;
sprintf(path, "%s/per_cpu/cpu%d/trace_pipe_raw", tracing, cpu);
- recorder->trace_fd = open(path, O_RDONLY);
+ recorder->trace_fd = open(path, O_RDONLY | O_NONBLOCK);
if (recorder->trace_fd < 0)
goto out_free;
@@ -172,14 +172,17 @@ static long splice_data(struct tracecmd_recorder *recorder)
long ret;
ret = splice(recorder->trace_fd, NULL, recorder->brass[1], NULL,
- recorder->page_size, 1 /* SPLICE_F_MOVE */);
+ recorder->page_size, SPLICE_F_MOVE | SPLICE_F_NONBLOCK);
if (ret < 0) {
- warning("recorder error in splice input");
- return -1;
+ if (errno != EAGAIN) {
+ warning("recorder error in splice input");
+ return -1;
+ }
+ return 0; /* Buffer is empty */
}
ret = splice(recorder->brass[0], NULL, recorder->fd, NULL,
- recorder->page_size, 3 /* and NON_BLOCK */);
+ recorder->page_size, SPLICE_F_MOVE | SPLICE_F_NONBLOCK);
if (ret < 0) {
if (errno != EAGAIN) {
warning("recorder error in splice output");
^ permalink raw reply related
* [PATCH 5/5] trace-cmd: Use polling function
From: Yoshihiro YUNOMAE @ 2012-08-22 8:43 UTC (permalink / raw)
To: Steven Rostedt
Cc: Herbert Xu, Arnd Bergmann, qemu-devel, Frederic Weisbecker,
linux-kernel, Borislav Petkov, virtualization, Masami Hiramatsu,
Franch Ch. Eigler, Ingo Molnar, Mathieu Desnoyers,
Anthony Liguori, Greg Kroah-Hartman, Amit Shah, yrl.pp-manager.tt
In-Reply-To: <20120822084251.17293.69086.stgit@ltc189.sdl.hitachi.co.jp>
Use poll() for avoiding a busy loop to read trace data of a guest from FIFO.
Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
---
trace-recorder.c | 42 ++++++++++++++++++++++++++++++++++++------
1 files changed, 36 insertions(+), 6 deletions(-)
diff --git a/trace-recorder.c b/trace-recorder.c
index 6577fe8..bdf9798 100644
--- a/trace-recorder.c
+++ b/trace-recorder.c
@@ -34,9 +34,12 @@
#include <ctype.h>
#include <errno.h>
#include <stdbool.h>
+#include <poll.h>
#include "trace-cmd.h"
+#define WAIT_MSEC 1
+
struct tracecmd_recorder {
int fd;
int trace_fd;
@@ -235,9 +238,37 @@ static void stop_operation_to_trace_agent(int ctl_fd)
operation_to_trace_agent(ctl_fd, false);
}
-int tracecmd_start_recording(struct tracecmd_recorder *recorder, unsigned long sleep)
+static int wait_data(struct tracecmd_recorder *recorder, unsigned long sleep)
{
+ struct pollfd poll_fd;
struct timespec req;
+ int ret = 0;
+
+ if (recorder->agent_existing) {
+ poll_fd.fd = recorder->trace_fd;
+ poll_fd.events = POLLIN;
+ while (1) {
+ ret = poll(&poll_fd, 1, WAIT_MSEC);
+
+ if(ret < 0) {
+ warning("polling error");
+ return ret;
+ }
+
+ if (ret)
+ break;
+ }
+ } else if (sleep) {
+ req.tv_sec = sleep / 1000000;
+ req.tv_nsec = (sleep % 1000000) * 1000;
+ nanosleep(&req, NULL);
+ }
+
+ return ret;
+}
+
+int tracecmd_start_recording(struct tracecmd_recorder *recorder, unsigned long sleep)
+{
long ret;
recorder->stop = 0;
@@ -246,11 +277,10 @@ int tracecmd_start_recording(struct tracecmd_recorder *recorder, unsigned long s
run_operation_to_trace_agent(recorder->ctl_fd);
do {
- if (sleep) {
- req.tv_sec = sleep / 1000000;
- req.tv_nsec = (sleep % 1000000) * 1000;
- nanosleep(&req, NULL);
- }
+ ret = wait_data(recorder, sleep);
+ if (ret < 0)
+ return ret;
+
ret = splice_data(recorder);
if (ret < 0)
return ret;
^ permalink raw reply related
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Michael S. Tsirkin @ 2012-08-22 9:33 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120822011930.GA23753@t510.redhat.com>
On Tue, Aug 21, 2012 at 10:19:31PM -0300, Rafael Aquini wrote:
> On Wed, Aug 22, 2012 at 03:07:41AM +0300, Michael S. Tsirkin wrote:
> > On Tue, Aug 21, 2012 at 05:45:56PM -0300, Rafael Aquini wrote:
> > > On Tue, Aug 21, 2012 at 10:30:31PM +0300, Michael S. Tsirkin wrote:
> > > > On Tue, Aug 21, 2012 at 04:23:58PM -0300, Rafael Aquini wrote:
> > > > > On Tue, Aug 21, 2012 at 10:13:30PM +0300, Michael S. Tsirkin wrote:
> > > > > > >
> > > > > > > I believe rcu_dereference_protected() is what I want/need here, since this code
> > > > > > > is always called for pages which we hold locked (PG_locked bit).
> > > > > >
> > > > > > It would only help if we locked the page while updating the mapping,
> > > > > > as far as I can see we don't.
> > > > > >
> > > > >
> > > > > But we can do it. In fact, by doing it (locking the page) we can easily avoid
> > > > > the nasty race balloon_isolate_page / leak_balloon, in a much simpler way, IMHO.
> > > >
> > > > Absolutely. Further, we should look hard at whether most RCU uses
> > > > in this patchset can be replaced with page lock.
> > > >
> > >
> > > Yeah, In fact, by testing/grabbing the page lock at leak_balloon() even the
> > > module unload X migration / putback race seems to fade away, since migration
> > > code holds the page locked all the way.
> > > And that seems a quite easy task to be accomplished:
> > >
> > > ....
> > > @@ -169,21 +197,61 @@ static void leak_balloon(struct virtio_balloon *vb, size_t
> > > num)
> > > /* We can only do one array worth at a time. */
> > > num = min(num, ARRAY_SIZE(vb->pfns));
> > >
> > > + mutex_lock(&vb->balloon_lock);
> > > for (vb->num_pfns = 0; vb->num_pfns < num;
> > > vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> > > + spin_lock(&vb->pages_lock);
> > > + /*
> > > + * 'virtballoon_isolatepage()' can drain vb->pages list
> > > + * making us to stumble across a _temporarily_ empty list.
> >
> > This still worries me. If this happens we do not
> > lock the page so module can go away?
> > if not need to document why.
> >
> The module won't unload unless it leaks all its pages. If we hit that test that
> worries you, leak_balloon() will get back to its caller -- remove_common(), and
> it will kept looping at:
>
> /* There might be pages left in the balloon: free them. */
> while (vb->num_pages)
> leak_balloon(vb, vb->num_pages);
>
> This is true because we do not mess with vb->num_pages while isolating/migrating
> balloon pages, so the module will only unload when all isolated pages get back
> to vb->pages_list and leak_balloon() reap them appropriatelly. As we will be
> doing isolation/migration/putback steps under 'page lock' that race is gone.
Hmm, so this will busy wait which is unelegant.
We need some event IMO.
Also, reading num_pages without a lock here
which seems wrong.
A similar concern applies to normal leaking
of the balloon: here we might leak less than
required, then wait for the next config change
event.
How about we signal config_change
event when pages are back to pages_list?
--
MST
^ permalink raw reply
* Re: [PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
From: Cong Meng @ 2012-08-22 11:04 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: stefanha, zwanp, linuxram, qemu-devel, virtualization
In-Reply-To: <50335F78.1030005@redhat.com>
On 08/21/2012 06:14 PM, Paolo Bonzini wrote:
> Il 21/08/2012 11:52, Stefan Hajnoczi ha scritto:
>>>>>> Using /sys/dev/block or /sys/dev/char seems easier, and lets you
>>>>>> retrieve the parameters for block devices too.
>>>>>>
>>>> what do you mean with "block devices"? Using "/dev/sda" instead of
>>>> "/dev/sg0"?
>
> Yes.
>
>>>>>> However, I'm worried of the consequences this has for migration. You
>>>>>> could have the same physical disk accessed with two different HBAs, with
>>>>>> different limits. So I don't know if this can really be solved at all.
>>>>>>
>>>> I know little about qemu migration now. The pending scsi commands will be
>>>> saved and
>>>> transfered to remote machine when starting migration?
>>
>> Passthrough is already a migration blocker if both hosts do not have
>> access to the same LUNs.
>
> Yes, but requiring the exact same hardware may be too much. I'm trying
> to understand the problem better before committing to a threefold
> spec/qemu/kernel change.
>
> Cong, what is the limit that the host HBA enforces (and what is the
> HBA)? What commands see a problem? Is it fixed by using scsi-block
> instead of scsi-generic (if you can use scsi-block at all, i.e. it's not
> a tape or similar device)?
>
I don't see real problem caused by the the queue limits actually. It's a
bug which Stefan told me.
> With scsi-generic, QEMU uses a bounce buffer for non-I/O commands to a
> SCSI passthrough device, so the only problem in that case should be the
> maximum segment size. This could change in the future, but max_segments
> and max_sectors should not yet be a problem.
about bounce buffer, do you meat the buffer allocated in
scsi_send_command() of hw/scsi-generic.c?
Cong.
>
> With scsi-block, QEMU will use read/write on the block device and the
> host kernel will then obey the host HBA's block limits. QEMU will still
> use a bounce buffer for non-I/O commands to a scsi-block device, but the
> payload is usually small for non-I/O commands.
>
> Paolo
>
>> When both hosts do have access to the same LUNs it's possible to
>> extract the block queue limits (using sysfs) and compare them.
>>
>> Today you can start QEMU with different image files on both hosts.
>> Migration will appear to work but the disk image on the destination
>> host could be junk. This is a similar case, I don't see a problem
>> except that there should be a safety check (maybe at the libvirt
>> level) to make this safe.
>
^ permalink raw reply
* Re: [PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
From: Paolo Bonzini @ 2012-08-22 12:09 UTC (permalink / raw)
To: Cong Meng; +Cc: stefanha, zwanp, linuxram, qemu-devel, virtualization
In-Reply-To: <5034BCD1.9020603@linux.vnet.ibm.com>
Il 22/08/2012 13:04, Cong Meng ha scritto:
>>
>> Cong, what is the limit that the host HBA enforces (and what is the
>> HBA)? What commands see a problem? Is it fixed by using scsi-block
>> instead of scsi-generic (if you can use scsi-block at all, i.e. it's not
>> a tape or similar device)?
>>
> I don't see real problem caused by the the queue limits actually. It's a
> bug which Stefan told me.
I'd rather avoid patching the specification if not to solve real (rather
than known-but-theoretical) problems.
>> With scsi-generic, QEMU uses a bounce buffer for non-I/O commands to a
>> SCSI passthrough device, so the only problem in that case should be the
>> maximum segment size. This could change in the future, but max_segments
>> and max_sectors should not yet be a problem.
>
> about bounce buffer, do you meat the buffer allocated in
> scsi_send_command() of hw/scsi-generic.c?
Yes.
Paolo
^ permalink raw reply
* Re: [PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
From: Stefan Hajnoczi @ 2012-08-22 13:13 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: zwanp, linuxram, qemu-devel, virtualization, Cong Meng
In-Reply-To: <5034CBF8.3050602@redhat.com>
On Wed, Aug 22, 2012 at 02:09:28PM +0200, Paolo Bonzini wrote:
> Il 22/08/2012 13:04, Cong Meng ha scritto:
> >>
> >> Cong, what is the limit that the host HBA enforces (and what is the
> >> HBA)? What commands see a problem? Is it fixed by using scsi-block
> >> instead of scsi-generic (if you can use scsi-block at all, i.e. it's not
> >> a tape or similar device)?
> >>
> > I don't see real problem caused by the the queue limits actually. It's a
> > bug which Stefan told me.
>
> I'd rather avoid patching the specification if not to solve real (rather
> than known-but-theoretical) problems.
Benjamin Herrenschmidt reported this in problem in 2010:
http://lists.gnu.org/archive/html/qemu-devel/2010-12/msg01741.html
"This is a real problem in practice. IE. the USB CD-ROM on this POWER7
blade limits transfers to 0x1e000 bytes for example and the Linux "sr"
driver on the guest is going to try to give me bigger requests than that
if I don't start limiting them, which will cause all sort of errors."
It cannot be fixed for emulated SCSI HBAs in general but it can for
virtio-scsi.
I/O requests will be failed with EIO if they exceed block queue limits.
Take a look at block/blk-core.c:generic_make_request_checks() and
blk_rq_check_limits().
Cong: Have you checked the block queue limits on your test machine and
exceeded them to see what happens?
Stefan
^ permalink raw reply
* Re: [PATCH 1/5] trace-cmd: Use TRACE_DIR envrionment variable if defined
From: Steven Rostedt @ 2012-08-22 13:37 UTC (permalink / raw)
To: Yoshihiro YUNOMAE
Cc: Herbert Xu, Arnd Bergmann, qemu-devel, Frederic Weisbecker,
linux-kernel, Borislav Petkov, virtualization, Masami Hiramatsu,
Franch Ch. Eigler, Ingo Molnar, Mathieu Desnoyers,
Anthony Liguori, Greg Kroah-Hartman, Amit Shah, yrl.pp-manager.tt
In-Reply-To: <20120822084301.17293.90649.stgit@ltc189.sdl.hitachi.co.jp>
On Wed, 2012-08-22 at 17:43 +0900, Yoshihiro YUNOMAE wrote:
> From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>
> Use TRACE_DIR environment variable for setting
TRACING_DIR would be better, as we are searching for /debug/tracing and
not /debug/trace. Perhaps DEBUG_TRACING_DIR would be even better as to
be less of a generic term.
-- Steve
> debugfs/tracing directory if defined. This is
> for controlling guest(or remote) ftrace.
>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
> ---
>
> trace-util.c | 9 +++++++++
> 1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/trace-util.c b/trace-util.c
> index e128188..d5a3eb4 100644
> --- a/trace-util.c
> +++ b/trace-util.c
> @@ -311,6 +311,15 @@ char *tracecmd_find_tracing_dir(void)
> char type[100];
> FILE *fp;
>
> + tracing_dir = getenv("TRACE_DIR");
> + if (tracing_dir) {
> + tracing_dir = strdup(tracing_dir);
> + if (!tracing_dir)
> + die("malloc");
> + warning("Use environmental tracing directory: %s\n", tracing_dir);
> + return tracing_dir;
> + }
> +
> if ((fp = fopen("/proc/mounts","r")) == NULL) {
> warning("Can't open /proc/mounts for read");
> return NULL;
>
^ permalink raw reply
* Re: [PATCH 2/5] trace-cmd: Use tracing directory to count CPUs
From: Steven Rostedt @ 2012-08-22 13:41 UTC (permalink / raw)
To: Yoshihiro YUNOMAE
Cc: Herbert Xu, Arnd Bergmann, qemu-devel, Frederic Weisbecker,
linux-kernel, Borislav Petkov, virtualization, Masami Hiramatsu,
Franch Ch. Eigler, Ingo Molnar, Mathieu Desnoyers,
Anthony Liguori, Greg Kroah-Hartman, Amit Shah, yrl.pp-manager.tt
In-Reply-To: <20120822084312.17293.47596.stgit@ltc189.sdl.hitachi.co.jp>
On Wed, 2012-08-22 at 17:43 +0900, Yoshihiro YUNOMAE wrote:
> From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>
> Count debugfs/tracing/per_cpu/cpu* to determine the
> number of CPUs.
I'm curious, do you find that sysconf doesn't return the # of CPUs the
system has? I've had boxes where the per_cpu/cpu* had more cpus than the
box actually holds. But this was a bug in the kernel, not the tool. This
change log needs to have rational instead of just explaining what the
patch does.
Thanks,
-- Steve
>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
> ---
^ permalink raw reply
* Re: [PATCH 3/5] trace-cmd: Support trace-agent of virtio-trace
From: Steven Rostedt @ 2012-08-22 13:51 UTC (permalink / raw)
To: Yoshihiro YUNOMAE
Cc: Herbert Xu, Arnd Bergmann, qemu-devel, Frederic Weisbecker,
linux-kernel, Borislav Petkov, virtualization, Masami Hiramatsu,
Franch Ch. Eigler, Ingo Molnar, Mathieu Desnoyers,
Anthony Liguori, Greg Kroah-Hartman, Amit Shah, yrl.pp-manager.tt
In-Reply-To: <20120822084322.17293.70186.stgit@ltc189.sdl.hitachi.co.jp>
On Wed, 2012-08-22 at 17:43 +0900, Yoshihiro YUNOMAE wrote:
> Add read path and control path to use trace-agent of virtio-trace.
> When we use trace-agent, trace-cmd will be used as follows:
> # AGENT_READ_DIR=/tmp/virtio-trace/tracing \
> AGENT_CTL=/tmp/virtio-trace/agent-ctl-path.in \
> TRACING_DIR=/tmp/virtio-trace/debugfs/tracing \\
Ha! You used "TRACING_DIR" but patch one introduces TRACE_DIR. Lets
change this to DEBUG_TRACING_DIR instead anyway.
Also, I don't like the generic environment variables. Perhaps
VIRTIO_TRACE_DIR, or AGENT_TRACE_DIR and AGENT_TRACE_CTL. Lets try to
keep the environment namespace sparse.
> trace-cmd record -e "sched:*"
> Here, AGENT_READ_DIR is the path for a reading directory of virtio-trace,
> AGENT_CTL is a control path of trace-agent, and TRACING_DIR is a debugfs path
> of a guest.
>
> Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
> ---
>
> trace-cmd.h | 1 +
> trace-recorder.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> trace-util.c | 18 +++++++++++++++++
> 3 files changed, 75 insertions(+), 1 deletions(-)
>
> diff --git a/trace-cmd.h b/trace-cmd.h
> index f904dc5..75506ed 100644
> --- a/trace-cmd.h
> +++ b/trace-cmd.h
> @@ -72,6 +72,7 @@ static inline int tracecmd_host_bigendian(void)
> }
>
> char *tracecmd_find_tracing_dir(void);
> +char *guest_agent_tracing_read_dir(void);
>
> /* --- Opening and Reading the trace.dat file --- */
>
> diff --git a/trace-recorder.c b/trace-recorder.c
> index 215affc..3b750e9 100644
> --- a/trace-recorder.c
> +++ b/trace-recorder.c
> @@ -33,6 +33,7 @@
> #include <unistd.h>
> #include <ctype.h>
> #include <errno.h>
> +#include <stdbool.h>
>
> #include "trace-cmd.h"
>
> @@ -43,6 +44,8 @@ struct tracecmd_recorder {
> int page_size;
> int cpu;
> int stop;
> + int ctl_fd;
> + bool agent_existing;
Thanks for the reminder. I need to convert a lot to use 'bool' instead.
> };
>
> void tracecmd_free_recorder(struct tracecmd_recorder *recorder)
> @@ -59,11 +62,29 @@ void tracecmd_free_recorder(struct tracecmd_recorder *recorder)
> free(recorder);
> }
>
> +static char *use_trace_agent_dir(char *ctl_path,
> + struct tracecmd_recorder *recorder)
> +{
> + ctl_path = strdup(ctl_path);
> + if (!ctl_path)
> + die("malloc");
> + warning("Use environmental control path: %s\n", ctl_path);
s/Use/Using/
-- Steve
> +
> + recorder->ctl_fd = open(ctl_path, O_WRONLY);
> + if (recorder->ctl_fd < 0)
> + return NULL;
> +
> + recorder->agent_existing = true;
> +
> + return guest_agent_tracing_read_dir();
> +}
> +
^ permalink raw reply
* Re: [PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
From: Paolo Bonzini @ 2012-08-22 14:13 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: zwanp, linuxram, qemu-devel, virtualization, Cong Meng
In-Reply-To: <20120822131348.GA3512@stefanha-thinkpad.localdomain>
Il 22/08/2012 15:13, Stefan Hajnoczi ha scritto:
> http://lists.gnu.org/archive/html/qemu-devel/2010-12/msg01741.html
>
> "This is a real problem in practice. IE. the USB CD-ROM on this POWER7
> blade limits transfers to 0x1e000 bytes for example and the Linux "sr"
> driver on the guest is going to try to give me bigger requests than that
> if I don't start limiting them, which will cause all sort of errors."
>
> It cannot be fixed for emulated SCSI HBAs in general but it can for
> virtio-scsi.
For disks, this should be fixed simply by using scsi-block instead of
scsi-generic.
CD-ROMs are indeed more complicated because burning CDs cannot be done
with syscalls. :/
Paolo
^ permalink raw reply
* RE: [PATCH V3 08/14] Tools: hv: Gather DNS information
From: KY Srinivasan @ 2012-08-22 15:45 UTC (permalink / raw)
To: Greg KH
Cc: linux-kernel@vger.kernel.org, devel@linuxdriverproject.org,
virtualization@lists.osdl.org, olaf@aepfle.de, apw@canonical.com,
ben@decadent.org.uk, Thomas Graf (tgraf@redhat.com)
In-Reply-To: <20120817165109.GA11323@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Friday, August 17, 2012 12:51 PM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; olaf@aepfle.de; apw@canonical.com;
> ben@decadent.org.uk
> Subject: Re: [PATCH V3 08/14] Tools: hv: Gather DNS information
>
> On Thu, Aug 16, 2012 at 06:32:19PM -0700, K. Y. Srinivasan wrote:
> > Now, gather DNS information. Invoke an external script (that can be
> > distro dependent) to gather the DNS information.
>
> Where is that script?
If you think it is useful, I could send you the scripts that I have used to test this
functionality as example scripts.
> Why a script, this really isn't in a standard
> method for all Linux distros? If not, why not work to do that like the
> freedesktop.org people have done for other system configuration items?
When discussing this while preparing the patches it was clear that there
really is no single way to obtain the network information, if you are
using network manager you need to use dbus, basic debian networking as
on a server you need to look in /etc/resolv.conf, if you have dnsmasq
installed you need to look in a third place. On the SLES side that I am somewhat
familiar with, if I recall correctly, none of the appliance images have Network Manager installed.
The consensus was that we should avoid pulling a vast pile of distro smarts into the source which
lead to the hook API proposed in these patches.
The approach chosen here does not preclude using Network Manager in the external scripts
if a particular distro chooses to go that route. The current implementation supports maximum
flexibility without sacrificing anything. While the current scripts I have do not use NM APIs, we could
work on external scripts using DBUS APIs.
If it is ok with you, I could send you the remaining 7 patches along with the scripts I am
currently using to test this code. These scripts should be viewed just example code.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 2/5] trace-cmd: Use tracing directory to count CPUs
From: Masami Hiramatsu @ 2012-08-23 2:01 UTC (permalink / raw)
To: Steven Rostedt
Cc: Herbert Xu, Arnd Bergmann, qemu-devel, Frederic Weisbecker,
linux-kernel, Borislav Petkov, virtualization, Franch Ch. Eigler,
Ingo Molnar, Mathieu Desnoyers, Anthony Liguori,
Greg Kroah-Hartman, Amit Shah, yrl.pp-manager.tt
In-Reply-To: <1345642893.5069.31.camel@gandalf.local.home>
(2012/08/22 22:41), Steven Rostedt wrote:
> On Wed, 2012-08-22 at 17:43 +0900, Yoshihiro YUNOMAE wrote:
>> From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>>
>> Count debugfs/tracing/per_cpu/cpu* to determine the
>> number of CPUs.
>
> I'm curious, do you find that sysconf doesn't return the # of CPUs the
> system has?
No, sysconf returns the number of hosts CPUs, not guests.
> I've had boxes where the per_cpu/cpu* had more cpus than the
> box actually holds. But this was a bug in the kernel, not the tool. This
> change log needs to have rational instead of just explaining what the
> patch does.
Ah, I see. Hmm, then this should be enabled by a command line
option or an environment variable.
Thank you,
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Rafael Aquini @ 2012-08-23 2:19 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120822093317.GC10680@redhat.com>
On Wed, Aug 22, 2012 at 12:33:17PM +0300, Michael S. Tsirkin wrote:
> Hmm, so this will busy wait which is unelegant.
> We need some event IMO.
No, it does not busy wait. leak_balloon() is mutual exclusive with migration
steps, so for the case we have one racing against the other, we really want
leak_balloon() dropping the mutex temporarily to allow migration complete its
work of refilling vb->pages list. Also, leak_balloon() calls tell_host(), which
will potentially make it to schedule for each round of vb->pfns leak_balloon()
will release. So, when remove_common() calls leak_balloon() looping on
vb->num_pages, that won't become a tight loop.
The scheme was apparently working before this series, and it will remain working
after it.
> Also, reading num_pages without a lock here
> which seems wrong.
I'll protect it with vb->balloon_lock mutex. That will be consistent with the
lock protection scheme this patch is introducing for struct virtio_balloon
elements.
> A similar concern applies to normal leaking
> of the balloon: here we might leak less than
> required, then wait for the next config change
> event.
Just as before, same thing here. If you leaked less than required, balloon()
will keep calling leak_balloon() until the balloon target is reached. This
scheme was working before, and it will keep working after this patch.
> How about we signal config_change
> event when pages are back to pages_list?
I really don't know what to tell you here, but, to me, it seems like an
overcomplication that isn't directly entangled with this patch purposes.
Besides, you cannot expect compation / migration happening and racing against
leak_balloon() all the time to make them signal events to the later, so we might
just be creating a wait-forever condition for leak_balloon(), IMHO.
Cheers!
^ permalink raw reply
* Re: [PATCH 2/5] trace-cmd: Use tracing directory to count CPUs
From: Masami Hiramatsu @ 2012-08-23 3:00 UTC (permalink / raw)
To: Steven Rostedt
Cc: Herbert Xu, Arnd Bergmann, qemu-devel, Frederic Weisbecker,
linux-kernel, Borislav Petkov, virtualization, Franch Ch. Eigler,
Ingo Molnar, Mathieu Desnoyers, Anthony Liguori,
Greg Kroah-Hartman, Amit Shah, yrl.pp-manager.tt
In-Reply-To: <50358F13.1030607@hitachi.com>
(2012/08/23 11:01), Masami Hiramatsu wrote:
> (2012/08/22 22:41), Steven Rostedt wrote:
>> On Wed, 2012-08-22 at 17:43 +0900, Yoshihiro YUNOMAE wrote:
>>> From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>>>
>>> Count debugfs/tracing/per_cpu/cpu* to determine the
>>> number of CPUs.
>>
>> I'm curious, do you find that sysconf doesn't return the # of CPUs the
>> system has?
>
> No, sysconf returns the number of hosts CPUs, not guests.
>
>> I've had boxes where the per_cpu/cpu* had more cpus than the
>> box actually holds. But this was a bug in the kernel, not the tool. This
>> change log needs to have rational instead of just explaining what the
>> patch does.
>
> Ah, I see. Hmm, then this should be enabled by a command line
> option or an environment variable.
Oops, I misunderstood. I'll add more comment for why this
should be tried instead of sysconf.
Thank you,
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com
^ permalink raw reply
* Re: Re: [PATCH 3/5] trace-cmd: Support trace-agent of virtio-trace
From: Yoshihiro YUNOMAE @ 2012-08-23 3:13 UTC (permalink / raw)
To: Steven Rostedt
Cc: Herbert Xu, Arnd Bergmann, qemu-devel, Frederic Weisbecker,
linux-kernel, Borislav Petkov, virtualization, Masami Hiramatsu,
Franch Ch. Eigler, Ingo Molnar, Mathieu Desnoyers,
Anthony Liguori, Greg Kroah-Hartman, Amit Shah, yrl.pp-manager.tt
In-Reply-To: <1345643505.5069.36.camel@gandalf.local.home>
Hi Steven,
(2012/08/22 22:51), Steven Rostedt wrote:
> On Wed, 2012-08-22 at 17:43 +0900, Yoshihiro YUNOMAE wrote:
>> Add read path and control path to use trace-agent of virtio-trace.
>> When we use trace-agent, trace-cmd will be used as follows:
>> # AGENT_READ_DIR=/tmp/virtio-trace/tracing \
>> AGENT_CTL=/tmp/virtio-trace/agent-ctl-path.in \
>> TRACING_DIR=/tmp/virtio-trace/debugfs/tracing \\
>
> Ha! You used "TRACING_DIR" but patch one introduces TRACE_DIR. Lets
> change this to DEBUG_TRACING_DIR instead anyway.
Oh, sorry for the confusion.
> Also, I don't like the generic environment variables. Perhaps
> VIRTIO_TRACE_DIR, or AGENT_TRACE_DIR and AGENT_TRACE_CTL. Lets try to
> keep the environment namespace sparse.
OK, I'll change these name of environment variables as follows:
AGENT_READ_DIR
AGENT_TRACE_CTL
GUEST_TRACING_DIR
>> trace-cmd record -e "sched:*"
>> Here, AGENT_READ_DIR is the path for a reading directory of virtio-trace,
>> AGENT_CTL is a control path of trace-agent, and TRACING_DIR is a debugfs path
>> of a guest.
>>
>> Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
>> ---
>>
>> trace-cmd.h | 1 +
>> trace-recorder.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> trace-util.c | 18 +++++++++++++++++
>> 3 files changed, 75 insertions(+), 1 deletions(-)
>>
>> diff --git a/trace-cmd.h b/trace-cmd.h
>> index f904dc5..75506ed 100644
>> --- a/trace-cmd.h
>> +++ b/trace-cmd.h
>> @@ -72,6 +72,7 @@ static inline int tracecmd_host_bigendian(void)
>> }
>>
>> char *tracecmd_find_tracing_dir(void);
>> +char *guest_agent_tracing_read_dir(void);
>>
>> /* --- Opening and Reading the trace.dat file --- */
>>
>> diff --git a/trace-recorder.c b/trace-recorder.c
>> index 215affc..3b750e9 100644
>> --- a/trace-recorder.c
>> +++ b/trace-recorder.c
>> @@ -33,6 +33,7 @@
>> #include <unistd.h>
>> #include <ctype.h>
>> #include <errno.h>
>> +#include <stdbool.h>
>>
>> #include "trace-cmd.h"
>>
>> @@ -43,6 +44,8 @@ struct tracecmd_recorder {
>> int page_size;
>> int cpu;
>> int stop;
>> + int ctl_fd;
>> + bool agent_existing;
>
> Thanks for the reminder. I need to convert a lot to use 'bool' instead.
I'll change 'int' just for flag to use 'bool' as much as possible
after finishing this patch set.
>> };
>>
>> void tracecmd_free_recorder(struct tracecmd_recorder *recorder)
>> @@ -59,11 +62,29 @@ void tracecmd_free_recorder(struct tracecmd_recorder *recorder)
>> free(recorder);
>> }
>>
>> +static char *use_trace_agent_dir(char *ctl_path,
>> + struct tracecmd_recorder *recorder)
>> +{
>> + ctl_path = strdup(ctl_path);
>> + if (!ctl_path)
>> + die("malloc");
>> + warning("Use environmental control path: %s\n", ctl_path);
>
> s/Use/Using/
OK, I'll correct this.
Thank you,
--
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: yoshihiro.yunomae.ez@hitachi.com
^ permalink raw reply
* Re: [PATCH 2/5] trace-cmd: Use tracing directory to count CPUs
From: Steven Rostedt @ 2012-08-23 9:08 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Herbert Xu, Arnd Bergmann, qemu-devel, Frederic Weisbecker,
linux-kernel, Borislav Petkov, virtualization, Franch Ch. Eigler,
Ingo Molnar, Mathieu Desnoyers, Anthony Liguori,
Greg Kroah-Hartman, Amit Shah, yrl.pp-manager.tt
In-Reply-To: <50359CE9.3000509@hitachi.com>
On Thu, 2012-08-23 at 12:00 +0900, Masami Hiramatsu wrote:
> (2012/08/23 11:01), Masami Hiramatsu wrote:
> > (2012/08/22 22:41), Steven Rostedt wrote:
> >> On Wed, 2012-08-22 at 17:43 +0900, Yoshihiro YUNOMAE wrote:
> >>> From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> >>>
> >>> Count debugfs/tracing/per_cpu/cpu* to determine the
> >>> number of CPUs.
> >>
> >> I'm curious, do you find that sysconf doesn't return the # of CPUs the
> >> system has?
> >
> > No, sysconf returns the number of hosts CPUs, not guests.
> >
> >> I've had boxes where the per_cpu/cpu* had more cpus than the
> >> box actually holds. But this was a bug in the kernel, not the tool. This
> >> change log needs to have rational instead of just explaining what the
> >> patch does.
> >
> > Ah, I see. Hmm, then this should be enabled by a command line
> > option or an environment variable.
>
> Oops, I misunderstood. I'll add more comment for why this
> should be tried instead of sysconf.
And now that I understand why you are doing this, why not only do this
if the TRACE_AGENT or DEBUG_TRACING_DIR is defined. That is, if we are
doing it against a bare metal system, then sysconf should suffice, but
if we are tracing against a guest, then it should use the tracing
directory to determine the buffers.
We could add options to override this, but I would think the default
should just Do The Right Thing(tm).
-- Steve
^ permalink raw reply
* Re: [PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
From: Cong Meng @ 2012-08-23 9:31 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Stefan Hajnoczi, zwanp, linuxram, qemu-devel, virtualization
In-Reply-To: <5034E918.4030305@redhat.com>
On Wed 22 Aug 2012 10:13:44 PM CST, Paolo Bonzini wrote:
> Il 22/08/2012 15:13, Stefan Hajnoczi ha scritto:
>> http://lists.gnu.org/archive/html/qemu-devel/2010-12/msg01741.html
>>
>> "This is a real problem in practice. IE. the USB CD-ROM on this POWER7
>> blade limits transfers to 0x1e000 bytes for example and the Linux "sr"
>> driver on the guest is going to try to give me bigger requests than that
>> if I don't start limiting them, which will cause all sort of errors."
>>
>> It cannot be fixed for emulated SCSI HBAs in general but it can for
>> virtio-scsi.
>
> For disks, this should be fixed simply by using scsi-block instead of
> scsi-generic.
>
> CD-ROMs are indeed more complicated because burning CDs cannot be done
> with syscalls. :/
>
So, as the problem exist to CD-ROM, I will continue to get these
patches move on.
As Paolo pointed out, the only limit is max_sectors (the total size of
a scatter-list),
I will drop the other 2 parameters.
Paolo, what's your opinion?
Cong
> Paolo
>
^ permalink raw reply
* Re: [PATCH v8 1/5] mm: introduce a common interface for balloon pages mobility
From: Michael S. Tsirkin @ 2012-08-23 10:01 UTC (permalink / raw)
To: Rafael Aquini
Cc: Rik van Riel, Konrad Rzeszutek Wilk, Peter Zijlstra, linux-kernel,
virtualization, linux-mm, Andi Kleen, Minchan Kim, Andrew Morton,
Paul E. McKenney
In-Reply-To: <20120823021903.GA23660@x61.redhat.com>
On Wed, Aug 22, 2012 at 11:19:04PM -0300, Rafael Aquini wrote:
> On Wed, Aug 22, 2012 at 12:33:17PM +0300, Michael S. Tsirkin wrote:
> > Hmm, so this will busy wait which is unelegant.
> > We need some event IMO.
>
> No, it does not busy wait. leak_balloon() is mutual exclusive with migration
> steps, so for the case we have one racing against the other, we really want
> leak_balloon() dropping the mutex temporarily to allow migration complete its
> work of refilling vb->pages list. Also, leak_balloon() calls tell_host(), which
> will potentially make it to schedule for each round of vb->pfns leak_balloon()
> will release.
tell_host might not even cause an exit to host. Even if it does
it does not involve guest scheduler.
> So, when remove_common() calls leak_balloon() looping on
> vb->num_pages, that won't become a tight loop.
> The scheme was apparently working before this series, and it will remain working
> after it.
It seems that before we would always leak all requested memory
in one go. I can't tell why we have a while loop there at all.
Rusty, could you clarify please?
>
> > Also, reading num_pages without a lock here
> > which seems wrong.
>
> I'll protect it with vb->balloon_lock mutex. That will be consistent with the
> lock protection scheme this patch is introducing for struct virtio_balloon
> elements.
>
>
> > A similar concern applies to normal leaking
> > of the balloon: here we might leak less than
> > required, then wait for the next config change
> > event.
>
> Just as before, same thing here. If you leaked less than required, balloon()
> will keep calling leak_balloon() until the balloon target is reached. This
> scheme was working before, and it will keep working after this patch.
>
IIUC we never hit this path before.
> > How about we signal config_change
> > event when pages are back to pages_list?
>
> I really don't know what to tell you here, but, to me, it seems like an
> overcomplication that isn't directly entangled with this patch purposes.
> Besides, you cannot expect compation / migration happening and racing against
> leak_balloon() all the time to make them signal events to the later, so we might
> just be creating a wait-forever condition for leak_balloon(), IMHO.
So use wait_event or similar, check for existance of isolated pages.
> Cheers!
^ permalink raw reply
* Re: [PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
From: Paolo Bonzini @ 2012-08-23 10:03 UTC (permalink / raw)
To: Cong Meng; +Cc: Stefan Hajnoczi, zwanp, linuxram, qemu-devel, virtualization
In-Reply-To: <5035F873.6090305@linux.vnet.ibm.com>
Il 23/08/2012 11:31, Cong Meng ha scritto:
>> For disks, this should be fixed simply by using scsi-block instead of
>> scsi-generic.
>>
>> CD-ROMs are indeed more complicated because burning CDs cannot be done
>> with syscalls. :/
>
> So, as the problem exist to CD-ROM, I will continue to get these patches
> move on.
I'm still trying to understand the extent of the problem.
The problem occurs for _USB_ CD-ROMs according to Ben. Passthrough of
USB storage devices should be done via USB passthrough, not virtio-scsi.
If we do USB passthrough via the SCSI layer we miss on all the quirks
that the OS may do based on the USB product/vendor pairs. There's no
end to these, and some of the quirks may cause the device to lock up or
corruption.
I'd rather see a reproducer using SAS/ATA/ATAPI disks before punting.
Paolo
^ permalink raw reply
* Re: [PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
From: Stefan Hajnoczi @ 2012-08-23 10:08 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Stefan Hajnoczi, zwanp, linuxram, qemu-devel, virtualization,
Cong Meng
In-Reply-To: <5035FFF4.4040603@redhat.com>
On Thu, Aug 23, 2012 at 11:03 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 23/08/2012 11:31, Cong Meng ha scritto:
>>> For disks, this should be fixed simply by using scsi-block instead of
>>> scsi-generic.
>>>
>>> CD-ROMs are indeed more complicated because burning CDs cannot be done
>>> with syscalls. :/
>>
>> So, as the problem exist to CD-ROM, I will continue to get these patches
>> move on.
>
> I'm still trying to understand the extent of the problem.
>
> The problem occurs for _USB_ CD-ROMs according to Ben. Passthrough of
> USB storage devices should be done via USB passthrough, not virtio-scsi.
> If we do USB passthrough via the SCSI layer we miss on all the quirks
> that the OS may do based on the USB product/vendor pairs. There's no
> end to these, and some of the quirks may cause the device to lock up or
> corruption.
>
> I'd rather see a reproducer using SAS/ATA/ATAPI disks before punting.
This issue affects passthrough: either an entire sg device or at least
a SG_IO ioctl (e.g. a non-READ/WRITE SCSI command).
To reproduce it, check host queue limits and guest virtio-scsi queue
limits. Then pick a command that can exceed the limits and try it
from inside the guest :).
Stefan
^ permalink raw reply
* Re: [PATCH 1/2 v1] blkdrv: Add queue limits parameters for sg block drive
From: Paolo Bonzini @ 2012-08-23 10:52 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Stefan Hajnoczi, zwanp, linuxram, qemu-devel, virtualization,
Cong Meng
In-Reply-To: <CAJSP0QXG3yzxnwAJd3s0M0eSUM39XRdYEmGtj0h83_de4mxaAA@mail.gmail.com>
Il 23/08/2012 12:08, Stefan Hajnoczi ha scritto:
>> I'm still trying to understand the extent of the problem.
>>
>> The problem occurs for _USB_ CD-ROMs according to Ben. Passthrough of
>> USB storage devices should be done via USB passthrough, not virtio-scsi.
>> If we do USB passthrough via the SCSI layer we miss on all the quirks
>> that the OS may do based on the USB product/vendor pairs. There's no
>> end to these, and some of the quirks may cause the device to lock up or
>> corruption.
>>
>> I'd rather see a reproducer using SAS/ATA/ATAPI disks before punting.
>
> This issue affects passthrough: either an entire sg device or at least
> a SG_IO ioctl (e.g. a non-READ/WRITE SCSI command).
>
> To reproduce it, check host queue limits and guest virtio-scsi queue
> limits. Then pick a command that can exceed the limits and try it
> from inside the guest :).
Yes, so much is clear. But does it happen _in practice_? Do initiators
actually issue commands that are that big?
Paolo
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox