Re: [Qemu-devel] [PATCH v3 16/16] hub: add the support for hub own flow control

[Zhi Yong Wu]

On Fri, May 25, 2012 at 3:04 PM, Paolo Bonzini <address@hidden> wrote:
> Il 24/05/2012 19:59, address@hidden ha scritto:
>> From: Zhi Yong Wu <address@hidden>
>>
>> Signed-off-by: Zhi Yong Wu <address@hidden>
>> ---
>>  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;
>>  }
>>
>



-- 
Regards,

Zhi Yong Wu