From patchwork Thu Oct 31 16:48:28 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Michael S. Tsirkin" X-Patchwork-Id: 287562 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 654E32C03EE for ; Fri, 1 Nov 2013 03:46:18 +1100 (EST) Received: from localhost ([::1]:58367 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VbvNw-00068Y-IL for incoming@patchwork.ozlabs.org; Thu, 31 Oct 2013 12:46:16 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60215) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VbvNY-00066x-Dp for qemu-devel@nongnu.org; Thu, 31 Oct 2013 12:45:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VbvNR-0007t7-0H for qemu-devel@nongnu.org; Thu, 31 Oct 2013 12:45:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48356) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VbvNQ-0007sy-Bv for qemu-devel@nongnu.org; Thu, 31 Oct 2013 12:45:44 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r9VGjdrO026270 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 31 Oct 2013 12:45:39 -0400 Received: from redhat.com (vpn1-5-193.ams2.redhat.com [10.36.5.193]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with SMTP id r9VGjXpM011794; Thu, 31 Oct 2013 12:45:34 -0400 Date: Thu, 31 Oct 2013 18:48:28 +0200 From: "Michael S. Tsirkin" To: Paolo Bonzini Message-ID: <20131031164828.GA10862@redhat.com> References: <20131031145234.GC9948@redhat.com> <52726FAA.6000502@redhat.com> <20131031150955.GE9948@redhat.com> <52727695.8080007@redhat.com> <20131031154556.GB10424@redhat.com> <52727D97.3070209@redhat.com> <20131031161404.GA10710@redhat.com> <52728294.9050305@redhat.com> <20131031162652.GA10789@redhat.com> <52728790.7010404@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <52728790.7010404@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: pkrempa@redhat.com, lersek@redhat.com, marcel.a@redhat.com, libvir-list@redhat.com, Hu Tao , qemu-devel@nongnu.org, armbru@redhat.com, rhod@redhat.com, kraxel@redhat.com, anthony@codemonkey.ws, lcapitulino@redhat.com, afaerber@suse.de Subject: Re: [Qemu-devel] pvpanic plans? X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Thu, Oct 31, 2013 at 05:38:40PM +0100, Paolo Bonzini wrote: > Il 31/10/2013 17:26, Michael S. Tsirkin ha scritto: > > On Thu, Oct 31, 2013 at 05:17:24PM +0100, Paolo Bonzini wrote: > >> Il 31/10/2013 17:14, Michael S. Tsirkin ha scritto: > >>>> PANICKED->DEBUG was added by commit bc7d0e667. That commit can be > >>>> reverted if the panicked state is removed from runstate_needs_reset. > >>> > >>> Okay so let's drop the code duplication and explicitly make > >>> them the same? > >>> > >>> Signed-off-by: Michael S. Tsirkin > >>> > >>> > >>> diff --git a/vl.c b/vl.c > >>> index 46c29c4..e12d317 100644 > >>> --- a/vl.c > >>> +++ b/vl.c > >>> @@ -638,10 +638,6 @@ static const RunStateTransition runstate_transitions_def[] = { > >>> { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING }, > >>> { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE }, > >>> > >>> - { RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED }, > >>> - { RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE }, > >>> - { RUN_STATE_GUEST_PANICKED, RUN_STATE_DEBUG }, > >>> - > >>> { RUN_STATE_MAX, RUN_STATE_MAX }, > >>> }; > >>> > >>> @@ -660,6 +656,12 @@ static void runstate_init(void) > >>> > >>> for (p = &runstate_transitions_def[0]; p->from != RUN_STATE_MAX; p++) { > >>> runstate_valid_transitions[p->from][p->to] = true; > >>> + /* Panicked state is same as paused, we only made it different so > >>> + * management can detect a panic. > >>> + */ > >>> + if (p->from == RUN_STATE_PAUSED) { > >>> + runstate_valid_transitions[RUN_STATE_GUEST_PANICKED][p->to] = true; > >> > >> It makes only sense to me if you do that for IO_ERROR and WATCHDOG as > >> well, and perhaps there are others I'm missing. Just add a comment > >> before runstate_transitions_def's entries for PANICKED, IO_ERROR and > >> WATCHDOG. > >> > >> But again, it is somewhat separate from the issue at hand, which is to > >> finally make pvpanic usable and hopefully before 1.7. > >> > >> Paolo > > > > The issue is that you can't continue from panicked state. > > You should be able to do that without going through paused. > > Yes, that's what my patch (posted the link before) does: > > - { RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED }, > + { RUN_STATE_GUEST_PANICKED, RUN_STATE_RUNNING }, > { RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE }, > - { RUN_STATE_GUEST_PANICKED, RUN_STATE_DEBUG }, > > > Comments don't compile, but are also easier to understand than code. > Special logic in runstate_init is unnecessarily complicated, for a table > that hardly sees any change. English works better, whoever modifies the > table has it under their eyes. > > Paolo But code duplication is bad. I think IO error for example is broken in that you can't pause but can run then pause. Seems strange. Internal error has same bug as panicked. So it's the same bug for a bunch of states, let's just have a way to say "this is same as paused". How's this? diff --git a/vl.c b/vl.c index 46c29c4..4388c95 100644 --- a/vl.c +++ b/vl.c @@ -593,12 +593,6 @@ static const RunStateTransition runstate_transitions_def[] = { { RUN_STATE_INMIGRATE, RUN_STATE_RUNNING }, { RUN_STATE_INMIGRATE, RUN_STATE_PAUSED }, - { RUN_STATE_INTERNAL_ERROR, RUN_STATE_PAUSED }, - { RUN_STATE_INTERNAL_ERROR, RUN_STATE_FINISH_MIGRATE }, - - { RUN_STATE_IO_ERROR, RUN_STATE_RUNNING }, - { RUN_STATE_IO_ERROR, RUN_STATE_FINISH_MIGRATE }, - { RUN_STATE_PAUSED, RUN_STATE_RUNNING }, { RUN_STATE_PAUSED, RUN_STATE_FINISH_MIGRATE }, @@ -635,16 +629,17 @@ static const RunStateTransition runstate_transitions_def[] = { { RUN_STATE_SUSPENDED, RUN_STATE_RUNNING }, { RUN_STATE_SUSPENDED, RUN_STATE_FINISH_MIGRATE }, - { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING }, - { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE }, - - { RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED }, - { RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE }, - { RUN_STATE_GUEST_PANICKED, RUN_STATE_DEBUG }, - { RUN_STATE_MAX, RUN_STATE_MAX }, }; +static const RunState runstate_paused[] = { + { RUN_STATE_GUEST_PANICKED}, + { RUN_STATE_IO_ERROR}, + { RUN_STATE_INTERNAL_ERROR}, + { RUN_STATE_WATCHDOG}, + { RUN_STATE_MAX }, +}; + static bool runstate_valid_transitions[RUN_STATE_MAX][RUN_STATE_MAX]; bool runstate_check(RunState state) @@ -655,12 +650,21 @@ bool runstate_check(RunState state) static void runstate_init(void) { const RunStateTransition *p; + const RunState *i; memset(&runstate_valid_transitions, 0, sizeof(runstate_valid_transitions)); for (p = &runstate_transitions_def[0]; p->from != RUN_STATE_MAX; p++) { runstate_valid_transitions[p->from][p->to] = true; } + /* Allow two-way transitions between identical states */ + for (i = &runstate_paused[0]; *p != RUN_STATE_MAX; p++) { + runstate_valid_transitions[*i][RUN_STATE_PAUSED] = true; + runstate_valid_transitions[RUN_STATE_PAUSED][*i] = true; + memcpy(&runstate_valid_transitions[*i], + &runstate_valid_transitions[RUN_STATE_PAUSED], + sizeof(runstate_valid_transitions[RUN_STATE_PAUSED])); + } } /* This function will abort() on invalid state transitions */ @@ -686,8 +690,6 @@ int runstate_is_running(void) bool runstate_needs_reset(void) { - return runstate_check(RUN_STATE_INTERNAL_ERROR) || - runstate_check(RUN_STATE_SHUTDOWN) || - runstate_check(RUN_STATE_GUEST_PANICKED); + return runstate_check(RUN_STATE_SHUTDOWN); } StatusInfo *qmp_query_status(Error **errp)