Message ID | 20131031161404.GA10710@redhat.com |
---|---|
State | New |
Headers | show |
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 <mst@redhat.com> > > > 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 > + } > } > } > > @@ -686,8 +688,7 @@ 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); > + runstate_check(RUN_STATE_SHUTDOWN); > } > > StatusInfo *qmp_query_status(Error **errp) >
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 <mst@redhat.com> > > > > > > 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. > > + } > > } > > } > > > > @@ -686,8 +688,7 @@ 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); > > + runstate_check(RUN_STATE_SHUTDOWN); > > } > > > > StatusInfo *qmp_query_status(Error **errp) > >
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 <mst@redhat.com> > > > > > > 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, Yea, let's do that. > and perhaps there are others I'm missing. > Just add a comment > before runstate_transitions_def's entries for PANICKED, IO_ERROR and > WATCHDOG. comments don't compile :) > But again, it is somewhat separate from the issue at hand, which is to > finally make pvpanic usable and hopefully before 1.7. > > Paolo > > > + } > > } > > } > > > > @@ -686,8 +688,7 @@ 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); > > + runstate_check(RUN_STATE_SHUTDOWN); > > } > > > > StatusInfo *qmp_query_status(Error **errp) > >
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 <mst@redhat.com> >>> >>> >>> 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
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 <mst@redhat.com> > >>> > >>> > >>> 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 }, > Anyway I agree with your patch. How about we drop RUN_STATE_DEBUG and drop RUN_STATE_GUEST_PANICKED from need reset list? > 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
Il 31/10/2013 18:01, Michael S. Tsirkin ha scritto: > > 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 }, > > Anyway I agree with your patch. How about we drop RUN_STATE_DEBUG and > drop RUN_STATE_GUEST_PANICKED from need reset list? Yes, and also modify gdbstub.c. It's all in the URL I posted a few hours ago. Paolo
On Thu, Oct 31, 2013 at 06:10:36PM +0100, Paolo Bonzini wrote: > Il 31/10/2013 18:01, Michael S. Tsirkin ha scritto: > > > 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 }, > > > > Anyway I agree with your patch. How about we drop RUN_STATE_DEBUG and > > drop RUN_STATE_GUEST_PANICKED from need reset list? > > Yes, and also modify gdbstub.c. It's all in the URL I posted a few > hours ago. > > Paolo OK, so can you pls post patches 1 and 2? I'll review and ack.
Il 31/10/2013 18:18, Michael S. Tsirkin ha scritto: > On Thu, Oct 31, 2013 at 06:10:36PM +0100, Paolo Bonzini wrote: >> Il 31/10/2013 18:01, Michael S. Tsirkin ha scritto: >>>> 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 }, >>> >>> Anyway I agree with your patch. How about we drop RUN_STATE_DEBUG and >>> drop RUN_STATE_GUEST_PANICKED from need reset list? >> >> Yes, and also modify gdbstub.c. It's all in the URL I posted a few >> hours ago. >> >> Paolo > > OK, so can you pls post patches 1 and 2? I'll review and ack. Next Monday I will. Paolo
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; + } } } @@ -686,8 +688,7 @@ 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); + runstate_check(RUN_STATE_SHUTDOWN); } StatusInfo *qmp_query_status(Error **errp)