Patchwork [3/5] iohandlers: Allow each iohandler to be enabled/disabled individually

login
register
mail settings
Submitter Amit Shah
Date Jan. 13, 2011, 1 p.m.
Message ID <1bdf0a5a5de06cfb332ac17c439ef79cabb835db.1294923288.git.amit.shah@redhat.com>
Download mbox | patch
Permalink /patch/78747/
State New
Headers show

Comments

Amit Shah - Jan. 13, 2011, 1 p.m.
Each iohandler for an fd can now be individually enabled or disabled.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 qemu-char.h |    4 ++++
 vl.c        |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 58 insertions(+), 4 deletions(-)
Gerd Hoffmann - Jan. 13, 2011, 1:55 p.m.
On 01/13/11 14:00, Amit Shah wrote:
>   {
> -    return assign_fd_handlers(fd, fd_read_poll, fd_read, fd_write, opaque);
> +    assign_fd_handlers(fd, fd_read_poll, fd_read, fd_write, opaque);
> +    set_read_poll_fd_action(fd, true);
> +    set_read_fd_action(fd, true);
> +    set_write_fd_action(fd, true);
> +    return 0;
>   }

I'd suggest to move the *action calls into assign_fd_handlers() so the 
handlers default to being enabled in all cases.  This should match what 
most users need and thus minimize the number of *_action calls needed.

cheers,
   Gerd
Amit Shah - Jan. 13, 2011, 2 p.m.
On (Thu) Jan 13 2011 [14:55:25], Gerd Hoffmann wrote:
> On 01/13/11 14:00, Amit Shah wrote:
> >  {
> >-    return assign_fd_handlers(fd, fd_read_poll, fd_read, fd_write, opaque);
> >+    assign_fd_handlers(fd, fd_read_poll, fd_read, fd_write, opaque);
> >+    set_read_poll_fd_action(fd, true);
> >+    set_read_fd_action(fd, true);
> >+    set_write_fd_action(fd, true);
> >+    return 0;
> >  }
> 
> I'd suggest to move the *action calls into assign_fd_handlers() so
> the handlers default to being enabled in all cases.  This should
> match what most users need and thus minimize the number of *_action
> calls needed.

What may happen with that is the fd may get select()-ed for an operation
that it didn't want to be put on the queue for.

		Amit
Gerd Hoffmann - Jan. 13, 2011, 2:08 p.m.
On 01/13/11 15:00, Amit Shah wrote:
> On (Thu) Jan 13 2011 [14:55:25], Gerd Hoffmann wrote:
>> On 01/13/11 14:00, Amit Shah wrote:
>>>   {
>>> -    return assign_fd_handlers(fd, fd_read_poll, fd_read, fd_write, opaque);
>>> +    assign_fd_handlers(fd, fd_read_poll, fd_read, fd_write, opaque);
>>> +    set_read_poll_fd_action(fd, true);
>>> +    set_read_fd_action(fd, true);
>>> +    set_write_fd_action(fd, true);
>>> +    return 0;
>>>   }
>>
>> I'd suggest to move the *action calls into assign_fd_handlers() so
>> the handlers default to being enabled in all cases.  This should
>> match what most users need and thus minimize the number of *_action
>> calls needed.
>
> What may happen with that is the fd may get select()-ed for an operation
> that it didn't want to be put on the queue for.

I can't see such a race window given that most qemu code runs serialized 
anyway.  If you call assign_fd_handlers() + set_write_fd_action(false) 
in sequence I can't see how a select call can happen inbetween ...

cheers,
   Gerd
Amit Shah - Jan. 13, 2011, 2:18 p.m.
On (Thu) Jan 13 2011 [15:08:51], Gerd Hoffmann wrote:
> On 01/13/11 15:00, Amit Shah wrote:
> >On (Thu) Jan 13 2011 [14:55:25], Gerd Hoffmann wrote:
> >>On 01/13/11 14:00, Amit Shah wrote:
> >>>  {
> >>>-    return assign_fd_handlers(fd, fd_read_poll, fd_read, fd_write, opaque);
> >>>+    assign_fd_handlers(fd, fd_read_poll, fd_read, fd_write, opaque);
> >>>+    set_read_poll_fd_action(fd, true);
> >>>+    set_read_fd_action(fd, true);
> >>>+    set_write_fd_action(fd, true);
> >>>+    return 0;
> >>>  }
> >>
> >>I'd suggest to move the *action calls into assign_fd_handlers() so
> >>the handlers default to being enabled in all cases.  This should
> >>match what most users need and thus minimize the number of *_action
> >>calls needed.
> >
> >What may happen with that is the fd may get select()-ed for an operation
> >that it didn't want to be put on the queue for.
> 
> I can't see such a race window given that most qemu code runs
> serialized anyway.  If you call assign_fd_handlers() +
> set_write_fd_action(false) in sequence I can't see how a select call
> can happen inbetween ...

Not today, but later when we have threads doing this stuff?  Should I
just leave a comment to take care of this for later?

		Amit
Gerd Hoffmann - Jan. 13, 2011, 2:53 p.m.
Hi,

>> I can't see such a race window given that most qemu code runs
>> serialized anyway.  If you call assign_fd_handlers() +
>> set_write_fd_action(false) in sequence I can't see how a select call
>> can happen inbetween ...
>
> Not today, but later when we have threads doing this stuff?

Unlikely I think.  Seems we will go offload specific tasks to threads 
using threadlets (especially in the block layer), but I expect the main 
even loop will not be splitted into multiple threads.

>  Should I
> just leave a comment to take care of this for later?

Thats fine I guess.  Or maybe add arguments to assign_fd_handlers() with 
the initial state.

cheers,
   Gerd

Patch

diff --git a/qemu-char.h b/qemu-char.h
index 0ef83f4..e88a108 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -115,6 +115,10 @@  int assign_fd_handlers(int fd,
                        IOHandler *fd_write,
                        void *opaque);
 void remove_fd_handlers(int fd);
+int set_read_poll_fd_action(int fd, bool enable);
+int set_read_fd_action(int fd, bool enable);
+int set_write_fd_action(int fd, bool enable);
+
 int qemu_set_fd_handler2(int fd,
                          IOCanReadHandler *fd_read_poll,
                          IOHandler *fd_read,
diff --git a/vl.c b/vl.c
index 38e0a3c..a0b14b5 100644
--- a/vl.c
+++ b/vl.c
@@ -1014,6 +1014,9 @@  typedef struct IOHandlerRecord {
     IOHandler *fd_write;
     int deleted;
     void *opaque;
+    bool read_poll_enabled;
+    bool read_enabled;
+    bool write_enabled;
     /* temporary data */
     struct pollfd *ufd;
     QLIST_ENTRY(IOHandlerRecord) next;
@@ -1062,6 +1065,7 @@  int assign_fd_handlers(int fd,
     ioh->fd_write = fd_write;
     ioh->opaque = opaque;
     ioh->deleted = 0;
+    ioh->read_poll_enabled = ioh->read_enabled = ioh->write_enabled = false;
 
     return 0;
 }
@@ -1071,13 +1075,59 @@  void remove_fd_handlers(int fd)
     assign_fd_handlers(fd, NULL, NULL, NULL, NULL);
 }
 
+int set_read_poll_fd_action(int fd, bool enable)
+{
+    IOHandlerRecord *ioh;
+
+    ioh = get_iohandler(fd);
+
+    if (!ioh) {
+        return -1;
+    }
+    ioh->read_poll_enabled = enable;
+
+    return 0;
+}
+
+int set_read_fd_action(int fd, bool enable)
+{
+    IOHandlerRecord *ioh;
+
+    ioh = get_iohandler(fd);
+
+    if (!ioh) {
+        return -1;
+    }
+    ioh->read_enabled = enable;
+
+    return 0;
+}
+
+int set_write_fd_action(int fd, bool enable)
+{
+    IOHandlerRecord *ioh;
+
+    ioh = get_iohandler(fd);
+
+    if (!ioh) {
+        return -1;
+    }
+    ioh->write_enabled = enable;
+
+    return 0;
+}
+
 int qemu_set_fd_handler2(int fd,
                          IOCanReadHandler *fd_read_poll,
                          IOHandler *fd_read,
                          IOHandler *fd_write,
                          void *opaque)
 {
-    return assign_fd_handlers(fd, fd_read_poll, fd_read, fd_write, opaque);
+    assign_fd_handlers(fd, fd_read_poll, fd_read, fd_write, opaque);
+    set_read_poll_fd_action(fd, true);
+    set_read_fd_action(fd, true);
+    set_write_fd_action(fd, true);
+    return 0;
 }
 
 int qemu_set_fd_handler(int fd,
@@ -1341,14 +1391,14 @@  void main_loop_wait(int nonblocking)
     QLIST_FOREACH(ioh, &io_handlers, next) {
         if (ioh->deleted)
             continue;
-        if (ioh->fd_read &&
+        if (ioh->fd_read && ioh->read_enabled &&
             (!ioh->fd_read_poll ||
-             ioh->fd_read_poll(ioh->opaque) != 0)) {
+             (!ioh->read_poll_enabled || ioh->fd_read_poll(ioh->opaque) != 0))) {
             FD_SET(ioh->fd, &rfds);
             if (ioh->fd > nfds)
                 nfds = ioh->fd;
         }
-        if (ioh->fd_write) {
+        if (ioh->fd_write && ioh->write_enabled) {
             FD_SET(ioh->fd, &wfds);
             if (ioh->fd > nfds)
                 nfds = ioh->fd;