Skip to contentSkip to conversation

iohandlers: Add support for enabling/disabling individual handlers

PATCH In review Amit Shah 4 participants 13 messages Jan 13, 2011 – Jan 18, 2011

lore
Share & tools
lore

Amit Shah

author
#0 lore raw

v2 cover iohandlers: Add support for enabling/disabling individual handlers

Hi,

This patchset adds new interfaces to work with iohandlers. It adds:

int assign_fd_handlers(int fd, IOHandlerOps *ops, void *opaque)
-- Specify io handlers for an fd
int remove_fd_handlers(int fd)
-- Remove fd handlers for fd (mark ioh for deletion)
int set_read_poll_fd_action(int fd, bool enable)
-- Enable or disable the fd_read_poll fd handler
int set_read_fd_action(int fd, bool enable)
-- Enable or disable the fd_read fd handler
int set_write_fd_action(int fd, bool enable)
-- Enable or disable the fd_read fd handler

A new struct, IOHandlerOps, is added, to collect all the ops together instead of passing individual ones to functions.

The older function, qemu_set_fd_handler2(), is now a wrapper to assign_fd_handlers() and can be deprecated by converting the existing usage to assign_fd_handlers().

v2: Address comments from Gerd:
- add comments to new interfaces
- enable all specified handlers by default in assign_fd_handlers()
- Add comments for TODO items on deprecation of older interfaces.

Please apply.

Amit Shah (5):
iohandlers: Avoid code duplication
iohandlers: Introduce assign_fd_handlers() and remove_fd_handlers
iohandlers: Allow each iohandler to be enabled/disabled individually
iohandlers: Enable an iohandler only if the associated handler exists
iohandlers: Add IOHandlerOps struct

 qemu-char.h |    7 ++
 vl.c        |  197 ++++++++++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 170 insertions(+), 34 deletions(-)
Signature · 1 line
1.7.3.4
author
#1 lore raw

PATCH v2 1/5 iohandlers: Avoid code duplication

Add a get_iohandler() function instead of looking up the ioh twice in qemu_set_fd_handler2().

---
 vl.c |   44 ++++++++++++++++++++++++++------------------
 1 files changed, 26 insertions(+), 18 deletions(-)
vl.c +26 -18
index 0292184..9e365f6 100644
@@ -1022,6 +1022,17 @@ typedef struct IOHandlerRecord {
 static QLIST_HEAD(, IOHandlerRecord) io_handlers =
     QLIST_HEAD_INITIALIZER(io_handlers);
 
+static IOHandlerRecord *get_iohandler(int fd)
+{
+    IOHandlerRecord *ioh;
+
+    QLIST_FOREACH(ioh, &io_handlers, next) {
+        if (ioh->fd == fd) {
+            return ioh;
+        }
+    }
+    return NULL;
+}
 
 /* XXX: fd_read_poll should be suppressed, but an API change is
    necessary in the character devices to suppress fd_can_read(). */
@@ -1033,28 +1044,25 @@ int qemu_set_fd_handler2(int fd,
 {
     IOHandlerRecord *ioh;
 
-    if (!fd_read && !fd_write) {
-        QLIST_FOREACH(ioh, &io_handlers, next) {
-            if (ioh->fd == fd) {
-                ioh->deleted = 1;
-                break;
-            }
-        }
-    } else {
-        QLIST_FOREACH(ioh, &io_handlers, next) {
-            if (ioh->fd == fd)
-                goto found;
-        }
+    ioh = get_iohandler(fd);
+
+    if (ioh && !fd_read && !fd_write) {
+        ioh->deleted = 1;
+        return 0;
+    }
+
+    if (!ioh) {
         ioh = qemu_mallocz(sizeof(IOHandlerRecord));
         QLIST_INSERT_HEAD(&io_handlers, ioh, next);
-    found:
+
         ioh->fd = fd;
-        ioh->fd_read_poll = fd_read_poll;
-        ioh->fd_read = fd_read;
-        ioh->fd_write = fd_write;
-        ioh->opaque = opaque;
-        ioh->deleted = 0;
     }
+    ioh->fd_read_poll = fd_read_poll;
+    ioh->fd_read = fd_read;
+    ioh->fd_write = fd_write;
+    ioh->opaque = opaque;
+    ioh->deleted = 0;
+
     return 0;
 }
 
Signature · 1 line
1.7.3.4
author
#2 lore raw

PATCH v2 2/5 iohandlers: Introduce assign_fd_handlers() and remove_fd_handlers

In reply to Amit Shah

This function will be used to assign fd handlers. Future commits will be enable each handler to be enabled/disabled individually.

Make qemu_set_fd_handler2() a wrapper to assign_fd_handlers().

remove_fd_handlers() removes all the assigned handlers and marks the iohandler for deletion. It's a wrapper to assign_fd_handlers() with NULL handlers.

---
 qemu-char.h |    6 ++++++
 vl.c        |   36 +++++++++++++++++++++++++++++-------
 2 files changed, 35 insertions(+), 7 deletions(-)
qemu-char.h +6 -0
index e6ee6c4..0ef83f4 100644
@@ -109,6 +109,12 @@ size_t qemu_chr_mem_osize(const CharDriverState *chr);
 
 /* async I/O support */
 
+int assign_fd_handlers(int fd,
+                       IOCanReadHandler *fd_read_poll,
+                       IOHandler *fd_read,
+                       IOHandler *fd_write,
+                       void *opaque);
+void remove_fd_handlers(int fd);
 int qemu_set_fd_handler2(int fd,
                          IOCanReadHandler *fd_read_poll,
                          IOHandler *fd_read,
vl.c +29 -7
index 9e365f6..30256e1 100644
@@ -1034,13 +1034,17 @@ static IOHandlerRecord *get_iohandler(int fd)
     return NULL;
 }
 
-/* XXX: fd_read_poll should be suppressed, but an API change is
-   necessary in the character devices to suppress fd_can_read(). */
-int qemu_set_fd_handler2(int fd,
-                         IOCanReadHandler *fd_read_poll,
-                         IOHandler *fd_read,
-                         IOHandler *fd_write,
-                         void *opaque)
+/*
+ * Specify function pointers for the various IOHandlers for an fd here.
+ *
+ * XXX: fd_read_poll should be suppressed, but an API change is
+ * necessary in the character devices to suppress fd_can_read().
+ */
+int assign_fd_handlers(int fd,
+                       IOCanReadHandler *fd_read_poll,
+                       IOHandler *fd_read,
+                       IOHandler *fd_write,
+                       void *opaque)
 {
     IOHandlerRecord *ioh;
 
@@ -1066,6 +1070,24 @@ int qemu_set_fd_handler2(int fd,
     return 0;
 }
 
+/*
+ * Remove IO handlers for fd.  Marks the IOHandler, if one exists, for
+ * deletion.
+ */
+void remove_fd_handlers(int fd)
+{
+    assign_fd_handlers(fd, NULL, NULL, NULL, NULL);
+}
+
+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);
+}
+
 int qemu_set_fd_handler(int fd,
                         IOHandler *fd_read,
                         IOHandler *fd_write,
Signature · 1 line
1.7.3.4
author
#3 lore raw

PATCH v2 3/5 iohandlers: Allow each iohandler to be enabled/disabled individually

In reply to Amit Shah

Each iohandler for an fd can now be individually enabled or disabled.

---
 qemu-char.h |    4 +++
 vl.c        |   78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 79 insertions(+), 3 deletions(-)
qemu-char.h +4 -0
index 0ef83f4..e88a108 100644
@@ -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,
vl.c +75 -3

113 lines not on this page · raw message

Signature · 1 line
1.7.3.4
author
#4 lore raw

PATCH v2 4/5 iohandlers: Enable an iohandler only if the associated handler exists

In reply to Amit Shah

If an iohandler is asked to be enabled but the handler doesn't exist, don't enable the handler.

This can be used to simplify the conditions in main_loop_wait().

---
 vl.c |   29 ++++++++++++++++++++---------
 1 files changed, 20 insertions(+), 9 deletions(-)
vl.c +20 -9
index b9c902a..4aa4158 100644
@@ -1049,7 +1049,11 @@ int set_read_poll_fd_action(int fd, bool enable)
     if (!ioh) {
         return -1;
     }
-    ioh->read_poll_enabled = enable;
+
+    ioh->read_poll_enabled = false;
+    if (enable && ioh->fd_read_poll) {
+        ioh->read_poll_enabled = true;
+    }
 
     return 0;
 }
@@ -1066,7 +1070,11 @@ int set_read_fd_action(int fd, bool enable)
     if (!ioh) {
         return -1;
     }
-    ioh->read_enabled = enable;
+
+    ioh->read_enabled = false;
+    if (enable && ioh->fd_read) {
+        ioh->read_enabled = true;
+    }
 
     return 0;
 }
@@ -1083,7 +1091,11 @@ int set_write_fd_action(int fd, bool enable)
     if (!ioh) {
         return -1;
     }
-    ioh->write_enabled = enable;
+
+    ioh->write_enabled = false;
+    if (enable && ioh->fd_write) {
+        ioh->write_enabled = true;
+    }
 
     return 0;
 }
@@ -1421,14 +1433,13 @@ void main_loop_wait(int nonblocking)
     QLIST_FOREACH(ioh, &io_handlers, next) {
         if (ioh->deleted)
             continue;
-        if (ioh->fd_read && ioh->read_enabled &&
-            (!ioh->fd_read_poll ||
-             (!ioh->read_poll_enabled || ioh->fd_read_poll(ioh->opaque) != 0))) {
+        if (ioh->read_enabled &&
+            (!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 && ioh->write_enabled) {
+        if (ioh->write_enabled) {
             FD_SET(ioh->fd, &wfds);
             if (ioh->fd > nfds)
                 nfds = ioh->fd;
@@ -1447,10 +1458,10 @@ void main_loop_wait(int nonblocking)
         IOHandlerRecord *pioh;
 
         QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
-            if (!ioh->deleted && ioh->fd_read && FD_ISSET(ioh->fd, &rfds)) {
+            if (!ioh->deleted && ioh->read_enabled && FD_ISSET(ioh->fd, &rfds)) {
                 ioh->fd_read(ioh->opaque);
             }
-            if (!ioh->deleted && ioh->fd_write && FD_ISSET(ioh->fd, &wfds)) {
+            if (!ioh->deleted && ioh->write_enabled && FD_ISSET(ioh->fd, &wfds)) {
                 ioh->fd_write(ioh->opaque);
             }
 
Signature · 1 line
1.7.3.4
author
#5 lore raw

PATCH v2 5/5 iohandlers: Add IOHandlerOps struct

In reply to Amit Shah

Collect all the handlers in a IOHandlerOps struct instead of being passed one at a time to each function.

---
 qemu-char.h |    7 ++-----
 vl.c        |   56 ++++++++++++++++++++++++++++++++++++--------------------
 2 files changed, 38 insertions(+), 25 deletions(-)
qemu-char.h +2 -5
index e88a108..ebf9cd1 100644
@@ -108,12 +108,9 @@ QString *qemu_chr_mem_to_qs(CharDriverState *chr);
 size_t qemu_chr_mem_osize(const CharDriverState *chr);
 
 /* async I/O support */
+typedef struct IOHandlerOps IOHandlerOps;
 
-int assign_fd_handlers(int fd,
-                       IOCanReadHandler *fd_read_poll,
-                       IOHandler *fd_read,
-                       IOHandler *fd_write,
-                       void *opaque);
+int assign_fd_handlers(int fd, const IOHandlerOps *ops, 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);
vl.c +36 -20

136 lines not on this page · raw message

Signature · 1 line
1.7.3.4
Acked-by on the v2 series
#6 lore raw
In reply to Amit Shah
Quoted 13 lines · Amit Shah

This patchset adds new interfaces to work with iohandlers. It adds:

int assign_fd_handlers(int fd, IOHandlerOps *ops, void *opaque)
-- Specify io handlers for an fd
int remove_fd_handlers(int fd)
-- Remove fd handlers for fd (mark ioh for deletion)
int set_read_poll_fd_action(int fd, bool enable)
-- Enable or disable the fd_read_poll fd handler
int set_read_fd_action(int fd, bool enable)
-- Enable or disable the fd_read fd handler
int set_write_fd_action(int fd, bool enable)
-- Enable or disable the fd_read fd handler

Quoted 7 lines

A new struct, IOHandlerOps, is added, to collect all the ops together instead of passing individual ones to functions.

The older function, qemu_set_fd_handler2(), is now a wrapper to assign_fd_handlers() and can be deprecated by converting the existing usage to assign_fd_handlers().

Looks good now.

cheers,
Gerd

#7 lore raw
In reply to Amit Shah
Quoted 15 lines · Amit Shah

Hi,

This patchset adds new interfaces to work with iohandlers. It adds:

int assign_fd_handlers(int fd, IOHandlerOps *ops, void *opaque)
-- Specify io handlers for an fd
int remove_fd_handlers(int fd)
-- Remove fd handlers for fd (mark ioh for deletion)
int set_read_poll_fd_action(int fd, bool enable)
-- Enable or disable the fd_read_poll fd handler
int set_read_fd_action(int fd, bool enable)
-- Enable or disable the fd_read fd handler
int set_write_fd_action(int fd, bool enable)
-- Enable or disable the fd_read fd handler

Quoted 3 lines

A new struct, IOHandlerOps, is added, to collect all the ops together instead of passing individual ones to functions.

Instead of inventing new interfaces, I think we should steal^Wlearn from established interfaces. Both libevent and glib have interfaces that essentially boil down to:

handle add_fd_event(loop, fd, ConditionMask, callback, opaque) remove_event(loop, handle)

I think that's what we should move to. All the stuff in our current loop around allowing suppressing of read events is terrible as it forces the main loop to poll. That makes it impossible to use other main loops because it's completely unusual.

Regards,

Anthony Liguori

Quoted 23 lines

The older function, qemu_set_fd_handler2(), is now a wrapper to assign_fd_handlers() and can be deprecated by converting the existing usage to assign_fd_handlers().

v2: Address comments from Gerd:
- add comments to new interfaces
- enable all specified handlers by default in assign_fd_handlers()
- Add comments for TODO items on deprecation of older interfaces.

Please apply.

Amit Shah (5):
iohandlers: Avoid code duplication
iohandlers: Introduce assign_fd_handlers() and remove_fd_handlers
iohandlers: Allow each iohandler to be enabled/disabled individually
iohandlers: Enable an iohandler only if the associated handler exists
iohandlers: Add IOHandlerOps struct

  qemu-char.h |    7 ++
  vl.c        |  197 ++++++++++++++++++++++++++++++++++++++++++++++++----------
  2 files changed, 170 insertions(+), 34 deletions(-)
author
#8 lore raw
Quoted 16 lines · Anthony Liguori
Quoted 15 lines · Amit Shah

Hi,

This patchset adds new interfaces to work with iohandlers. It adds:

int assign_fd_handlers(int fd, IOHandlerOps *ops, void *opaque)
-- Specify io handlers for an fd
int remove_fd_handlers(int fd)
-- Remove fd handlers for fd (mark ioh for deletion)
int set_read_poll_fd_action(int fd, bool enable)
-- Enable or disable the fd_read_poll fd handler
int set_read_fd_action(int fd, bool enable)
-- Enable or disable the fd_read fd handler
int set_write_fd_action(int fd, bool enable)
-- Enable or disable the fd_read fd handler

Quoted 5 lines
Quoted 2 lines

A new struct, IOHandlerOps, is added, to collect all the ops together instead of passing individual ones to functions.

Instead of inventing new interfaces, I think we should steal^Wlearn from established interfaces.

Agreed.

I do also think it'll be worthwhile pulling in one of the libraries to reduce the amount of qemu-specific code we have in the other cases as well.

Quoted 5 lines

Both libevent and glib have interfaces
that essentially boil down to:

handle add_fd_event(loop, fd, ConditionMask, callback, opaque) remove_event(loop, handle)

This is quite similar to the Linux polling API.

I don't know what the 'loop' parameter would do, though.

Quoted 4 lines

I think that's what we should move to. All the stuff in our current loop around allowing suppressing of read events is terrible as it forces the main loop to poll. That makes it impossible to use other main loops because it's completely unusual.

While setting new APIs possibly requires more discussion if we want to do it once and right, since it's also an internal API, we can keep evolving them.

So what's the accepted course of action now - take this new API, rework it to look like that of some other library's and then rework the main_loop, keep developing something in parallel till it satisfies everyone, etc.?

Also -- this patchset was prompted by a bug in qemu chardevs that freezes guests if they write faster than the chardevs can consume. What should the strategy on fixing that bug be?

Amit

#9 lore raw
Quoted 35 lines · Amit Shah
Quoted 17 lines · Anthony Liguori
Quoted 15 lines · Amit Shah

Hi,

This patchset adds new interfaces to work with iohandlers. It adds:

int assign_fd_handlers(int fd, IOHandlerOps *ops, void *opaque)
-- Specify io handlers for an fd
int remove_fd_handlers(int fd)
-- Remove fd handlers for fd (mark ioh for deletion)
int set_read_poll_fd_action(int fd, bool enable)
-- Enable or disable the fd_read_poll fd handler
int set_read_fd_action(int fd, bool enable)
-- Enable or disable the fd_read fd handler
int set_write_fd_action(int fd, bool enable)
-- Enable or disable the fd_read fd handler

Quoted 6 lines
Quoted 3 lines

A new struct, IOHandlerOps, is added, to collect all the ops together instead of passing individual ones to functions.

Instead of inventing new interfaces, I think we should steal^Wlearn from established interfaces.

Agreed.

I do also think it'll be worthwhile pulling in one of the libraries to reduce the amount of qemu-specific code we have in the other cases as well.

Quoted 3 lines

Both libevent and glib have interfaces
that essentially boil down to:

Quoted 7 lines
Quoted 3 lines

handle add_fd_event(loop, fd, ConditionMask, callback, opaque) remove_event(loop, handle)

This is quite similar to the Linux polling API.

I don't know what the 'loop' parameter would do, though.

It's to allow for multiple I/O threads. There's one loop context per thread and you can register events specifically to a thread.

Quoted 9 lines
Quoted 5 lines

I think that's what we should move to. All the stuff in our current loop around allowing suppressing of read events is terrible as it forces the main loop to poll. That makes it impossible to use other main loops because it's completely unusual.

While setting new APIs possibly requires more discussion if we want to do it once and right, since it's also an internal API, we can keep evolving them.

Yeah, but we should avoid unnecessary churn. If we're going to do a major refactoring of an internal API, we should try to make the refactoring make sense as much as possible.

Eliminating the polling callbacks has been on the TODO for years now. I don't think it make sense to refactor the API without at least doing this.

Quoted 9 lines

So what's the accepted course of action now - take this new API, rework it to look like that of some other library's and then rework the main_loop, keep developing something in parallel till it satisfies everyone, etc.?

Also -- this patchset was prompted by a bug in qemu chardevs that freezes guests if they write faster than the chardevs can consume. What should the strategy on fixing that bug be?

Fix the loop API such that we're not just fixing one bug but that we can address a bunch of other bugs that are out there.

Regards,

Anthony Liguori

Quoted 2 lines

Amit

#10 lore raw
Quoted 31 lines · Anthony Liguori
Quoted 30 lines · Amit Shah
Quoted 16 lines · Anthony Liguori
Quoted 15 lines · Amit Shah

Hi,

This patchset adds new interfaces to work with iohandlers. It adds:

int assign_fd_handlers(int fd, IOHandlerOps *ops, void *opaque) -- Specify io handlers for an fd
int remove_fd_handlers(int fd)
-- Remove fd handlers for fd (mark ioh for deletion) int set_read_poll_fd_action(int fd, bool enable)
-- Enable or disable the fd_read_poll fd handler
int set_read_fd_action(int fd, bool enable)
-- Enable or disable the fd_read fd handler
int set_write_fd_action(int fd, bool enable)
-- Enable or disable the fd_read fd handler

Quoted 4 lines
Quoted 2 lines

A new struct, IOHandlerOps, is added, to collect all the ops together instead of passing individual ones to functions.

Instead of inventing new interfaces, I think we should steal^Wlearn from established interfaces.

Agreed.

I do also think it'll be worthwhile pulling in one of the libraries to reduce the amount of qemu-specific code we have in the other cases as well.

Quoted 3 lines

Both libevent and glib have interfaces
that essentially boil down to:

Quoted 9 lines
Quoted 5 lines
Quoted 2 lines

handle add_fd_event(loop, fd, ConditionMask, callback, opaque) remove_event(loop, handle)

This is quite similar to the Linux polling API.

I don't know what the 'loop' parameter would do, though.

It's to allow for multiple I/O threads. There's one loop context per thread and you can register events specifically to a thread.

In case you're interested, to make the fd handler stuff accessible to qemu tools I have some code that moves these interfaces into backend functions in another file, qemu-ioh.c, where the IOHandlerRecord list is supplied by the caller to track state.

This would extend naturally to threads by just having them maintain their own IOHandlerRecord lists as well, and stuff that is meant to interact with vl.c:io_handlers can be done via wrapper functions for backward-compatibility.

If that seems like a reasonable approach maybe you could leverage these patches remove some of the scope for your proposed changes:

http://repo.or.cz/w/qemu/mdroth.git/patch/56a391e85124382ddcbdb64dcc05aadfc965f000

http://repo.or.cz/w/qemu/mdroth.git/patch/f4428e2cdf5da231bb30ce1ed9eb5ce567a9dc55

author
#11 lore raw
In reply to Anthony Liguori
Quoted 6 lines · Anthony Liguori
Quoted 3 lines

Also -- this patchset was prompted by a bug in qemu chardevs that freezes guests if they write faster than the chardevs can consume. What should the strategy on fixing that bug be?

Fix the loop API such that we're not just fixing one bug but that we can address a bunch of other bugs that are out there.

But what's the right fix?

* Pull in glib or some other library
* Mimic API from some other library

Amit

#12 lore raw
Quoted 6 lines · Amit Shah
Quoted 4 lines · Anthony Liguori
Quoted 4 lines

Also -- this patchset was prompted by a bug in qemu chardevs that freezes guests if they write faster than the chardevs can consume. What should the strategy on fixing that bug be?

Quoted 8 lines
Quoted 3 lines

Fix the loop API such that we're not just fixing one bug but that we can address a bunch of other bugs that are out there.

But what's the right fix?

* Pull in glib or some other library
* Mimic API from some other library

1) get rid of poll callbacks

These kill us right now because it makes it impossible to use poll/epoll or to use some other API.

2) switch to a single callback and an event mask

We can't do this without (1) because the can_read callback takes a different signature. But read/write callbacks can be a single callback (but it needs to be changed to accept an event mask).

3) allow the event mask to be changed via a new API

This gives the functionality you're looking for while making a huge improvement to the internal API.

A quick python/perl script can probably do 90% of the work here.

Regards,

Anthony Liguori

Quoted 2 lines

Amit