[PATCH 0/3] colo-compare bugfixes

[PATCH 0/3] colo-compare bugfixes

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 446 bytes --]

Hello Everyone,
Here are fixes for bugs that I found in my tests.

Regards,
Lukas Straub

Lukas Straub (3):
  net/colo-compare.c: Create event_bh with the right AioContext
  chardev/char.c: Use qemu_co_sleep_ns if in coroutine
  net/colo-compare.c: Fix deadlock

 chardev/char.c     |  7 +++-
 net/colo-compare.c | 85 +++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 79 insertions(+), 13 deletions(-)

-- 
2.20.1

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 1/3] net/colo-compare.c: Create event_bh with the right AioContext

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 1311 bytes --]

qemu_bh_new will set the bh to be executed in the main
loop. This causes problems as colo_compare_handle_event assumes
that it has exclusive access the queues, which are also
accessed in the iothread. It also assumes that it runs in a
different thread than the caller and takes the appropriate
locks.

Create the bh with the AioContext of the iothread to fulfill
these assumptions.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 net/colo-compare.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 10c0239f9d..1de4220fe2 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -890,6 +890,7 @@ static void colo_compare_handle_event(void *opaque)
 
 static void colo_compare_iothread(CompareState *s)
 {
+    AioContext *ctx = iothread_get_aio_context(s->iothread);
     object_ref(OBJECT(s->iothread));
     s->worker_context = iothread_get_g_main_context(s->iothread);
 
@@ -906,7 +907,7 @@ static void colo_compare_iothread(CompareState *s)
     }
 
     colo_compare_timer_init(s);
-    s->event_bh = qemu_bh_new(colo_compare_handle_event, s);
+    s->event_bh = aio_bh_new(ctx, colo_compare_handle_event, s);
 }
 
 static char *compare_get_pri_indev(Object *obj, Error **errp)
-- 
2.20.1


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 1/3] net/colo-compare.c: Create event_bh with the right AioContext

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Thursday, April 9, 2020 2:34 AM
> To: qemu-devel <qemu-devel@nongnu.org>
> Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>
> Subject: [PATCH 1/3] net/colo-compare.c: Create event_bh with the right
> AioContext
> 
> qemu_bh_new will set the bh to be executed in the main loop. This causes
> problems as colo_compare_handle_event assumes that it has exclusive
> access the queues, which are also accessed in the iothread. It also assumes
> that it runs in a different thread than the caller and takes the appropriate
> locks.
> 
> Create the bh with the AioContext of the iothread to fulfill these
> assumptions.
> 

Looks good for me, I assume it will increase performance. Do you have related data?

Thanks
Zhang Chen

> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>  net/colo-compare.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> 10c0239f9d..1de4220fe2 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -890,6 +890,7 @@ static void colo_compare_handle_event(void
> *opaque)
> 
>  static void colo_compare_iothread(CompareState *s)  {
> +    AioContext *ctx = iothread_get_aio_context(s->iothread);
>      object_ref(OBJECT(s->iothread));
>      s->worker_context = iothread_get_g_main_context(s->iothread);
> 
> @@ -906,7 +907,7 @@ static void colo_compare_iothread(CompareState *s)
>      }
> 
>      colo_compare_timer_init(s);
> -    s->event_bh = qemu_bh_new(colo_compare_handle_event, s);
> +    s->event_bh = aio_bh_new(ctx, colo_compare_handle_event, s);
>  }
> 
>  static char *compare_get_pri_indev(Object *obj, Error **errp)
> --
> 2.20.1

Re: [PATCH 1/3] net/colo-compare.c: Create event_bh with the right AioContext

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 2306 bytes --]

On Wed, 22 Apr 2020 08:29:39 +0000
"Zhang, Chen" <chen.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: Lukas Straub <lukasstraub2@web.de>
> > Sent: Thursday, April 9, 2020 2:34 AM
> > To: qemu-devel <qemu-devel@nongnu.org>
> > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > <pbonzini@redhat.com>
> > Subject: [PATCH 1/3] net/colo-compare.c: Create event_bh with the right
> > AioContext
> > 
> > qemu_bh_new will set the bh to be executed in the main loop. This causes
> > problems as colo_compare_handle_event assumes that it has exclusive
> > access the queues, which are also accessed in the iothread. It also assumes
> > that it runs in a different thread than the caller and takes the appropriate
> > locks.
> > 
> > Create the bh with the AioContext of the iothread to fulfill these
> > assumptions.
> >   
> 
> Looks good for me, I assume it will increase performance. Do you have related data?

No, this fixes several crashes because the queues where accessed concurrently from
multiple threads. Sorry for my bad wording.

Regards,
Lukas Straub

> Thanks
> Zhang Chen
> 
> > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > ---
> >  net/colo-compare.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > 10c0239f9d..1de4220fe2 100644
> > --- a/net/colo-compare.c
> > +++ b/net/colo-compare.c
> > @@ -890,6 +890,7 @@ static void colo_compare_handle_event(void
> > *opaque)
> > 
> >  static void colo_compare_iothread(CompareState *s)  {
> > +    AioContext *ctx = iothread_get_aio_context(s->iothread);
> >      object_ref(OBJECT(s->iothread));
> >      s->worker_context = iothread_get_g_main_context(s->iothread);
> > 
> > @@ -906,7 +907,7 @@ static void colo_compare_iothread(CompareState *s)
> >      }
> > 
> >      colo_compare_timer_init(s);
> > -    s->event_bh = qemu_bh_new(colo_compare_handle_event, s);
> > +    s->event_bh = aio_bh_new(ctx, colo_compare_handle_event, s);
> >  }
> > 
> >  static char *compare_get_pri_indev(Object *obj, Error **errp)
> > --
> > 2.20.1  
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 1/3] net/colo-compare.c: Create event_bh with the right AioContext

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Wednesday, April 22, 2020 4:43 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>
> Subject: Re: [PATCH 1/3] net/colo-compare.c: Create event_bh with the right
> AioContext
> 
> On Wed, 22 Apr 2020 08:29:39 +0000
> "Zhang, Chen" <chen.zhang@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Lukas Straub <lukasstraub2@web.de>
> > > Sent: Thursday, April 9, 2020 2:34 AM
> > > To: qemu-devel <qemu-devel@nongnu.org>
> > > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > > <pbonzini@redhat.com>
> > > Subject: [PATCH 1/3] net/colo-compare.c: Create event_bh with the
> > > right AioContext
> > >
> > > qemu_bh_new will set the bh to be executed in the main loop. This
> > > causes problems as colo_compare_handle_event assumes that it has
> > > exclusive access the queues, which are also accessed in the
> > > iothread. It also assumes that it runs in a different thread than
> > > the caller and takes the appropriate locks.
> > >
> > > Create the bh with the AioContext of the iothread to fulfill these
> > > assumptions.
> > >
> >
> > Looks good for me, I assume it will increase performance. Do you have
> related data?
> 
> No, this fixes several crashes because the queues where accessed
> concurrently from multiple threads. Sorry for my bad wording.

Can you describe some details about the crash? Step by step?
Maybe I can re-produce and test it for this patch.

Thanks
Zhang Chen

> 
> Regards,
> Lukas Straub
> 
> > Thanks
> > Zhang Chen
> >
> > > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > > ---
> > >  net/colo-compare.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > 10c0239f9d..1de4220fe2 100644
> > > --- a/net/colo-compare.c
> > > +++ b/net/colo-compare.c
> > > @@ -890,6 +890,7 @@ static void colo_compare_handle_event(void
> > > *opaque)
> > >
> > >  static void colo_compare_iothread(CompareState *s)  {
> > > +    AioContext *ctx = iothread_get_aio_context(s->iothread);
> > >      object_ref(OBJECT(s->iothread));
> > >      s->worker_context = iothread_get_g_main_context(s->iothread);
> > >
> > > @@ -906,7 +907,7 @@ static void colo_compare_iothread(CompareState
> *s)
> > >      }
> > >
> > >      colo_compare_timer_init(s);
> > > -    s->event_bh = qemu_bh_new(colo_compare_handle_event, s);
> > > +    s->event_bh = aio_bh_new(ctx, colo_compare_handle_event, s);
> > >  }
> > >
> > >  static char *compare_get_pri_indev(Object *obj, Error **errp)
> > > --
> > > 2.20.1
> >

Re: [PATCH 1/3] net/colo-compare.c: Create event_bh with the right AioContext

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 3625 bytes --]

On Wed, 22 Apr 2020 09:03:00 +0000
"Zhang, Chen" <chen.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: Lukas Straub <lukasstraub2@web.de>
> > Sent: Wednesday, April 22, 2020 4:43 PM
> > To: Zhang, Chen <chen.zhang@intel.com>
> > Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > <pbonzini@redhat.com>
> > Subject: Re: [PATCH 1/3] net/colo-compare.c: Create event_bh with the right
> > AioContext
> > 
> > On Wed, 22 Apr 2020 08:29:39 +0000
> > "Zhang, Chen" <chen.zhang@intel.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Lukas Straub <lukasstraub2@web.de>
> > > > Sent: Thursday, April 9, 2020 2:34 AM
> > > > To: qemu-devel <qemu-devel@nongnu.org>
> > > > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> > > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > > > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > > > <pbonzini@redhat.com>
> > > > Subject: [PATCH 1/3] net/colo-compare.c: Create event_bh with the
> > > > right AioContext
> > > >
> > > > qemu_bh_new will set the bh to be executed in the main loop. This
> > > > causes problems as colo_compare_handle_event assumes that it has
> > > > exclusive access the queues, which are also accessed in the
> > > > iothread. It also assumes that it runs in a different thread than
> > > > the caller and takes the appropriate locks.
> > > >
> > > > Create the bh with the AioContext of the iothread to fulfill these
> > > > assumptions.
> > > >  
> > >
> > > Looks good for me, I assume it will increase performance. Do you have  
> > related data?
> > 
> > No, this fixes several crashes because the queues where accessed
> > concurrently from multiple threads. Sorry for my bad wording.  
> 
> Can you describe some details about the crash? Step by step?
> Maybe I can re-produce and test it for this patch.

There is no clear test case. For me the crashes happened after 1-20h of runtime
with lots of checkpoints (800ms) and some network traffic. The coredump always
showed that two threads where doing operations on the queues simultaneously.
Unfortunately, I don't have the coredumps anymore.

Regards,
Lukas Straub

> Thanks
> Zhang Chen
> 
> > 
> > Regards,
> > Lukas Straub
> >   
> > > Thanks
> > > Zhang Chen
> > >  
> > > > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > > > ---
> > > >  net/colo-compare.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > > 10c0239f9d..1de4220fe2 100644
> > > > --- a/net/colo-compare.c
> > > > +++ b/net/colo-compare.c
> > > > @@ -890,6 +890,7 @@ static void colo_compare_handle_event(void
> > > > *opaque)
> > > >
> > > >  static void colo_compare_iothread(CompareState *s)  {
> > > > +    AioContext *ctx = iothread_get_aio_context(s->iothread);
> > > >      object_ref(OBJECT(s->iothread));
> > > >      s->worker_context = iothread_get_g_main_context(s->iothread);
> > > >
> > > > @@ -906,7 +907,7 @@ static void colo_compare_iothread(CompareState  
> > *s)  
> > > >      }
> > > >
> > > >      colo_compare_timer_init(s);
> > > > -    s->event_bh = qemu_bh_new(colo_compare_handle_event, s);
> > > > +    s->event_bh = aio_bh_new(ctx, colo_compare_handle_event, s);
> > > >  }
> > > >
> > > >  static char *compare_get_pri_indev(Object *obj, Error **errp)
> > > > --
> > > > 2.20.1  
> > >  
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 1/3] net/colo-compare.c: Create event_bh with the right AioContext

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Wednesday, April 22, 2020 5:40 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>
> Subject: Re: [PATCH 1/3] net/colo-compare.c: Create event_bh with the right
> AioContext
> 
> On Wed, 22 Apr 2020 09:03:00 +0000
> "Zhang, Chen" <chen.zhang@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Lukas Straub <lukasstraub2@web.de>
> > > Sent: Wednesday, April 22, 2020 4:43 PM
> > > To: Zhang, Chen <chen.zhang@intel.com>
> > > Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > > <pbonzini@redhat.com>
> > > Subject: Re: [PATCH 1/3] net/colo-compare.c: Create event_bh with
> > > the right AioContext
> > >
> > > On Wed, 22 Apr 2020 08:29:39 +0000
> > > "Zhang, Chen" <chen.zhang@intel.com> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Lukas Straub <lukasstraub2@web.de>
> > > > > Sent: Thursday, April 9, 2020 2:34 AM
> > > > > To: qemu-devel <qemu-devel@nongnu.org>
> > > > > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> > > > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>;
> > > > > Marc- André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > > > > <pbonzini@redhat.com>
> > > > > Subject: [PATCH 1/3] net/colo-compare.c: Create event_bh with
> > > > > the right AioContext
> > > > >
> > > > > qemu_bh_new will set the bh to be executed in the main loop.
> > > > > This causes problems as colo_compare_handle_event assumes that
> > > > > it has exclusive access the queues, which are also accessed in
> > > > > the iothread. It also assumes that it runs in a different thread
> > > > > than the caller and takes the appropriate locks.
> > > > >
> > > > > Create the bh with the AioContext of the iothread to fulfill
> > > > > these assumptions.
> > > > >
> > > >
> > > > Looks good for me, I assume it will increase performance. Do you
> > > > have
> > > related data?
> > >
> > > No, this fixes several crashes because the queues where accessed
> > > concurrently from multiple threads. Sorry for my bad wording.
> >
> > Can you describe some details about the crash? Step by step?
> > Maybe I can re-produce and test it for this patch.
> 
> There is no clear test case. For me the crashes happened after 1-20h of
> runtime with lots of checkpoints (800ms) and some network traffic. The
> coredump always showed that two threads where doing operations on the
> queues simultaneously.
> Unfortunately, I don't have the coredumps anymore.

OK, Although I have not encountered the problem you described.
I have test this patch, looks running fine.

Reviewed-by: Zhang Chen <chen.zhang@intel.com>

Thanks
Zhang Chen

> 
> Regards,
> Lukas Straub
> 
> > Thanks
> > Zhang Chen
> >
> > >
> > > Regards,
> > > Lukas Straub
> > >
> > > > Thanks
> > > > Zhang Chen
> > > >
> > > > > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > > > > ---
> > > > >  net/colo-compare.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > > > 10c0239f9d..1de4220fe2 100644
> > > > > --- a/net/colo-compare.c
> > > > > +++ b/net/colo-compare.c
> > > > > @@ -890,6 +890,7 @@ static void colo_compare_handle_event(void
> > > > > *opaque)
> > > > >
> > > > >  static void colo_compare_iothread(CompareState *s)  {
> > > > > +    AioContext *ctx = iothread_get_aio_context(s->iothread);
> > > > >      object_ref(OBJECT(s->iothread));
> > > > >      s->worker_context =
> > > > > iothread_get_g_main_context(s->iothread);
> > > > >
> > > > > @@ -906,7 +907,7 @@ static void
> > > > > colo_compare_iothread(CompareState
> > > *s)
> > > > >      }
> > > > >
> > > > >      colo_compare_timer_init(s);
> > > > > -    s->event_bh = qemu_bh_new(colo_compare_handle_event, s);
> > > > > +    s->event_bh = aio_bh_new(ctx, colo_compare_handle_event,
> > > > > + s);
> > > > >  }
> > > > >
> > > > >  static char *compare_get_pri_indev(Object *obj, Error **errp)
> > > > > --
> > > > > 2.20.1
> > > >
> >

Re: [PATCH 1/3] net/colo-compare.c: Create event_bh with the right AioContext

[Derek Su]

On 2020/4/23 下午3:29, Zhang, Chen wrote:
> 
> 
>> -----Original Message-----
>> From: Lukas Straub <lukasstraub2@web.de>
>> Sent: Wednesday, April 22, 2020 5:40 PM
>> To: Zhang, Chen <chen.zhang@intel.com>
>> Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
>> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
>> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
>> <pbonzini@redhat.com>
>> Subject: Re: [PATCH 1/3] net/colo-compare.c: Create event_bh with the right
>> AioContext
>>
>> On Wed, 22 Apr 2020 09:03:00 +0000
>> "Zhang, Chen" <chen.zhang@intel.com> wrote:
>>
>>>> -----Original Message-----
>>>> From: Lukas Straub <lukasstraub2@web.de>
>>>> Sent: Wednesday, April 22, 2020 4:43 PM
>>>> To: Zhang, Chen <chen.zhang@intel.com>
>>>> Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
>>>> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
>>>> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
>>>> <pbonzini@redhat.com>
>>>> Subject: Re: [PATCH 1/3] net/colo-compare.c: Create event_bh with
>>>> the right AioContext
>>>>
>>>> On Wed, 22 Apr 2020 08:29:39 +0000
>>>> "Zhang, Chen" <chen.zhang@intel.com> wrote:
>>>>
>>>>>> -----Original Message-----
>>>>>> From: Lukas Straub <lukasstraub2@web.de>
>>>>>> Sent: Thursday, April 9, 2020 2:34 AM
>>>>>> To: qemu-devel <qemu-devel@nongnu.org>
>>>>>> Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
>>>>>> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>;
>>>>>> Marc- André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
>>>>>> <pbonzini@redhat.com>
>>>>>> Subject: [PATCH 1/3] net/colo-compare.c: Create event_bh with
>>>>>> the right AioContext
>>>>>>
>>>>>> qemu_bh_new will set the bh to be executed in the main loop.
>>>>>> This causes problems as colo_compare_handle_event assumes that
>>>>>> it has exclusive access the queues, which are also accessed in
>>>>>> the iothread. It also assumes that it runs in a different thread
>>>>>> than the caller and takes the appropriate locks.
>>>>>>
>>>>>> Create the bh with the AioContext of the iothread to fulfill
>>>>>> these assumptions.
>>>>>>
>>>>>
>>>>> Looks good for me, I assume it will increase performance. Do you
>>>>> have
>>>> related data?
>>>>
>>>> No, this fixes several crashes because the queues where accessed
>>>> concurrently from multiple threads. Sorry for my bad wording.
>>>
>>> Can you describe some details about the crash? Step by step?
>>> Maybe I can re-produce and test it for this patch.
>>
>> There is no clear test case. For me the crashes happened after 1-20h of
>> runtime with lots of checkpoints (800ms) and some network traffic. The
>> coredump always showed that two threads where doing operations on the
>> queues simultaneously.
>> Unfortunately, I don't have the coredumps anymore.
> 
> OK, Although I have not encountered the problem you described.
> I have test this patch, looks running fine.
> 
> Reviewed-by: Zhang Chen <chen.zhang@intel.com>
> 
> Thanks
> Zhang Chen


Hi,

I encountered PVM crash caused by the race condition issue in v4.2.0.
Here is the coredump for reference.

```
warning: core file may not match specified executable file.
  Core was generated by `qemu-system-x86_64 -name source -enable-kvm 
-cpu core2duo -m 1024 -global kvm-a'.
  Program terminated with signal SIGSEGV, Segmentation fault.
  #0 0x000055cb478bcd25 in qemu_hexdump (buf=0x0, fp=0x7f6e9122b680 
<IO_2_1_stderr>, prefix=0x55cb47a388f5 "colo-compare spkt", size=1514) 
at util/hexdump.c:34
  34 fprintf(fp, " %02x", (unsigned char)buf[b + i]);
  [Current thread is 1 (Thread 0x7f6da1ade700 (LWP 6119))]
  (gdb) where
  #0 0x000055cb478bcd25 in qemu_hexdump (buf=0x0, fp=0x7f6e9122b680 
<IO_2_1_stderr>, prefix=0x55cb47a388f5 "colo-compare spkt", size=1514) 
at util/hexdump.c:34
  #1 0x000055cb476fa1b5 in colo_compare_tcp (s=0x55cb496429f0, 
conn=0x7f6e78003e30) at net/colo-compare.c:462
  #2 0x000055cb476fa8c1 in colo_compare_connection 
(opaque=0x7f6e78003e30, user_data=0x55cb496429f0) at net/colo-compare.c:687
  #3 0x000055cb476fb4ab in compare_pri_rs_finalize 
(pri_rs=0x55cb49642b18) at net/colo-compare.c:1001
  #4 0x000055cb476eb46f in net_fill_rstate (rs=0x55cb49642b18, 
buf=0x7f6da1add2c8 "", size=1064) at net/net.c:1764
  #5 0x000055cb476faafa in compare_pri_chr_in (opaque=0x55cb496429f0, 
buf=0x7f6da1adc6f0 "`E˧V\210RT", size=4096) at net/colo-compare.c:776
  #6 0x000055cb47815363 in qemu_chr_be_write_impl (s=0x55cb48c87ec0, 
buf=0x7f6da1adc6f0 "`E˧V\210RT", len=4096) at chardev/char.c:177
  #7 0x000055cb478153c7 in qemu_chr_be_write (s=0x55cb48c87ec0, 
buf=0x7f6da1adc6f0 "`E˧V\210RT", len=4096) at chardev/char.c:189
  #8 0x000055cb4781e002 in tcp_chr_read (chan=0x55cb48ef7690, 
cond=G_IO_IN, opaque=0x55cb48c87ec0) at chardev/char-socket.c:525
  #9 0x000055cb47839655 in qio_channel_fd_source_dispatch 
(source=0x7f6e78002050, callback=0x55cb4781de53 <tcp_chr_read>, 
user_data=0x55cb48c87ec0) at io/channel-watch.c:84
  #10 0x00007f6e950e1285 in g_main_context_dispatch () at 
/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
  #11 0x00007f6e950e1650 in () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
  #12 0x00007f6e950e1962 in g_main_loop_run () at 
/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
  #13 0x000055cb474920ae in iothread_run (opaque=0x55cb48c67f10) at 
iothread.c:82
  #14 0x000055cb478a699d in qemu_thread_start (args=0x55cb498035d0) at 
util/qemu-thread-posix.c:519
  #15 0x00007f6e912376db in start_thread (arg=0x7f6da1ade700) at 
pthread_create.c:463
  #16 0x00007f6e90f6088f in clone () at 
../sysdeps/unix/sysv/linux/x86_64/clone.S:95
  (gdb)
```

COLO works well after applying this patch in my tests.

Reviewed-by: Derek Su <dereksu@qnap.com>
Tested-by: Derek Su <dereksu@qnap.com>

Regards,
Derek




> 
>>
>> Regards,
>> Lukas Straub
>>
>>> Thanks
>>> Zhang Chen
>>>
>>>>
>>>> Regards,
>>>> Lukas Straub
>>>>
>>>>> Thanks
>>>>> Zhang Chen
>>>>>
>>>>>> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
>>>>>> ---
>>>>>>   net/colo-compare.c | 3 ++-
>>>>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/net/colo-compare.c b/net/colo-compare.c index
>>>>>> 10c0239f9d..1de4220fe2 100644
>>>>>> --- a/net/colo-compare.c
>>>>>> +++ b/net/colo-compare.c
>>>>>> @@ -890,6 +890,7 @@ static void colo_compare_handle_event(void
>>>>>> *opaque)
>>>>>>
>>>>>>   static void colo_compare_iothread(CompareState *s)  {
>>>>>> +    AioContext *ctx = iothread_get_aio_context(s->iothread);
>>>>>>       object_ref(OBJECT(s->iothread));
>>>>>>       s->worker_context =
>>>>>> iothread_get_g_main_context(s->iothread);
>>>>>>
>>>>>> @@ -906,7 +907,7 @@ static void
>>>>>> colo_compare_iothread(CompareState
>>>> *s)
>>>>>>       }
>>>>>>
>>>>>>       colo_compare_timer_init(s);
>>>>>> -    s->event_bh = qemu_bh_new(colo_compare_handle_event, s);
>>>>>> +    s->event_bh = aio_bh_new(ctx, colo_compare_handle_event,
>>>>>> + s);
>>>>>>   }
>>>>>>
>>>>>>   static char *compare_get_pri_indev(Object *obj, Error **errp)
>>>>>> --
>>>>>> 2.20.1
>>>>>
>>>
> 

RE: [PATCH 1/3] net/colo-compare.c: Create event_bh with the right AioContext

[Zhang, Chen]

> -----Original Message-----
> From: Derek Su <dereksu@qnap.com>
> Sent: Friday, April 24, 2020 12:37 PM
> To: Zhang, Chen <chen.zhang@intel.com>; Lukas Straub
> <lukasstraub2@web.de>
> Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>
> Subject: Re: [PATCH 1/3] net/colo-compare.c: Create event_bh with the right
> AioContext
> 
> On 2020/4/23 下午3:29, Zhang, Chen wrote:
> >
> >
> >> -----Original Message-----
> >> From: Lukas Straub <lukasstraub2@web.de>
> >> Sent: Wednesday, April 22, 2020 5:40 PM
> >> To: Zhang, Chen <chen.zhang@intel.com>
> >> Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> >> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> >> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> >> <pbonzini@redhat.com>
> >> Subject: Re: [PATCH 1/3] net/colo-compare.c: Create event_bh with the
> >> right AioContext
> >>
> >> On Wed, 22 Apr 2020 09:03:00 +0000
> >> "Zhang, Chen" <chen.zhang@intel.com> wrote:
> >>
> >>>> -----Original Message-----
> >>>> From: Lukas Straub <lukasstraub2@web.de>
> >>>> Sent: Wednesday, April 22, 2020 4:43 PM
> >>>> To: Zhang, Chen <chen.zhang@intel.com>
> >>>> Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> >>>> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>;
> Marc-
> >>>> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> >>>> <pbonzini@redhat.com>
> >>>> Subject: Re: [PATCH 1/3] net/colo-compare.c: Create event_bh with
> >>>> the right AioContext
> >>>>
> >>>> On Wed, 22 Apr 2020 08:29:39 +0000
> >>>> "Zhang, Chen" <chen.zhang@intel.com> wrote:
> >>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Lukas Straub <lukasstraub2@web.de>
> >>>>>> Sent: Thursday, April 9, 2020 2:34 AM
> >>>>>> To: qemu-devel <qemu-devel@nongnu.org>
> >>>>>> Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> >>>>>> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>;
> >>>>>> Marc- André Lureau <marcandre.lureau@redhat.com>; Paolo
> Bonzini
> >>>>>> <pbonzini@redhat.com>
> >>>>>> Subject: [PATCH 1/3] net/colo-compare.c: Create event_bh with the
> >>>>>> right AioContext
> >>>>>>
> >>>>>> qemu_bh_new will set the bh to be executed in the main loop.
> >>>>>> This causes problems as colo_compare_handle_event assumes that
> it
> >>>>>> has exclusive access the queues, which are also accessed in the
> >>>>>> iothread. It also assumes that it runs in a different thread than
> >>>>>> the caller and takes the appropriate locks.
> >>>>>>
> >>>>>> Create the bh with the AioContext of the iothread to fulfill
> >>>>>> these assumptions.
> >>>>>>
> >>>>>
> >>>>> Looks good for me, I assume it will increase performance. Do you
> >>>>> have
> >>>> related data?
> >>>>
> >>>> No, this fixes several crashes because the queues where accessed
> >>>> concurrently from multiple threads. Sorry for my bad wording.
> >>>
> >>> Can you describe some details about the crash? Step by step?
> >>> Maybe I can re-produce and test it for this patch.
> >>
> >> There is no clear test case. For me the crashes happened after 1-20h
> >> of runtime with lots of checkpoints (800ms) and some network traffic.
> >> The coredump always showed that two threads where doing operations
> on
> >> the queues simultaneously.
> >> Unfortunately, I don't have the coredumps anymore.
> >
> > OK, Although I have not encountered the problem you described.
> > I have test this patch, looks running fine.
> >
> > Reviewed-by: Zhang Chen <chen.zhang@intel.com>
> >
> > Thanks
> > Zhang Chen
> 
> 
> Hi,
> 
> I encountered PVM crash caused by the race condition issue in v4.2.0.
> Here is the coredump for reference.
> 
> ```
> warning: core file may not match specified executable file.
>   Core was generated by `qemu-system-x86_64 -name source -enable-kvm -
> cpu core2duo -m 1024 -global kvm-a'.
>   Program terminated with signal SIGSEGV, Segmentation fault.
>   #0 0x000055cb478bcd25 in qemu_hexdump (buf=0x0, fp=0x7f6e9122b680
> <IO_2_1_stderr>, prefix=0x55cb47a388f5 "colo-compare spkt", size=1514) at
> util/hexdump.c:34
>   34 fprintf(fp, " %02x", (unsigned char)buf[b + i]);
>   [Current thread is 1 (Thread 0x7f6da1ade700 (LWP 6119))]
>   (gdb) where
>   #0 0x000055cb478bcd25 in qemu_hexdump (buf=0x0, fp=0x7f6e9122b680
> <IO_2_1_stderr>, prefix=0x55cb47a388f5 "colo-compare spkt", size=1514) at
> util/hexdump.c:34
>   #1 0x000055cb476fa1b5 in colo_compare_tcp (s=0x55cb496429f0,
> conn=0x7f6e78003e30) at net/colo-compare.c:462
>   #2 0x000055cb476fa8c1 in colo_compare_connection
> (opaque=0x7f6e78003e30, user_data=0x55cb496429f0) at net/colo-
> compare.c:687
>   #3 0x000055cb476fb4ab in compare_pri_rs_finalize
> (pri_rs=0x55cb49642b18) at net/colo-compare.c:1001
>   #4 0x000055cb476eb46f in net_fill_rstate (rs=0x55cb49642b18,
> buf=0x7f6da1add2c8 "", size=1064) at net/net.c:1764
>   #5 0x000055cb476faafa in compare_pri_chr_in (opaque=0x55cb496429f0,
> buf=0x7f6da1adc6f0 "`E˧V\210RT", size=4096) at net/colo-compare.c:776
>   #6 0x000055cb47815363 in qemu_chr_be_write_impl (s=0x55cb48c87ec0,
> buf=0x7f6da1adc6f0 "`E˧V\210RT", len=4096) at chardev/char.c:177
>   #7 0x000055cb478153c7 in qemu_chr_be_write (s=0x55cb48c87ec0,
> buf=0x7f6da1adc6f0 "`E˧V\210RT", len=4096) at chardev/char.c:189
>   #8 0x000055cb4781e002 in tcp_chr_read (chan=0x55cb48ef7690,
> cond=G_IO_IN, opaque=0x55cb48c87ec0) at chardev/char-socket.c:525
>   #9 0x000055cb47839655 in qio_channel_fd_source_dispatch
> (source=0x7f6e78002050, callback=0x55cb4781de53 <tcp_chr_read>,
> user_data=0x55cb48c87ec0) at io/channel-watch.c:84
>   #10 0x00007f6e950e1285 in g_main_context_dispatch () at
> /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
>   #11 0x00007f6e950e1650 in () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
>   #12 0x00007f6e950e1962 in g_main_loop_run () at
> /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
>   #13 0x000055cb474920ae in iothread_run (opaque=0x55cb48c67f10) at
> iothread.c:82
>   #14 0x000055cb478a699d in qemu_thread_start (args=0x55cb498035d0) at
> util/qemu-thread-posix.c:519
>   #15 0x00007f6e912376db in start_thread (arg=0x7f6da1ade700) at
> pthread_create.c:463
>   #16 0x00007f6e90f6088f in clone () at
> ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
>   (gdb)
> ```
> 
> COLO works well after applying this patch in my tests.
> 
> Reviewed-by: Derek Su <dereksu@qnap.com>
> Tested-by: Derek Su <dereksu@qnap.com>
> 

Thanks Derek.

> Regards,
> Derek
> 
> 
> 
> 
> >
> >>
> >> Regards,
> >> Lukas Straub
> >>
> >>> Thanks
> >>> Zhang Chen
> >>>
> >>>>
> >>>> Regards,
> >>>> Lukas Straub
> >>>>
> >>>>> Thanks
> >>>>> Zhang Chen
> >>>>>
> >>>>>> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> >>>>>> ---
> >>>>>>   net/colo-compare.c | 3 ++-
> >>>>>>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> >>>>>> 10c0239f9d..1de4220fe2 100644
> >>>>>> --- a/net/colo-compare.c
> >>>>>> +++ b/net/colo-compare.c
> >>>>>> @@ -890,6 +890,7 @@ static void colo_compare_handle_event(void
> >>>>>> *opaque)
> >>>>>>
> >>>>>>   static void colo_compare_iothread(CompareState *s)  {
> >>>>>> +    AioContext *ctx = iothread_get_aio_context(s->iothread);
> >>>>>>       object_ref(OBJECT(s->iothread));
> >>>>>>       s->worker_context =
> >>>>>> iothread_get_g_main_context(s->iothread);
> >>>>>>
> >>>>>> @@ -906,7 +907,7 @@ static void
> >>>>>> colo_compare_iothread(CompareState
> >>>> *s)
> >>>>>>       }
> >>>>>>
> >>>>>>       colo_compare_timer_init(s);
> >>>>>> -    s->event_bh = qemu_bh_new(colo_compare_handle_event, s);
> >>>>>> +    s->event_bh = aio_bh_new(ctx, colo_compare_handle_event, s);
> >>>>>>   }
> >>>>>>
> >>>>>>   static char *compare_get_pri_indev(Object *obj, Error **errp)
> >>>>>> --
> >>>>>> 2.20.1
> >>>>>
> >>>
> >

[PATCH 2/3] chardev/char.c: Use qemu_co_sleep_ns if in coroutine

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 933 bytes --]

This will be needed in the next patch.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 chardev/char.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/chardev/char.c b/chardev/char.c
index 04075389bf..51ad0dc6b3 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -38,6 +38,7 @@
 #include "qemu/module.h"
 #include "qemu/option.h"
 #include "qemu/id.h"
+#include "qemu/coroutine.h"
 
 #include "chardev/char-mux.h"
 
@@ -119,7 +120,11 @@ static int qemu_chr_write_buffer(Chardev *s,
     retry:
         res = cc->chr_write(s, buf + *offset, len - *offset);
         if (res < 0 && errno == EAGAIN && write_all) {
-            g_usleep(100);
+            if (qemu_in_coroutine()) {
+                qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 100000);
+            } else {
+                g_usleep(100);
+            }
             goto retry;
         }
 
-- 
2.20.1


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/3] chardev/char.c: Use qemu_co_sleep_ns if in coroutine

[Marc-André Lureau]

Hi

On Wed, Apr 8, 2020 at 8:37 PM Lukas Straub <lukasstraub2@web.de> wrote:
>
> This will be needed in the next patch.
>
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>  chardev/char.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index 04075389bf..51ad0dc6b3 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -38,6 +38,7 @@
>  #include "qemu/module.h"
>  #include "qemu/option.h"
>  #include "qemu/id.h"
> +#include "qemu/coroutine.h"
>
>  #include "chardev/char-mux.h"
>
> @@ -119,7 +120,11 @@ static int qemu_chr_write_buffer(Chardev *s,
>      retry:
>          res = cc->chr_write(s, buf + *offset, len - *offset);
>          if (res < 0 && errno == EAGAIN && write_all) {
> -            g_usleep(100);
> +            if (qemu_in_coroutine()) {
> +                qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 100000);
> +            } else {
> +                g_usleep(100);
> +            }
>              goto retry;

Although we don't have any coroutine code in chardev/, it dos this
kind of coroutine handling indirectly through qio. Thus it should be
fine too here.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


>          }
>
> --
> 2.20.1
>


-- 
Marc-André Lureau

RE: [PATCH 2/3] chardev/char.c: Use qemu_co_sleep_ns if in coroutine

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Thursday, April 9, 2020 2:34 AM
> To: qemu-devel <qemu-devel@nongnu.org>
> Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>
> Subject: [PATCH 2/3] chardev/char.c: Use qemu_co_sleep_ns if in coroutine
> 
> This will be needed in the next patch.
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>


Reviewed-by: Zhang Chen <chen.zhang@intel.com>

> ---
>  chardev/char.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/chardev/char.c b/chardev/char.c index 04075389bf..51ad0dc6b3
> 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -38,6 +38,7 @@
>  #include "qemu/module.h"
>  #include "qemu/option.h"
>  #include "qemu/id.h"
> +#include "qemu/coroutine.h"
> 
>  #include "chardev/char-mux.h"
> 
> @@ -119,7 +120,11 @@ static int qemu_chr_write_buffer(Chardev *s,
>      retry:
>          res = cc->chr_write(s, buf + *offset, len - *offset);
>          if (res < 0 && errno == EAGAIN && write_all) {
> -            g_usleep(100);
> +            if (qemu_in_coroutine()) {
> +                qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 100000);
> +            } else {
> +                g_usleep(100);
> +            }
>              goto retry;
>          }
> 
> --
> 2.20.1

[PATCH 3/3] net/colo-compare.c: Fix deadlock

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 4980 bytes --]

The chr_out chardev is connected to a filter-redirector
running in the main loop. qemu_chr_fe_write_all might block
here in compare_chr_send if the (socket-)buffer is full.
If another filter-redirector in the main loop want's to
send data to chr_pri_in it might also block if the buffer
is full. This leads to a deadlock because both event loops
get blocked.

Fix this by converting compare_chr_send to a coroutine
and return error if it is in use.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 net/colo-compare.c | 82 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 71 insertions(+), 11 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 1de4220fe2..82787d3055 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -32,6 +32,9 @@
 #include "migration/migration.h"
 #include "util.h"
 
+#include "block/aio-wait.h"
+#include "qemu/coroutine.h"
+
 #define TYPE_COLO_COMPARE "colo-compare"
 #define COLO_COMPARE(obj) \
     OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE)
@@ -77,6 +80,17 @@ static int event_unhandled_count;
  *                    |packet  |  |packet  +    |packet  | |packet  +
  *                    +--------+  +--------+    +--------+ +--------+
  */
+
+typedef struct SendCo {
+    Coroutine *co;
+    uint8_t *buf;
+    uint32_t size;
+    uint32_t vnet_hdr_len;
+    bool notify_remote_frame;
+    bool done;
+    int ret;
+} SendCo;
+
 typedef struct CompareState {
     Object parent;
 
@@ -91,6 +105,7 @@ typedef struct CompareState {
     SocketReadState pri_rs;
     SocketReadState sec_rs;
     SocketReadState notify_rs;
+    SendCo sendco;
     bool vnet_hdr;
     uint32_t compare_timeout;
     uint32_t expired_scan_cycle;
@@ -699,19 +714,17 @@ static void colo_compare_connection(void *opaque, void *user_data)
     }
 }
 
-static int compare_chr_send(CompareState *s,
-                            const uint8_t *buf,
-                            uint32_t size,
-                            uint32_t vnet_hdr_len,
-                            bool notify_remote_frame)
+static void coroutine_fn _compare_chr_send(void *opaque)
 {
+    CompareState *s = opaque;
+    SendCo *sendco = &s->sendco;
+    const uint8_t *buf = sendco->buf;
+    uint32_t size = sendco->size;
+    uint32_t vnet_hdr_len = sendco->vnet_hdr_len;
+    bool notify_remote_frame = sendco->notify_remote_frame;
     int ret = 0;
     uint32_t len = htonl(size);
 
-    if (!size) {
-        return 0;
-    }
-
     if (notify_remote_frame) {
         ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
                                     (uint8_t *)&len,
@@ -754,10 +767,50 @@ static int compare_chr_send(CompareState *s,
         goto err;
     }
 
-    return 0;
+    sendco->ret = 0;
+    goto out;
 
 err:
-    return ret < 0 ? ret : -EIO;
+    sendco->ret = ret < 0 ? ret : -EIO;
+out:
+    sendco->co = NULL;
+    g_free(sendco->buf);
+    sendco->buf = NULL;
+    sendco->done = true;
+    aio_wait_kick();
+}
+
+static int compare_chr_send(CompareState *s,
+                            const uint8_t *buf,
+                            uint32_t size,
+                            uint32_t vnet_hdr_len,
+                            bool notify_remote_frame)
+{
+    SendCo *sendco = &s->sendco;
+
+    if (!size) {
+        return 0;
+    }
+
+    if (sendco->done) {
+        sendco->co = qemu_coroutine_create(_compare_chr_send, s);
+        sendco->buf = g_malloc(size);
+        sendco->size = size;
+        sendco->vnet_hdr_len = vnet_hdr_len;
+        sendco->notify_remote_frame = notify_remote_frame;
+        sendco->done = false;
+        memcpy(sendco->buf, buf, size);
+        qemu_coroutine_enter(sendco->co);
+        if (sendco->done) {
+            /* report early errors */
+            return sendco->ret;
+        } else {
+            /* else assume success */
+            return 0;
+        }
+    }
+
+    return -ENOBUFS;
 }
 
 static int compare_chr_can_read(void *opaque)
@@ -1146,6 +1199,8 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
     CompareState *s = COLO_COMPARE(uc);
     Chardev *chr;
 
+    s->sendco.done = true;
+
     if (!s->pri_indev || !s->sec_indev || !s->outdev || !s->iothread) {
         error_setg(errp, "colo compare needs 'primary_in' ,"
                    "'secondary_in','outdev','iothread' property set");
@@ -1281,6 +1336,11 @@ static void colo_compare_finalize(Object *obj)
     CompareState *s = COLO_COMPARE(obj);
     CompareState *tmp = NULL;
 
+    AioContext *ctx = iothread_get_aio_context(s->iothread);
+    aio_context_acquire(ctx);
+    AIO_WAIT_WHILE(ctx, !s->sendco.done);
+    aio_context_release(ctx);
+
     qemu_chr_fe_deinit(&s->chr_pri_in, false);
     qemu_chr_fe_deinit(&s->chr_sec_in, false);
     qemu_chr_fe_deinit(&s->chr_out, false);
-- 
2.20.1

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 3/3] net/colo-compare.c: Fix deadlock

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Thursday, April 9, 2020 2:34 AM
> To: qemu-devel <qemu-devel@nongnu.org>
> Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>
> Subject: [PATCH 3/3] net/colo-compare.c: Fix deadlock
> 
> The chr_out chardev is connected to a filter-redirector running in the main
> loop. qemu_chr_fe_write_all might block here in compare_chr_send if the
> (socket-)buffer is full.
> If another filter-redirector in the main loop want's to send data to chr_pri_in
> it might also block if the buffer is full. This leads to a deadlock because both
> event loops get blocked.
> 
> Fix this by converting compare_chr_send to a coroutine and return error if it
> is in use.
> 

I have tested this series, running fine currently.
Can you share performance data after this patch?

Thanks
Zhang Chen

> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>  net/colo-compare.c | 82
> +++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 71 insertions(+), 11 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> 1de4220fe2..82787d3055 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -32,6 +32,9 @@
>  #include "migration/migration.h"
>  #include "util.h"
> 
> +#include "block/aio-wait.h"
> +#include "qemu/coroutine.h"
> +
>  #define TYPE_COLO_COMPARE "colo-compare"
>  #define COLO_COMPARE(obj) \
>      OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE) @@ -77,6
> +80,17 @@ static int event_unhandled_count;
>   *                    |packet  |  |packet  +    |packet  | |packet  +
>   *                    +--------+  +--------+    +--------+ +--------+
>   */
> +
> +typedef struct SendCo {
> +    Coroutine *co;
> +    uint8_t *buf;
> +    uint32_t size;
> +    uint32_t vnet_hdr_len;
> +    bool notify_remote_frame;
> +    bool done;
> +    int ret;
> +} SendCo;
> +
>  typedef struct CompareState {
>      Object parent;
> 
> @@ -91,6 +105,7 @@ typedef struct CompareState {
>      SocketReadState pri_rs;
>      SocketReadState sec_rs;
>      SocketReadState notify_rs;
> +    SendCo sendco;
>      bool vnet_hdr;
>      uint32_t compare_timeout;
>      uint32_t expired_scan_cycle;
> @@ -699,19 +714,17 @@ static void colo_compare_connection(void
> *opaque, void *user_data)
>      }
>  }
> 
> -static int compare_chr_send(CompareState *s,
> -                            const uint8_t *buf,
> -                            uint32_t size,
> -                            uint32_t vnet_hdr_len,
> -                            bool notify_remote_frame)
> +static void coroutine_fn _compare_chr_send(void *opaque)
>  {
> +    CompareState *s = opaque;
> +    SendCo *sendco = &s->sendco;
> +    const uint8_t *buf = sendco->buf;
> +    uint32_t size = sendco->size;
> +    uint32_t vnet_hdr_len = sendco->vnet_hdr_len;
> +    bool notify_remote_frame = sendco->notify_remote_frame;
>      int ret = 0;
>      uint32_t len = htonl(size);
> 
> -    if (!size) {
> -        return 0;
> -    }
> -
>      if (notify_remote_frame) {
>          ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
>                                      (uint8_t *)&len, @@ -754,10 +767,50 @@ static int
> compare_chr_send(CompareState *s,
>          goto err;
>      }
> 
> -    return 0;
> +    sendco->ret = 0;
> +    goto out;
> 
>  err:
> -    return ret < 0 ? ret : -EIO;
> +    sendco->ret = ret < 0 ? ret : -EIO;
> +out:
> +    sendco->co = NULL;
> +    g_free(sendco->buf);
> +    sendco->buf = NULL;
> +    sendco->done = true;
> +    aio_wait_kick();
> +}
> +
> +static int compare_chr_send(CompareState *s,
> +                            const uint8_t *buf,
> +                            uint32_t size,
> +                            uint32_t vnet_hdr_len,
> +                            bool notify_remote_frame) {
> +    SendCo *sendco = &s->sendco;
> +
> +    if (!size) {
> +        return 0;
> +    }
> +
> +    if (sendco->done) {
> +        sendco->co = qemu_coroutine_create(_compare_chr_send, s);
> +        sendco->buf = g_malloc(size);
> +        sendco->size = size;
> +        sendco->vnet_hdr_len = vnet_hdr_len;
> +        sendco->notify_remote_frame = notify_remote_frame;
> +        sendco->done = false;
> +        memcpy(sendco->buf, buf, size);
> +        qemu_coroutine_enter(sendco->co);
> +        if (sendco->done) {
> +            /* report early errors */
> +            return sendco->ret;
> +        } else {
> +            /* else assume success */
> +            return 0;
> +        }
> +    }
> +
> +    return -ENOBUFS;
>  }
> 
>  static int compare_chr_can_read(void *opaque) @@ -1146,6 +1199,8 @@
> static void colo_compare_complete(UserCreatable *uc, Error **errp)
>      CompareState *s = COLO_COMPARE(uc);
>      Chardev *chr;
> 
> +    s->sendco.done = true;
> +
>      if (!s->pri_indev || !s->sec_indev || !s->outdev || !s->iothread) {
>          error_setg(errp, "colo compare needs 'primary_in' ,"
>                     "'secondary_in','outdev','iothread' property set"); @@ -1281,6
> +1336,11 @@ static void colo_compare_finalize(Object *obj)
>      CompareState *s = COLO_COMPARE(obj);
>      CompareState *tmp = NULL;
> 
> +    AioContext *ctx = iothread_get_aio_context(s->iothread);
> +    aio_context_acquire(ctx);
> +    AIO_WAIT_WHILE(ctx, !s->sendco.done);
> +    aio_context_release(ctx);
> +
>      qemu_chr_fe_deinit(&s->chr_pri_in, false);
>      qemu_chr_fe_deinit(&s->chr_sec_in, false);
>      qemu_chr_fe_deinit(&s->chr_out, false);
> --
> 2.20.1

Re: [PATCH 3/3] net/colo-compare.c: Fix deadlock

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 6731 bytes --]

On Wed, 22 Apr 2020 08:40:40 +0000
"Zhang, Chen" <chen.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: Lukas Straub <lukasstraub2@web.de>
> > Sent: Thursday, April 9, 2020 2:34 AM
> > To: qemu-devel <qemu-devel@nongnu.org>
> > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > <pbonzini@redhat.com>
> > Subject: [PATCH 3/3] net/colo-compare.c: Fix deadlock
> > 
> > The chr_out chardev is connected to a filter-redirector running in the main
> > loop. qemu_chr_fe_write_all might block here in compare_chr_send if the
> > (socket-)buffer is full.
> > If another filter-redirector in the main loop want's to send data to chr_pri_in
> > it might also block if the buffer is full. This leads to a deadlock because both
> > event loops get blocked.
> > 
> > Fix this by converting compare_chr_send to a coroutine and return error if it
> > is in use.
> >   
> 
> I have tested this series, running fine currently.
> Can you share performance data after this patch?
> 
> Thanks
> Zhang Chen

Hello,
Here are the results (using iperf3):
Client-to-server tcp:
without patch: ~64.2 Mbit/s
with patch: ~28.9 Mbit/s
Server-to-client tcp:
without patch: 360 Kbit/s (when it doesn't deadlock :)
with patch: 220 Kbit/s

Yeah, it hurts performance somewhat, but the deadlock happens often with lots
of server-to-client traffic. (It deadlocked in 2 of 4 runs)

Do you have a better idea to solve this issue?

Regards,
Lukas Straub

> > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > ---
> >  net/colo-compare.c | 82
> > +++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 71 insertions(+), 11 deletions(-)
> > 
> > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > 1de4220fe2..82787d3055 100644
> > --- a/net/colo-compare.c
> > +++ b/net/colo-compare.c
> > @@ -32,6 +32,9 @@
> >  #include "migration/migration.h"
> >  #include "util.h"
> > 
> > +#include "block/aio-wait.h"
> > +#include "qemu/coroutine.h"
> > +
> >  #define TYPE_COLO_COMPARE "colo-compare"
> >  #define COLO_COMPARE(obj) \
> >      OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE) @@ -77,6
> > +80,17 @@ static int event_unhandled_count;
> >   *                    |packet  |  |packet  +    |packet  | |packet  +
> >   *                    +--------+  +--------+    +--------+ +--------+
> >   */
> > +
> > +typedef struct SendCo {
> > +    Coroutine *co;
> > +    uint8_t *buf;
> > +    uint32_t size;
> > +    uint32_t vnet_hdr_len;
> > +    bool notify_remote_frame;
> > +    bool done;
> > +    int ret;
> > +} SendCo;
> > +
> >  typedef struct CompareState {
> >      Object parent;
> > 
> > @@ -91,6 +105,7 @@ typedef struct CompareState {
> >      SocketReadState pri_rs;
> >      SocketReadState sec_rs;
> >      SocketReadState notify_rs;
> > +    SendCo sendco;
> >      bool vnet_hdr;
> >      uint32_t compare_timeout;
> >      uint32_t expired_scan_cycle;
> > @@ -699,19 +714,17 @@ static void colo_compare_connection(void
> > *opaque, void *user_data)
> >      }
> >  }
> > 
> > -static int compare_chr_send(CompareState *s,
> > -                            const uint8_t *buf,
> > -                            uint32_t size,
> > -                            uint32_t vnet_hdr_len,
> > -                            bool notify_remote_frame)
> > +static void coroutine_fn _compare_chr_send(void *opaque)
> >  {
> > +    CompareState *s = opaque;
> > +    SendCo *sendco = &s->sendco;
> > +    const uint8_t *buf = sendco->buf;
> > +    uint32_t size = sendco->size;
> > +    uint32_t vnet_hdr_len = sendco->vnet_hdr_len;
> > +    bool notify_remote_frame = sendco->notify_remote_frame;
> >      int ret = 0;
> >      uint32_t len = htonl(size);
> > 
> > -    if (!size) {
> > -        return 0;
> > -    }
> > -
> >      if (notify_remote_frame) {
> >          ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
> >                                      (uint8_t *)&len, @@ -754,10 +767,50 @@ static int
> > compare_chr_send(CompareState *s,
> >          goto err;
> >      }
> > 
> > -    return 0;
> > +    sendco->ret = 0;
> > +    goto out;
> > 
> >  err:
> > -    return ret < 0 ? ret : -EIO;
> > +    sendco->ret = ret < 0 ? ret : -EIO;
> > +out:
> > +    sendco->co = NULL;
> > +    g_free(sendco->buf);
> > +    sendco->buf = NULL;
> > +    sendco->done = true;
> > +    aio_wait_kick();
> > +}
> > +
> > +static int compare_chr_send(CompareState *s,
> > +                            const uint8_t *buf,
> > +                            uint32_t size,
> > +                            uint32_t vnet_hdr_len,
> > +                            bool notify_remote_frame) {
> > +    SendCo *sendco = &s->sendco;
> > +
> > +    if (!size) {
> > +        return 0;
> > +    }
> > +
> > +    if (sendco->done) {
> > +        sendco->co = qemu_coroutine_create(_compare_chr_send, s);
> > +        sendco->buf = g_malloc(size);
> > +        sendco->size = size;
> > +        sendco->vnet_hdr_len = vnet_hdr_len;
> > +        sendco->notify_remote_frame = notify_remote_frame;
> > +        sendco->done = false;
> > +        memcpy(sendco->buf, buf, size);
> > +        qemu_coroutine_enter(sendco->co);
> > +        if (sendco->done) {
> > +            /* report early errors */
> > +            return sendco->ret;
> > +        } else {
> > +            /* else assume success */
> > +            return 0;
> > +        }
> > +    }
> > +
> > +    return -ENOBUFS;
> >  }
> > 
> >  static int compare_chr_can_read(void *opaque) @@ -1146,6 +1199,8 @@
> > static void colo_compare_complete(UserCreatable *uc, Error **errp)
> >      CompareState *s = COLO_COMPARE(uc);
> >      Chardev *chr;
> > 
> > +    s->sendco.done = true;
> > +
> >      if (!s->pri_indev || !s->sec_indev || !s->outdev || !s->iothread) {
> >          error_setg(errp, "colo compare needs 'primary_in' ,"
> >                     "'secondary_in','outdev','iothread' property set"); @@ -1281,6
> > +1336,11 @@ static void colo_compare_finalize(Object *obj)
> >      CompareState *s = COLO_COMPARE(obj);
> >      CompareState *tmp = NULL;
> > 
> > +    AioContext *ctx = iothread_get_aio_context(s->iothread);
> > +    aio_context_acquire(ctx);
> > +    AIO_WAIT_WHILE(ctx, !s->sendco.done);
> > +    aio_context_release(ctx);
> > +
> >      qemu_chr_fe_deinit(&s->chr_pri_in, false);
> >      qemu_chr_fe_deinit(&s->chr_sec_in, false);
> >      qemu_chr_fe_deinit(&s->chr_out, false);
> > --
> > 2.20.1  


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]