| Message ID | 1345564351-14693-2-git-send-email-stefanha@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
In net_socket_update_fd_handler() shouldnt you call qemu_notify_event() if any of the handlers have changed from NULL to non-NULL ? or else it might take a while before the change takes effect. regards ronnie sahlberg On Wed, Aug 22, 2012 at 1:52 AM, Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote: > The net/socket.c net client is not truly asynchronous. This patch > borrows the qemu_set_fd_handler2() code from net/tap.c as the basis for > proper asynchronous send/receive. > > Only read packets from the socket when the peer is able to receive. > This avoids needless queuing. > > Later patches implement asynchronous send. > > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > --- > net/socket.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 52 insertions(+), 6 deletions(-) > > diff --git a/net/socket.c b/net/socket.c > index c172c24..54e32f0 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -42,9 +42,51 @@ typedef struct NetSocketState { > unsigned int packet_len; > uint8_t buf[4096]; > struct sockaddr_in dgram_dst; /* contains inet host and port destination iff connectionless (SOCK_DGRAM) */ > + IOHandler *send_fn; /* differs between SOCK_STREAM/SOCK_DGRAM */ > + bool read_poll; /* waiting to receive data? */ > + bool write_poll; /* waiting to transmit data? */ > } NetSocketState; > > static void net_socket_accept(void *opaque); > +static void net_socket_writable(void *opaque); > + > +/* Only read packets from socket when peer can receive them */ > +static int net_socket_can_send(void *opaque) > +{ > + NetSocketState *s = opaque; > + > + return qemu_can_send_packet(&s->nc); > +} > + > +static void net_socket_update_fd_handler(NetSocketState *s) > +{ > + qemu_set_fd_handler2(s->fd, > + s->read_poll ? net_socket_can_send : NULL, > + s->read_poll ? s->send_fn : NULL, > + s->write_poll ? net_socket_writable : NULL, > + s); > +} > + > +static void net_socket_read_poll(NetSocketState *s, bool enable) > +{ > + s->read_poll = enable; > + net_socket_update_fd_handler(s); > +} > + > +static void net_socket_write_poll(NetSocketState *s, bool enable) > +{ > + s->write_poll = enable; > + net_socket_update_fd_handler(s); > +} > + > +static void net_socket_writable(void *opaque) > +{ > + NetSocketState *s = opaque; > + > + net_socket_write_poll(s, false); > + > + qemu_flush_queued_packets(&s->nc); > +} > > /* XXX: we consider we can send the whole packet without blocking */ > static ssize_t net_socket_receive(NetClientState *nc, const uint8_t *buf, size_t size) > @@ -81,7 +123,8 @@ static void net_socket_send(void *opaque) > } else if (size == 0) { > /* end of connection */ > eoc: > - qemu_set_fd_handler(s->fd, NULL, NULL, NULL); > + net_socket_read_poll(s, false); > + net_socket_write_poll(s, false); > if (s->listen_fd != -1) { > qemu_set_fd_handler(s->listen_fd, net_socket_accept, NULL, s); > } > @@ -152,7 +195,8 @@ static void net_socket_send_dgram(void *opaque) > return; > if (size == 0) { > /* end of connection */ > - qemu_set_fd_handler(s->fd, NULL, NULL, NULL); > + net_socket_read_poll(s, false); > + net_socket_write_poll(s, false); > return; > } > qemu_send_packet(&s->nc, s->buf, size); > @@ -243,7 +287,8 @@ static void net_socket_cleanup(NetClientState *nc) > { > NetSocketState *s = DO_UPCAST(NetSocketState, nc, nc); > if (s->fd != -1) { > - qemu_set_fd_handler(s->fd, NULL, NULL, NULL); > + net_socket_read_poll(s, false); > + net_socket_write_poll(s, false); > close(s->fd); > s->fd = -1; > } > @@ -314,8 +359,8 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer, > > s->fd = fd; > s->listen_fd = -1; > - > - qemu_set_fd_handler(s->fd, net_socket_send_dgram, NULL, s); > + s->send_fn = net_socket_send_dgram; > + net_socket_read_poll(s, true); > > /* mcast: save bound address as dst */ > if (is_connected) { > @@ -332,7 +377,8 @@ err: > static void net_socket_connect(void *opaque) > { > NetSocketState *s = opaque; > - qemu_set_fd_handler(s->fd, net_socket_send, NULL, s); > + s->send_fn = net_socket_send; > + net_socket_read_poll(s, true); > } > > static NetClientInfo net_socket_info = { > -- > 1.7.10.4 > >
Il 21/08/2012 18:39, ronnie sahlberg ha scritto: > In > net_socket_update_fd_handler() > > shouldnt you call qemu_notify_event() if any of the handlers have > changed from NULL to non-NULL ? > or else it might take a while before the change takes effect. Why haven't we applied yet the patch to do that unconditionally? http://permalink.gmane.org/gmane.comp.emulators.qemu/162828 Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 21/08/2012 18:39, ronnie sahlberg ha scritto: >> In >> net_socket_update_fd_handler() >> >> shouldnt you call qemu_notify_event() if any of the handlers have >> changed from NULL to non-NULL ? >> or else it might take a while before the change takes effect. > > Why haven't we applied yet the patch to do that unconditionally? > > http://permalink.gmane.org/gmane.comp.emulators.qemu/162828 Can you ping the actual patch... Gmane seems to be down right now. Regards, Anthony Liguori > > Paolo
On Tue, Aug 21, 2012 at 5:39 PM, ronnie sahlberg <ronniesahlberg@gmail.com> wrote: > In > net_socket_update_fd_handler() > > shouldnt you call qemu_notify_event() if any of the handlers have > changed from NULL to non-NULL ? > or else it might take a while before the change takes effect. In this case it's not necessary: net_socket_send()/net_socket_send_dgram()/net_socket_writable() are called as fd handlers and therefore we know the event loop isn't stuck in select(2). Stefan
Il 21/08/2012 21:06, Anthony Liguori ha scritto: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> Il 21/08/2012 18:39, ronnie sahlberg ha scritto: >>> In >>> net_socket_update_fd_handler() >>> >>> shouldnt you call qemu_notify_event() if any of the handlers have >>> changed from NULL to non-NULL ? >>> or else it might take a while before the change takes effect. >> >> Why haven't we applied yet the patch to do that unconditionally? >> >> http://permalink.gmane.org/gmane.comp.emulators.qemu/162828 > > Can you ping the actual patch... Gmane seems to be down right now. David Gibson sent it again a few hours ago ("[PATCH] eventfd: making it thread safe") with my Reviewed-by. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 21/08/2012 21:06, Anthony Liguori ha scritto: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> Il 21/08/2012 18:39, ronnie sahlberg ha scritto: >>>> In >>>> net_socket_update_fd_handler() >>>> >>>> shouldnt you call qemu_notify_event() if any of the handlers have >>>> changed from NULL to non-NULL ? >>>> or else it might take a while before the change takes effect. >>> >>> Why haven't we applied yet the patch to do that unconditionally? >>> >>> http://permalink.gmane.org/gmane.comp.emulators.qemu/162828 >> >> Can you ping the actual patch... Gmane seems to be down right now. > > David Gibson sent it again a few hours ago ("[PATCH] eventfd: making it > thread safe") with my Reviewed-by. Yup, I've applied it. Thanks. Regards, Anthony Liguori > > Paolo
diff --git a/net/socket.c b/net/socket.c index c172c24..54e32f0 100644 --- a/net/socket.c +++ b/net/socket.c @@ -42,9 +42,51 @@ typedef struct NetSocketState { unsigned int packet_len; uint8_t buf[4096]; struct sockaddr_in dgram_dst; /* contains inet host and port destination iff connectionless (SOCK_DGRAM) */ + IOHandler *send_fn; /* differs between SOCK_STREAM/SOCK_DGRAM */ + bool read_poll; /* waiting to receive data? */ + bool write_poll; /* waiting to transmit data? */ } NetSocketState; static void net_socket_accept(void *opaque); +static void net_socket_writable(void *opaque); + +/* Only read packets from socket when peer can receive them */ +static int net_socket_can_send(void *opaque) +{ + NetSocketState *s = opaque; + + return qemu_can_send_packet(&s->nc); +} + +static void net_socket_update_fd_handler(NetSocketState *s) +{ + qemu_set_fd_handler2(s->fd, + s->read_poll ? net_socket_can_send : NULL, + s->read_poll ? s->send_fn : NULL, + s->write_poll ? net_socket_writable : NULL, + s); +} + +static void net_socket_read_poll(NetSocketState *s, bool enable) +{ + s->read_poll = enable; + net_socket_update_fd_handler(s); +} + +static void net_socket_write_poll(NetSocketState *s, bool enable) +{ + s->write_poll = enable; + net_socket_update_fd_handler(s); +} + +static void net_socket_writable(void *opaque) +{ + NetSocketState *s = opaque; + + net_socket_write_poll(s, false); + + qemu_flush_queued_packets(&s->nc); +} /* XXX: we consider we can send the whole packet without blocking */ static ssize_t net_socket_receive(NetClientState *nc, const uint8_t *buf, size_t size) @@ -81,7 +123,8 @@ static void net_socket_send(void *opaque) } else if (size == 0) { /* end of connection */ eoc: - qemu_set_fd_handler(s->fd, NULL, NULL, NULL); + net_socket_read_poll(s, false); + net_socket_write_poll(s, false); if (s->listen_fd != -1) { qemu_set_fd_handler(s->listen_fd, net_socket_accept, NULL, s); } @@ -152,7 +195,8 @@ static void net_socket_send_dgram(void *opaque) return; if (size == 0) { /* end of connection */ - qemu_set_fd_handler(s->fd, NULL, NULL, NULL); + net_socket_read_poll(s, false); + net_socket_write_poll(s, false); return; } qemu_send_packet(&s->nc, s->buf, size); @@ -243,7 +287,8 @@ static void net_socket_cleanup(NetClientState *nc) { NetSocketState *s = DO_UPCAST(NetSocketState, nc, nc); if (s->fd != -1) { - qemu_set_fd_handler(s->fd, NULL, NULL, NULL); + net_socket_read_poll(s, false); + net_socket_write_poll(s, false); close(s->fd); s->fd = -1; } @@ -314,8 +359,8 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer, s->fd = fd; s->listen_fd = -1; - - qemu_set_fd_handler(s->fd, net_socket_send_dgram, NULL, s); + s->send_fn = net_socket_send_dgram; + net_socket_read_poll(s, true); /* mcast: save bound address as dst */ if (is_connected) { @@ -332,7 +377,8 @@ err: static void net_socket_connect(void *opaque) { NetSocketState *s = opaque; - qemu_set_fd_handler(s->fd, net_socket_send, NULL, s); + s->send_fn = net_socket_send; + net_socket_read_poll(s, true); } static NetClientInfo net_socket_info = {
The net/socket.c net client is not truly asynchronous. This patch borrows the qemu_set_fd_handler2() code from net/tap.c as the basis for proper asynchronous send/receive. Only read packets from the socket when the peer is able to receive. This avoids needless queuing. Later patches implement asynchronous send. Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> --- net/socket.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 52 insertions(+), 6 deletions(-)