[v3,16/16] hub: add the support for hub own flow control

From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>

Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
---
 net/hub.c   |   35 ++++++++++++++++++++++++++++++++---
 net/hub.h   |    2 ++
 net/queue.c |    5 +++++
 3 files changed, 39 insertions(+), 3 deletions(-)

Il 24/05/2012 19:59, zwu.kernel@gmail.com ha scritto:
> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> 
> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> ---
>  net/hub.c   |   35 ++++++++++++++++++++++++++++++++---
>  net/hub.h   |    2 ++
>  net/queue.c |    5 +++++
>  3 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/net/hub.c b/net/hub.c
> index 8a583ab..d27c52a 100644
> --- a/net/hub.c
> +++ b/net/hub.c
> @@ -28,6 +28,7 @@ typedef struct NetHubPort {
>      QLIST_ENTRY(NetHubPort) next;
>      NetHub *hub;
>      unsigned int id;
> +    uint64_t nr_packets;
>  } NetHubPort;
>  
>  struct NetHub {
> @@ -39,19 +40,37 @@ struct NetHub {
>  
>  static QLIST_HEAD(, NetHub) hubs = QLIST_HEAD_INITIALIZER(&hubs);
>  
> +static void net_hub_receive_completed(NetClientState *nc, ssize_t len)
> +{
> +    NetHubPort *port = DO_UPCAST(NetHubPort, nc, nc);
> +    port->nr_packets--;
> +    if (!port->nr_packets) {
> +        qemu_net_queue_flush(nc->peer->send_queue);
> +    }
> +}
> +
> +void net_hub_port_packet_stats(NetClientState *nc)
> +{
> +    NetHubPort *port = DO_UPCAST(NetHubPort, nc, nc);
> +
> +    port->nr_packets++;
> +}

Isn't this being called also for non-hubport clients?

>  static ssize_t net_hub_receive(NetHub *hub, NetHubPort *source_port,
>                                 const uint8_t *buf, size_t len)
>  {
>      NetHubPort *port;
> +    ssize_t ret = 0;
>  
>      QLIST_FOREACH(port, &hub->ports, next) {
>          if (port == source_port) {
>              continue;
>          }
>  
> -        qemu_send_packet(&port->nc, buf, len);
> +       ret = qemu_send_packet_async(&port->nc, buf, len,
> +                                    net_hub_receive_completed);

Just increment nr_packets here:

    ret = qemu_send_packet_async
    if (ret == 0) {
        port->nr_packets++;
    }

>      }
> -    return len;
> +    return ret;

You can return len here.  In fact returning ret is wrong because the
value comes from a random port (the last one).

>  }
>  
>  static ssize_t net_hub_receive_iov(NetHub *hub, NetHubPort *source_port,
> @@ -65,7 +84,8 @@ static ssize_t net_hub_receive_iov(NetHub *hub, NetHubPort *source_port,
>              continue;
>          }
>  
> -        ret = qemu_sendv_packet(&port->nc, iov, iovcnt);
> +        ret = qemu_sendv_packet_async(&port->nc, iov, iovcnt,
> +                                      net_hub_receive_completed);

Same here (increment nr_packets)

>      }
>      return ret;

Same here (return len).

>  }
> @@ -84,6 +104,13 @@ static NetHub *net_hub_new(unsigned int id)
>      return hub;
>  }
>  
> +static int net_hub_port_can_receive(NetClientState *nc)
> +{
> +    NetHubPort *port = DO_UPCAST(NetHubPort, nc, nc);
> +
> +    return port->nr_packets ? 0 : 1;
> +}

This is "return port->nr_packets == 0;".

But I think you need to implement this on the hub rather than on the
port, and return true only if port->nr_packets == 0 for all ports.
Probably you can move nr_packets to the hub itself rather than the port?

>  static ssize_t net_hub_port_receive(NetClientState *nc,
>                                      const uint8_t *buf, size_t len)
>  {
> @@ -110,6 +137,7 @@ static void net_hub_port_cleanup(NetClientState *nc)
>  static NetClientInfo net_hub_port_info = {
>      .type = NET_CLIENT_TYPE_HUB,
>      .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,
> @@ -128,6 +156,7 @@ static NetHubPort *net_hub_port_new(NetHub *hub)
>      port = DO_UPCAST(NetHubPort, nc, nc);
>      port->id = id;
>      port->hub = hub;
> +    port->nr_packets = 0;
>  
>      QLIST_INSERT_HEAD(&hub->ports, port, next);
>  

Everything below this has to go away (sender is not necessarily a hub
port!).

> diff --git a/net/hub.h b/net/hub.h
> index d04f1b1..542e657 100644
> --- a/net/hub.h
> +++ b/net/hub.h
> @@ -23,4 +23,6 @@ void net_hub_info(Monitor *mon);
>  int net_hub_id_for_client(NetClientState *nc, unsigned int *id);
>  void net_hub_check_clients(void);
>  
> +void net_hub_port_packet_stats(NetClientState *nc);
> +
>  #endif /* NET_HUB_H */
> diff --git a/net/queue.c b/net/queue.c
> index 7484d2a..ebf18aa 100644
> --- a/net/queue.c
> +++ b/net/queue.c
> @@ -22,6 +22,7 @@
>   */
>  
>  #include "net/queue.h"
> +#include "net/hub.h"
>  #include "qemu-queue.h"
>  #include "net.h"
>  
> @@ -101,6 +102,8 @@ static ssize_t qemu_net_queue_append(NetQueue *queue,
>  
>      QTAILQ_INSERT_TAIL(&queue->packets, packet, entry);
>  
> +    net_hub_port_packet_stats(sender);
> +
>      return size;
>  }
>  
> @@ -134,6 +137,8 @@ static ssize_t qemu_net_queue_append_iov(NetQueue *queue,
>  
>      QTAILQ_INSERT_TAIL(&queue->packets, packet, entry);
>  
> +    net_hub_port_packet_stats(sender);
> +    
>      return packet->size;
>  }
>

On Fri, May 25, 2012 at 3:04 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 24/05/2012 19:59, zwu.kernel@gmail.com ha scritto:
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>> ---
>>  net/hub.c   |   35 ++++++++++++++++++++++++++++++++---
>>  net/hub.h   |    2 ++
>>  net/queue.c |    5 +++++
>>  3 files changed, 39 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/hub.c b/net/hub.c
>> index 8a583ab..d27c52a 100644
>> --- a/net/hub.c
>> +++ b/net/hub.c
>> @@ -28,6 +28,7 @@ typedef struct NetHubPort {
>>      QLIST_ENTRY(NetHubPort) next;
>>      NetHub *hub;
>>      unsigned int id;
>> +    uint64_t nr_packets;
>>  } NetHubPort;
>>
>>  struct NetHub {
>> @@ -39,19 +40,37 @@ struct NetHub {
>>
>>  static QLIST_HEAD(, NetHub) hubs = QLIST_HEAD_INITIALIZER(&hubs);
>>
>> +static void net_hub_receive_completed(NetClientState *nc, ssize_t len)
>> +{
>> +    NetHubPort *port = DO_UPCAST(NetHubPort, nc, nc);
>> +    port->nr_packets--;
>> +    if (!port->nr_packets) {
>> +        qemu_net_queue_flush(nc->peer->send_queue);
>> +    }
>> +}
>> +
>> +void net_hub_port_packet_stats(NetClientState *nc)
>> +{
>> +    NetHubPort *port = DO_UPCAST(NetHubPort, nc, nc);
>> +
>> +    port->nr_packets++;
>> +}
>
> Isn't this being called also for non-hubport clients?
Thanks.
>
>>  static ssize_t net_hub_receive(NetHub *hub, NetHubPort *source_port,
>>                                 const uint8_t *buf, size_t len)
>>  {
>>      NetHubPort *port;
>> +    ssize_t ret = 0;
>>
>>      QLIST_FOREACH(port, &hub->ports, next) {
>>          if (port == source_port) {
>>              continue;
>>          }
>>
>> -        qemu_send_packet(&port->nc, buf, len);
>> +       ret = qemu_send_packet_async(&port->nc, buf, len,
>> +                                    net_hub_receive_completed);
>
> Just increment nr_packets here:
>
>    ret = qemu_send_packet_async
>    if (ret == 0) {
>        port->nr_packets++;
>    }
>
>>      }
>> -    return len;
>> +    return ret;
>
> You can return len here.  In fact returning ret is wrong because the
> value comes from a random port (the last one).
If the return value from the last port doesn't equal to len, you let
this function return len, it will be also wrong.
>
>>  }
>>
>>  static ssize_t net_hub_receive_iov(NetHub *hub, NetHubPort *source_port,
>> @@ -65,7 +84,8 @@ static ssize_t net_hub_receive_iov(NetHub *hub, NetHubPort *source_port,
>>              continue;
>>          }
>>
>> -        ret = qemu_sendv_packet(&port->nc, iov, iovcnt);
>> +        ret = qemu_sendv_packet_async(&port->nc, iov, iovcnt,
>> +                                      net_hub_receive_completed);
>
> Same here (increment nr_packets)
>
>>      }
>>      return ret;
>
> Same here (return len).
No, it has no such variable called as len, I think that here should
return ret, not len.
Do you think that it is necessary to calc len by iov and viocnt?
>
>>  }
>> @@ -84,6 +104,13 @@ static NetHub *net_hub_new(unsigned int id)
>>      return hub;
>>  }
>>
>> +static int net_hub_port_can_receive(NetClientState *nc)
>> +{
>> +    NetHubPort *port = DO_UPCAST(NetHubPort, nc, nc);
>> +
>> +    return port->nr_packets ? 0 : 1;
>> +}
>
> This is "return port->nr_packets == 0;".
thanks.
>
> But I think you need to implement this on the hub rather than on the
> port, and return true only if port->nr_packets == 0 for all ports.
Can you explain why to need based on hub, not port?
> Probably you can move nr_packets to the hub itself rather than the port?
I think that the counter brings a lot of issues.
1.) The packet delivery is bidirectional. one direction is blocked
doesn't mean another direction also is blocked.
2.) If the packets comes from guest to outside, when hubport's
can_receive fails, the following packets will all be enqueued to
hubport queue, How will these packets in hubport queue are trigered to
send again? Maybe this will lead to introduce one timer for each
hubport queue.

>
>>  static ssize_t net_hub_port_receive(NetClientState *nc,
>>                                      const uint8_t *buf, size_t len)
>>  {
>> @@ -110,6 +137,7 @@ static void net_hub_port_cleanup(NetClientState *nc)
>>  static NetClientInfo net_hub_port_info = {
>>      .type = NET_CLIENT_TYPE_HUB,
>>      .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,
>> @@ -128,6 +156,7 @@ static NetHubPort *net_hub_port_new(NetHub *hub)
>>      port = DO_UPCAST(NetHubPort, nc, nc);
>>      port->id = id;
>>      port->hub = hub;
>> +    port->nr_packets = 0;
>>
>>      QLIST_INSERT_HEAD(&hub->ports, port, next);
>>
>
> Everything below this has to go away (sender is not necessarily a hub
> port!).
>
>> diff --git a/net/hub.h b/net/hub.h
>> index d04f1b1..542e657 100644
>> --- a/net/hub.h
>> +++ b/net/hub.h
>> @@ -23,4 +23,6 @@ void net_hub_info(Monitor *mon);
>>  int net_hub_id_for_client(NetClientState *nc, unsigned int *id);
>>  void net_hub_check_clients(void);
>>
>> +void net_hub_port_packet_stats(NetClientState *nc);
>> +
>>  #endif /* NET_HUB_H */
>> diff --git a/net/queue.c b/net/queue.c
>> index 7484d2a..ebf18aa 100644
>> --- a/net/queue.c
>> +++ b/net/queue.c
>> @@ -22,6 +22,7 @@
>>   */
>>
>>  #include "net/queue.h"
>> +#include "net/hub.h"
>>  #include "qemu-queue.h"
>>  #include "net.h"
>>
>> @@ -101,6 +102,8 @@ static ssize_t qemu_net_queue_append(NetQueue *queue,
>>
>>      QTAILQ_INSERT_TAIL(&queue->packets, packet, entry);
>>
>> +    net_hub_port_packet_stats(sender);
>> +
>>      return size;
>>  }
>>
>> @@ -134,6 +137,8 @@ static ssize_t qemu_net_queue_append_iov(NetQueue *queue,
>>
>>      QTAILQ_INSERT_TAIL(&queue->packets, packet, entry);
>>
>> +    net_hub_port_packet_stats(sender);
>> +
>>      return packet->size;
>>  }
>>
>

On Fri, May 25, 2012 at 3:04 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 24/05/2012 19:59, zwu.kernel@gmail.com ha scritto:
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>> ---
>>  net/hub.c   |   35 ++++++++++++++++++++++++++++++++---
>>  net/hub.h   |    2 ++
>>  net/queue.c |    5 +++++
>>  3 files changed, 39 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/hub.c b/net/hub.c
>> index 8a583ab..d27c52a 100644
>> --- a/net/hub.c
>> +++ b/net/hub.c
>> @@ -28,6 +28,7 @@ typedef struct NetHubPort {
>>      QLIST_ENTRY(NetHubPort) next;
>>      NetHub *hub;
>>      unsigned int id;
>> +    uint64_t nr_packets;
>>  } NetHubPort;
>>
>>  struct NetHub {
>> @@ -39,19 +40,37 @@ struct NetHub {
>>
>>  static QLIST_HEAD(, NetHub) hubs = QLIST_HEAD_INITIALIZER(&hubs);
>>
>> +static void net_hub_receive_completed(NetClientState *nc, ssize_t len)
>> +{
>> +    NetHubPort *port = DO_UPCAST(NetHubPort, nc, nc);
>> +    port->nr_packets--;
>> +    if (!port->nr_packets) {
>> +        qemu_net_queue_flush(nc->peer->send_queue);
>> +    }
>> +}
>> +
>> +void net_hub_port_packet_stats(NetClientState *nc)
>> +{
>> +    NetHubPort *port = DO_UPCAST(NetHubPort, nc, nc);
>> +
>> +    port->nr_packets++;
>> +}
>
> Isn't this being called also for non-hubport clients?
>
>>  static ssize_t net_hub_receive(NetHub *hub, NetHubPort *source_port,
>>                                 const uint8_t *buf, size_t len)
>>  {
>>      NetHubPort *port;
>> +    ssize_t ret = 0;
>>
>>      QLIST_FOREACH(port, &hub->ports, next) {
>>          if (port == source_port) {
>>              continue;
>>          }
>>
>> -        qemu_send_packet(&port->nc, buf, len);
>> +       ret = qemu_send_packet_async(&port->nc, buf, len,
>> +                                    net_hub_receive_completed);
>
> Just increment nr_packets here:
>
>    ret = qemu_send_packet_async
>    if (ret == 0) {
>        port->nr_packets++;
>    }
This is wrong, if you check the code, sent_cb is only called when the
send queue is not empty. That is, sent_cb is used for those enqueued
packets. For those packets which aren't enqueued, The counter will be
not decreased. And when qemu_send_packet_async/qemu_sendv_packet_async
return, flush function has been executed. But you increase the
coutner, when the next following packets come, they will be enqueued
without condition. And no timer triger the hubport queue to fire
again.
>
>>      }
>> -    return len;
>> +    return ret;
>
> You can return len here.  In fact returning ret is wrong because the
> value comes from a random port (the last one).
>
>>  }
>>
>>  static ssize_t net_hub_receive_iov(NetHub *hub, NetHubPort *source_port,
>> @@ -65,7 +84,8 @@ static ssize_t net_hub_receive_iov(NetHub *hub, NetHubPort *source_port,
>>              continue;
>>          }
>>
>> -        ret = qemu_sendv_packet(&port->nc, iov, iovcnt);
>> +        ret = qemu_sendv_packet_async(&port->nc, iov, iovcnt,
>> +                                      net_hub_receive_completed);
>
> Same here (increment nr_packets)
>
>>      }
>>      return ret;
>
> Same here (return len).
>
>>  }
>> @@ -84,6 +104,13 @@ static NetHub *net_hub_new(unsigned int id)
>>      return hub;
>>  }
>>
>> +static int net_hub_port_can_receive(NetClientState *nc)
>> +{
>> +    NetHubPort *port = DO_UPCAST(NetHubPort, nc, nc);
>> +
>> +    return port->nr_packets ? 0 : 1;
>> +}
>
> This is "return port->nr_packets == 0;".
>
> But I think you need to implement this on the hub rather than on the
> port, and return true only if port->nr_packets == 0 for all ports.
> Probably you can move nr_packets to the hub itself rather than the port?
>
>>  static ssize_t net_hub_port_receive(NetClientState *nc,
>>                                      const uint8_t *buf, size_t len)
>>  {
>> @@ -110,6 +137,7 @@ static void net_hub_port_cleanup(NetClientState *nc)
>>  static NetClientInfo net_hub_port_info = {
>>      .type = NET_CLIENT_TYPE_HUB,
>>      .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,
>> @@ -128,6 +156,7 @@ static NetHubPort *net_hub_port_new(NetHub *hub)
>>      port = DO_UPCAST(NetHubPort, nc, nc);
>>      port->id = id;
>>      port->hub = hub;
>> +    port->nr_packets = 0;
>>
>>      QLIST_INSERT_HEAD(&hub->ports, port, next);
>>
>
> Everything below this has to go away (sender is not necessarily a hub
> port!).
>
>> diff --git a/net/hub.h b/net/hub.h
>> index d04f1b1..542e657 100644
>> --- a/net/hub.h
>> +++ b/net/hub.h
>> @@ -23,4 +23,6 @@ void net_hub_info(Monitor *mon);
>>  int net_hub_id_for_client(NetClientState *nc, unsigned int *id);
>>  void net_hub_check_clients(void);
>>
>> +void net_hub_port_packet_stats(NetClientState *nc);
>> +
>>  #endif /* NET_HUB_H */
>> diff --git a/net/queue.c b/net/queue.c
>> index 7484d2a..ebf18aa 100644
>> --- a/net/queue.c
>> +++ b/net/queue.c
>> @@ -22,6 +22,7 @@
>>   */
>>
>>  #include "net/queue.h"
>> +#include "net/hub.h"
>>  #include "qemu-queue.h"
>>  #include "net.h"
>>
>> @@ -101,6 +102,8 @@ static ssize_t qemu_net_queue_append(NetQueue *queue,
>>
>>      QTAILQ_INSERT_TAIL(&queue->packets, packet, entry);
>>
>> +    net_hub_port_packet_stats(sender);
>> +
>>      return size;
>>  }
>>
>> @@ -134,6 +137,8 @@ static ssize_t qemu_net_queue_append_iov(NetQueue *queue,
>>
>>      QTAILQ_INSERT_TAIL(&queue->packets, packet, entry);
>>
>> +    net_hub_port_packet_stats(sender);
>> +
>>      return packet->size;
>>  }
>>
>

On Fri, May 25, 2012 at 3:04 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 24/05/2012 19:59, zwu.kernel@gmail.com ha scritto:
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>> ---
>>  net/hub.c   |   35 ++++++++++++++++++++++++++++++++---
>>  net/hub.h   |    2 ++
>>  net/queue.c |    5 +++++
>>  3 files changed, 39 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/hub.c b/net/hub.c
>> index 8a583ab..d27c52a 100644
>> --- a/net/hub.c
>> +++ b/net/hub.c
>> @@ -28,6 +28,7 @@ typedef struct NetHubPort {
>>      QLIST_ENTRY(NetHubPort) next;
>>      NetHub *hub;
>>      unsigned int id;
>> +    uint64_t nr_packets;
>>  } NetHubPort;
>>
>>  struct NetHub {
>> @@ -39,19 +40,37 @@ struct NetHub {
>>
>>  static QLIST_HEAD(, NetHub) hubs = QLIST_HEAD_INITIALIZER(&hubs);
>>
>> +static void net_hub_receive_completed(NetClientState *nc, ssize_t len)
>> +{
>> +    NetHubPort *port = DO_UPCAST(NetHubPort, nc, nc);
>> +    port->nr_packets--;
>> +    if (!port->nr_packets) {
>> +        qemu_net_queue_flush(nc->peer->send_queue);
>> +    }
>> +}
>> +
>> +void net_hub_port_packet_stats(NetClientState *nc)
>> +{
>> +    NetHubPort *port = DO_UPCAST(NetHubPort, nc, nc);
>> +
>> +    port->nr_packets++;
>> +}
>
> Isn't this being called also for non-hubport clients?
>
>>  static ssize_t net_hub_receive(NetHub *hub, NetHubPort *source_port,
>>                                 const uint8_t *buf, size_t len)
>>  {
>>      NetHubPort *port;
>> +    ssize_t ret = 0;
>>
>>      QLIST_FOREACH(port, &hub->ports, next) {
>>          if (port == source_port) {
>>              continue;
>>          }
>>
>> -        qemu_send_packet(&port->nc, buf, len);
>> +       ret = qemu_send_packet_async(&port->nc, buf, len,
>> +                                    net_hub_receive_completed);
>
> Just increment nr_packets here:
>
>    ret = qemu_send_packet_async
>    if (ret == 0) {
>        port->nr_packets++;
>    }
>
>>      }
>> -    return len;
>> +    return ret;
>
> You can return len here.  In fact returning ret is wrong because the
> value comes from a random port (the last one).
>
>>  }
>>
>>  static ssize_t net_hub_receive_iov(NetHub *hub, NetHubPort *source_port,
>> @@ -65,7 +84,8 @@ static ssize_t net_hub_receive_iov(NetHub *hub, NetHubPort *source_port,
>>              continue;
>>          }
>>
>> -        ret = qemu_sendv_packet(&port->nc, iov, iovcnt);
>> +        ret = qemu_sendv_packet_async(&port->nc, iov, iovcnt,
>> +                                      net_hub_receive_completed);
>
> Same here (increment nr_packets)
>
>>      }
>>      return ret;
>
> Same here (return len).
>
>>  }
>> @@ -84,6 +104,13 @@ static NetHub *net_hub_new(unsigned int id)
>>      return hub;
>>  }
>>
>> +static int net_hub_port_can_receive(NetClientState *nc)
>> +{
>> +    NetHubPort *port = DO_UPCAST(NetHubPort, nc, nc);
>> +
>> +    return port->nr_packets ? 0 : 1;
>> +}
>
> This is "return port->nr_packets == 0;".
>
> But I think you need to implement this on the hub rather than on the
> port, and return true only if port->nr_packets == 0 for all ports.
I don't think so.
e.g. guest <---> hubport1 -  hubport2 <--> network backend.
hubport1->nr_packets == 0 mean if guest can send packet through
hubport1 to outside.
while hubport2->nr_packets == 0 mean if network backend can send
packet through hubport1 to guest.
Their direction is different. So i don't understand why to need
"port->nr_packets == 0 for all ports"?

> Probably you can move nr_packets to the hub itself rather than the port?
>
>>  static ssize_t net_hub_port_receive(NetClientState *nc,
>>                                      const uint8_t *buf, size_t len)
>>  {
>> @@ -110,6 +137,7 @@ static void net_hub_port_cleanup(NetClientState *nc)
>>  static NetClientInfo net_hub_port_info = {
>>      .type = NET_CLIENT_TYPE_HUB,
>>      .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,
>> @@ -128,6 +156,7 @@ static NetHubPort *net_hub_port_new(NetHub *hub)
>>      port = DO_UPCAST(NetHubPort, nc, nc);
>>      port->id = id;
>>      port->hub = hub;
>> +    port->nr_packets = 0;
>>
>>      QLIST_INSERT_HEAD(&hub->ports, port, next);
>>
>
> Everything below this has to go away (sender is not necessarily a hub
> port!).
>
>> diff --git a/net/hub.h b/net/hub.h
>> index d04f1b1..542e657 100644
>> --- a/net/hub.h
>> +++ b/net/hub.h
>> @@ -23,4 +23,6 @@ void net_hub_info(Monitor *mon);
>>  int net_hub_id_for_client(NetClientState *nc, unsigned int *id);
>>  void net_hub_check_clients(void);
>>
>> +void net_hub_port_packet_stats(NetClientState *nc);
>> +
>>  #endif /* NET_HUB_H */
>> diff --git a/net/queue.c b/net/queue.c
>> index 7484d2a..ebf18aa 100644
>> --- a/net/queue.c
>> +++ b/net/queue.c
>> @@ -22,6 +22,7 @@
>>   */
>>
>>  #include "net/queue.h"
>> +#include "net/hub.h"
>>  #include "qemu-queue.h"
>>  #include "net.h"
>>
>> @@ -101,6 +102,8 @@ static ssize_t qemu_net_queue_append(NetQueue *queue,
>>
>>      QTAILQ_INSERT_TAIL(&queue->packets, packet, entry);
>>
>> +    net_hub_port_packet_stats(sender);
>> +
>>      return size;
>>  }
>>
>> @@ -134,6 +137,8 @@ static ssize_t qemu_net_queue_append_iov(NetQueue *queue,
>>
>>      QTAILQ_INSERT_TAIL(&queue->packets, packet, entry);
>>
>> +    net_hub_port_packet_stats(sender);
>> +
>>      return packet->size;
>>  }
>>
>

Il 25/05/2012 09:48, Zhi Yong Wu ha scritto:
>>>  static ssize_t net_hub_receive(NetHub *hub, NetHubPort *source_port,
>>>                                 const uint8_t *buf, size_t len)
>>>  {
>>>      NetHubPort *port;
>>> +    ssize_t ret = 0;
>>>
>>>      QLIST_FOREACH(port, &hub->ports, next) {
>>>          if (port == source_port) {
>>>              continue;
>>>          }
>>>
>>> -        qemu_send_packet(&port->nc, buf, len);
>>> +       ret = qemu_send_packet_async(&port->nc, buf, len,
>>> +                                    net_hub_receive_completed);
>>
>> Just increment nr_packets here:
>>
>>    ret = qemu_send_packet_async
>>    if (ret == 0) {
>>        port->nr_packets++;
>>    }
> This is wrong, if you check the code, sent_cb is only called when the
> send queue is not empty. That is, sent_cb is used for those enqueued
> packets. For those packets which aren't enqueued, The counter will be
> not decreased.

It will also not be incremented, because I'm checking for ret == 0.

>>>      }
>>> -    return len;
>>> +    return ret;
>>
>> You can return len here.  In fact returning ret is wrong because the
>> value comes from a random port (the last one).
> If the return value from the last port doesn't equal to len, you let
> this function return len, it will be also wrong.

But that's the whole point of implementing flow control.  We return len
because we _did_ process the packet; it is now in the port's queues.
However, can_receive will not admit new packets until all ports have
processed the previous one, so that all ports advance to new packets at
the same time.

>>
>>>  }
>>>
>>>  static ssize_t net_hub_receive_iov(NetHub *hub, NetHubPort *source_port,
>>> @@ -65,7 +84,8 @@ static ssize_t net_hub_receive_iov(NetHub *hub, NetHubPort *source_port,
>>>              continue;
>>>          }
>>>
>>> -        ret = qemu_sendv_packet(&port->nc, iov, iovcnt);
>>> +        ret = qemu_sendv_packet_async(&port->nc, iov, iovcnt,
>>> +                                      net_hub_receive_completed);
>>
>> Same here (increment nr_packets)
>>
>>>      }
>>>      return ret;
>>
>> Same here (return len).
> No, it has no such variable called as len, I think that here should
> return ret, not len.
> Do you think that it is necessary to calc len by iov and viocnt?

Yes, for the same reason as above.  Returning "ret" for a random port
(the last one) does not make sense!  But you could just punt: do not
implement net_hub_receive_iov at all...

>> But I think you need to implement this on the hub rather than on the
>> port, and return true only if port->nr_packets == 0 for all ports.
> Can you explain why to need based on hub, not port?

Because the purpose of the counter is to do flow control _on the hub_.
The ports can do their flow control just as well, but the hub has to
reconcile the decisions of the ports.

Taking your example from another message:

>   e.g. guest <---> hubport1 -  hubport2 <--> network backend.
>   hubport1->nr_packets == 0 mean if guest can send packet through
>   hubport1 to outside.
>   while hubport2->nr_packets == 0 mean if network backend can send
>   packet through hubport1 to guest.
>   Their direction is different.
>   So i don't understand why to need
>   "port->nr_packets == 0 for all ports"?

For simplicity.  Yes, this means hubs will be half-duplex.  In practice
I don't think you need to care.

If you want to make it full-duplex, you can keep the per-port counter
and in can_receive check if all ports except this one has a zero
nr_packets count.  In other words, your can_receive method is backwards:
a port can receive a packet if all of its sibling ports are ready to
receive it.

Don't think of it in terms of "directions".  It is not correct, because
it is a star topology.  Think of it in terms of where the packets enter
the hub, and where they are forwarded to.

>> Probably you can move nr_packets to the hub itself rather than the port?
> I think that the counter brings a lot of issues.

I said already that it's not *necessary*.  You're free to find another
solution.  Removing TODO comments and leaving the problem however is not
a solution.

Paolo

On Fri, May 25, 2012 at 6:08 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 25/05/2012 09:48, Zhi Yong Wu ha scritto:
>>>>  static ssize_t net_hub_receive(NetHub *hub, NetHubPort *source_port,
>>>>                                 const uint8_t *buf, size_t len)
>>>>  {
>>>>      NetHubPort *port;
>>>> +    ssize_t ret = 0;
>>>>
>>>>      QLIST_FOREACH(port, &hub->ports, next) {
>>>>          if (port == source_port) {
>>>>              continue;
>>>>          }
>>>>
>>>> -        qemu_send_packet(&port->nc, buf, len);
>>>> +       ret = qemu_send_packet_async(&port->nc, buf, len,
>>>> +                                    net_hub_receive_completed);
>>>
>>> Just increment nr_packets here:
>>>
>>>    ret = qemu_send_packet_async
>>>    if (ret == 0) {
>>>        port->nr_packets++;
>>>    }
>> This is wrong, if you check the code, sent_cb is only called when the
>> send queue is not empty. That is, sent_cb is used for those enqueued
>> packets. For those packets which aren't enqueued, The counter will be
>> not decreased.
>
> It will also not be incremented, because I'm checking for ret == 0.
>
>>>>      }
>>>> -    return len;
>>>> +    return ret;
>>>
>>> You can return len here.  In fact returning ret is wrong because the
>>> value comes from a random port (the last one).
>> If the return value from the last port doesn't equal to len, you let
>> this function return len, it will be also wrong.
>
> But that's the whole point of implementing flow control.  We return len
> because we _did_ process the packet; it is now in the port's queues.
> However, can_receive will not admit new packets until all ports have
> processed the previous one, so that all ports advance to new packets at
> the same time.
>
>>>
>>>>  }
>>>>
>>>>  static ssize_t net_hub_receive_iov(NetHub *hub, NetHubPort *source_port,
>>>> @@ -65,7 +84,8 @@ static ssize_t net_hub_receive_iov(NetHub *hub, NetHubPort *source_port,
>>>>              continue;
>>>>          }
>>>>
>>>> -        ret = qemu_sendv_packet(&port->nc, iov, iovcnt);
>>>> +        ret = qemu_sendv_packet_async(&port->nc, iov, iovcnt,
>>>> +                                      net_hub_receive_completed);
>>>
>>> Same here (increment nr_packets)
>>>
>>>>      }
>>>>      return ret;
>>>
>>> Same here (return len).
>> No, it has no such variable called as len, I think that here should
>> return ret, not len.
>> Do you think that it is necessary to calc len by iov and viocnt?
>
> Yes, for the same reason as above.  Returning "ret" for a random port
> (the last one) does not make sense!  But you could just punt: do not
> implement net_hub_receive_iov at all...
>
>>> But I think you need to implement this on the hub rather than on the
>>> port, and return true only if port->nr_packets == 0 for all ports.
>> Can you explain why to need based on hub, not port?
>
> Because the purpose of the counter is to do flow control _on the hub_.
> The ports can do their flow control just as well, but the hub has to
> reconcile the decisions of the ports.
>
> Taking your example from another message:
>
>>   e.g. guest <---> hubport1 -  hubport2 <--> network backend.
>>   hubport1->nr_packets == 0 mean if guest can send packet through
>>   hubport1 to outside.
>>   while hubport2->nr_packets == 0 mean if network backend can send
>>   packet through hubport1 to guest.
>>   Their direction is different.
>>   So i don't understand why to need
>>   "port->nr_packets == 0 for all ports"?
>
> For simplicity.  Yes, this means hubs will be half-duplex.  In practice
> I don't think you need to care.
>
> If you want to make it full-duplex, you can keep the per-port counter
> and in can_receive check if all ports except this one has a zero
> nr_packets count.  In other words, your can_receive method is backwards:
> a port can receive a packet if all of its sibling ports are ready to
> receive it.
>
> Don't think of it in terms of "directions".  It is not correct, because
> it is a star topology.  Think of it in terms of where the packets enter
> the hub, and where they are forwarded to.
>
>>> Probably you can move nr_packets to the hub itself rather than the port?
>> I think that the counter brings a lot of issues.
>
> I said already that it's not *necessary*.  You're free to find another
> solution.  Removing TODO comments and leaving the problem however is not
> a solution.
I like the words. I sent out v4 for this patch. In new version, i
define one new rule for hub port .can_receive().

anyway, thanks a lot for your comments. Have nice weekend!!
>
> Paolo