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

["Zhang, Chen" <chen.zhang@intel.com>]

> -----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
> > > >
> >