Re: [Qemu-devel] [PATCH v2 3/6] terminal3270: do not use backend timer sources

[Peter Xu <peterx@redhat.com>]

On Mon, Feb 11, 2019 at 10:57:01AM +0100, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Feb 11, 2019 at 8:13 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Thu, Feb 07, 2019 at 06:14:15PM +0100, Cornelia Huck wrote:
> > > On Wed,  6 Feb 2019 18:43:25 +0100
> > > Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
> > >
> > > > terminal3270 uses the front-end side of the chardev. It shouldn't
> > > > create sources from backend side context (with backend
> > > > functions).
> > > >
> > > > send_timing_mark_cb calls qemu_chr_fe_write_all() which should be
> > > > thread safe.
> > > >
> > > > This partially reverts changes from commit
> > > > 2c716ba1506769c9be2caa02f0f6d6e7c00f4304.
> > > >
> > > > CC: Peter Xu <peterx@redhat.com>
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > ---
> > > >  hw/char/terminal3270.c | 15 ++++++---------
> > > >  1 file changed, 6 insertions(+), 9 deletions(-)
> > >
> > > I've verified that 3270 continues to work as before.
> > >
> > > Acked-by: Cornelia Huck <cohuck@redhat.com>
> >
> > A pure question: is it broken before this patch?
> 
> No
> 
> >
> > Asked since I don't understand this patch and what it tries to fix...
> 
> A front-end shouldn't use backend functions.
> 
> > E.g., in terminal_init(), qemu_chr_fe_set_handlers() is always passing
> > NULL as chardev context so AFAICT this patch changes nothing from
> > functional-wise for now.  Meanwhile, if we pass in some non-NULL into
> > it in the future, IMHO this patch would break it instead of fixing
> > anything... anything I've missed?
> >
> 
> If the frontend switches the context and creates timers with this
> context, it should do it without using the backend functions.

I see your point (assuming that concurrent writes are safe).  But IMHO
we should first discuss on how to fix the existing multi-threading
issues (since it's not really safe yet).  After all this patch offers
nothing real for us for now, and if one day we want to run the chardev
in a single thread again then this will just break again unnoticed.

Frankly speaking I don't think making the chardev to be completely
multi-threading is very attractive to me - chardevs are mostly slow,
otherwise imho better techniques should be used (e.g., virtio).  OOB
did that majorly for only isolating chardev IOs out of main thread so
that chardevs won't be blocked by other unpredictable behaviors (like
userfaults), but it still wants to keep the old code running with as
less change as possible.  For this we can discuss in the other
thread too for sure...

Thanks,

-- 
Peter Xu