| Message ID | 1303131754-25072-3-git-send-email-levinsasha928@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 18, 2011 at 4:02 PM, Sasha Levin <levinsasha928@gmail.com> wrote: > Add --debug-io-delay-cycles and --debug-io-delay-amount to delay the completion of IO requests within virtio-blk. > This feature allows to verify and debug the threading within virtio-blk. > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com> Well, to be honest, I'm not convinced we need both of these. Isn't --debug-io-delay=<msec> enough for our use? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2011-04-19 at 19:52 +0300, Pekka Enberg wrote: > On Mon, Apr 18, 2011 at 4:02 PM, Sasha Levin <levinsasha928@gmail.com> wrote: > > Add --debug-io-delay-cycles and --debug-io-delay-amount to delay the completion of IO requests within virtio-blk. > > This feature allows to verify and debug the threading within virtio-blk. > > > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > > Well, to be honest, I'm not convinced we need both of these. Isn't > --debug-io-delay=<msec> enough for our use? This came up during our testing. Ingo suggested a large delay so we could easily see the results of threading. The problem we encountered was that having a delay right from the beginning will make the guest kernel take a rather long time to boot and would make actually testing the threading impossible. I've added a delay before the activation of the I/O request completion delay to give the tester/debugger enough time to boot into the guest and prepare anything needed for the test. Making it a constant is also rather hard because different kernels can take a very different amount of I/O requests to boot. Take the simple example of a whether fsck was running during the boot or not.
* Sasha Levin <levinsasha928@gmail.com> wrote: > On Tue, 2011-04-19 at 19:52 +0300, Pekka Enberg wrote: > > On Mon, Apr 18, 2011 at 4:02 PM, Sasha Levin <levinsasha928@gmail.com> wrote: > > > Add --debug-io-delay-cycles and --debug-io-delay-amount to delay the completion of IO requests within virtio-blk. > > > This feature allows to verify and debug the threading within virtio-blk. > > > > > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > > > > Well, to be honest, I'm not convinced we need both of these. Isn't > > --debug-io-delay=<msec> enough for our use? > > This came up during our testing. > > Ingo suggested a large delay so we could easily see the results of > threading. The problem we encountered was that having a delay right from > the beginning will make the guest kernel take a rather long time to boot > and would make actually testing the threading impossible. > > I've added a delay before the activation of the I/O request completion > delay to give the tester/debugger enough time to boot into the guest and > prepare anything needed for the test. > > Making it a constant is also rather hard because different kernels can > take a very different amount of I/O requests to boot. Take the simple > example of a whether fsck was running during the boot or not. I suspect we'll eventually want to have some sort of 'kvm set' subcommand that can modify attributes of running instances? Setting the debug delay would be one of the options: kvm set MyInstance-1 --debug-io-delay-ms 10000 That way the delay can be activated in a suitable moment - and disabled again after testing: kvm set MyInstance-1 --debug-io-delay-ms 0 ? Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/20/2011 01:11 AM, Ingo Molnar wrote: > > * Sasha Levin <levinsasha928@gmail.com> wrote: > >> On Tue, 2011-04-19 at 19:52 +0300, Pekka Enberg wrote: >>> On Mon, Apr 18, 2011 at 4:02 PM, Sasha Levin <levinsasha928@gmail.com> wrote: >>>> Add --debug-io-delay-cycles and --debug-io-delay-amount to delay the completion of IO requests within virtio-blk. >>>> This feature allows to verify and debug the threading within virtio-blk. >>>> >>>> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> >>> >>> Well, to be honest, I'm not convinced we need both of these. Isn't >>> --debug-io-delay=<msec> enough for our use? >> >> This came up during our testing. >> >> Ingo suggested a large delay so we could easily see the results of >> threading. The problem we encountered was that having a delay right from >> the beginning will make the guest kernel take a rather long time to boot >> and would make actually testing the threading impossible. >> >> I've added a delay before the activation of the I/O request completion >> delay to give the tester/debugger enough time to boot into the guest and >> prepare anything needed for the test. >> >> Making it a constant is also rather hard because different kernels can >> take a very different amount of I/O requests to boot. Take the simple >> example of a whether fsck was running during the boot or not. > > I suspect we'll eventually want to have some sort of 'kvm set' subcommand that > can modify attributes of running instances? Setting the debug delay would be > one of the options: > > kvm set MyInstance-1 --debug-io-delay-ms 10000 > > That way the delay can be activated in a suitable moment - and disabled again > after testing: > > kvm set MyInstance-1 --debug-io-delay-ms 0 > It's a very good idea. Do we need: kvm get MyInstance-1 which reports all the attributes, or kvm get MyInstance-1 --debug-io-delay-ms which only reports the interested attribute.
On Wed, Apr 20, 2011 at 2:10 AM, Asias He <asias.hejun@gmail.com> wrote: >>> This came up during our testing. >>> >>> Ingo suggested a large delay so we could easily see the results of >>> threading. The problem we encountered was that having a delay right from >>> the beginning will make the guest kernel take a rather long time to boot >>> and would make actually testing the threading impossible. >>> >>> I've added a delay before the activation of the I/O request completion >>> delay to give the tester/debugger enough time to boot into the guest and >>> prepare anything needed for the test. >>> >>> Making it a constant is also rather hard because different kernels can >>> take a very different amount of I/O requests to boot. Take the simple >>> example of a whether fsck was running during the boot or not. >> >> I suspect we'll eventually want to have some sort of 'kvm set' subcommand that >> can modify attributes of running instances? Setting the debug delay would be >> one of the options: >> >> kvm set MyInstance-1 --debug-io-delay-ms 10000 >> >> That way the delay can be activated in a suitable moment - and disabled again >> after testing: >> >> kvm set MyInstance-1 --debug-io-delay-ms 0 >> > > It's a very good idea. > > Do we need: > > kvm get MyInstance-1 > > which reports all the attributes, or > > kvm get MyInstance-1 --debug-io-delay-ms > > which only reports the interested attribute. Sorry for the bikeshedding but wouldn't it be better to follow Git's lead and have something like kvm config MyInstance-1 --set debug.io.delay.ms 100 and kvm config MyInstance-1 --list Pekka -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Pekka Enberg <penberg@kernel.org> wrote: > Sorry for the bikeshedding but wouldn't it be better to follow Git's lead and > have something like > > kvm config MyInstance-1 --set debug.io.delay.ms 100 > > and > > kvm config MyInstance-1 --list Yeah, agreed - 'kvm config' is intuitive. I tried to think of something better than 'kvm set' but failed. ( And no, being super diligent with high level, very user visible changes and names is not bikeshed painting. ) Note that 'git config' touches the .gitconfig IIRC - while here we really also want to include runtime, dynamic configuration - but i think that distinction is fine. Now the whole 'kvm config' thing needs more thought and the whole enumeration of KVM instances needs to be well thought out as well. How do we list instances - 'kvm list' - or should perhaps 'kvm config' list all the currently running instances? Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/tools/kvm/include/kvm/virtio-blk.h b/tools/kvm/include/kvm/virtio-blk.h index 9e91035..c0211a0 100644 --- a/tools/kvm/include/kvm/virtio-blk.h +++ b/tools/kvm/include/kvm/virtio-blk.h @@ -1,10 +1,14 @@ #ifndef KVM__BLK_VIRTIO_H #define KVM__BLK_VIRTIO_H +#include <stdint.h> + struct kvm; struct virtio_blk_parameters { - struct kvm *self; + struct kvm *self; + uint64_t debug_delay_cycles; + uint64_t debug_delay_amount; }; void virtio_blk__init(struct virtio_blk_parameters *params); diff --git a/tools/kvm/kvm-run.c b/tools/kvm/kvm-run.c index 5b71fb4..3392bfa 100644 --- a/tools/kvm/kvm-run.c +++ b/tools/kvm/kvm-run.c @@ -57,6 +57,8 @@ static void handle_sigalrm(int sig) } static u64 ram_size = MIN_RAM_SIZE_MB; +static u64 virtio_blk_delay_cycles = -1; +static u64 virtio_blk_delay_amount; static const char *kernel_cmdline; static const char *kernel_filename; static const char *initrd_filename; @@ -112,6 +114,10 @@ static const struct option options[] = { "Enable single stepping"), OPT_BOOLEAN('g', "ioport-debug", &ioport_debug, "Enable ioport debugging"), + OPT_U64('\0', "debug-io-delay-cycles", &virtio_blk_delay_cycles, + "Wait this amount of cycles before delay"), + OPT_U64('\0', "debug-io-delay-amount", &virtio_blk_delay_amount, + "Delay each I/O request by this amount (usec)"), OPT_END() }; @@ -319,7 +325,9 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix) pci__init(); blk_params = (struct virtio_blk_parameters) { - .self = kvm + .self = kvm, + .debug_delay_cycles = virtio_blk_delay_cycles, + .debug_delay_amount = virtio_blk_delay_amount }; virtio_blk__init(&blk_params); diff --git a/tools/kvm/virtio-blk.c b/tools/kvm/virtio-blk.c index 2470583..ea8c4e7 100644 --- a/tools/kvm/virtio-blk.c +++ b/tools/kvm/virtio-blk.c @@ -38,6 +38,9 @@ struct blk_device { uint16_t queue_selector; uint64_t virtio_blk_queue_set_flags; + uint64_t debug_delay_cycles; + uint64_t debug_delay_amount; + struct virt_queue vqs[NUM_VIRT_QUEUES]; }; @@ -174,6 +177,7 @@ static int virtio_blk_get_selected_queue(void) static void *virtio_blk_io_thread(void *ptr) { struct kvm *self = ptr; + uint64_t io_cycles = 0; int ret; mutex_lock(&blk_device.io_mutex); ret = pthread_cond_wait(&blk_device.io_cond, &blk_device.io_mutex); @@ -183,6 +187,10 @@ static void *virtio_blk_io_thread(void *ptr) while (queue_index >= 0) { struct virt_queue *vq = &blk_device.vqs[queue_index]; + if (blk_device.debug_delay_cycles != (uint64_t)-1 && + ++io_cycles > blk_device.debug_delay_cycles) + usleep(blk_device.debug_delay_amount); + while (virt_queue__available(vq)) virtio_blk_do_io_request(self, vq); @@ -293,6 +301,9 @@ void virtio_blk__init(struct virtio_blk_parameters *params) if (!self->disk_image) return; + blk_device.debug_delay_amount = params->debug_delay_amount; + blk_device.debug_delay_cycles = params->debug_delay_cycles; + pthread_create(&blk_device.io_thread, NULL, virtio_blk_io_thread, self); blk_device.blk_config.capacity = self->disk_image->size / SECTOR_SIZE;
Add --debug-io-delay-cycles and --debug-io-delay-amount to delay the completion of IO requests within virtio-blk. This feature allows to verify and debug the threading within virtio-blk. Signed-off-by: Sasha Levin <levinsasha928@gmail.com> --- tools/kvm/include/kvm/virtio-blk.h | 6 +++++- tools/kvm/kvm-run.c | 10 +++++++++- tools/kvm/virtio-blk.c | 11 +++++++++++ 3 files changed, 25 insertions(+), 2 deletions(-)