Re: [Qemu-devel] [PATCH v13 11/12] migration: Remove not needed semaphore and quit

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > * Juan Quintela (quintela@redhat.com) wrote:
> >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> >> > * Juan Quintela (quintela@redhat.com) wrote:
> >> >> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> >> >> > * Juan Quintela (quintela@redhat.com) wrote:
> >> >> >> We know quit closing the QIO.
> 
> ...
> >> > Two questions from that then:
> >> >   a) Are you sure it's safe to close the qio_channel while another
> >> > thread is in qio_channel_read_all_eof?  Is it really defined that it
> >> > causes the other thread to exit with an error;  close() in some stuff
> >> > frees data structures that the other thread is still reading; that's
> >> > why I've used shutdown(2) in the past rather than close on fd's
> >> 
> >> Dunno if it is safe (I think it is), but I agree that shutdown will also
> >> get what I need.
> >> 
> >> >   b) I don't think your answer explains why it's an object_unref?
> >> 
> >> That is the standard way to closing qios to not have to take into
> >> account who have it oppened.  See previous paragraph, it is better to
> >> use shutdown, done.
> >
> > OK, great;  I suspect it's unsafe because as soon as you do the unref
> > it could free the object; actually you should have a ref from each of
> > the threads to sotp it being freed while they use it.
> >
> > Dave
> >
> >> Thanks, Juan.
> >> 
> >> > Dave
> >> >
> >> >> Later, Juan.
> >> > --
> >> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> What do you think about this, to avoid me resend the whole series?
> 
> From e03d77de1ca179fa0168cead7c23cfeae57f1787 Mon Sep 17 00:00:00 2001
> From: Juan Quintela <quintela@redhat.com>
> Date: Wed, 18 Apr 2018 00:49:19 +0200
> Subject: [PATCH 17/18] migration: Remove not needed semaphore and quit
> 
> We know quit with shutdwon in the QIO.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> --
> Add comment
> Use shutdown() instead of unref()
> ---
>  migration/ram.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 2c3a452a7d..be5d26f4cb 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -594,14 +594,10 @@ typedef struct {
>      QemuThread thread;
>      /* communication channel */
>      QIOChannel *c;
> -    /* sem where to wait for more work */
> -    QemuSemaphore sem;
>      /* this mutex protects the following parameters */
>      QemuMutex mutex;
>      /* is this channel thread running */
>      bool running;
> -    /* should this thread finish */
> -    bool quit;
>      /* array of pages to receive */
>      MultiFDPages_t *pages;
>      /* packet allocated len */
> @@ -1152,8 +1148,11 @@ static void multifd_recv_terminate_threads(Error *err)
>          MultiFDRecvParams *p = &multifd_recv_state->params[i];
>  
>          qemu_mutex_lock(&p->mutex);
> -        p->quit = true;
> -        qemu_sem_post(&p->sem);
> +        /* We could arrive here for two reasons:
> +           - normal quit, i.e. everything went fine, just finished
> +           - error quit: We close the channels so the channel threads
> +             finish the qio_channel_read_all_eof() */
> +        qio_channel_shutdown(p->c, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);

OK, so with any luck all the threads now exit;  do we still have a
close/unref once we're sure they have all exited?

Dave

>          qemu_mutex_unlock(&p->mutex);
>      }
>  }
> @@ -1176,7 +1175,6 @@ int multifd_load_cleanup(Error **errp)
>          object_unref(OBJECT(p->c));
>          p->c = NULL;
>          qemu_mutex_destroy(&p->mutex);
> -        qemu_sem_destroy(&p->sem);
>          qemu_sem_destroy(&p->sem_sync);
>          g_free(p->name);
>          p->name = NULL;
> @@ -1299,9 +1297,7 @@ int multifd_load_setup(void)
>          MultiFDRecvParams *p = &multifd_recv_state->params[i];
>  
>          qemu_mutex_init(&p->mutex);
> -        qemu_sem_init(&p->sem, 0);
>          qemu_sem_init(&p->sem_sync, 0);
> -        p->quit = false;
>          p->id = i;
>          p->pages = multifd_pages_init(page_count);
>          p->packet_len = sizeof(MultiFDPacket_t)
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK