We know quit closing the QIO.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/ram.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index a707d3ae80..62eea9e02f 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -587,14 +587,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 */
@@ -1115,8 +1111,8 @@ 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);
+        object_unref(OBJECT(p->c));
+        p->c = NULL;
         qemu_mutex_unlock(&p->mutex);
     }
 }
@@ -1139,7 +1135,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;
@@ -1262,9 +1257,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.0

* Juan Quintela (quintela@redhat.com) wrote:
> We know quit closing the QIO.

This patch does two different things; one of which I think I understand.

The 'quit' has been removed - I think that makes sense because the
multifd threads terminate when either they come to the end of a stream
or hit an error; there's no case of asking them to quit explicitly.
The 'sem' was basically for kicking the recv-thread to quit; which again
isn't needed.

Now, what about the object_unref on p->c ?
If I've got this right multifd_recv_terminate_threads is only called in
the error case; but doesn't multifd_load_cleanup also get called in that
case - it does the unref and p->c = NULL as well.

Dave

> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/ram.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index a707d3ae80..62eea9e02f 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -587,14 +587,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 */
> @@ -1115,8 +1111,8 @@ 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);
> +        object_unref(OBJECT(p->c));
> +        p->c = NULL;
>          qemu_mutex_unlock(&p->mutex);
>      }
>  }
> @@ -1139,7 +1135,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;
> @@ -1262,9 +1257,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.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> We know quit closing the QIO.
>
> This patch does two different things; one of which I think I understand.
>
> The 'quit' has been removed - I think that makes sense because the
> multifd threads terminate when either they come to the end of a stream
> or hit an error; there's no case of asking them to quit explicitly.
> The 'sem' was basically for kicking the recv-thread to quit; which again
> isn't needed.
>
> Now, what about the object_unref on p->c ?
> If I've got this right multifd_recv_terminate_threads is only called in
> the error case; but doesn't multifd_load_cleanup also get called in that
> case - it does the unref and p->c = NULL as well.

Adding another comment O:-)

In normal exit case: we don't care.

In error case, we do a close of the p->c.  Doing a close, means that the
channel threads that are waiting on qio_channel_read_all_eof() will stop
the waiting and return an error, so we are safe.

        ret = qio_channel_read_all_eof(p->c, (void *)p->packet,
                                       p->packet_len, &local_err);
        if (ret == 0) {   /* EOF */
            break;
        }
        if (ret == -1) {   /* Error */
            break;
        }

We have to wait for three things:
- we receive a synchronization packet
- we receive EOF and finish
- we got somehow an error and have to quit.

Old versions have a semaphore, and we have three places where we set
that semaphore "we are ready", when we receive one error, when we have
data waiting to be read (it had a qio whatcher) or we just got a normal
exit.

Waiting on two things make code more complex that it used to be (and
whatchers are really lame, because we know there are data ready, but not
how much, dealing with non-whole packets/pages is a real mess).  So we
moved to:
-  quit is gone: we just close the channel, that makes the *_read_eof()
   to end.  Notice that we don't care if the close is due to one error
   or because we have finished.  It is just done.
- Now reception channel only have to wait on _read_eof(), the three
  cases that we talked before are handled correctly.

I understand the confusion, this only makes sense when you came from the
previous version of the patchset.

Later, Juan.

* 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.
> >
> > This patch does two different things; one of which I think I understand.
> >
> > The 'quit' has been removed - I think that makes sense because the
> > multifd threads terminate when either they come to the end of a stream
> > or hit an error; there's no case of asking them to quit explicitly.
> > The 'sem' was basically for kicking the recv-thread to quit; which again
> > isn't needed.
> >
> > Now, what about the object_unref on p->c ?
> > If I've got this right multifd_recv_terminate_threads is only called in
> > the error case; but doesn't multifd_load_cleanup also get called in that
> > case - it does the unref and p->c = NULL as well.
> 
> Adding another comment O:-)
> 
> In normal exit case: we don't care.
> 
> In error case, we do a close of the p->c.  Doing a close, means that the
> channel threads that are waiting on qio_channel_read_all_eof() will stop
> the waiting and return an error, so we are safe.
> 
>         ret = qio_channel_read_all_eof(p->c, (void *)p->packet,
>                                        p->packet_len, &local_err);
>         if (ret == 0) {   /* EOF */
>             break;
>         }
>         if (ret == -1) {   /* Error */
>             break;
>         }
> 
> We have to wait for three things:
> - we receive a synchronization packet
> - we receive EOF and finish
> - we got somehow an error and have to quit.
> 
> Old versions have a semaphore, and we have three places where we set
> that semaphore "we are ready", when we receive one error, when we have
> data waiting to be read (it had a qio whatcher) or we just got a normal
> exit.
> 
> Waiting on two things make code more complex that it used to be (and
> whatchers are really lame, because we know there are data ready, but not
> how much, dealing with non-whole packets/pages is a real mess).  So we
> moved to:
> -  quit is gone: we just close the channel, that makes the *_read_eof()
>    to end.  Notice that we don't care if the close is due to one error
>    or because we have finished.  It is just done.
> - Now reception channel only have to wait on _read_eof(), the three
>   cases that we talked before are handled correctly.
> 
> I understand the confusion, this only makes sense when you came from the
> previous version of the patchset.

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

  b) I don't think your answer explains why it's an object_unref?

Dave

> Later, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

"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.
>> >
>> > This patch does two different things; one of which I think I understand.
>> >
>> > The 'quit' has been removed - I think that makes sense because the
>> > multifd threads terminate when either they come to the end of a stream
>> > or hit an error; there's no case of asking them to quit explicitly.
>> > The 'sem' was basically for kicking the recv-thread to quit; which again
>> > isn't needed.
>> >
>> > Now, what about the object_unref on p->c ?
>> > If I've got this right multifd_recv_terminate_threads is only called in
>> > the error case; but doesn't multifd_load_cleanup also get called in that
>> > case - it does the unref and p->c = NULL as well.
>> 
>> Adding another comment O:-)
>> 
>> In normal exit case: we don't care.
>> 
>> In error case, we do a close of the p->c.  Doing a close, means that the
>> channel threads that are waiting on qio_channel_read_all_eof() will stop
>> the waiting and return an error, so we are safe.
>> 
>>         ret = qio_channel_read_all_eof(p->c, (void *)p->packet,
>>                                        p->packet_len, &local_err);
>>         if (ret == 0) {   /* EOF */
>>             break;
>>         }
>>         if (ret == -1) {   /* Error */
>>             break;
>>         }
>> 
>> We have to wait for three things:
>> - we receive a synchronization packet
>> - we receive EOF and finish
>> - we got somehow an error and have to quit.
>> 
>> Old versions have a semaphore, and we have three places where we set
>> that semaphore "we are ready", when we receive one error, when we have
>> data waiting to be read (it had a qio whatcher) or we just got a normal
>> exit.
>> 
>> Waiting on two things make code more complex that it used to be (and
>> whatchers are really lame, because we know there are data ready, but not
>> how much, dealing with non-whole packets/pages is a real mess).  So we
>> moved to:
>> -  quit is gone: we just close the channel, that makes the *_read_eof()
>>    to end.  Notice that we don't care if the close is due to one error
>>    or because we have finished.  It is just done.
>> - Now reception channel only have to wait on _read_eof(), the three
>>   cases that we talked before are handled correctly.
>> 
>> I understand the confusion, this only makes sense when you came from the
>> previous version of the patchset.
>
> 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.

Thanks, Juan.

> Dave
>
>> Later, Juan.
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

* 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.
> >> >
> >> > This patch does two different things; one of which I think I understand.
> >> >
> >> > The 'quit' has been removed - I think that makes sense because the
> >> > multifd threads terminate when either they come to the end of a stream
> >> > or hit an error; there's no case of asking them to quit explicitly.
> >> > The 'sem' was basically for kicking the recv-thread to quit; which again
> >> > isn't needed.
> >> >
> >> > Now, what about the object_unref on p->c ?
> >> > If I've got this right multifd_recv_terminate_threads is only called in
> >> > the error case; but doesn't multifd_load_cleanup also get called in that
> >> > case - it does the unref and p->c = NULL as well.
> >> 
> >> Adding another comment O:-)
> >> 
> >> In normal exit case: we don't care.
> >> 
> >> In error case, we do a close of the p->c.  Doing a close, means that the
> >> channel threads that are waiting on qio_channel_read_all_eof() will stop
> >> the waiting and return an error, so we are safe.
> >> 
> >>         ret = qio_channel_read_all_eof(p->c, (void *)p->packet,
> >>                                        p->packet_len, &local_err);
> >>         if (ret == 0) {   /* EOF */
> >>             break;
> >>         }
> >>         if (ret == -1) {   /* Error */
> >>             break;
> >>         }
> >> 
> >> We have to wait for three things:
> >> - we receive a synchronization packet
> >> - we receive EOF and finish
> >> - we got somehow an error and have to quit.
> >> 
> >> Old versions have a semaphore, and we have three places where we set
> >> that semaphore "we are ready", when we receive one error, when we have
> >> data waiting to be read (it had a qio whatcher) or we just got a normal
> >> exit.
> >> 
> >> Waiting on two things make code more complex that it used to be (and
> >> whatchers are really lame, because we know there are data ready, but not
> >> how much, dealing with non-whole packets/pages is a real mess).  So we
> >> moved to:
> >> -  quit is gone: we just close the channel, that makes the *_read_eof()
> >>    to end.  Notice that we don't care if the close is due to one error
> >>    or because we have finished.  It is just done.
> >> - Now reception channel only have to wait on _read_eof(), the three
> >>   cases that we talked before are handled correctly.
> >> 
> >> I understand the confusion, this only makes sense when you came from the
> >> previous version of the patchset.
> >
> > 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

"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);
         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

* 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

"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:
>> >> >> "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?

Yeap.

static void multifd_recv_terminate_threads(Error *err)
{
    [...]
    for (i = 0; i < migrate_multifd_channels(); i++) {
        MultiFDRecvParams *p = &multifd_recv_state->params[i];

        qemu_mutex_lock(&p->mutex);
        /* 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);
        qemu_mutex_unlock(&p->mutex);
    }
}

int multifd_load_cleanup(Error **errp)
{
    int i;
    int ret = 0;

    if (!migrate_use_multifd()) {
        return 0;
    }
    multifd_recv_terminate_threads(NULL);
    for (i = 0; i < migrate_multifd_channels(); i++) {
        MultiFDRecvParams *p = &multifd_recv_state->params[i];

        if (p->running) {
            qemu_thread_join(&p->thread);
        }
        object_unref(OBJECT(p->c));
        p->c = NULL;
        [...]
    }
    [...]
}

Omited the things that we don't care about for this.

Thanks, Juan.

* 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:
> >> >> >> "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?
> 
> Yeap.
> 
> static void multifd_recv_terminate_threads(Error *err)
> {
>     [...]
>     for (i = 0; i < migrate_multifd_channels(); i++) {
>         MultiFDRecvParams *p = &multifd_recv_state->params[i];
> 
>         qemu_mutex_lock(&p->mutex);
>         /* 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);
>         qemu_mutex_unlock(&p->mutex);
>     }
> }
> 
> int multifd_load_cleanup(Error **errp)
> {
>     int i;
>     int ret = 0;
> 
>     if (!migrate_use_multifd()) {
>         return 0;
>     }
>     multifd_recv_terminate_threads(NULL);
>     for (i = 0; i < migrate_multifd_channels(); i++) {
>         MultiFDRecvParams *p = &multifd_recv_state->params[i];
> 
>         if (p->running) {
>             qemu_thread_join(&p->thread);
>         }
>         object_unref(OBJECT(p->c));
>         p->c = NULL;
>         [...]
>     }
>     [...]
> }
> 
> Omited the things that we don't care about for this.

I think that looks OK; you've got one unref at the end, and you do the
shutdown to actually kick them out.

Dave

> Thanks, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK