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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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



-- 
Regards,

Zhi Yong Wu
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[KVM ARM]     [KVM ia64]     [KVM ppc]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]    [Yosemite Photos]    [Linux Kernel]     [Linux SCSI]     [XFree86]

Add to Google Powered by Linux