Patchwork [01/14] Convert io handlers to QLIST

login
register
mail settings
Submitter Juan Quintela
Date March 10, 2010, 10:03 a.m.
Message ID <4f197c437a4d2f813ce518b4fd2d86fc225f500e.1268214633.git.quintela@redhat.com>
Download mbox | patch
Permalink /patch/47215/
State New
Headers show

Comments

Juan Quintela - March 10, 2010, 10:03 a.m.
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 vl.c |   35 ++++++++++++++---------------------
 1 files changed, 14 insertions(+), 21 deletions(-)
malc - March 10, 2010, 11:57 a.m.
On Wed, 10 Mar 2010, Juan Quintela wrote:

> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  vl.c |   35 ++++++++++++++---------------------
>  1 files changed, 14 insertions(+), 21 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 10d8e34..83ff652 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2597,10 +2597,12 @@ typedef struct IOHandlerRecord {
>      void *opaque;
>      /* temporary data */
>      struct pollfd *ufd;
> -    struct IOHandlerRecord *next;
> +    QTAILQ_ENTRY(IOHandlerRecord) next;
>  } IOHandlerRecord;
> 
> -static IOHandlerRecord *first_io_handler;
> +static QTAILQ_HEAD(, IOHandlerRecord) io_handlers =
> +    QTAILQ_HEAD_INITIALIZER(io_handlers);
> +
> 
>  /* XXX: fd_read_poll should be suppressed, but an API change is
>     necessary in the character devices to suppress fd_can_read(). */
> @@ -2610,28 +2612,22 @@ int qemu_set_fd_handler2(int fd,
>                           IOHandler *fd_write,
>                           void *opaque)
>  {
> -    IOHandlerRecord **pioh, *ioh;
> +    IOHandlerRecord *ioh;
> 
>      if (!fd_read && !fd_write) {
> -        pioh = &first_io_handler;
> -        for(;;) {
> -            ioh = *pioh;
> -            if (ioh == NULL)
> -                break;
> +        QTAILQ_FOREACH(ioh, &io_handlers, next) {
>              if (ioh->fd == fd) {
>                  ioh->deleted = 1;
>                  break;
>              }
> -            pioh = &ioh->next;
>          }
>      } else {
> -        for(ioh = first_io_handler; ioh != NULL; ioh = ioh->next) {
> +        QTAILQ_FOREACH(ioh, &io_handlers, next) {
>              if (ioh->fd == fd)
>                  goto found;
>          }
>          ioh = qemu_mallocz(sizeof(IOHandlerRecord));
> -        ioh->next = first_io_handler;
> -        first_io_handler = ioh;
> +        QTAILQ_INSERT_TAIL(&io_handlers, ioh, next);

The old code inserted at the head, didn't it?

>      found:
>          ioh->fd = fd;
>          ioh->fd_read_poll = fd_read_poll;
> @@ -3822,7 +3818,7 @@ void main_loop_wait(int timeout)
>      FD_ZERO(&rfds);
>      FD_ZERO(&wfds);
>      FD_ZERO(&xfds);
> -    for(ioh = first_io_handler; ioh != NULL; ioh = ioh->next) {
> +    QTAILQ_FOREACH(ioh, &io_handlers, next) {
>          if (ioh->deleted)
>              continue;
>          if (ioh->fd_read &&
> @@ -3848,9 +3844,9 @@ void main_loop_wait(int timeout)
>      ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
>      qemu_mutex_lock_iothread();
>      if (ret > 0) {
> -        IOHandlerRecord **pioh;
> +        IOHandlerRecord *pioh;
> 
> -        for(ioh = first_io_handler; ioh != NULL; ioh = ioh->next) {
> +        QTAILQ_FOREACH(ioh, &io_handlers, next) {
>              if (!ioh->deleted && ioh->fd_read && FD_ISSET(ioh->fd, &rfds)) {
>                  ioh->fd_read(ioh->opaque);
>              }
> @@ -3860,14 +3856,11 @@ void main_loop_wait(int timeout)
>          }
> 
>  	/* remove deleted IO handlers */
> -	pioh = &first_io_handler;
> -	while (*pioh) {
> -            ioh = *pioh;
> +        QTAILQ_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
>              if (ioh->deleted) {
> -                *pioh = ioh->next;
> +                QTAILQ_REMOVE(&io_handlers, ioh, next);
>                  qemu_free(ioh);
> -            } else
> -                pioh = &ioh->next;
> +            }
>          }
>      }
> 
>
Juan Quintela - March 10, 2010, 12:06 p.m.
malc <av1474@comtv.ru> wrote:
> On Wed, 10 Mar 2010, Juan Quintela wrote:

>> -        for(ioh = first_io_handler; ioh != NULL; ioh = ioh->next) {
>> +        QTAILQ_FOREACH(ioh, &io_handlers, next) {
>>              if (ioh->fd == fd)
>>                  goto found;
>>          }
>>          ioh = qemu_mallocz(sizeof(IOHandlerRecord));
>> -        ioh->next = first_io_handler;
>> -        first_io_handler = ioh;
>> +        QTAILQ_INSERT_TAIL(&io_handlers, ioh, next);
>
> The old code inserted at the head, didn't it?

Sorry, you are right, it shouldn't matter too much, but it is a change.

Later, Juan.
malc - March 10, 2010, 12:08 p.m.
On Wed, 10 Mar 2010, Juan Quintela wrote:

> malc <av1474@comtv.ru> wrote:
> > On Wed, 10 Mar 2010, Juan Quintela wrote:
> 
> >> -        for(ioh = first_io_handler; ioh != NULL; ioh = ioh->next) {
> >> +        QTAILQ_FOREACH(ioh, &io_handlers, next) {
> >>              if (ioh->fd == fd)
> >>                  goto found;
> >>          }
> >>          ioh = qemu_mallocz(sizeof(IOHandlerRecord));
> >> -        ioh->next = first_io_handler;
> >> -        first_io_handler = ioh;
> >> +        QTAILQ_INSERT_TAIL(&io_handlers, ioh, next);
> >
> > The old code inserted at the head, didn't it?
> 
> Sorry, you are right, it shouldn't matter too much, but it is a change.

If it did, why queue instead of list?

> 
> Later, Juan.
>
Juan Quintela - March 10, 2010, 12:20 p.m.
malc <av1474@comtv.ru> wrote:
> On Wed, 10 Mar 2010, Juan Quintela wrote:
>
>> malc <av1474@comtv.ru> wrote:
>> > On Wed, 10 Mar 2010, Juan Quintela wrote:
>> 
>> >> -        for(ioh = first_io_handler; ioh != NULL; ioh = ioh->next) {
>> >> +        QTAILQ_FOREACH(ioh, &io_handlers, next) {
>> >>              if (ioh->fd == fd)
>> >>                  goto found;
>> >>          }
>> >>          ioh = qemu_mallocz(sizeof(IOHandlerRecord));
>> >> -        ioh->next = first_io_handler;
>> >> -        first_io_handler = ioh;
>> >> +        QTAILQ_INSERT_TAIL(&io_handlers, ioh, next);
>> >
>> > The old code inserted at the head, didn't it?
>> 
>> Sorry, you are right, it shouldn't matter too much, but it is a change.
>
> If it did, why queue instead of list?

Arbitrary.  Example conversion nearer was QTAIL.

Use is:
- insert at the beggining
- search for removal
- loop all list

insert/searchs should be less than loops.  I am more
interested/intrigued if setting more fd's in the select call could
change behaviour.

Changing to QLIST is search/replace, no big deal.

>> 
>> Later, Juan.
>>
malc - March 10, 2010, 12:54 p.m.
On Wed, 10 Mar 2010, Juan Quintela wrote:

> malc <av1474@comtv.ru> wrote:
> > On Wed, 10 Mar 2010, Juan Quintela wrote:
> >
> >> malc <av1474@comtv.ru> wrote:
> >> > On Wed, 10 Mar 2010, Juan Quintela wrote:
> >> 
> >> >> -        for(ioh = first_io_handler; ioh != NULL; ioh = ioh->next) {
> >> >> +        QTAILQ_FOREACH(ioh, &io_handlers, next) {
> >> >>              if (ioh->fd == fd)
> >> >>                  goto found;
> >> >>          }
> >> >>          ioh = qemu_mallocz(sizeof(IOHandlerRecord));
> >> >> -        ioh->next = first_io_handler;
> >> >> -        first_io_handler = ioh;
> >> >> +        QTAILQ_INSERT_TAIL(&io_handlers, ioh, next);
> >> >
> >> > The old code inserted at the head, didn't it?
> >> 
> >> Sorry, you are right, it shouldn't matter too much, but it is a change.
> >
> > If it did, why queue instead of list?
> 
> Arbitrary.  Example conversion nearer was QTAIL.

Please do `man 3 queue'. Specifically the comparison between the tail
queues and lists.

> 
> Use is:
> - insert at the beggining
> - search for removal
> - loop all list
> 
> insert/searchs should be less than loops.  I am more
> interested/intrigued if setting more fd's in the select call could
> change behaviour.
> 
> Changing to QLIST is search/replace, no big deal.
> 
> >> 
> >> Later, Juan.
> >> 
>
Juan Quintela - March 10, 2010, 1:03 p.m.
malc <av1474@comtv.ru> wrote:
> On Wed, 10 Mar 2010, Juan Quintela wrote:
>
>> malc <av1474@comtv.ru> wrote:
>> > On Wed, 10 Mar 2010, Juan Quintela wrote:
>> >
>> >> malc <av1474@comtv.ru> wrote:
>> >> > On Wed, 10 Mar 2010, Juan Quintela wrote:
>> >> 
>> >> >> -        for(ioh = first_io_handler; ioh != NULL; ioh = ioh->next) {
>> >> >> +        QTAILQ_FOREACH(ioh, &io_handlers, next) {
>> >> >>              if (ioh->fd == fd)
>> >> >>                  goto found;
>> >> >>          }
>> >> >>          ioh = qemu_mallocz(sizeof(IOHandlerRecord));
>> >> >> -        ioh->next = first_io_handler;
>> >> >> -        first_io_handler = ioh;
>> >> >> +        QTAILQ_INSERT_TAIL(&io_handlers, ioh, next);
>> >> >
>> >> > The old code inserted at the head, didn't it?
>> >> 
>> >> Sorry, you are right, it shouldn't matter too much, but it is a change.
>> >
>> > If it did, why queue instead of list?
>> 
>> Arbitrary.  Example conversion nearer was QTAIL.
>
> Please do `man 3 queue'. Specifically the comparison between the tail
> queues and lists.

Thanks very much for the info.  Didn't knew that page.

You win.  Will change to QLISTS.  Waiting for more comments.

Later, Juan.

Patch

diff --git a/vl.c b/vl.c
index 10d8e34..83ff652 100644
--- a/vl.c
+++ b/vl.c
@@ -2597,10 +2597,12 @@  typedef struct IOHandlerRecord {
     void *opaque;
     /* temporary data */
     struct pollfd *ufd;
-    struct IOHandlerRecord *next;
+    QTAILQ_ENTRY(IOHandlerRecord) next;
 } IOHandlerRecord;

-static IOHandlerRecord *first_io_handler;
+static QTAILQ_HEAD(, IOHandlerRecord) io_handlers =
+    QTAILQ_HEAD_INITIALIZER(io_handlers);
+

 /* XXX: fd_read_poll should be suppressed, but an API change is
    necessary in the character devices to suppress fd_can_read(). */
@@ -2610,28 +2612,22 @@  int qemu_set_fd_handler2(int fd,
                          IOHandler *fd_write,
                          void *opaque)
 {
-    IOHandlerRecord **pioh, *ioh;
+    IOHandlerRecord *ioh;

     if (!fd_read && !fd_write) {
-        pioh = &first_io_handler;
-        for(;;) {
-            ioh = *pioh;
-            if (ioh == NULL)
-                break;
+        QTAILQ_FOREACH(ioh, &io_handlers, next) {
             if (ioh->fd == fd) {
                 ioh->deleted = 1;
                 break;
             }
-            pioh = &ioh->next;
         }
     } else {
-        for(ioh = first_io_handler; ioh != NULL; ioh = ioh->next) {
+        QTAILQ_FOREACH(ioh, &io_handlers, next) {
             if (ioh->fd == fd)
                 goto found;
         }
         ioh = qemu_mallocz(sizeof(IOHandlerRecord));
-        ioh->next = first_io_handler;
-        first_io_handler = ioh;
+        QTAILQ_INSERT_TAIL(&io_handlers, ioh, next);
     found:
         ioh->fd = fd;
         ioh->fd_read_poll = fd_read_poll;
@@ -3822,7 +3818,7 @@  void main_loop_wait(int timeout)
     FD_ZERO(&rfds);
     FD_ZERO(&wfds);
     FD_ZERO(&xfds);
-    for(ioh = first_io_handler; ioh != NULL; ioh = ioh->next) {
+    QTAILQ_FOREACH(ioh, &io_handlers, next) {
         if (ioh->deleted)
             continue;
         if (ioh->fd_read &&
@@ -3848,9 +3844,9 @@  void main_loop_wait(int timeout)
     ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
     qemu_mutex_lock_iothread();
     if (ret > 0) {
-        IOHandlerRecord **pioh;
+        IOHandlerRecord *pioh;

-        for(ioh = first_io_handler; ioh != NULL; ioh = ioh->next) {
+        QTAILQ_FOREACH(ioh, &io_handlers, next) {
             if (!ioh->deleted && ioh->fd_read && FD_ISSET(ioh->fd, &rfds)) {
                 ioh->fd_read(ioh->opaque);
             }
@@ -3860,14 +3856,11 @@  void main_loop_wait(int timeout)
         }

 	/* remove deleted IO handlers */
-	pioh = &first_io_handler;
-	while (*pioh) {
-            ioh = *pioh;
+        QTAILQ_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
             if (ioh->deleted) {
-                *pioh = ioh->next;
+                QTAILQ_REMOVE(&io_handlers, ioh, next);
                 qemu_free(ioh);
-            } else
-                pioh = &ioh->next;
+            }
         }
     }