From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
To: Wei Yang <richard.weiyang@gmail.com>
Cc: quintela@redhat.com, Wei Yang <richardw.yang@linux.intel.com>,
qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH] migrtion: define MigrationState/MigrationIncomingState.state as MigrationStatus
Date: Fri, 23 Aug 2019 17:18:07 +0100 [thread overview]
Message-ID: <20190823161807.GP2784@work-vm> (raw)
In-Reply-To: <20190823152916.GN2784@work-vm>
* Dr. David Alan Gilbert (dgilbert@redhat.com) wrote:
> * Wei Yang (richard.weiyang@gmail.com) wrote:
> > On Mon, Aug 19, 2019 at 12:26:32PM +0100, Dr. David Alan Gilbert wrote:
> > >* Wei Yang (richardw.yang@linux.intel.com) wrote:
> > >> No functional change. Add default case to fix warning.
> > >
> > >I think the problem with this is that migrate_set_state uses an
> > >atomic_cmpxchg and so we have to be careful that the type we use
> > >is compatible with that.
> > >MigrationStatus is an enum and I think compilers are allowed to
> > >choose the types of that; so I'm not sure we're guaranteed
> > >that an enum is always OK for the atomic_cmpxchg, and if it is
> >
> > Took a look into the definition of atomic_cmpxchg, which finally calls
> >
> > * __atomic_compare_exchange_n for c++11
> > * __sync_val_compare_and_swap
> >
> > Both of them take two pointers to compare and exchange its content.
> >
> > Per C99 standard, http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf,
> > it mentioned:
> >
> > Each enumerated type shall be compatible with char, a signed integer type,
> > or an unsigned integer type. The choice of type is implementation-defined,
> > but shall be capable of representing the values of all the members of the
> > enumeration.
> >
> > Based on this, I think atomic_cmpxchg should work fine with enum.
>
> Hmm OK, but I'd need you to test n some 32bit and other comp=ilers etc;
> make sure it works on 32bit, clang, gcc, 32bit ARM etc - because
> otherwise I'm going to have to worry about checking all those.
Actually since QEMU is defined to be C99, the test is to check with some
C99 compilers; eblake assures me that in theory C11 should be OK,
but C99 and our atomics are more of an interesting question.
Dave
> Dave
>
> > >then we also might have to make the old_state and new_state
> > >variables match.
> > >
> >
> > You are right.
> > >Dave
> > >
> > >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> > >> ---
> > >> migration/migration.c | 8 +++++++-
> > >> migration/migration.h | 6 +++---
> > >> 2 files changed, 10 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/migration/migration.c b/migration/migration.c
> > >> index 2865ae3fa9..0fd2364961 100644
> > >> --- a/migration/migration.c
> > >> +++ b/migration/migration.c
> > >> @@ -946,6 +946,8 @@ static void fill_source_migration_info(MigrationInfo *info)
> > >> case MIGRATION_STATUS_CANCELLED:
> > >> info->has_status = true;
> > >> break;
> > >> + default:
> > >> + return;
> > >> }
> > >> info->status = s->state;
> > >> }
> > >> @@ -1054,6 +1056,8 @@ static void fill_destination_migration_info(MigrationInfo *info)
> > >> info->has_status = true;
> > >> fill_destination_postcopy_migration_info(info);
> > >> break;
> > >> + default:
> > >> + return;
> > >> }
> > >> info->status = mis->state;
> > >> }
> > >> @@ -1446,7 +1450,7 @@ void qmp_migrate_start_postcopy(Error **errp)
> > >>
> > >> /* shared migration helpers */
> > >>
> > >> -void migrate_set_state(int *state, int old_state, int new_state)
> > >> +void migrate_set_state(MigrationStatus *state, int old_state, int new_state)
> > >> {
> > >> assert(new_state < MIGRATION_STATUS__MAX);
> > >> if (atomic_cmpxchg(state, old_state, new_state) == old_state) {
> > >> @@ -1683,6 +1687,8 @@ bool migration_is_idle(void)
> > >> return false;
> > >> case MIGRATION_STATUS__MAX:
> > >> g_assert_not_reached();
> > >> + default:
> > >> + g_assert_not_reached();
> > >> }
> > >>
> > >> return false;
> > >> diff --git a/migration/migration.h b/migration/migration.h
> > >> index 5e8f09c6db..418ee00053 100644
> > >> --- a/migration/migration.h
> > >> +++ b/migration/migration.h
> > >> @@ -65,7 +65,7 @@ struct MigrationIncomingState {
> > >>
> > >> QEMUBH *bh;
> > >>
> > >> - int state;
> > >> + MigrationStatus state;
> > >>
> > >> bool have_colo_incoming_thread;
> > >> QemuThread colo_incoming_thread;
> > >> @@ -151,7 +151,7 @@ struct MigrationState
> > >> /* params from 'migrate-set-parameters' */
> > >> MigrationParameters parameters;
> > >>
> > >> - int state;
> > >> + MigrationStatus state;
> > >>
> > >> /* State related to return path */
> > >> struct {
> > >> @@ -234,7 +234,7 @@ struct MigrationState
> > >> bool decompress_error_check;
> > >> };
> > >>
> > >> -void migrate_set_state(int *state, int old_state, int new_state);
> > >> +void migrate_set_state(MigrationStatus *state, int old_state, int new_state);
> > >>
> > >> void migration_fd_process_incoming(QEMUFile *f);
> > >> void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp);
> > >> --
> > >> 2.19.1
> > >>
> > >--
> > >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
> > --
> > Wei Yang
> > Help you, Help me
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
next prev parent reply other threads:[~2019-08-23 16:19 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-21 14:27 [Qemu-devel] [PATCH] migrtion: define MigrationState/MigrationIncomingState.state as MigrationStatus Wei Yang
2019-08-19 3:29 ` Wei Yang
2019-08-19 11:26 ` Dr. David Alan Gilbert
2019-08-19 13:32 ` Wei Yang
2019-08-21 8:51 ` Wei Yang
2019-08-19 11:26 ` Dr. David Alan Gilbert
2019-08-19 14:08 ` Wei Yang
2019-08-23 15:29 ` Dr. David Alan Gilbert
2019-08-23 16:18 ` Dr. David Alan Gilbert [this message]
2019-08-23 16:21 ` Eric Blake
2019-08-23 23:49 ` Wei Yang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190823161807.GP2784@work-vm \
--to=dgilbert@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=quintela@redhat.com \
--cc=richard.weiyang@gmail.com \
--cc=richardw.yang@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).