[Qemu-devel] [PATCH for-2.4] net-hub: Drop can_receive

[Qemu-devel] [PATCH for-2.4] net-hub: Drop can_receive

[Fam Zheng]

It returns true as long as there is another attached port. This is not
strictly necessary because even if there is only one port (the sender),
net_hub_port_receive could succeed with a NOP. So always deliver the
packets, instead of queuing them.

This fixes the possible hanging issue after net layer changed how
can_read is handled. That is, if net_hub_port_can_receive returned
false, the peer would disable the queue until it's explicitly flushed
(for example, a call to qemu_flush_queued_packets() in net_hub_add_port,
where net_hub_port_can_receive() would become true). This patch avoids
that complication.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 net/hub.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/net/hub.c b/net/hub.c
index 3047f12..65a8e09 100644
--- a/net/hub.c
+++ b/net/hub.c
@@ -87,25 +87,6 @@ static NetHub *net_hub_new(int id)
     return hub;
 }
 
-static int net_hub_port_can_receive(NetClientState *nc)
-{
-    NetHubPort *port;
-    NetHubPort *src_port = DO_UPCAST(NetHubPort, nc, nc);
-    NetHub *hub = src_port->hub;
-
-    QLIST_FOREACH(port, &hub->ports, next) {
-        if (port == src_port) {
-            continue;
-        }
-
-        if (qemu_can_send_packet(&port->nc)) {
-            return 1;
-        }
-    }
-
-    return 0;
-}
-
 static ssize_t net_hub_port_receive(NetClientState *nc,
                                     const uint8_t *buf, size_t len)
 {
@@ -132,7 +113,6 @@ static void net_hub_port_cleanup(NetClientState *nc)
 static NetClientInfo net_hub_port_info = {
     .type = NET_CLIENT_OPTIONS_KIND_HUBPORT,
     .size = sizeof(NetHubPort),
-    .can_receive = net_hub_port_can_receive,
     .receive = net_hub_port_receive,
     .receive_iov = net_hub_port_receive_iov,
     .cleanup = net_hub_port_cleanup,
-- 
2.4.4

Re: [Qemu-devel] [PATCH for-2.4] net-hub: Drop can_receive

[Thomas Huth]

On Tue, 30 Jun 2015 10:29:20 +0800
Fam Zheng <famz@redhat.com> wrote:

> It returns true as long as there is another attached port. This is not
> strictly necessary because even if there is only one port (the sender),
> net_hub_port_receive could succeed with a NOP. So always deliver the
> packets, instead of queuing them.
> 
> This fixes the possible hanging issue after net layer changed how
> can_read is handled. That is, if net_hub_port_can_receive returned
> false, the peer would disable the queue until it's explicitly flushed
> (for example, a call to qemu_flush_queued_packets() in net_hub_add_port,
> where net_hub_port_can_receive() would become true). This patch avoids
> that complication.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  net/hub.c | 20 --------------------
>  1 file changed, 20 deletions(-)
> 
> diff --git a/net/hub.c b/net/hub.c
> index 3047f12..65a8e09 100644
> --- a/net/hub.c
> +++ b/net/hub.c
> @@ -87,25 +87,6 @@ static NetHub *net_hub_new(int id)
>      return hub;
>  }
>  
> -static int net_hub_port_can_receive(NetClientState *nc)
> -{
> -    NetHubPort *port;
> -    NetHubPort *src_port = DO_UPCAST(NetHubPort, nc, nc);
> -    NetHub *hub = src_port->hub;
> -
> -    QLIST_FOREACH(port, &hub->ports, next) {
> -        if (port == src_port) {
> -            continue;
> -        }
> -
> -        if (qemu_can_send_packet(&port->nc)) {
> -            return 1;
> -        }
> -    }
> -
> -    return 0;
> -}
> -
>  static ssize_t net_hub_port_receive(NetClientState *nc,
>                                      const uint8_t *buf, size_t len)
>  {
> @@ -132,7 +113,6 @@ static void net_hub_port_cleanup(NetClientState *nc)
>  static NetClientInfo net_hub_port_info = {
>      .type = NET_CLIENT_OPTIONS_KIND_HUBPORT,
>      .size = sizeof(NetHubPort),
> -    .can_receive = net_hub_port_can_receive,
>      .receive = net_hub_port_receive,
>      .receive_iov = net_hub_port_receive_iov,
>      .cleanup = net_hub_port_cleanup,

Sounds reasonable.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH for-2.4] net-hub: Drop can_receive

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2294 bytes --]

On Tue, Jun 30, 2015 at 10:29:20AM +0800, Fam Zheng wrote:
> It returns true as long as there is another attached port.

No, other ports may also return false from .can_receive().  So this
function can return false when there are multiple ports.

> This is not
> strictly necessary because even if there is only one port (the sender),
> net_hub_port_receive could succeed with a NOP. So always deliver the
> packets, instead of queuing them.
> 
> This fixes the possible hanging issue after net layer changed how
> can_read is handled. That is, if net_hub_port_can_receive returned
> false, the peer would disable the queue until it's explicitly flushed
> (for example, a call to qemu_flush_queued_packets() in net_hub_add_port,
> where net_hub_port_can_receive() would become true). This patch avoids
> that complication.

Wait, have you seen:

static
void qemu_flush_or_purge_queued_packets(NetClientState *nc, bool purge)
{
    nc->receive_disabled = 0;

    if (nc->peer && nc->peer->info->type == NET_CLIENT_OPTIONS_KIND_HUBPORT) {
        if (net_hub_flush(nc->peer)) {   <--- wake up all ports
            qemu_notify_event();
        }
    }

The net code is designed to propagate across the hub, so I don't think
the bug exists.

Do you have a reproducer?

> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  net/hub.c | 20 --------------------
>  1 file changed, 20 deletions(-)

Deleteing this function disables flow control across the hub.  There is
no way to tell a peer to stop sending.  This is a departure from normal
net client semantics where sending should stop if the peer cannot
receive further packets.

With this patch, packets will be queued if the destination net client is
unable to receive and that queue will keep growing until nq_maxlen is
reached and then packets will be dropped.

So this patch basically says flow control is not needed.

It is needed because:

1. USB network devices would suffer horrible traffic loss without
   queuing due to the nature of the device.  They only have 1 packet rx
   buffer and it needs to be collected by the guest before more packets
   can be received.

2. To save CPU cycles/power by not reading from file descriptors if the
   packet cannot be delivered yet.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH for-2.4] net-hub: Drop can_receive

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1963 bytes --]

On Tue, Jun 30, 2015 at 10:29:20AM +0800, Fam Zheng wrote:
> It returns true as long as there is another attached port. This is not
> strictly necessary because even if there is only one port (the sender),
> net_hub_port_receive could succeed with a NOP. So always deliver the
> packets, instead of queuing them.
> 
> This fixes the possible hanging issue after net layer changed how
> can_read is handled. That is, if net_hub_port_can_receive returned
> false, the peer would disable the queue until it's explicitly flushed
> (for example, a call to qemu_flush_queued_packets() in net_hub_add_port,
> where net_hub_port_can_receive() would become true). This patch avoids
> that complication.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  net/hub.c | 20 --------------------
>  1 file changed, 20 deletions(-)

Hmm...I misread the hub code:

net_hub_port_can_receive() returns true if *any* port can receive.

net_hub_receive_iov() always accepts packets.  It never discards or
queues.

So in order to move to the semantics you want, let's drop
net_hub_port_can_receive() *and* change net_hub_receive_iov():

  static ssize_t net_hub_receive(NetHub *hub, NetHubPort *source_port,
                                 const uint8_t *buf, size_t len)
  {
      NetHubPort *port;

      QLIST_FOREACH(port, &hub->ports, next) {
          if (port == source_port) {
              continue;
          }

          /* No need for a callback because net_hub_flush() is called
           * when the peer flushes the queue anyway.
           *
           * Note that packets are duplicated if there are multiple
           * ports and some of them accepted a packet before a later
           * port queued it.  Live with it, the network tolerates
           * duplicates.
           */
          if (qemu_send_packet(&port->nc, buf, len) == 0) {
              return 0;
          }
      }
      return len;
  }

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH for-2.4] net-hub: Drop can_receive

[Fam Zheng]

On Thu, 07/02 14:30, Stefan Hajnoczi wrote:
> On Tue, Jun 30, 2015 at 10:29:20AM +0800, Fam Zheng wrote:
> > It returns true as long as there is another attached port. This is not
> > strictly necessary because even if there is only one port (the sender),
> > net_hub_port_receive could succeed with a NOP. So always deliver the
> > packets, instead of queuing them.
> > 
> > This fixes the possible hanging issue after net layer changed how
> > can_read is handled. That is, if net_hub_port_can_receive returned
> > false, the peer would disable the queue until it's explicitly flushed
> > (for example, a call to qemu_flush_queued_packets() in net_hub_add_port,
> > where net_hub_port_can_receive() would become true). This patch avoids
> > that complication.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  net/hub.c | 20 --------------------
> >  1 file changed, 20 deletions(-)
> 
> Hmm...I misread the hub code:
> 
> net_hub_port_can_receive() returns true if *any* port can receive.
> 
> net_hub_receive_iov() always accepts packets.  It never discards or
> queues.
> 
> So in order to move to the semantics you want, let's drop
> net_hub_port_can_receive() *and* change net_hub_receive_iov():
> 
>   static ssize_t net_hub_receive(NetHub *hub, NetHubPort *source_port,
>                                  const uint8_t *buf, size_t len)
>   {
>       NetHubPort *port;
> 
>       QLIST_FOREACH(port, &hub->ports, next) {
>           if (port == source_port) {
>               continue;
>           }
> 
>           /* No need for a callback because net_hub_flush() is called
>            * when the peer flushes the queue anyway.
>            *
>            * Note that packets are duplicated if there are multiple
>            * ports and some of them accepted a packet before a later
>            * port queued it.  Live with it, the network tolerates
>            * duplicates.
>            */
>           if (qemu_send_packet(&port->nc, buf, len) == 0) {
>               return 0;

I'm not sure if it is a good idea to return 0 from either net_hub_port_receive
or net_hub_receive. Because that way if one out of ten ports is down, other
ones will no longer get more packets from tap.

Fam


>           }
>       }
>       return len;
>   }

Re: [Qemu-devel] [PATCH for-2.4] net-hub: Drop can_receive

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2682 bytes --]

On Fri, Jul 03, 2015 at 08:42:15AM +0800, Fam Zheng wrote:
> On Thu, 07/02 14:30, Stefan Hajnoczi wrote:
> > On Tue, Jun 30, 2015 at 10:29:20AM +0800, Fam Zheng wrote:
> > > It returns true as long as there is another attached port. This is not
> > > strictly necessary because even if there is only one port (the sender),
> > > net_hub_port_receive could succeed with a NOP. So always deliver the
> > > packets, instead of queuing them.
> > > 
> > > This fixes the possible hanging issue after net layer changed how
> > > can_read is handled. That is, if net_hub_port_can_receive returned
> > > false, the peer would disable the queue until it's explicitly flushed
> > > (for example, a call to qemu_flush_queued_packets() in net_hub_add_port,
> > > where net_hub_port_can_receive() would become true). This patch avoids
> > > that complication.
> > > 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > ---
> > >  net/hub.c | 20 --------------------
> > >  1 file changed, 20 deletions(-)
> > 
> > Hmm...I misread the hub code:
> > 
> > net_hub_port_can_receive() returns true if *any* port can receive.
> > 
> > net_hub_receive_iov() always accepts packets.  It never discards or
> > queues.
> > 
> > So in order to move to the semantics you want, let's drop
> > net_hub_port_can_receive() *and* change net_hub_receive_iov():
> > 
> >   static ssize_t net_hub_receive(NetHub *hub, NetHubPort *source_port,
> >                                  const uint8_t *buf, size_t len)
> >   {
> >       NetHubPort *port;
> > 
> >       QLIST_FOREACH(port, &hub->ports, next) {
> >           if (port == source_port) {
> >               continue;
> >           }
> > 
> >           /* No need for a callback because net_hub_flush() is called
> >            * when the peer flushes the queue anyway.
> >            *
> >            * Note that packets are duplicated if there are multiple
> >            * ports and some of them accepted a packet before a later
> >            * port queued it.  Live with it, the network tolerates
> >            * duplicates.
> >            */
> >           if (qemu_send_packet(&port->nc, buf, len) == 0) {
> >               return 0;
> 
> I'm not sure if it is a good idea to return 0 from either net_hub_port_receive
> or net_hub_receive. Because that way if one out of ten ports is down, other
> ones will no longer get more packets from tap.

You are right, in order to carry over existing semantics it needs to
return 0 when all ports returned 0.

That way .can_receive has been eliminated but behavior is identical
except for duplicate packets, which can happen on the network anyway.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]