| Message ID | 1432032670-15124-4-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, May 19, 2015 at 10:51:00AM +0000, Fam Zheng wrote: > This callback is called by main loop before polling s->fd, if it returns > false, the fd will not be polled in this iteration. > > This is redundant with checks inside read callback. After this patch, > the data will be copied from s->fd to s->msgvec when it arrives. If the > device can't receive, it will be queued to incoming_queue, and when the > device status changes, this queue will be flushed. This doesn't work because s->msgvec can fill up when qemu_can_send_packet() returns false. At that point we burn 100% CPU because the file descriptor is still being monitored. It really is necessary to stop monitoring the file descriptor until we can send again. Or the code needs to be changed to drop packets in QEMU (currently it's the host kernel where packets may be dropped). Stefan
On Tue, 05/19 15:48, Stefan Hajnoczi wrote: > On Tue, May 19, 2015 at 10:51:00AM +0000, Fam Zheng wrote: > > This callback is called by main loop before polling s->fd, if it returns > > false, the fd will not be polled in this iteration. > > > > This is redundant with checks inside read callback. After this patch, > > the data will be copied from s->fd to s->msgvec when it arrives. If the > > device can't receive, it will be queued to incoming_queue, and when the > > device status changes, this queue will be flushed. > > This doesn't work because s->msgvec can fill up when > qemu_can_send_packet() returns false. At that point we burn 100% CPU > because the file descriptor is still being monitored. If qemu_can_send_packet returns false, we do stop monitoring the fd. In net_l2tpv3_process_queue: size = qemu_send_packet_async( &s->nc, vec->iov_base, data_size, l2tpv3_send_completed ); if (size == 0) { l2tpv3_read_poll(s, false); } The packet is queued and size is 0, so the read poll will be disabled until it's flushed. What am I missing? Fam
On Tue, May 26, 2015 at 02:52:48PM +0800, Fam Zheng wrote: > On Tue, 05/19 15:48, Stefan Hajnoczi wrote: > > On Tue, May 19, 2015 at 10:51:00AM +0000, Fam Zheng wrote: > > > This callback is called by main loop before polling s->fd, if it returns > > > false, the fd will not be polled in this iteration. > > > > > > This is redundant with checks inside read callback. After this patch, > > > the data will be copied from s->fd to s->msgvec when it arrives. If the > > > device can't receive, it will be queued to incoming_queue, and when the > > > device status changes, this queue will be flushed. > > > > This doesn't work because s->msgvec can fill up when > > qemu_can_send_packet() returns false. At that point we burn 100% CPU > > because the file descriptor is still being monitored. > > If qemu_can_send_packet returns false, we do stop monitoring the fd. In > net_l2tpv3_process_queue: > > size = qemu_send_packet_async( > &s->nc, > vec->iov_base, > data_size, > l2tpv3_send_completed > ); > if (size == 0) { > l2tpv3_read_poll(s, false); > } > > The packet is queued and size is 0, so the read poll will be disabled until > it's flushed. > > What am I missing? I think you are right. I was looking at the while loop qemu_can_send_packet(): } while ( (s->queue_depth > 0) && qemu_can_send_packet(&s->nc) && ((size > 0) || bad_read) ); and missed the portion you quoted. Stefan
diff --git a/net/l2tpv3.c b/net/l2tpv3.c index 8c598b0..8eed06b 100644 --- a/net/l2tpv3.c +++ b/net/l2tpv3.c @@ -133,14 +133,12 @@ typedef struct NetL2TPV3State { } NetL2TPV3State; -static int l2tpv3_can_send(void *opaque); static void net_l2tpv3_send(void *opaque); static void l2tpv3_writable(void *opaque); static void l2tpv3_update_fd_handler(NetL2TPV3State *s) { - qemu_set_fd_handler2(s->fd, - s->read_poll ? l2tpv3_can_send : NULL, + qemu_set_fd_handler2(s->fd, NULL, s->read_poll ? net_l2tpv3_send : NULL, s->write_poll ? l2tpv3_writable : NULL, s); @@ -169,13 +167,6 @@ static void l2tpv3_writable(void *opaque) qemu_flush_queued_packets(&s->nc); } -static int l2tpv3_can_send(void *opaque) -{ - NetL2TPV3State *s = opaque; - - return qemu_can_send_packet(&s->nc); -} - static void l2tpv3_send_completed(NetClientState *nc, ssize_t len) { NetL2TPV3State *s = DO_UPCAST(NetL2TPV3State, nc, nc);
This callback is called by main loop before polling s->fd, if it returns false, the fd will not be polled in this iteration. This is redundant with checks inside read callback. After this patch, the data will be copied from s->fd to s->msgvec when it arrives. If the device can't receive, it will be queued to incoming_queue, and when the device status changes, this queue will be flushed. Signed-off-by: Fam Zheng <famz@redhat.com> --- net/l2tpv3.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)