Re: [Qemu-devel] [PATCH v5 10/17] migration: Create ram_multifd_page
On 08/08/2017 21:14, Dr. David Alan Gilbert wrote: >> There is no barrier there that I can see. I know that it probably work >> on x86, but in others? I think that it *** HERE we need that >> memory barrier that we don't have. > Yes, I think that's smp_mb_release() - and you have to do an > smp_mb_acquire after reading the pages->num before accessing the iov. Yes, I think that's correct. Paolo > (Probably worth checking with Paolo). > Or just stick with mutex's. > >
Re: [Qemu-devel] [PATCH v5 10/17] migration: Create ram_multifd_page
On Tue, Aug 08, 2017 at 06:04:54PM +0200, Juan Quintela wrote:
> Peter Xu wrote:
> > On Wed, Jul 19, 2017 at 08:02:39PM +0100, Dr. David Alan Gilbert wrote:
> >> * Juan Quintela (quint...@redhat.com) wrote:
>
> >> > struct MultiFDSendParams {
> >> > +/* not changed */
> >> > uint8_t id;
> >> > QemuThread thread;
> >> > QIOChannel *c;
> >> > QemuSemaphore sem;
> >> > QemuMutex mutex;
> >> > +/* protected by param mutex */
> >> > bool quit;
> >>
> >> Should probably comment to say what address space address is in - this
> >> is really a qemu pointer - and that's why we can treat 0 as special?
> >
> > I believe this comment is for "address" below.
> >
> > Yes, it would be nice to comment it. IIUC it belongs to virtual
> > address space of QEMU, so it should be okay to use zero as a "special"
> > value.
>
> See new comments.
>
> >>
> >> > +uint8_t *address;
> >> > +/* protected by multifd mutex */
> >> > +bool done;
> >>
> >> done needs a comment to explain what it is because
> >> it sounds similar to quit; I think 'done' is saying that
> >> the thread is idle having done what was asked?
> >
> > Since we know that valid address won't be zero, not sure whether we
> > can just avoid introducing the "done" field (even, not sure whether we
> > will need lock when modifying "address", I think not as well? Please
> > see below). For what I see this, it works like a state machine, and
> > "address" can be the state:
> >
> > + send thread -+
> > | |
> >\|/ |
> > address==0 (IDLE) address!=0 (ACTIVE)
> > | /|\
> > | |
> > + main thread -+
> >
> > Then...
>
> It is needed, we change things later in the series. We could treat as
> an special case page.num == 0. But then we can differentiate the case
> where we have finished the last round and that we are in the beggining
> of the new one.
(Will comment below)
>
> >>
> >> > };
> >> > typedef struct MultiFDSendParams MultiFDSendParams;
> >> >
> >> > @@ -375,6 +381,8 @@ struct {
> >> > MultiFDSendParams *params;
> >> > /* number of created threads */
> >> > int count;
> >> > +QemuMutex mutex;
> >> > +QemuSemaphore sem;
> >> > } *multifd_send_state;
> >> >
> >> > static void terminate_multifd_send_threads(void)
> >> > @@ -443,6 +451,7 @@ static void *multifd_send_thread(void *opaque)
> >> > } else {
> >> > qio_channel_write(p->c, string, MULTIFD_UUID_MSG, &error_abort);
> >> > }
> >> > +qemu_sem_post(&multifd_send_state->sem);
> >> >
> >> > while (!exit) {
> >> > qemu_mutex_lock(&p->mutex);
> >> > @@ -450,6 +459,15 @@ static void *multifd_send_thread(void *opaque)
> >> > qemu_mutex_unlock(&p->mutex);
> >> > break;
> >> > }
> >> > +if (p->address) {
> >> > +p->address = 0;
> >> > +qemu_mutex_unlock(&p->mutex);
> >> > +qemu_mutex_lock(&multifd_send_state->mutex);
> >> > +p->done = true;
> >> > +qemu_mutex_unlock(&multifd_send_state->mutex);
> >> > +qemu_sem_post(&multifd_send_state->sem);
> >> > +continue;
> >
> > Here instead of setting up address=0 at the entry, can we do this (no
> > "done" for this time)?
> >
> > // send the page before clearing p->address
> > send_page(p->address);
> > // clear p->address to switch to "IDLE" state
> > atomic_set(&p->address, 0);
> > // tell main thread, in case it's waiting
> > qemu_sem_post(&multifd_send_state->sem);
> >
> > And on the main thread...
> >
> >> > +}
> >> > qemu_mutex_unlock(&p->mutex);
> >> > qemu_sem_wait(&p->sem);
> >> > }
> >> > @@ -469,6 +487,8 @@ int multifd_save_setup(void)
> >> > multifd_send_state = g_malloc0(sizeof(*multifd_send_state));
> >> > multifd_send_state->params = g_new0(MultiFDSendParams,
> >> > thread_count);
> >> > multifd_send_state->count = 0;
> >> > +qemu_mutex_init(&multifd_send_state->mutex);
> >> > +qemu_sem_init(&multifd_send_state->sem, 0);
> >> > for (i = 0; i < thread_count; i++) {
> >> > char thread_name[16];
> >> > MultiFDSendParams *p = &multifd_send_state->params[i];
> >> > @@ -477,6 +497,8 @@ int multifd_save_setup(void)
> >> > qemu_sem_init(&p->sem, 0);
> >> > p->quit = false;
> >> > p->id = i;
> >> > +p->done = true;
> >> > +p->address = 0;
> >> > p->c = socket_send_channel_create();
> >> > if (!p->c) {
> >> > error_report("Error creating a send channel");
> >> > @@ -491,6 +513,30 @@ int multifd_save_setup(void)
> >> > return 0;
> >>
Re: [Qemu-devel] [PATCH v5 10/17] migration: Create ram_multifd_page
* Juan Quintela (quint...@redhat.com) wrote:
> "Dr. David Alan Gilbert" wrote:
> > * Juan Quintela (quint...@redhat.com) wrote:
> >> "Dr. David Alan Gilbert" wrote:
> >> > * Juan Quintela (quint...@redhat.com) wrote:
>
> ...
>
> >> > My feeling, without having fully thought it through, is that
> >> > the locking around 'address' can be simplified; especially if the
> >> > sending-thread never actually changes it.
> >> >
> >> > http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_11
> >> > defines that most of the pthread_ functions act as barriers;
> >> > including the sem_post and pthread_cond_signal that qemu_sem_post
> >> > uses.
> >>
> >> At the end of the series the code is this:
> >>
> >> qemu_mutex_lock(&p->mutex);
> >> p->pages.num = pages->num;
> >> iov_copy(p->pages.iov, pages->num, pages->iov, pages->num, 0,
> >> iov_size(pages->iov, pages->num));
>
> ** HERE **
>
> >> pages->num = 0;
> >> qemu_mutex_unlock(&p->mutex);
> >>
> >> Are you sure that it looks like a good idea to drop the mutex?
> >>
> >> The other thread uses pages->num to know if things are ready.
> >
> > Well, I wont push it too hard, but; if you:
> > a) Know that the other thread isn't accessing the iov
> > (because you previously know that it had set done)
>
> This bit I know it is true.
>
> > b) Know the other thread wont access it until pages->num gets
> > set
>
>
>
> > c) Ensure that all changes to the iov are visible before
> > the pages->num write is visible - appropriate barriers/ordering
>
> There is no barrier there that I can see. I know that it probably work
> on x86, but in others? I think that it *** HERE we need that
> memory barrier that we don't have.
Yes, I think that's smp_mb_release() - and you have to do an
smp_mb_acquire after reading the pages->num before accessing the iov.
(Probably worth checking with Paolo).
Or just stick with mutex's.
> > then you're good. However, the mutex might be simpler.
>
> Code (after all the changes) is:
>
> qemu_sem_wait(&multifd_send_state->sem);
> qemu_mutex_lock(&multifd_send_state->mutex);
> for (i = 0; i < multifd_send_state->count; i++) {
> p = &multifd_send_state->params[i];
>
> if (p->done) {
> p->done = false;
> break;
> }
> }
> qemu_mutex_unlock(&multifd_send_state->mutex);
> qemu_mutex_lock(&p->mutex);
> p->pages.num = pages->num; /* we could probably switch this
>statement with the next, but I doubt
>this would make a big difference */
> iov_copy(p->pages.iov, pages->num, pages->iov, pages->num, 0,
> iov_size(pages->iov, pages->num));
> pages->num = 0;
> qemu_mutex_unlock(&p->mutex);
> qemu_sem_post(&p->sem);
>
>
> And the other thread
>
> qemu_mutex_lock(&p->mutex);
> [...]
> if (p->pages.num) {
> int num;
>
> num = p->pages.num;
> p->pages.num = 0;
> qemu_mutex_unlock(&p->mutex);
>
> if (qio_channel_writev_all(p->c, p->pages.iov,
>num, &error_abort)
> != num * TARGET_PAGE_SIZE) {
> MigrationState *s = migrate_get_current();
>
> migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
> MIGRATION_STATUS_FAILED);
> terminate_multifd_send_threads();
> return NULL;
> }
> qemu_mutex_lock(&multifd_send_state->mutex);
> p->done = true;
> qemu_mutex_unlock(&multifd_send_state->mutex);
> qemu_sem_post(&multifd_send_state->sem);
> continue;
> }
> qemu_mutex_unlock(&p->mutex);
> qemu_sem_wait(&p->sem);
>
> This code used to have condition variables for waiting. With
> semaphores, we can probably remove the p->mutex, but then we need to
> think a lot each time that we do a change.
>
> Later, Juan.
Dave
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH v5 10/17] migration: Create ram_multifd_page
"Dr. David Alan Gilbert" wrote:
> * Juan Quintela (quint...@redhat.com) wrote:
>> "Dr. David Alan Gilbert" wrote:
>> > * Juan Quintela (quint...@redhat.com) wrote:
...
>> > My feeling, without having fully thought it through, is that
>> > the locking around 'address' can be simplified; especially if the
>> > sending-thread never actually changes it.
>> >
>> > http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_11
>> > defines that most of the pthread_ functions act as barriers;
>> > including the sem_post and pthread_cond_signal that qemu_sem_post
>> > uses.
>>
>> At the end of the series the code is this:
>>
>> qemu_mutex_lock(&p->mutex);
>> p->pages.num = pages->num;
>> iov_copy(p->pages.iov, pages->num, pages->iov, pages->num, 0,
>> iov_size(pages->iov, pages->num));
** HERE **
>> pages->num = 0;
>> qemu_mutex_unlock(&p->mutex);
>>
>> Are you sure that it looks like a good idea to drop the mutex?
>>
>> The other thread uses pages->num to know if things are ready.
>
> Well, I wont push it too hard, but; if you:
> a) Know that the other thread isn't accessing the iov
> (because you previously know that it had set done)
This bit I know it is true.
> b) Know the other thread wont access it until pages->num gets
> set
> c) Ensure that all changes to the iov are visible before
> the pages->num write is visible - appropriate barriers/ordering
There is no barrier there that I can see. I know that it probably work
on x86, but in others? I think that it *** HERE we need that
memory barrier that we don't have.
> then you're good. However, the mutex might be simpler.
Code (after all the changes) is:
qemu_sem_wait(&multifd_send_state->sem);
qemu_mutex_lock(&multifd_send_state->mutex);
for (i = 0; i < multifd_send_state->count; i++) {
p = &multifd_send_state->params[i];
if (p->done) {
p->done = false;
break;
}
}
qemu_mutex_unlock(&multifd_send_state->mutex);
qemu_mutex_lock(&p->mutex);
p->pages.num = pages->num; /* we could probably switch this
statement with the next, but I doubt
this would make a big difference */
iov_copy(p->pages.iov, pages->num, pages->iov, pages->num, 0,
iov_size(pages->iov, pages->num));
pages->num = 0;
qemu_mutex_unlock(&p->mutex);
qemu_sem_post(&p->sem);
And the other thread
qemu_mutex_lock(&p->mutex);
[...]
if (p->pages.num) {
int num;
num = p->pages.num;
p->pages.num = 0;
qemu_mutex_unlock(&p->mutex);
if (qio_channel_writev_all(p->c, p->pages.iov,
num, &error_abort)
!= num * TARGET_PAGE_SIZE) {
MigrationState *s = migrate_get_current();
migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
MIGRATION_STATUS_FAILED);
terminate_multifd_send_threads();
return NULL;
}
qemu_mutex_lock(&multifd_send_state->mutex);
p->done = true;
qemu_mutex_unlock(&multifd_send_state->mutex);
qemu_sem_post(&multifd_send_state->sem);
continue;
}
qemu_mutex_unlock(&p->mutex);
qemu_sem_wait(&p->sem);
This code used to have condition variables for waiting. With
semaphores, we can probably remove the p->mutex, but then we need to
think a lot each time that we do a change.
Later, Juan.
Re: [Qemu-devel] [PATCH v5 10/17] migration: Create ram_multifd_page
* Juan Quintela (quint...@redhat.com) wrote:
> "Dr. David Alan Gilbert" wrote:
> > * Juan Quintela (quint...@redhat.com) wrote:
> >> The function still don't use multifd, but we have simplified
> >> ram_save_page, xbzrle and RDMA stuff is gone. We have added a new
> >> counter and a new flag for this type of pages.
> >>
> >> Signed-off-by: Juan Quintela
> >> ---
> >> hmp.c | 2 ++
> >> migration/migration.c | 1 +
> >> migration/ram.c | 90
> >> ++-
> >> qapi-schema.json | 5 ++-
> >> 4 files changed, 96 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hmp.c b/hmp.c
> >> index b01605a..eeb308b 100644
> >> --- a/hmp.c
> >> +++ b/hmp.c
> >> @@ -234,6 +234,8 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
> >> monitor_printf(mon, "postcopy request count: %" PRIu64 "\n",
> >> info->ram->postcopy_requests);
> >> }
> >> +monitor_printf(mon, "multifd: %" PRIu64 " pages\n",
> >> + info->ram->multifd);
> >> }
> >>
> >> if (info->has_disk) {
> >> diff --git a/migration/migration.c b/migration/migration.c
> >> index e1c79d5..d9d5415 100644
> >> --- a/migration/migration.c
> >> +++ b/migration/migration.c
> >> @@ -528,6 +528,7 @@ static void populate_ram_info(MigrationInfo *info,
> >> MigrationState *s)
> >> info->ram->dirty_sync_count = ram_counters.dirty_sync_count;
> >> info->ram->postcopy_requests = ram_counters.postcopy_requests;
> >> info->ram->page_size = qemu_target_page_size();
> >> +info->ram->multifd = ram_counters.multifd;
> >>
> >> if (migrate_use_xbzrle()) {
> >> info->has_xbzrle_cache = true;
> >> diff --git a/migration/ram.c b/migration/ram.c
> >> index b80f511..2bf3fa7 100644
> >> --- a/migration/ram.c
> >> +++ b/migration/ram.c
> >> @@ -68,6 +68,7 @@
> >> #define RAM_SAVE_FLAG_XBZRLE 0x40
> >> /* 0x80 is reserved in migration.h start with 0x100 next */
> >> #define RAM_SAVE_FLAG_COMPRESS_PAGE0x100
> >> +#define RAM_SAVE_FLAG_MULTIFD_PAGE 0x200
> >>
> >> static inline bool is_zero_range(uint8_t *p, uint64_t size)
> >> {
> >> @@ -362,12 +363,17 @@ static void compress_threads_save_setup(void)
> >> /* Multiple fd's */
> >>
> >> struct MultiFDSendParams {
> >> +/* not changed */
> >> uint8_t id;
> >> QemuThread thread;
> >> QIOChannel *c;
> >> QemuSemaphore sem;
> >> QemuMutex mutex;
> >> +/* protected by param mutex */
> >> bool quit;
> >
> > Should probably comment to say what address space address is in - this
> > is really a qemu pointer - and that's why we can treat 0 as special?
>
> Ok. Added
>
> /* This is a temp field. We are using it now to transmit
>something the address of the page. Later in the series, we
>change it for the real page.
> */
>
>
> >
> >> +uint8_t *address;
> >> +/* protected by multifd mutex */
> >> +bool done;
> >
> > done needs a comment to explain what it is because
> > it sounds similar to quit; I think 'done' is saying that
> > the thread is idle having done what was asked?
>
> /* has the thread finish the last submitted job */
>
> >> +static int multifd_send_page(uint8_t *address)
> >> +{
> >> +int i;
> >> +MultiFDSendParams *p = NULL; /* make happy gcc */
> >> +
> >> +qemu_sem_wait(&multifd_send_state->sem);
> >> +qemu_mutex_lock(&multifd_send_state->mutex);
> >> +for (i = 0; i < multifd_send_state->count; i++) {
> >> +p = &multifd_send_state->params[i];
> >> +
> >> +if (p->done) {
> >> +p->done = false;
> >> +break;
> >> +}
> >> +}
> >> +qemu_mutex_unlock(&multifd_send_state->mutex);
> >> +qemu_mutex_lock(&p->mutex);
> >> +p->address = address;
> >> +qemu_mutex_unlock(&p->mutex);
> >> +qemu_sem_post(&p->sem);
> >
> > My feeling, without having fully thought it through, is that
> > the locking around 'address' can be simplified; especially if the
> > sending-thread never actually changes it.
> >
> > http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_11
> > defines that most of the pthread_ functions act as barriers;
> > including the sem_post and pthread_cond_signal that qemu_sem_post
> > uses.
>
> At the end of the series the code is this:
>
> qemu_mutex_lock(&p->mutex);
> p->pages.num = pages->num;
> iov_copy(p->pages.iov, pages->num, pages->iov, pages->num, 0,
> iov_size(pages->iov, pages->num));
> pages->num = 0;
> qemu_mutex_unlock(&p->mutex);
>
> Are you sure that it looks like a good idea to drop the mutex?
>
> The other thread uses pages->num to know if things are ready.
Well, I wont push it too hard, but; if you:
a) Know that the other thread isn't accessing the iov
(because you previously know that it had set done)
b) Know the other thread wont access it until page
Re: [Qemu-devel] [PATCH v5 10/17] migration: Create ram_multifd_page
Peter Xu wrote:
> On Wed, Jul 19, 2017 at 08:02:39PM +0100, Dr. David Alan Gilbert wrote:
>> * Juan Quintela (quint...@redhat.com) wrote:
>> > struct MultiFDSendParams {
>> > +/* not changed */
>> > uint8_t id;
>> > QemuThread thread;
>> > QIOChannel *c;
>> > QemuSemaphore sem;
>> > QemuMutex mutex;
>> > +/* protected by param mutex */
>> > bool quit;
>>
>> Should probably comment to say what address space address is in - this
>> is really a qemu pointer - and that's why we can treat 0 as special?
>
> I believe this comment is for "address" below.
>
> Yes, it would be nice to comment it. IIUC it belongs to virtual
> address space of QEMU, so it should be okay to use zero as a "special"
> value.
See new comments.
>>
>> > +uint8_t *address;
>> > +/* protected by multifd mutex */
>> > +bool done;
>>
>> done needs a comment to explain what it is because
>> it sounds similar to quit; I think 'done' is saying that
>> the thread is idle having done what was asked?
>
> Since we know that valid address won't be zero, not sure whether we
> can just avoid introducing the "done" field (even, not sure whether we
> will need lock when modifying "address", I think not as well? Please
> see below). For what I see this, it works like a state machine, and
> "address" can be the state:
>
> + send thread -+
> | |
>\|/ |
> address==0 (IDLE) address!=0 (ACTIVE)
> | /|\
> | |
> + main thread -+
>
> Then...
It is needed, we change things later in the series. We could treat as
an special case page.num == 0. But then we can differentiate the case
where we have finished the last round and that we are in the beggining
of the new one.
>>
>> > };
>> > typedef struct MultiFDSendParams MultiFDSendParams;
>> >
>> > @@ -375,6 +381,8 @@ struct {
>> > MultiFDSendParams *params;
>> > /* number of created threads */
>> > int count;
>> > +QemuMutex mutex;
>> > +QemuSemaphore sem;
>> > } *multifd_send_state;
>> >
>> > static void terminate_multifd_send_threads(void)
>> > @@ -443,6 +451,7 @@ static void *multifd_send_thread(void *opaque)
>> > } else {
>> > qio_channel_write(p->c, string, MULTIFD_UUID_MSG, &error_abort);
>> > }
>> > +qemu_sem_post(&multifd_send_state->sem);
>> >
>> > while (!exit) {
>> > qemu_mutex_lock(&p->mutex);
>> > @@ -450,6 +459,15 @@ static void *multifd_send_thread(void *opaque)
>> > qemu_mutex_unlock(&p->mutex);
>> > break;
>> > }
>> > +if (p->address) {
>> > +p->address = 0;
>> > +qemu_mutex_unlock(&p->mutex);
>> > +qemu_mutex_lock(&multifd_send_state->mutex);
>> > +p->done = true;
>> > +qemu_mutex_unlock(&multifd_send_state->mutex);
>> > +qemu_sem_post(&multifd_send_state->sem);
>> > +continue;
>
> Here instead of setting up address=0 at the entry, can we do this (no
> "done" for this time)?
>
> // send the page before clearing p->address
> send_page(p->address);
> // clear p->address to switch to "IDLE" state
> atomic_set(&p->address, 0);
> // tell main thread, in case it's waiting
> qemu_sem_post(&multifd_send_state->sem);
>
> And on the main thread...
>
>> > +}
>> > qemu_mutex_unlock(&p->mutex);
>> > qemu_sem_wait(&p->sem);
>> > }
>> > @@ -469,6 +487,8 @@ int multifd_save_setup(void)
>> > multifd_send_state = g_malloc0(sizeof(*multifd_send_state));
>> > multifd_send_state->params = g_new0(MultiFDSendParams, thread_count);
>> > multifd_send_state->count = 0;
>> > +qemu_mutex_init(&multifd_send_state->mutex);
>> > +qemu_sem_init(&multifd_send_state->sem, 0);
>> > for (i = 0; i < thread_count; i++) {
>> > char thread_name[16];
>> > MultiFDSendParams *p = &multifd_send_state->params[i];
>> > @@ -477,6 +497,8 @@ int multifd_save_setup(void)
>> > qemu_sem_init(&p->sem, 0);
>> > p->quit = false;
>> > p->id = i;
>> > +p->done = true;
>> > +p->address = 0;
>> > p->c = socket_send_channel_create();
>> > if (!p->c) {
>> > error_report("Error creating a send channel");
>> > @@ -491,6 +513,30 @@ int multifd_save_setup(void)
>> > return 0;
>> > }
>> >
>> > +static int multifd_send_page(uint8_t *address)
>> > +{
>> > +int i;
>> > +MultiFDSendParams *p = NULL; /* make happy gcc */
>> > +
>
>
>> > +qemu_sem_wait(&multifd_send_state->sem);
>> > +qemu_mutex_lock(&multifd_send_state->mutex);
>> > +for (i = 0; i < multifd_send_state->count; i++) {
>> > +p =
Re: [Qemu-devel] [PATCH v5 10/17] migration: Create ram_multifd_page
"Dr. David Alan Gilbert" wrote:
> * Peter Xu (pet...@redhat.com) wrote:
>> On Wed, Jul 19, 2017 at 08:02:39PM +0100, Dr. David Alan Gilbert wrote:
>> ... here can we just do this?
>>
>> retry:
>> // don't take any lock, only read each p->address
>> for (i = 0; i < multifd_send_state->count; i++) {
>> p = &multifd_send_state->params[i];
>> if (!p->address) {
>> // we found one IDLE send thread
>> break;
>> }
>> }
>> if (!p) {
>> qemu_sem_wait(&multifd_send_state->sem);
>> goto retry;
>> }
>> // we switch its state, IDLE -> ACTIVE
>> atomic_set(&p->address, address);
>> // tell the thread to start work
>> qemu_sem_post(&p->sem);
>>
>> Above didn't really use any lock at all (either the per thread lock,
>> or the global lock). Would it work?
>
> I think what's there can certainly be simplified; but also note
> that the later patch gets rid of 'address' and turns it into a count.
> My suggest was to keep the 'done' and stop using 'address' as something
> special; i.e. never write address in the thread; but I think yours might
> work as well.
I substitute the test from address == 0 to page.num == 0.
Notice that this is temporal, just to check that I am doing the things
right. we end sending the pages here.
Later, Juan.
Re: [Qemu-devel] [PATCH v5 10/17] migration: Create ram_multifd_page
"Dr. David Alan Gilbert" wrote:
> * Juan Quintela (quint...@redhat.com) wrote:
>> The function still don't use multifd, but we have simplified
>> ram_save_page, xbzrle and RDMA stuff is gone. We have added a new
>> counter and a new flag for this type of pages.
>>
>> Signed-off-by: Juan Quintela
>> ---
>> hmp.c | 2 ++
>> migration/migration.c | 1 +
>> migration/ram.c | 90
>> ++-
>> qapi-schema.json | 5 ++-
>> 4 files changed, 96 insertions(+), 2 deletions(-)
>>
>> diff --git a/hmp.c b/hmp.c
>> index b01605a..eeb308b 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -234,6 +234,8 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>> monitor_printf(mon, "postcopy request count: %" PRIu64 "\n",
>> info->ram->postcopy_requests);
>> }
>> +monitor_printf(mon, "multifd: %" PRIu64 " pages\n",
>> + info->ram->multifd);
>> }
>>
>> if (info->has_disk) {
>> diff --git a/migration/migration.c b/migration/migration.c
>> index e1c79d5..d9d5415 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -528,6 +528,7 @@ static void populate_ram_info(MigrationInfo *info,
>> MigrationState *s)
>> info->ram->dirty_sync_count = ram_counters.dirty_sync_count;
>> info->ram->postcopy_requests = ram_counters.postcopy_requests;
>> info->ram->page_size = qemu_target_page_size();
>> +info->ram->multifd = ram_counters.multifd;
>>
>> if (migrate_use_xbzrle()) {
>> info->has_xbzrle_cache = true;
>> diff --git a/migration/ram.c b/migration/ram.c
>> index b80f511..2bf3fa7 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -68,6 +68,7 @@
>> #define RAM_SAVE_FLAG_XBZRLE 0x40
>> /* 0x80 is reserved in migration.h start with 0x100 next */
>> #define RAM_SAVE_FLAG_COMPRESS_PAGE0x100
>> +#define RAM_SAVE_FLAG_MULTIFD_PAGE 0x200
>>
>> static inline bool is_zero_range(uint8_t *p, uint64_t size)
>> {
>> @@ -362,12 +363,17 @@ static void compress_threads_save_setup(void)
>> /* Multiple fd's */
>>
>> struct MultiFDSendParams {
>> +/* not changed */
>> uint8_t id;
>> QemuThread thread;
>> QIOChannel *c;
>> QemuSemaphore sem;
>> QemuMutex mutex;
>> +/* protected by param mutex */
>> bool quit;
>
> Should probably comment to say what address space address is in - this
> is really a qemu pointer - and that's why we can treat 0 as special?
Ok. Added
/* This is a temp field. We are using it now to transmit
something the address of the page. Later in the series, we
change it for the real page.
*/
>
>> +uint8_t *address;
>> +/* protected by multifd mutex */
>> +bool done;
>
> done needs a comment to explain what it is because
> it sounds similar to quit; I think 'done' is saying that
> the thread is idle having done what was asked?
/* has the thread finish the last submitted job */
>> +static int multifd_send_page(uint8_t *address)
>> +{
>> +int i;
>> +MultiFDSendParams *p = NULL; /* make happy gcc */
>> +
>> +qemu_sem_wait(&multifd_send_state->sem);
>> +qemu_mutex_lock(&multifd_send_state->mutex);
>> +for (i = 0; i < multifd_send_state->count; i++) {
>> +p = &multifd_send_state->params[i];
>> +
>> +if (p->done) {
>> +p->done = false;
>> +break;
>> +}
>> +}
>> +qemu_mutex_unlock(&multifd_send_state->mutex);
>> +qemu_mutex_lock(&p->mutex);
>> +p->address = address;
>> +qemu_mutex_unlock(&p->mutex);
>> +qemu_sem_post(&p->sem);
>
> My feeling, without having fully thought it through, is that
> the locking around 'address' can be simplified; especially if the
> sending-thread never actually changes it.
>
> http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_11
> defines that most of the pthread_ functions act as barriers;
> including the sem_post and pthread_cond_signal that qemu_sem_post
> uses.
At the end of the series the code is this:
qemu_mutex_lock(&p->mutex);
p->pages.num = pages->num;
iov_copy(p->pages.iov, pages->num, pages->iov, pages->num, 0,
iov_size(pages->iov, pages->num));
pages->num = 0;
qemu_mutex_unlock(&p->mutex);
Are you sure that it looks like a good idea to drop the mutex?
The other thread uses pages->num to know if things are ready.
>
>> +return 0;
>> +}
>> +
>> struct MultiFDRecvParams {
>> uint8_t id;
>> QemuThread thread;
>> @@ -537,6 +583,7 @@ void multifd_load_cleanup(void)
>> qemu_sem_destroy(&p->sem);
>> socket_recv_channel_destroy(p->c);
>> g_free(p);
>> +multifd_recv_state->params[i] = NULL;
>> }
>> g_free(multifd_recv_state->params);
>> multifd_recv_state->params = NULL;
>> @@ -1058,6 +1105,32 @@ static int ram_save_page(RAMState *rs,
>> PageSearchStatus *pss, bo
Re: [Qemu-devel] [PATCH v5 10/17] migration: Create ram_multifd_page
* Peter Xu (pet...@redhat.com) wrote:
> On Wed, Jul 19, 2017 at 08:02:39PM +0100, Dr. David Alan Gilbert wrote:
> > * Juan Quintela (quint...@redhat.com) wrote:
> > > The function still don't use multifd, but we have simplified
> > > ram_save_page, xbzrle and RDMA stuff is gone. We have added a new
> > > counter and a new flag for this type of pages.
> > >
> > > Signed-off-by: Juan Quintela
> > > ---
> > > hmp.c | 2 ++
> > > migration/migration.c | 1 +
> > > migration/ram.c | 90
> > > ++-
> > > qapi-schema.json | 5 ++-
> > > 4 files changed, 96 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hmp.c b/hmp.c
> > > index b01605a..eeb308b 100644
> > > --- a/hmp.c
> > > +++ b/hmp.c
> > > @@ -234,6 +234,8 @@ void hmp_info_migrate(Monitor *mon, const QDict
> > > *qdict)
> > > monitor_printf(mon, "postcopy request count: %" PRIu64 "\n",
> > > info->ram->postcopy_requests);
> > > }
> > > +monitor_printf(mon, "multifd: %" PRIu64 " pages\n",
> > > + info->ram->multifd);
> > > }
> > >
> > > if (info->has_disk) {
> > > diff --git a/migration/migration.c b/migration/migration.c
> > > index e1c79d5..d9d5415 100644
> > > --- a/migration/migration.c
> > > +++ b/migration/migration.c
> > > @@ -528,6 +528,7 @@ static void populate_ram_info(MigrationInfo *info,
> > > MigrationState *s)
> > > info->ram->dirty_sync_count = ram_counters.dirty_sync_count;
> > > info->ram->postcopy_requests = ram_counters.postcopy_requests;
> > > info->ram->page_size = qemu_target_page_size();
> > > +info->ram->multifd = ram_counters.multifd;
> > >
> > > if (migrate_use_xbzrle()) {
> > > info->has_xbzrle_cache = true;
> > > diff --git a/migration/ram.c b/migration/ram.c
> > > index b80f511..2bf3fa7 100644
> > > --- a/migration/ram.c
> > > +++ b/migration/ram.c
> > > @@ -68,6 +68,7 @@
> > > #define RAM_SAVE_FLAG_XBZRLE 0x40
> > > /* 0x80 is reserved in migration.h start with 0x100 next */
> > > #define RAM_SAVE_FLAG_COMPRESS_PAGE0x100
> > > +#define RAM_SAVE_FLAG_MULTIFD_PAGE 0x200
> > >
> > > static inline bool is_zero_range(uint8_t *p, uint64_t size)
> > > {
> > > @@ -362,12 +363,17 @@ static void compress_threads_save_setup(void)
> > > /* Multiple fd's */
> > >
> > > struct MultiFDSendParams {
> > > +/* not changed */
> > > uint8_t id;
> > > QemuThread thread;
> > > QIOChannel *c;
> > > QemuSemaphore sem;
> > > QemuMutex mutex;
> > > +/* protected by param mutex */
> > > bool quit;
> >
> > Should probably comment to say what address space address is in - this
> > is really a qemu pointer - and that's why we can treat 0 as special?
>
> I believe this comment is for "address" below.
>
> Yes, it would be nice to comment it. IIUC it belongs to virtual
> address space of QEMU, so it should be okay to use zero as a "special"
> value.
>
> >
> > > +uint8_t *address;
> > > +/* protected by multifd mutex */
> > > +bool done;
> >
> > done needs a comment to explain what it is because
> > it sounds similar to quit; I think 'done' is saying that
> > the thread is idle having done what was asked?
>
> Since we know that valid address won't be zero, not sure whether we
> can just avoid introducing the "done" field (even, not sure whether we
> will need lock when modifying "address", I think not as well? Please
> see below). For what I see this, it works like a state machine, and
> "address" can be the state:
>
> + send thread -+
> | |
>\|/ |
> address==0 (IDLE) address!=0 (ACTIVE)
> | /|\
> | |
> + main thread -+
>
> Then...
>
> >
> > > };
> > > typedef struct MultiFDSendParams MultiFDSendParams;
> > >
> > > @@ -375,6 +381,8 @@ struct {
> > > MultiFDSendParams *params;
> > > /* number of created threads */
> > > int count;
> > > +QemuMutex mutex;
> > > +QemuSemaphore sem;
> > > } *multifd_send_state;
> > >
> > > static void terminate_multifd_send_threads(void)
> > > @@ -443,6 +451,7 @@ static void *multifd_send_thread(void *opaque)
> > > } else {
> > > qio_channel_write(p->c, string, MULTIFD_UUID_MSG, &error_abort);
> > > }
> > > +qemu_sem_post(&multifd_send_state->sem);
> > >
> > > while (!exit) {
> > > qemu_mutex_lock(&p->mutex);
> > > @@ -450,6 +459,15 @@ static void *multifd_send_thread(void *opaque)
> > > qemu_mutex_unlock(&p->mutex);
> > > break;
> > > }
> > > +if (p->address) {
> > > +p->address = 0;
> > > +qemu_mutex_unlock(&p->mutex);
> > > +qemu_mutex_lock(&multif
Re: [Qemu-devel] [PATCH v5 10/17] migration: Create ram_multifd_page
On Wed, Jul 19, 2017 at 08:02:39PM +0100, Dr. David Alan Gilbert wrote:
> * Juan Quintela (quint...@redhat.com) wrote:
> > The function still don't use multifd, but we have simplified
> > ram_save_page, xbzrle and RDMA stuff is gone. We have added a new
> > counter and a new flag for this type of pages.
> >
> > Signed-off-by: Juan Quintela
> > ---
> > hmp.c | 2 ++
> > migration/migration.c | 1 +
> > migration/ram.c | 90
> > ++-
> > qapi-schema.json | 5 ++-
> > 4 files changed, 96 insertions(+), 2 deletions(-)
> >
> > diff --git a/hmp.c b/hmp.c
> > index b01605a..eeb308b 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -234,6 +234,8 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
> > monitor_printf(mon, "postcopy request count: %" PRIu64 "\n",
> > info->ram->postcopy_requests);
> > }
> > +monitor_printf(mon, "multifd: %" PRIu64 " pages\n",
> > + info->ram->multifd);
> > }
> >
> > if (info->has_disk) {
> > diff --git a/migration/migration.c b/migration/migration.c
> > index e1c79d5..d9d5415 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -528,6 +528,7 @@ static void populate_ram_info(MigrationInfo *info,
> > MigrationState *s)
> > info->ram->dirty_sync_count = ram_counters.dirty_sync_count;
> > info->ram->postcopy_requests = ram_counters.postcopy_requests;
> > info->ram->page_size = qemu_target_page_size();
> > +info->ram->multifd = ram_counters.multifd;
> >
> > if (migrate_use_xbzrle()) {
> > info->has_xbzrle_cache = true;
> > diff --git a/migration/ram.c b/migration/ram.c
> > index b80f511..2bf3fa7 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -68,6 +68,7 @@
> > #define RAM_SAVE_FLAG_XBZRLE 0x40
> > /* 0x80 is reserved in migration.h start with 0x100 next */
> > #define RAM_SAVE_FLAG_COMPRESS_PAGE0x100
> > +#define RAM_SAVE_FLAG_MULTIFD_PAGE 0x200
> >
> > static inline bool is_zero_range(uint8_t *p, uint64_t size)
> > {
> > @@ -362,12 +363,17 @@ static void compress_threads_save_setup(void)
> > /* Multiple fd's */
> >
> > struct MultiFDSendParams {
> > +/* not changed */
> > uint8_t id;
> > QemuThread thread;
> > QIOChannel *c;
> > QemuSemaphore sem;
> > QemuMutex mutex;
> > +/* protected by param mutex */
> > bool quit;
>
> Should probably comment to say what address space address is in - this
> is really a qemu pointer - and that's why we can treat 0 as special?
I believe this comment is for "address" below.
Yes, it would be nice to comment it. IIUC it belongs to virtual
address space of QEMU, so it should be okay to use zero as a "special"
value.
>
> > +uint8_t *address;
> > +/* protected by multifd mutex */
> > +bool done;
>
> done needs a comment to explain what it is because
> it sounds similar to quit; I think 'done' is saying that
> the thread is idle having done what was asked?
Since we know that valid address won't be zero, not sure whether we
can just avoid introducing the "done" field (even, not sure whether we
will need lock when modifying "address", I think not as well? Please
see below). For what I see this, it works like a state machine, and
"address" can be the state:
+ send thread -+
| |
\|/ |
address==0 (IDLE) address!=0 (ACTIVE)
| /|\
| |
+ main thread -+
Then...
>
> > };
> > typedef struct MultiFDSendParams MultiFDSendParams;
> >
> > @@ -375,6 +381,8 @@ struct {
> > MultiFDSendParams *params;
> > /* number of created threads */
> > int count;
> > +QemuMutex mutex;
> > +QemuSemaphore sem;
> > } *multifd_send_state;
> >
> > static void terminate_multifd_send_threads(void)
> > @@ -443,6 +451,7 @@ static void *multifd_send_thread(void *opaque)
> > } else {
> > qio_channel_write(p->c, string, MULTIFD_UUID_MSG, &error_abort);
> > }
> > +qemu_sem_post(&multifd_send_state->sem);
> >
> > while (!exit) {
> > qemu_mutex_lock(&p->mutex);
> > @@ -450,6 +459,15 @@ static void *multifd_send_thread(void *opaque)
> > qemu_mutex_unlock(&p->mutex);
> > break;
> > }
> > +if (p->address) {
> > +p->address = 0;
> > +qemu_mutex_unlock(&p->mutex);
> > +qemu_mutex_lock(&multifd_send_state->mutex);
> > +p->done = true;
> > +qemu_mutex_unlock(&multifd_send_state->mutex);
> > +qemu_sem_post(&multifd_send_state->sem);
> > +continue;
Here instead of setting up address=0 at the entry, can we do this (no
"done" for this time)?
Re: [Qemu-devel] [PATCH v5 10/17] migration: Create ram_multifd_page
* Juan Quintela (quint...@redhat.com) wrote:
> The function still don't use multifd, but we have simplified
> ram_save_page, xbzrle and RDMA stuff is gone. We have added a new
> counter and a new flag for this type of pages.
>
> Signed-off-by: Juan Quintela
> ---
> hmp.c | 2 ++
> migration/migration.c | 1 +
> migration/ram.c | 90
> ++-
> qapi-schema.json | 5 ++-
> 4 files changed, 96 insertions(+), 2 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index b01605a..eeb308b 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -234,6 +234,8 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
> monitor_printf(mon, "postcopy request count: %" PRIu64 "\n",
> info->ram->postcopy_requests);
> }
> +monitor_printf(mon, "multifd: %" PRIu64 " pages\n",
> + info->ram->multifd);
> }
>
> if (info->has_disk) {
> diff --git a/migration/migration.c b/migration/migration.c
> index e1c79d5..d9d5415 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -528,6 +528,7 @@ static void populate_ram_info(MigrationInfo *info,
> MigrationState *s)
> info->ram->dirty_sync_count = ram_counters.dirty_sync_count;
> info->ram->postcopy_requests = ram_counters.postcopy_requests;
> info->ram->page_size = qemu_target_page_size();
> +info->ram->multifd = ram_counters.multifd;
>
> if (migrate_use_xbzrle()) {
> info->has_xbzrle_cache = true;
> diff --git a/migration/ram.c b/migration/ram.c
> index b80f511..2bf3fa7 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -68,6 +68,7 @@
> #define RAM_SAVE_FLAG_XBZRLE 0x40
> /* 0x80 is reserved in migration.h start with 0x100 next */
> #define RAM_SAVE_FLAG_COMPRESS_PAGE0x100
> +#define RAM_SAVE_FLAG_MULTIFD_PAGE 0x200
>
> static inline bool is_zero_range(uint8_t *p, uint64_t size)
> {
> @@ -362,12 +363,17 @@ static void compress_threads_save_setup(void)
> /* Multiple fd's */
>
> struct MultiFDSendParams {
> +/* not changed */
> uint8_t id;
> QemuThread thread;
> QIOChannel *c;
> QemuSemaphore sem;
> QemuMutex mutex;
> +/* protected by param mutex */
> bool quit;
Should probably comment to say what address space address is in - this
is really a qemu pointer - and that's why we can treat 0 as special?
> +uint8_t *address;
> +/* protected by multifd mutex */
> +bool done;
done needs a comment to explain what it is because
it sounds similar to quit; I think 'done' is saying that
the thread is idle having done what was asked?
> };
> typedef struct MultiFDSendParams MultiFDSendParams;
>
> @@ -375,6 +381,8 @@ struct {
> MultiFDSendParams *params;
> /* number of created threads */
> int count;
> +QemuMutex mutex;
> +QemuSemaphore sem;
> } *multifd_send_state;
>
> static void terminate_multifd_send_threads(void)
> @@ -443,6 +451,7 @@ static void *multifd_send_thread(void *opaque)
> } else {
> qio_channel_write(p->c, string, MULTIFD_UUID_MSG, &error_abort);
> }
> +qemu_sem_post(&multifd_send_state->sem);
>
> while (!exit) {
> qemu_mutex_lock(&p->mutex);
> @@ -450,6 +459,15 @@ static void *multifd_send_thread(void *opaque)
> qemu_mutex_unlock(&p->mutex);
> break;
> }
> +if (p->address) {
> +p->address = 0;
> +qemu_mutex_unlock(&p->mutex);
> +qemu_mutex_lock(&multifd_send_state->mutex);
> +p->done = true;
> +qemu_mutex_unlock(&multifd_send_state->mutex);
> +qemu_sem_post(&multifd_send_state->sem);
> +continue;
> +}
> qemu_mutex_unlock(&p->mutex);
> qemu_sem_wait(&p->sem);
> }
> @@ -469,6 +487,8 @@ int multifd_save_setup(void)
> multifd_send_state = g_malloc0(sizeof(*multifd_send_state));
> multifd_send_state->params = g_new0(MultiFDSendParams, thread_count);
> multifd_send_state->count = 0;
> +qemu_mutex_init(&multifd_send_state->mutex);
> +qemu_sem_init(&multifd_send_state->sem, 0);
> for (i = 0; i < thread_count; i++) {
> char thread_name[16];
> MultiFDSendParams *p = &multifd_send_state->params[i];
> @@ -477,6 +497,8 @@ int multifd_save_setup(void)
> qemu_sem_init(&p->sem, 0);
> p->quit = false;
> p->id = i;
> +p->done = true;
> +p->address = 0;
> p->c = socket_send_channel_create();
> if (!p->c) {
> error_report("Error creating a send channel");
> @@ -491,6 +513,30 @@ int multifd_save_setup(void)
> return 0;
> }
>
> +static int multifd_send_page(uint8_t *address)
> +{
> +int i;
> +MultiFDSendParams *p = NULL; /* make happy gcc */
> +
> +qemu_sem_wait(&multifd_send_state->sem);
> +qemu_mutex_lock(&multifd_send_state->mutex);
> +for (i = 0;
