Patchwork chardev: Use timer instead of bottom-half to postpone open event

login
register
mail settings
Submitter Jan Kiszka
Date Oct. 8, 2012, 5:07 p.m.
Message ID <5073083C.1060201@siemens.com>
Download mbox | patch
Permalink /patch/190061/
State New
Headers show

Comments

Jan Kiszka - Oct. 8, 2012, 5:07 p.m.
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(-)
Aurelien Jarno - Oct. 8, 2012, 6:07 p.m.
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>
Hans de Goede - Oct. 8, 2012, 8:12 p.m.
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;
>
Hans de Goede - Oct. 8, 2012, 8:14 p.m.
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;
>>

Patch

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;