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(-)
diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
index e9c45e55b1..35b079d5c4 100644
--- a/hw/char/terminal3270.c
+++ b/hw/char/terminal3270.c
@@ -31,7 +31,7 @@ typedef struct Terminal3270 {
uint8_t outv[OUTPUT_BUFFER_SIZE];
int in_len;
bool handshake_done;
- GSource *timer_src;
+ guint timer_tag;
} Terminal3270;
#define TYPE_TERMINAL_3270 "x-terminal3270"
@@ -47,10 +47,9 @@ static int terminal_can_read(void *opaque)
static void terminal_timer_cancel(Terminal3270 *t)
{
- if (t->timer_src) {
- g_source_destroy(t->timer_src);
- g_source_unref(t->timer_src);
- t->timer_src = NULL;
+ if (t->timer_tag) {
+ g_source_remove(t->timer_tag);
+ t->timer_tag = 0;
}
}
@@ -100,8 +99,7 @@ static void terminal_read(void *opaque, const uint8_t *buf, int size)
assert(size <= (INPUT_BUFFER_SIZE - t->in_len));
terminal_timer_cancel(t);
- t->timer_src = qemu_chr_timeout_add_ms(t->chr.chr, 600 * 1000,
- send_timing_mark_cb, t);
+ t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
memcpy(&t->inv[t->in_len], buf, size);
t->in_len += size;
if (t->in_len < 2) {
@@ -160,8 +158,7 @@ static void chr_event(void *opaque, int event)
* char-socket.c. Once qemu receives the terminal-type of the
* client, mark handshake done and trigger everything rolling again.
*/
- t->timer_src = qemu_chr_timeout_add_ms(t->chr.chr, 600 * 1000,
- send_timing_mark_cb, t);
+ t->timer_tag = g_timeout_add_seconds(600, send_timing_mark_cb, t);
break;
case CHR_EVENT_CLOSED:
sch->curr_status.scsw.dstat = SCSW_DSTAT_DEVICE_END;
--
2.20.1.519.g8feddda32c
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>
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? Asked since I don't understand this patch and what it tries to fix... 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? Thanks, -- Peter Xu
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. thanks -- Marc-André Lureau
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
Hi On Mon, Feb 11, 2019 at 11:27 AM Peter Xu <peterx@redhat.com> 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 <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 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
© 2016 - 2025 Red Hat, Inc.