| Message ID | 1bdf0a5a5de06cfb332ac17c439ef79cabb835db.1294923288.git.amit.shah@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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;
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(-)