From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:45717) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RJjHk-0002K5-QF for qemu-devel@nongnu.org; Fri, 28 Oct 2011 06:03:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RJjHj-0007EG-L2 for qemu-devel@nongnu.org; Fri, 28 Oct 2011 06:03:36 -0400 Received: from e28smtp05.in.ibm.com ([122.248.162.5]:47401) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RJjHi-0007C4-Sn for qemu-devel@nongnu.org; Fri, 28 Oct 2011 06:03:35 -0400 Received: from d28relay03.in.ibm.com (d28relay03.in.ibm.com [9.184.220.60]) by e28smtp05.in.ibm.com (8.14.4/8.13.1) with ESMTP id p9SA3UIc012209 for ; Fri, 28 Oct 2011 15:33:30 +0530 Received: from d28av03.in.ibm.com (d28av03.in.ibm.com [9.184.220.65]) by d28relay03.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p9SA3Ukx3915840 for ; Fri, 28 Oct 2011 15:33:30 +0530 Received: from d28av03.in.ibm.com (loopback [127.0.0.1]) by d28av03.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p9SA3TdI005804 for ; Fri, 28 Oct 2011 21:03:29 +1100 Received: from oc4654482034.ibm.com ([9.115.122.78]) by d28av03.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with ESMTP id p9SA3T4n003408 for ; Fri, 28 Oct 2011 21:03:29 +1100 Message-ID: <4EAA7DBF.8000306@linux.vnet.ibm.com> Date: Fri, 28 Oct 2011 18:02:39 +0800 From: Mark Wu MIME-Version: 1.0 References: <1319706145-26599-1-git-send-email-wudxw@linux.vnet.ibm.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] net: Only flush queue or call sent callback on successful delivery List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org On 10/28/2011 05:13 PM, Stefan Hajnoczi wrote: > On Thu, Oct 27, 2011 at 10:02 AM, Mark Wu wrote: >> Now queue flushing and sent callback could be invoked even on delivery >> failure. We add a checking of receiver's return value to avoid this >> case. >> >> Signed-off-by: Mark Wu >> --- >> net/queue.c | 12 +++++++----- >> 1 files changed, 7 insertions(+), 5 deletions(-) > What problem are you trying to fix? > >> @@ -251,7 +253,7 @@ void qemu_net_queue_flush(NetQueue *queue) >> break; >> } >> >> - if (packet->sent_cb) { >> + if (ret> 0&& packet->sent_cb) { >> packet->sent_cb(packet->sender, ret); > This looks wrong. ret is passed as an argument to the callback. You > are skipping the callback on error and not giving it a chance to see > negative ret. > > Looking at virtio_net_tx_complete() this causes a virtqueue element leak. Thanks for your review! Yes, that's a problem. I thought only tap call queue send function with a callback (tap_send_completed) and confirmed that no memory leak in the case of tap. I agree that it will cause a descriptor leak, but actually virtio_net_tx_complete doesn't check 'ret'. It just pushes the elem to the virtqueue with 'async_tx.len' and flushes the tx queue. I think it assumes the callback is only called on success. Otherwise, it doesn't make sense for me. My point is that flush shouldn't happen on a deliver failure. Probably it will cause more failures. tap_send_completed assumes it's called on successfully deliver a packet too because it re-enables polling of tap fd. That's why I add a checking of 'ret'. I am not sure if the original code really needs a fix because it will not cause any visible problems. > Stefan >