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++;
>    }
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;
>>  }
>>
>



-- 
Regards,

Zhi Yong Wu