From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44770) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UUZaF-0003Mn-MF for qemu-devel@nongnu.org; Tue, 23 Apr 2013 05:32:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UUZaD-0004yZ-Nu for qemu-devel@nongnu.org; Tue, 23 Apr 2013 05:32:19 -0400 Received: from mailout3.w1.samsung.com ([210.118.77.13]:62865) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UUZaD-0004xH-G3 for qemu-devel@nongnu.org; Tue, 23 Apr 2013 05:32:17 -0400 Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout3.w1.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0MLP00BXKBPFDW70@mailout3.w1.samsung.com> for qemu-devel@nongnu.org; Tue, 23 Apr 2013 10:32:13 +0100 (BST) Message-id: <5176551B.1020606@samsung.com> Date: Tue, 23 Apr 2013 13:32:11 +0400 From: Fedorov Sergey MIME-version: 1.0 References: <1366284715-10107-1-git-send-email-s.fedorov@samsung.com> <20130422114711.GA21317@stefanha-thinkpad.redhat.com> <51752C68.6030408@samsung.com> <20130422145749.GA28049@stefanha-thinkpad.redhat.com> <517556D9.2080809@samsung.com> <20130423065841.GA6447@stefanha-thinkpad.redhat.com> In-reply-to: <20130423065841.GA6447@stefanha-thinkpad.redhat.com> Content-type: text/plain; charset=ISO-8859-1; format=flowed Content-transfer-encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] net/hub: remove can_receive handler List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Stefan Hajnoczi , aliguori@us.ibm.com, a.basov@samsung.com, qemu-devel@nongnu.org On 04/23/2013 10:58 AM, Stefan Hajnoczi wrote: > On Mon, Apr 22, 2013 at 07:27:21PM +0400, Fedorov Sergey wrote: >> On 04/22/2013 06:57 PM, Stefan Hajnoczi wrote: >>> On Mon, Apr 22, 2013 at 04:26:16PM +0400, Fedorov Sergey wrote: >>>> On 04/22/2013 03:47 PM, Stefan Hajnoczi wrote: >>>>> On Thu, Apr 18, 2013 at 03:31:55PM +0400, Sergey Fedorov wrote: >>>>>> Network hub should always receive incoming packets. Then forward them to >>>>>> the appropriate port queue and let the qemu_send_packet() do the right >>>>>> things. If the destination queue cannot receive the packet it will be >>>>>> appended to the queue. When the receiver call >>>>>> qemu_flush_queued_packets() later the queue will be really flushed and >>>>>> no packets will be stalled in the sender network queue. >>>>>> >>>>>> Signed-off-by: Sergey Fedorov >>>>>> --- >>>>>> net/hub.c | 20 -------------------- >>>>>> 1 file changed, 20 deletions(-) >>>>> What is the point of this change? There is no semantic difference for >>>>> well-behaved net clients. >>>>> >>>>> Does it fix a bug, if so, please include details? >>>>> >>>>> Stefan >>>>> >>>>> >>>> Yes, this fixes a bug. There were packet stalls when using user-mode >>>> networking with USB network device. slirp_output() calls >>>> qemu_send_packet() which eventually calls qemu_net_queue_send(). >>>> qemu_net_queue_send() calls qemu_can_send_packet(), which calls >>>> can_receive() callback of network hub. Then >>>> net_hub_port_can_receive() also calls qemu_can_send_packet() for >>>> each port except packet source port. >>>> >>>> Sometimes USB network device is not able to receive packet and >>>> qemu_can_send_packet() returns false. In my case there is no more >>>> ports and net_hub_port_can_receive() returns false. So >>>> qemu_net_queue_send() call qemu_net_queue_append() instead of >>>> qemu_net_queue_deliver(). qemu_net_queue_append() appends the packet >>>> to the receiving port of the network hub which is not flushed when >>>> USB netork device calls qemu_flush_queued_packets(). It is flushed >>>> only when slirp resend the packet by timeout. >>>> >>>> Actually there is no need in net_hub_port_can_receive() as the >>>> network hub can always receive packets and pass it to its port >>>> network clients with qemu_send_packet(). And if the destination port >>>> network client cannot receive the packet it will be queued in the >>>> *destination* port network client queue. Queued packets from that >>>> queue will be delivered as soon as the network client call >>>> qemu_flush_queued_packets(). >>> Please confirm the bug is still present in qemu.git/master. It should >>> have been fixed by the following commit: >>> >>> commit 199ee608f0d08510b5c6c37f31a7fbff211d63c4 >>> Author: Luigi Rizzo >>> Date: Tue Feb 5 17:53:31 2013 +0100 >>> >>> net: fix qemu_flush_queued_packets() in presence of a hub >>> >>> Stefan >>> >> Yes, this commit fixes a bug but from other side. I think it's >> better to just let the qemu_send_packet() do the right things. >> >> E.g. network hub has 3 ports. Suppose when iterating through port >> list in net_hub_port_can_receive() a packet is successfully >> delivered to the first port, and then is queued in the source port >> queue because the second port cannot receive packets. Later >> net_hub_flush() will flush the packet from the source port queue and >> it will be delivered in every port. But it had been already >> delivered to one of them. So it will be delivered twice to some >> ports. Moreover there is less chance to dequeue the packet if >> several clients can't receive periodically. > Did you mean "second port" instead of "source port" in the beginning? > > I don't see a scenario where the packet is delivered to the same port > twice. If one port can receive then the packet will be sent and other > ports will queue the packet. Whether dump network client can always receive? If so, then the packet will always be sent to the dump client regardless of whether the other clients can receive. Is there actually any synchronization? > Note that the source port will not queue > the packet. When the blocked ports can receive again their send queues > will be flushed. > > Paolo mentioned that we want to provide a consistent view of packets, > especially when -net dump is used. > > Beyond that, we also want to avoid growing net queues indefinitely. If > the hub does not implement .can_receive() then it relies on growing > queues (keeping packets buffered in memory). > > Stefan > -- Best regards, Sergey Fedorov, Junior Software Engineer, Samsung R&D Institute Rus. E-mail: s.fedorov@samsung.com