diff mbox

[v3,03/13] l2tpv3: Drop l2tpv3_can_send

Message ID 1432032670-15124-4-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng May 19, 2015, 10:51 a.m. UTC
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(-)

Comments

Stefan Hajnoczi May 19, 2015, 2:48 p.m. UTC | #1
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
Fam Zheng May 26, 2015, 6:52 a.m. UTC | #2
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
Stefan Hajnoczi May 26, 2015, 9:07 a.m. UTC | #3
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 mbox

Patch

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);