From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55992) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SXpVx-000482-2d for qemu-devel@nongnu.org; Fri, 25 May 2012 04:04:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SXpVq-0001Ig-Id for qemu-devel@nongnu.org; Fri, 25 May 2012 04:04:48 -0400 Received: from mail-pb0-f45.google.com ([209.85.160.45]:57739) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SXpVq-0001IP-7q for qemu-devel@nongnu.org; Fri, 25 May 2012 04:04:42 -0400 Received: by pbbro12 with SMTP id ro12so1600191pbb.4 for ; Fri, 25 May 2012 01:04:40 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <4FBF2EEC.5090202@redhat.com> References: <1337882362-20100-1-git-send-email-zwu.kernel@gmail.com> <1337882362-20100-17-git-send-email-zwu.kernel@gmail.com> <4FBF2EEC.5090202@redhat.com> Date: Fri, 25 May 2012 16:04:40 +0800 Message-ID: From: Zhi Yong Wu Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 16/16] hub: add the support for hub own flow control List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: jan.kiszka@siemens.com, wuzhy@linux.vnet.ibm.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, stefanha@linux.vnet.ibm.com On Fri, May 25, 2012 at 3:04 PM, Paolo Bonzini wrote: > Il 24/05/2012 19:59, zwu.kernel@gmail.com ha scritto: >> From: Zhi Yong Wu >> >> Signed-off-by: Zhi Yong Wu >> --- >> =A0net/hub.c =A0 | =A0 35 ++++++++++++++++++++++++++++++++--- >> =A0net/hub.h =A0 | =A0 =A02 ++ >> =A0net/queue.c | =A0 =A05 +++++ >> =A03 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 { >> =A0 =A0 =A0QLIST_ENTRY(NetHubPort) next; >> =A0 =A0 =A0NetHub *hub; >> =A0 =A0 =A0unsigned int id; >> + =A0 =A0uint64_t nr_packets; >> =A0} NetHubPort; >> >> =A0struct NetHub { >> @@ -39,19 +40,37 @@ struct NetHub { >> >> =A0static QLIST_HEAD(, NetHub) hubs =3D QLIST_HEAD_INITIALIZER(&hubs); >> >> +static void net_hub_receive_completed(NetClientState *nc, ssize_t len) >> +{ >> + =A0 =A0NetHubPort *port =3D DO_UPCAST(NetHubPort, nc, nc); >> + =A0 =A0port->nr_packets--; >> + =A0 =A0if (!port->nr_packets) { >> + =A0 =A0 =A0 =A0qemu_net_queue_flush(nc->peer->send_queue); >> + =A0 =A0} >> +} >> + >> +void net_hub_port_packet_stats(NetClientState *nc) >> +{ >> + =A0 =A0NetHubPort *port =3D DO_UPCAST(NetHubPort, nc, nc); >> + >> + =A0 =A0port->nr_packets++; >> +} > > Isn't this being called also for non-hubport clients? > >> =A0static ssize_t net_hub_receive(NetHub *hub, NetHubPort *source_port, >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 const ui= nt8_t *buf, size_t len) >> =A0{ >> =A0 =A0 =A0NetHubPort *port; >> + =A0 =A0ssize_t ret =3D 0; >> >> =A0 =A0 =A0QLIST_FOREACH(port, &hub->ports, next) { >> =A0 =A0 =A0 =A0 =A0if (port =3D=3D source_port) { >> =A0 =A0 =A0 =A0 =A0 =A0 =A0continue; >> =A0 =A0 =A0 =A0 =A0} >> >> - =A0 =A0 =A0 =A0qemu_send_packet(&port->nc, buf, len); >> + =A0 =A0 =A0 ret =3D qemu_send_packet_async(&port->nc, buf, len, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0net_hub_receive_completed); > > Just increment nr_packets here: > > =A0 =A0ret =3D qemu_send_packet_async > =A0 =A0if (ret =3D=3D 0) { > =A0 =A0 =A0 =A0port->nr_packets++; > =A0 =A0} 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. > >> =A0 =A0 =A0} >> - =A0 =A0return len; >> + =A0 =A0return ret; > > You can return len here. =A0In fact returning ret is wrong because the > value comes from a random port (the last one). > >> =A0} >> >> =A0static ssize_t net_hub_receive_iov(NetHub *hub, NetHubPort *source_po= rt, >> @@ -65,7 +84,8 @@ static ssize_t net_hub_receive_iov(NetHub *hub, NetHub= Port *source_port, >> =A0 =A0 =A0 =A0 =A0 =A0 =A0continue; >> =A0 =A0 =A0 =A0 =A0} >> >> - =A0 =A0 =A0 =A0ret =3D qemu_sendv_packet(&port->nc, iov, iovcnt); >> + =A0 =A0 =A0 =A0ret =3D qemu_sendv_packet_async(&port->nc, iov, iovcnt, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0net_hub_receive_completed); > > Same here (increment nr_packets) > >> =A0 =A0 =A0} >> =A0 =A0 =A0return ret; > > Same here (return len). > >> =A0} >> @@ -84,6 +104,13 @@ static NetHub *net_hub_new(unsigned int id) >> =A0 =A0 =A0return hub; >> =A0} >> >> +static int net_hub_port_can_receive(NetClientState *nc) >> +{ >> + =A0 =A0NetHubPort *port =3D DO_UPCAST(NetHubPort, nc, nc); >> + >> + =A0 =A0return port->nr_packets ? 0 : 1; >> +} > > This is "return port->nr_packets =3D=3D 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 =3D=3D 0 for all ports. > Probably you can move nr_packets to the hub itself rather than the port? > >> =A0static ssize_t net_hub_port_receive(NetClientState *nc, >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0const uint8_t *buf, size_t len) >> =A0{ >> @@ -110,6 +137,7 @@ static void net_hub_port_cleanup(NetClientState *nc) >> =A0static NetClientInfo net_hub_port_info =3D { >> =A0 =A0 =A0.type =3D NET_CLIENT_TYPE_HUB, >> =A0 =A0 =A0.size =3D sizeof(NetHubPort), >> + =A0 =A0.can_receive =3D net_hub_port_can_receive, >> =A0 =A0 =A0.receive =3D net_hub_port_receive, >> =A0 =A0 =A0.receive_iov =3D net_hub_port_receive_iov, >> =A0 =A0 =A0.cleanup =3D net_hub_port_cleanup, >> @@ -128,6 +156,7 @@ static NetHubPort *net_hub_port_new(NetHub *hub) >> =A0 =A0 =A0port =3D DO_UPCAST(NetHubPort, nc, nc); >> =A0 =A0 =A0port->id =3D id; >> =A0 =A0 =A0port->hub =3D hub; >> + =A0 =A0port->nr_packets =3D 0; >> >> =A0 =A0 =A0QLIST_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); >> =A0int net_hub_id_for_client(NetClientState *nc, unsigned int *id); >> =A0void net_hub_check_clients(void); >> >> +void net_hub_port_packet_stats(NetClientState *nc); >> + >> =A0#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 @@ >> =A0 */ >> >> =A0#include "net/queue.h" >> +#include "net/hub.h" >> =A0#include "qemu-queue.h" >> =A0#include "net.h" >> >> @@ -101,6 +102,8 @@ static ssize_t qemu_net_queue_append(NetQueue *queue= , >> >> =A0 =A0 =A0QTAILQ_INSERT_TAIL(&queue->packets, packet, entry); >> >> + =A0 =A0net_hub_port_packet_stats(sender); >> + >> =A0 =A0 =A0return size; >> =A0} >> >> @@ -134,6 +137,8 @@ static ssize_t qemu_net_queue_append_iov(NetQueue *q= ueue, >> >> =A0 =A0 =A0QTAILQ_INSERT_TAIL(&queue->packets, packet, entry); >> >> + =A0 =A0net_hub_port_packet_stats(sender); >> + >> =A0 =A0 =A0return packet->size; >> =A0} >> > --=20 Regards, Zhi Yong Wu