* Re: [Qemu-devel] [PATCH] chardev: Use timer instead of bottom-half to postpone open event
2012-10-08 17:07 [Qemu-devel] [PATCH] chardev: Use timer instead of bottom-half to postpone open event Jan Kiszka
@ 2012-10-08 18:07 ` Aurelien Jarno
2012-10-08 20:12 ` Hans de Goede
2012-10-12 7:52 ` [Qemu-devel] [PATCH v2] " Jan Kiszka
2 siblings, 0 replies; 7+ messages in thread
From: Aurelien Jarno @ 2012-10-08 18:07 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Peter Maydell, Anthony Liguori, qemu-devel, Paolo Bonzini
On Mon, Oct 08, 2012 at 07:07:08PM +0200, Jan Kiszka wrote:
> As the block layer may decide to flush bottom-halfs while the machine is
> still initializing (e.g. to read geometry data from the disk), our
> postponed open event may be processed before the last frontend
> registered with a muxed chardev. Until the semantics of BHs have been
> clarified, use an expired timer to achieve the same effect (suggested
> by Paolo Bonzini).
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>
> This obsoletes the versatilepb hack.
>
> qemu-char.c | 13 +++++++------
> qemu-char.h | 2 +-
> 2 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index f9ee2f0..fb4e3dc 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -123,19 +123,20 @@ void qemu_chr_be_event(CharDriverState *s, int event)
> s->chr_event(s->handler_opaque, event);
> }
>
> -static void qemu_chr_generic_open_bh(void *opaque)
> +static void qemu_chr_fire_open_event(void *opaque)
> {
> CharDriverState *s = opaque;
> qemu_chr_be_event(s, CHR_EVENT_OPENED);
> - qemu_bh_delete(s->bh);
> - s->bh = NULL;
> + qemu_free_timer(s->open_timer);
> + s->open_timer = NULL;
> }
>
> void qemu_chr_generic_open(CharDriverState *s)
> {
> - if (s->bh == NULL) {
> - s->bh = qemu_bh_new(qemu_chr_generic_open_bh, s);
> - qemu_bh_schedule(s->bh);
> + if (s->open_timer == NULL) {
> + s->open_timer = qemu_new_timer_ms(vm_clock,
> + qemu_chr_fire_open_event, s);
> + qemu_mod_timer(s->open_timer, qemu_get_clock_ms(vm_clock) - 1);
> }
> }
>
> diff --git a/qemu-char.h b/qemu-char.h
> index 486644b..297dd98 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -69,7 +69,7 @@ struct CharDriverState {
> void (*chr_guest_open)(struct CharDriverState *chr);
> void (*chr_guest_close)(struct CharDriverState *chr);
> void *opaque;
> - QEMUBH *bh;
> + QEMUTimer *open_timer;
> char *label;
> char *filename;
> int opened;
It works for me on both MIPS malta and ARM versatile. Thanks.
Tested-by: Aurelien Jarno <aurelien@aurel32.net>
--
Aurelien Jarno GPG: 1024D/F1BCDB73
aurelien@aurel32.net http://www.aurel32.net
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] chardev: Use timer instead of bottom-half to postpone open event
2012-10-08 17:07 [Qemu-devel] [PATCH] chardev: Use timer instead of bottom-half to postpone open event Jan Kiszka
2012-10-08 18:07 ` Aurelien Jarno
@ 2012-10-08 20:12 ` Hans de Goede
2012-10-08 20:14 ` Hans de Goede
2012-10-12 7:52 ` [Qemu-devel] [PATCH v2] " Jan Kiszka
2 siblings, 1 reply; 7+ messages in thread
From: Hans de Goede @ 2012-10-08 20:12 UTC (permalink / raw)
To: Jan Kiszka; +Cc: qemu-devel
Hi,
Funny that you ping about this just now, as I wanted to work on
this today, but sofar I did not get around to it. Maybe tonight I will :)
Long story short: Yes I've some remarks, but I pan to fix those myself and
then merge it in my tree.
I'll get back to you when this is done.
Regards,
Hans
On 10/08/2012 07:07 PM, Jan Kiszka wrote:
> As the block layer may decide to flush bottom-halfs while the machine is
> still initializing (e.g. to read geometry data from the disk), our
> postponed open event may be processed before the last frontend
> registered with a muxed chardev. Until the semantics of BHs have been
> clarified, use an expired timer to achieve the same effect (suggested
> by Paolo Bonzini).
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>
> This obsoletes the versatilepb hack.
>
> qemu-char.c | 13 +++++++------
> qemu-char.h | 2 +-
> 2 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index f9ee2f0..fb4e3dc 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -123,19 +123,20 @@ void qemu_chr_be_event(CharDriverState *s, int event)
> s->chr_event(s->handler_opaque, event);
> }
>
> -static void qemu_chr_generic_open_bh(void *opaque)
> +static void qemu_chr_fire_open_event(void *opaque)
> {
> CharDriverState *s = opaque;
> qemu_chr_be_event(s, CHR_EVENT_OPENED);
> - qemu_bh_delete(s->bh);
> - s->bh = NULL;
> + qemu_free_timer(s->open_timer);
> + s->open_timer = NULL;
> }
>
> void qemu_chr_generic_open(CharDriverState *s)
> {
> - if (s->bh == NULL) {
> - s->bh = qemu_bh_new(qemu_chr_generic_open_bh, s);
> - qemu_bh_schedule(s->bh);
> + if (s->open_timer == NULL) {
> + s->open_timer = qemu_new_timer_ms(vm_clock,
> + qemu_chr_fire_open_event, s);
> + qemu_mod_timer(s->open_timer, qemu_get_clock_ms(vm_clock) - 1);
> }
> }
>
> diff --git a/qemu-char.h b/qemu-char.h
> index 486644b..297dd98 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -69,7 +69,7 @@ struct CharDriverState {
> void (*chr_guest_open)(struct CharDriverState *chr);
> void (*chr_guest_close)(struct CharDriverState *chr);
> void *opaque;
> - QEMUBH *bh;
> + QEMUTimer *open_timer;
> char *label;
> char *filename;
> int opened;
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH] chardev: Use timer instead of bottom-half to postpone open event
2012-10-08 20:12 ` Hans de Goede
@ 2012-10-08 20:14 ` Hans de Goede
0 siblings, 0 replies; 7+ messages in thread
From: Hans de Goede @ 2012-10-08 20:14 UTC (permalink / raw)
To: Jan Kiszka; +Cc: qemu-devel
Erm
This was supposed to be a reply to the "Re: [PATCH v2] usb-redir: Allow to attach USB 2.0 devices to 1.1 host controller"
thread, in that context is should make more sense :)
Regards,
Hans
On 10/08/2012 10:12 PM, Hans de Goede wrote:
> Hi,
>
> Funny that you ping about this just now, as I wanted to work on
> this today, but sofar I did not get around to it. Maybe tonight I will :)
>
> Long story short: Yes I've some remarks, but I pan to fix those myself and
> then merge it in my tree.
>
> I'll get back to you when this is done.
>
> Regards,
>
> Hans
>
>
> On 10/08/2012 07:07 PM, Jan Kiszka wrote:
>> As the block layer may decide to flush bottom-halfs while the machine is
>> still initializing (e.g. to read geometry data from the disk), our
>> postponed open event may be processed before the last frontend
>> registered with a muxed chardev. Until the semantics of BHs have been
>> clarified, use an expired timer to achieve the same effect (suggested
>> by Paolo Bonzini).
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> This obsoletes the versatilepb hack.
>>
>> qemu-char.c | 13 +++++++------
>> qemu-char.h | 2 +-
>> 2 files changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index f9ee2f0..fb4e3dc 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -123,19 +123,20 @@ void qemu_chr_be_event(CharDriverState *s, int event)
>> s->chr_event(s->handler_opaque, event);
>> }
>>
>> -static void qemu_chr_generic_open_bh(void *opaque)
>> +static void qemu_chr_fire_open_event(void *opaque)
>> {
>> CharDriverState *s = opaque;
>> qemu_chr_be_event(s, CHR_EVENT_OPENED);
>> - qemu_bh_delete(s->bh);
>> - s->bh = NULL;
>> + qemu_free_timer(s->open_timer);
>> + s->open_timer = NULL;
>> }
>>
>> void qemu_chr_generic_open(CharDriverState *s)
>> {
>> - if (s->bh == NULL) {
>> - s->bh = qemu_bh_new(qemu_chr_generic_open_bh, s);
>> - qemu_bh_schedule(s->bh);
>> + if (s->open_timer == NULL) {
>> + s->open_timer = qemu_new_timer_ms(vm_clock,
>> + qemu_chr_fire_open_event, s);
>> + qemu_mod_timer(s->open_timer, qemu_get_clock_ms(vm_clock) - 1);
>> }
>> }
>>
>> diff --git a/qemu-char.h b/qemu-char.h
>> index 486644b..297dd98 100644
>> --- a/qemu-char.h
>> +++ b/qemu-char.h
>> @@ -69,7 +69,7 @@ struct CharDriverState {
>> void (*chr_guest_open)(struct CharDriverState *chr);
>> void (*chr_guest_close)(struct CharDriverState *chr);
>> void *opaque;
>> - QEMUBH *bh;
>> + QEMUTimer *open_timer;
>> char *label;
>> char *filename;
>> int opened;
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH v2] chardev: Use timer instead of bottom-half to postpone open event
2012-10-08 17:07 [Qemu-devel] [PATCH] chardev: Use timer instead of bottom-half to postpone open event Jan Kiszka
2012-10-08 18:07 ` Aurelien Jarno
2012-10-08 20:12 ` Hans de Goede
@ 2012-10-12 7:52 ` Jan Kiszka
2012-10-31 13:23 ` Jan Kiszka
2 siblings, 1 reply; 7+ messages in thread
From: Jan Kiszka @ 2012-10-12 7:52 UTC (permalink / raw)
To: qemu-devel, Anthony Liguori; +Cc: Peter Maydell, Aurelien Jarno, Paolo Bonzini
As the block layer may decide to flush bottom-halfs while the machine is
still initializing (e.g. to read geometry data from the disk), our
postponed open event may be processed before the last frontend
registered with a muxed chardev.
Until the semantics of BHs have been clarified, use an expired timer to
achieve the same effect (suggested by Paolo Bonzini). This requires to
perform the alarm timer initialization earlier as otherwise timer
subsystem can be used before being ready.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
Changes in v2:
- Move init_alarm_timer call to fix e.g. -monitor stdio
qemu-char.c | 13 +++++++------
qemu-char.h | 2 +-
vl.c | 10 +++++-----
3 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/qemu-char.c b/qemu-char.c
index b082bae..d8eed21 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -123,19 +123,20 @@ void qemu_chr_be_event(CharDriverState *s, int event)
s->chr_event(s->handler_opaque, event);
}
-static void qemu_chr_generic_open_bh(void *opaque)
+static void qemu_chr_fire_open_event(void *opaque)
{
CharDriverState *s = opaque;
qemu_chr_be_event(s, CHR_EVENT_OPENED);
- qemu_bh_delete(s->bh);
- s->bh = NULL;
+ qemu_free_timer(s->open_timer);
+ s->open_timer = NULL;
}
void qemu_chr_generic_open(CharDriverState *s)
{
- if (s->bh == NULL) {
- s->bh = qemu_bh_new(qemu_chr_generic_open_bh, s);
- qemu_bh_schedule(s->bh);
+ if (s->open_timer == NULL) {
+ s->open_timer = qemu_new_timer_ms(vm_clock,
+ qemu_chr_fire_open_event, s);
+ qemu_mod_timer(s->open_timer, qemu_get_clock_ms(vm_clock) - 1);
}
}
diff --git a/qemu-char.h b/qemu-char.h
index 486644b..297dd98 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -69,7 +69,7 @@ struct CharDriverState {
void (*chr_guest_open)(struct CharDriverState *chr);
void (*chr_guest_close)(struct CharDriverState *chr);
void *opaque;
- QEMUBH *bh;
+ QEMUTimer *open_timer;
char *label;
char *filename;
int opened;
diff --git a/vl.c b/vl.c
index 5b357a3..586773c 100644
--- a/vl.c
+++ b/vl.c
@@ -3439,6 +3439,11 @@ int main(int argc, char **argv, char **envp)
add_device_config(DEV_VIRTCON, "vc:80Cx24C");
}
+ if (init_timer_alarm() < 0) {
+ fprintf(stderr, "could not initialize alarm timer\n");
+ exit(1);
+ }
+
socket_init();
if (qemu_opts_foreach(qemu_find_opts("chardev"), chardev_init_func, NULL, 1) != 0)
@@ -3506,11 +3511,6 @@ int main(int argc, char **argv, char **envp)
os_set_line_buffering();
- if (init_timer_alarm() < 0) {
- fprintf(stderr, "could not initialize alarm timer\n");
- exit(1);
- }
-
#ifdef CONFIG_SPICE
/* spice needs the timers to be initialized by this point */
qemu_spice_init();
--
1.7.3.4
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH v2] chardev: Use timer instead of bottom-half to postpone open event
2012-10-12 7:52 ` [Qemu-devel] [PATCH v2] " Jan Kiszka
@ 2012-10-31 13:23 ` Jan Kiszka
2012-10-31 21:49 ` Aurelien Jarno
0 siblings, 1 reply; 7+ messages in thread
From: Jan Kiszka @ 2012-10-31 13:23 UTC (permalink / raw)
To: qemu-devel, Anthony Liguori; +Cc: Peter Maydell, Aurelien Jarno, Paolo Bonzini
On 2012-10-12 09:52, Jan Kiszka wrote:
> As the block layer may decide to flush bottom-halfs while the machine is
> still initializing (e.g. to read geometry data from the disk), our
> postponed open event may be processed before the last frontend
> registered with a muxed chardev.
>
> Until the semantics of BHs have been clarified, use an expired timer to
> achieve the same effect (suggested by Paolo Bonzini). This requires to
> perform the alarm timer initialization earlier as otherwise timer
> subsystem can be used before being ready.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>
> Changes in v2:
> - Move init_alarm_timer call to fix e.g. -monitor stdio
Ping?
Jan
>
> qemu-char.c | 13 +++++++------
> qemu-char.h | 2 +-
> vl.c | 10 +++++-----
> 3 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index b082bae..d8eed21 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -123,19 +123,20 @@ void qemu_chr_be_event(CharDriverState *s, int event)
> s->chr_event(s->handler_opaque, event);
> }
>
> -static void qemu_chr_generic_open_bh(void *opaque)
> +static void qemu_chr_fire_open_event(void *opaque)
> {
> CharDriverState *s = opaque;
> qemu_chr_be_event(s, CHR_EVENT_OPENED);
> - qemu_bh_delete(s->bh);
> - s->bh = NULL;
> + qemu_free_timer(s->open_timer);
> + s->open_timer = NULL;
> }
>
> void qemu_chr_generic_open(CharDriverState *s)
> {
> - if (s->bh == NULL) {
> - s->bh = qemu_bh_new(qemu_chr_generic_open_bh, s);
> - qemu_bh_schedule(s->bh);
> + if (s->open_timer == NULL) {
> + s->open_timer = qemu_new_timer_ms(vm_clock,
> + qemu_chr_fire_open_event, s);
> + qemu_mod_timer(s->open_timer, qemu_get_clock_ms(vm_clock) - 1);
> }
> }
>
> diff --git a/qemu-char.h b/qemu-char.h
> index 486644b..297dd98 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -69,7 +69,7 @@ struct CharDriverState {
> void (*chr_guest_open)(struct CharDriverState *chr);
> void (*chr_guest_close)(struct CharDriverState *chr);
> void *opaque;
> - QEMUBH *bh;
> + QEMUTimer *open_timer;
> char *label;
> char *filename;
> int opened;
> diff --git a/vl.c b/vl.c
> index 5b357a3..586773c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3439,6 +3439,11 @@ int main(int argc, char **argv, char **envp)
> add_device_config(DEV_VIRTCON, "vc:80Cx24C");
> }
>
> + if (init_timer_alarm() < 0) {
> + fprintf(stderr, "could not initialize alarm timer\n");
> + exit(1);
> + }
> +
> socket_init();
>
> if (qemu_opts_foreach(qemu_find_opts("chardev"), chardev_init_func, NULL, 1) != 0)
> @@ -3506,11 +3511,6 @@ int main(int argc, char **argv, char **envp)
>
> os_set_line_buffering();
>
> - if (init_timer_alarm() < 0) {
> - fprintf(stderr, "could not initialize alarm timer\n");
> - exit(1);
> - }
> -
> #ifdef CONFIG_SPICE
> /* spice needs the timers to be initialized by this point */
> qemu_spice_init();
>
--
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] [PATCH v2] chardev: Use timer instead of bottom-half to postpone open event
2012-10-31 13:23 ` Jan Kiszka
@ 2012-10-31 21:49 ` Aurelien Jarno
0 siblings, 0 replies; 7+ messages in thread
From: Aurelien Jarno @ 2012-10-31 21:49 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Peter Maydell, Anthony Liguori, qemu-devel, Paolo Bonzini
On Wed, Oct 31, 2012 at 02:23:15PM +0100, Jan Kiszka wrote:
> On 2012-10-12 09:52, Jan Kiszka wrote:
> > As the block layer may decide to flush bottom-halfs while the machine is
> > still initializing (e.g. to read geometry data from the disk), our
> > postponed open event may be processed before the last frontend
> > registered with a muxed chardev.
> >
> > Until the semantics of BHs have been clarified, use an expired timer to
> > achieve the same effect (suggested by Paolo Bonzini). This requires to
> > perform the alarm timer initialization earlier as otherwise timer
> > subsystem can be used before being ready.
> >
> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > ---
> >
> > Changes in v2:
> > - Move init_alarm_timer call to fix e.g. -monitor stdio
>
> Ping?
>
> Jan
>
> >
> > qemu-char.c | 13 +++++++------
> > qemu-char.h | 2 +-
> > vl.c | 10 +++++-----
> > 3 files changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/qemu-char.c b/qemu-char.c
> > index b082bae..d8eed21 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -123,19 +123,20 @@ void qemu_chr_be_event(CharDriverState *s, int event)
> > s->chr_event(s->handler_opaque, event);
> > }
> >
> > -static void qemu_chr_generic_open_bh(void *opaque)
> > +static void qemu_chr_fire_open_event(void *opaque)
> > {
> > CharDriverState *s = opaque;
> > qemu_chr_be_event(s, CHR_EVENT_OPENED);
> > - qemu_bh_delete(s->bh);
> > - s->bh = NULL;
> > + qemu_free_timer(s->open_timer);
> > + s->open_timer = NULL;
> > }
> >
> > void qemu_chr_generic_open(CharDriverState *s)
> > {
> > - if (s->bh == NULL) {
> > - s->bh = qemu_bh_new(qemu_chr_generic_open_bh, s);
> > - qemu_bh_schedule(s->bh);
> > + if (s->open_timer == NULL) {
> > + s->open_timer = qemu_new_timer_ms(vm_clock,
> > + qemu_chr_fire_open_event, s);
> > + qemu_mod_timer(s->open_timer, qemu_get_clock_ms(vm_clock) - 1);
> > }
> > }
> >
> > diff --git a/qemu-char.h b/qemu-char.h
> > index 486644b..297dd98 100644
> > --- a/qemu-char.h
> > +++ b/qemu-char.h
> > @@ -69,7 +69,7 @@ struct CharDriverState {
> > void (*chr_guest_open)(struct CharDriverState *chr);
> > void (*chr_guest_close)(struct CharDriverState *chr);
> > void *opaque;
> > - QEMUBH *bh;
> > + QEMUTimer *open_timer;
> > char *label;
> > char *filename;
> > int opened;
> > diff --git a/vl.c b/vl.c
> > index 5b357a3..586773c 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -3439,6 +3439,11 @@ int main(int argc, char **argv, char **envp)
> > add_device_config(DEV_VIRTCON, "vc:80Cx24C");
> > }
> >
> > + if (init_timer_alarm() < 0) {
> > + fprintf(stderr, "could not initialize alarm timer\n");
> > + exit(1);
> > + }
> > +
> > socket_init();
> >
> > if (qemu_opts_foreach(qemu_find_opts("chardev"), chardev_init_func, NULL, 1) != 0)
> > @@ -3506,11 +3511,6 @@ int main(int argc, char **argv, char **envp)
> >
> > os_set_line_buffering();
> >
> > - if (init_timer_alarm() < 0) {
> > - fprintf(stderr, "could not initialize alarm timer\n");
> > - exit(1);
> > - }
> > -
> > #ifdef CONFIG_SPICE
> > /* spice needs the timers to be initialized by this point */
> > qemu_spice_init();
> >
>
>
Applied.
--
Aurelien Jarno GPG: 1024D/F1BCDB73
aurelien@aurel32.net http://www.aurel32.net
^ permalink raw reply [flat|nested] 7+ messages in thread