From patchwork Thu Jan 31 10:53:59 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefan Hajnoczi X-Patchwork-Id: 217164 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id EBF382C008E for ; Thu, 31 Jan 2013 22:39:28 +1100 (EST) Received: from localhost ([::1]:51844 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U0rnE-0000wf-4f for incoming@patchwork.ozlabs.org; Thu, 31 Jan 2013 05:54:56 -0500 Received: from eggs.gnu.org ([208.118.235.92]:38143) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U0rmq-0000Sa-Fi for qemu-devel@nongnu.org; Thu, 31 Jan 2013 05:54:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1U0rmi-000457-L9 for qemu-devel@nongnu.org; Thu, 31 Jan 2013 05:54:32 -0500 Received: from mx1.redhat.com ([209.132.183.28]:7359) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U0rmi-000450-Ah for qemu-devel@nongnu.org; Thu, 31 Jan 2013 05:54:24 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r0VAsM1Q022436 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 31 Jan 2013 05:54:23 -0500 Received: from localhost (ovpn-112-28.ams2.redhat.com [10.36.112.28]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r0VAsLvF022768; Thu, 31 Jan 2013 05:54:22 -0500 From: Stefan Hajnoczi To: Date: Thu, 31 Jan 2013 11:53:59 +0100 Message-Id: <1359629644-21920-7-git-send-email-stefanha@redhat.com> In-Reply-To: <1359629644-21920-1-git-send-email-stefanha@redhat.com> References: <1359629644-21920-1-git-send-email-stefanha@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: Anthony Liguori , Jan Kiszka , Fabien Chouteau , Stefan Hajnoczi , Paolo Bonzini , Amos Kong Subject: [Qemu-devel] [PATCH 06/11] slirp: switch to Poller X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Slirp uses rfds/wfds/xfds more extensively than other QEMU components. The rarely-used out-of-band TCP data feature is used. That means we need the full table of select(2) to g_poll(3) events: rfds -> G_IO_IN | G_IO_HUP | G_IO_ERR wfds -> G_IO_OUT | G_IO_ERR xfds -> G_IO_PRI I came up with this table by looking at Linux fs/select.c which maps select(2) to poll(2) internally. Another detail to watch out for are the global variables that reference rfds/wfds/xfds during slirp_select_poll(). sofcantrcvmore() and sofcantsendmore() use these globals to clear fd_set bits. When sofcantrcvmore() is called, the wfds bit is cleared so that the write handler will no longer be run for this iteration of the event loop. This actually seems buggy to me since TCP connections can be half-closed and we'd still want to handle data in half-duplex fashion. I think the real intention is to avoid running the read/write handler when the socket has been fully closed. This is indicated with the SS_NOFDREF state bit so we now check for it before invoking the TCP write handler. Note that UDP/ICMP code paths don't care because they are connectionless. Note that slirp/ has a lot of tabs and sometimes mixed tabs with spaces. I followed the style of the surrounding code. Signed-off-by: Stefan Hajnoczi --- main-loop.c | 4 +- slirp/libslirp.h | 8 ++-- slirp/main.h | 1 - slirp/slirp.c | 113 ++++++++++++++++++++++++++++--------------------------- slirp/socket.c | 9 ----- slirp/socket.h | 2 + stubs/slirp.c | 6 +-- 7 files changed, 68 insertions(+), 75 deletions(-) diff --git a/main-loop.c b/main-loop.c index c48c8f5..3ecefb6 100644 --- a/main-loop.c +++ b/main-loop.c @@ -459,13 +459,13 @@ int main_loop_wait(int nonblocking) #ifdef CONFIG_SLIRP slirp_update_timeout(&timeout); - slirp_select_fill(&nfds, &rfds, &wfds, &xfds); + slirp_poller_fill(&poller); #endif qemu_iohandler_fill(&nfds, &rfds, &wfds, &xfds); ret = os_host_main_loop_wait(timeout); qemu_iohandler_poll(&rfds, &wfds, &xfds, ret); #ifdef CONFIG_SLIRP - slirp_select_poll(&rfds, &wfds, &xfds, (ret < 0)); + slirp_poller_poll(&poller, (ret < 0)); #endif qemu_run_all_timers(); diff --git a/slirp/libslirp.h b/slirp/libslirp.h index 49609c2..839813d 100644 --- a/slirp/libslirp.h +++ b/slirp/libslirp.h @@ -2,6 +2,8 @@ #define _LIBSLIRP_H #include "qemu-common.h" +#include "qemu/main-loop.h" +#include "qemu/poller.h" struct Slirp; typedef struct Slirp Slirp; @@ -17,11 +19,9 @@ Slirp *slirp_init(int restricted, struct in_addr vnetwork, void slirp_cleanup(Slirp *slirp); void slirp_update_timeout(uint32_t *timeout); -void slirp_select_fill(int *pnfds, - fd_set *readfds, fd_set *writefds, fd_set *xfds); +void slirp_poller_fill(Poller *poller); -void slirp_select_poll(fd_set *readfds, fd_set *writefds, fd_set *xfds, - int select_error); +void slirp_poller_poll(Poller *poller, int select_error); void slirp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len); diff --git a/slirp/main.h b/slirp/main.h index 66e4f92..f2e58cf 100644 --- a/slirp/main.h +++ b/slirp/main.h @@ -31,7 +31,6 @@ extern int ctty_closed; extern char *slirp_tty; extern char *exec_shell; extern u_int curtime; -extern fd_set *global_readfds, *global_writefds, *global_xfds; extern struct in_addr loopback_addr; extern unsigned long loopback_mask; extern char *username; diff --git a/slirp/slirp.c b/slirp/slirp.c index 0e6e232..175b089 100644 --- a/slirp/slirp.c +++ b/slirp/slirp.c @@ -39,9 +39,6 @@ static const uint8_t special_ethaddr[ETH_ALEN] = { static const uint8_t zero_ethaddr[ETH_ALEN] = { 0, 0, 0, 0, 0, 0 }; -/* XXX: suppress those select globals */ -fd_set *global_readfds, *global_writefds, *global_xfds; - u_int curtime; static u_int time_fasttimo, last_slowtimo; static int do_slowtimo; @@ -261,7 +258,6 @@ void slirp_cleanup(Slirp *slirp) #define CONN_CANFSEND(so) (((so)->so_state & (SS_FCANTSENDMORE|SS_ISFCONNECTED)) == SS_ISFCONNECTED) #define CONN_CANFRCV(so) (((so)->so_state & (SS_FCANTRCVMORE|SS_ISFCONNECTED)) == SS_ISFCONNECTED) -#define UPD_NFDS(x) if (nfds < (x)) nfds = (x) void slirp_update_timeout(uint32_t *timeout) { @@ -270,23 +266,15 @@ void slirp_update_timeout(uint32_t *timeout) } } -void slirp_select_fill(int *pnfds, - fd_set *readfds, fd_set *writefds, fd_set *xfds) +void slirp_poller_fill(Poller *poller) { Slirp *slirp; struct socket *so, *so_next; - int nfds; if (QTAILQ_EMPTY(&slirp_instances)) { return; } - /* fail safe */ - global_readfds = NULL; - global_writefds = NULL; - global_xfds = NULL; - - nfds = *pnfds; /* * First, TCP sockets */ @@ -302,8 +290,12 @@ void slirp_select_fill(int *pnfds, for (so = slirp->tcb.so_next; so != &slirp->tcb; so = so_next) { + int events = 0; + so_next = so->so_next; + so->poller_idx = -1; + /* * See if we need a tcp_fasttimo */ @@ -321,8 +313,8 @@ void slirp_select_fill(int *pnfds, * Set for reading sockets which are accepting */ if (so->so_state & SS_FACCEPTCONN) { - FD_SET(so->s, readfds); - UPD_NFDS(so->s); + so->poller_idx = poller_add_fd(poller, + so->s, G_IO_IN | G_IO_HUP | G_IO_ERR); continue; } @@ -330,8 +322,8 @@ void slirp_select_fill(int *pnfds, * Set for writing sockets which are connecting */ if (so->so_state & SS_ISFCONNECTING) { - FD_SET(so->s, writefds); - UPD_NFDS(so->s); + so->poller_idx = poller_add_fd(poller, so->s, + G_IO_OUT | G_IO_ERR); continue; } @@ -340,8 +332,7 @@ void slirp_select_fill(int *pnfds, * we have something to send */ if (CONN_CANFSEND(so) && so->so_rcv.sb_cc) { - FD_SET(so->s, writefds); - UPD_NFDS(so->s); + events |= G_IO_OUT | G_IO_ERR; } /* @@ -349,9 +340,11 @@ void slirp_select_fill(int *pnfds, * receive more, and we have room for it XXX /2 ? */ if (CONN_CANFRCV(so) && (so->so_snd.sb_cc < (so->so_snd.sb_datalen/2))) { - FD_SET(so->s, readfds); - FD_SET(so->s, xfds); - UPD_NFDS(so->s); + events |= G_IO_IN | G_IO_HUP | G_IO_ERR | G_IO_PRI; + } + + if (events) { + so->poller_idx = poller_add_fd(poller, so->s, events); } } @@ -362,6 +355,8 @@ void slirp_select_fill(int *pnfds, so = so_next) { so_next = so->so_next; + so->poller_idx = -1; + /* * See if it's timed out */ @@ -384,8 +379,8 @@ void slirp_select_fill(int *pnfds, * (XXX <= 4 ?) */ if ((so->so_state & SS_ISFCONNECTED) && so->so_queued <= 4) { - FD_SET(so->s, readfds); - UPD_NFDS(so->s); + so->poller_idx = poller_add_fd(poller, so->s, + G_IO_IN | G_IO_HUP | G_IO_ERR); } } @@ -396,6 +391,8 @@ void slirp_select_fill(int *pnfds, so = so_next) { so_next = so->so_next; + so->poller_idx = -1; + /* * See if it's timed out */ @@ -409,17 +406,15 @@ void slirp_select_fill(int *pnfds, } if (so->so_state & SS_ISFCONNECTED) { - FD_SET(so->s, readfds); - UPD_NFDS(so->s); + so->poller_idx = poller_add_fd(poller, so->s, + G_IO_IN | G_IO_HUP | + G_IO_ERR); } } } - - *pnfds = nfds; } -void slirp_select_poll(fd_set *readfds, fd_set *writefds, fd_set *xfds, - int select_error) +void slirp_poller_poll(Poller *poller, int select_error) { Slirp *slirp; struct socket *so, *so_next; @@ -429,10 +424,6 @@ void slirp_select_poll(fd_set *readfds, fd_set *writefds, fd_set *xfds, return; } - global_readfds = readfds; - global_writefds = writefds; - global_xfds = xfds; - curtime = qemu_get_clock_ms(rt_clock); QTAILQ_FOREACH(slirp, &slirp_instances, entry) { @@ -458,26 +449,31 @@ void slirp_select_poll(fd_set *readfds, fd_set *writefds, fd_set *xfds, */ for (so = slirp->tcb.so_next; so != &slirp->tcb; so = so_next) { + int revents; + so_next = so->so_next; - /* - * FD_ISSET is meaningless on these sockets - * (and they can crash the program) - */ - if (so->so_state & SS_NOFDREF || so->s == -1) + revents = 0; + if (so->poller_idx != -1) { + revents = poller_get_revents(poller, so->poller_idx); + } + + if (so->so_state & SS_NOFDREF || so->s == -1) { continue; + } /* * Check for URG data * This will soread as well, so no need to - * test for readfds below if this succeeds + * test for G_IO_IN below if this succeeds */ - if (FD_ISSET(so->s, xfds)) + if (revents & G_IO_PRI) { sorecvoob(so); + } /* * Check sockets for reading */ - else if (FD_ISSET(so->s, readfds)) { + else if (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR)) { /* * Check for incoming connections */ @@ -495,7 +491,8 @@ void slirp_select_poll(fd_set *readfds, fd_set *writefds, fd_set *xfds, /* * Check sockets for writing */ - if (FD_ISSET(so->s, writefds)) { + if (!(so->so_state & SS_NOFDREF) && + (revents & (G_IO_OUT | G_IO_ERR))) { /* * Check for non-blocking, still-connecting sockets */ @@ -576,9 +573,16 @@ void slirp_select_poll(fd_set *readfds, fd_set *writefds, fd_set *xfds, */ for (so = slirp->udb.so_next; so != &slirp->udb; so = so_next) { + int revents; + so_next = so->so_next; - if (so->s != -1 && FD_ISSET(so->s, readfds)) { + revents = 0; + if (so->poller_idx != -1) { + revents = poller_get_revents(poller, so->poller_idx); + } + + if (so->s != -1 && (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR))) { sorecvfrom(so); } } @@ -588,9 +592,17 @@ void slirp_select_poll(fd_set *readfds, fd_set *writefds, fd_set *xfds, */ for (so = slirp->icmp.so_next; so != &slirp->icmp; so = so_next) { - so_next = so->so_next; + int revents; - if (so->s != -1 && FD_ISSET(so->s, readfds)) { + so_next = so->so_next; + + revents = 0; + if (so->poller_idx != -1) { + revents = poller_get_revents(poller, so->poller_idx); + } + + if (so->s != -1 && + (revents & (G_IO_IN | G_IO_HUP | G_IO_ERR))) { icmp_receive(so); } } @@ -598,15 +610,6 @@ void slirp_select_poll(fd_set *readfds, fd_set *writefds, fd_set *xfds, if_start(slirp); } - - /* clear global file descriptor sets. - * these reside on the stack in vl.c - * so they're unusable if we're not in - * slirp_select_fill or slirp_select_poll. - */ - global_readfds = NULL; - global_writefds = NULL; - global_xfds = NULL; } static void arp_input(Slirp *slirp, const uint8_t *pkt, int pkt_len) diff --git a/slirp/socket.c b/slirp/socket.c index 77b0c98..a7ab933 100644 --- a/slirp/socket.c +++ b/slirp/socket.c @@ -680,9 +680,6 @@ sofcantrcvmore(struct socket *so) { if ((so->so_state & SS_NOFDREF) == 0) { shutdown(so->s,0); - if(global_writefds) { - FD_CLR(so->s,global_writefds); - } } so->so_state &= ~(SS_ISFCONNECTING); if (so->so_state & SS_FCANTSENDMORE) { @@ -698,12 +695,6 @@ sofcantsendmore(struct socket *so) { if ((so->so_state & SS_NOFDREF) == 0) { shutdown(so->s,1); /* send FIN to fhost */ - if (global_readfds) { - FD_CLR(so->s,global_readfds); - } - if (global_xfds) { - FD_CLR(so->s,global_xfds); - } } so->so_state &= ~(SS_ISFCONNECTING); if (so->so_state & SS_FCANTRCVMORE) { diff --git a/slirp/socket.h b/slirp/socket.h index 857b0da..dbcbb4e 100644 --- a/slirp/socket.h +++ b/slirp/socket.h @@ -20,6 +20,8 @@ struct socket { int s; /* The actual socket */ + int poller_idx; /* Poller revents index */ + Slirp *slirp; /* managing slirp instance */ /* XXX union these with not-yet-used sbuf params */ diff --git a/stubs/slirp.c b/stubs/slirp.c index 9a3309a..76e3259 100644 --- a/stubs/slirp.c +++ b/stubs/slirp.c @@ -5,13 +5,11 @@ void slirp_update_timeout(uint32_t *timeout) { } -void slirp_select_fill(int *pnfds, fd_set *readfds, - fd_set *writefds, fd_set *xfds) +void slirp_poller_fill(Poller *poller) { } -void slirp_select_poll(fd_set *readfds, fd_set *writefds, - fd_set *xfds, int select_error) +void slirp_poller_poll(Poller *poller, int select_error) { }