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

[Marc-André Lureau]

Hi

On Mon, Feb 11, 2019 at 11:27 AM Peter Xu <address@hidden> wrote:
>
> 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 <address@hidden> 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 <address@hidden> 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 <address@hidden>
> > > > > Signed-off-by: Marc-André Lureau <address@hidden>
> > > > > ---
> > > > >  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 <address@hidden>
> > >
> > > 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

Yes, various people are looking at this problem. We need to fix it.

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

I assume you are not speaking about that patch in particular, when you
say it will "break again unnoticed".

This patch helps when you do whole-tree code review or change over a
particular aspect of the backend code. In my case, I wrote this patch
because I did a review of all the backend context users. And this one
is a "bad" user, I think you agree by now. So let's apply this change.
And let's focus on multi-thread bugs in respective threads.

thanks


-- 
Marc-André Lureau