From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55176) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZeIpj-0007U3-8O for qemu-devel@nongnu.org; Tue, 22 Sep 2015 04:21:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZeIpg-0001iU-1h for qemu-devel@nongnu.org; Tue, 22 Sep 2015 04:21:51 -0400 Received: from [59.151.112.132] (port=28106 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZeIpd-0001g5-1F for qemu-devel@nongnu.org; Tue, 22 Sep 2015 04:21:47 -0400 Message-ID: <56010F8F.6030006@cn.fujitsu.com> Date: Tue, 22 Sep 2015 16:21:35 +0800 From: Yang Hongyang MIME-Version: 1.0 References: <1442405768-23019-1-git-send-email-yanghy@cn.fujitsu.com> <1442405768-23019-5-git-send-email-yanghy@cn.fujitsu.com> <56010391.4070806@redhat.com> <560106C2.5080607@cn.fujitsu.com> <56010DEF.1050501@redhat.com> In-Reply-To: <56010DEF.1050501@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v11 04/12] net: merge qemu_deliver_packet and qemu_deliver_packet_iov List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang , qemu-devel@nongnu.org Cc: lizhijian@cn.fujitsu.com, thuth@redhat.com, armbru@redhat.com, stefanha@redhat.com, zhang.zhanghailiang@huawei.com On 09/22/2015 04:14 PM, Jason Wang wrote: > > > On 09/22/2015 03:44 PM, Yang Hongyang wrote: >> Hi Jason, >> >> Thanks for the review, >> >> On 09/22/2015 03:30 PM, Jason Wang wrote: >>> >>> >>> On 09/16/2015 08:16 PM, Yang Hongyang wrote: >>>> qemu_deliver_packet_iov already have the compat delivery, we >>>> can drop qemu_deliver_packet. >>>> >>>> Signed-off-by: Yang Hongyang >>>> --- >>>> include/net/net.h | 5 ----- >>>> net/net.c | 40 +++++++--------------------------------- >>>> net/queue.c | 6 +++++- >>>> 3 files changed, 12 insertions(+), 39 deletions(-) >>>> >>>> diff --git a/include/net/net.h b/include/net/net.h >>>> index 36e5fab..7af3e15 100644 >>>> --- a/include/net/net.h >>>> +++ b/include/net/net.h >>>> @@ -152,11 +152,6 @@ void qemu_check_nic_model(NICInfo *nd, const >>>> char *model); >>>> int qemu_find_nic_model(NICInfo *nd, const char * const *models, >>>> const char *default_model); >>>> >>>> -ssize_t qemu_deliver_packet(NetClientState *sender, >>>> - unsigned flags, >>>> - const uint8_t *data, >>>> - size_t size, >>>> - void *opaque); >>>> ssize_t qemu_deliver_packet_iov(NetClientState *sender, >>>> unsigned flags, >>>> const struct iovec *iov, >>>> diff --git a/net/net.c b/net/net.c >>>> index ad37419..ca35b27 100644 >>>> --- a/net/net.c >>>> +++ b/net/net.c >>>> @@ -597,36 +597,6 @@ static ssize_t filter_receive(NetClientState >>>> *nc, NetFilterChain chain, >>>> return filter_receive_iov(nc, chain, sender, flags, &iov, 1, >>>> sent_cb); >>>> } >>>> >>>> -ssize_t qemu_deliver_packet(NetClientState *sender, >>>> - unsigned flags, >>>> - const uint8_t *data, >>>> - size_t size, >>>> - void *opaque) >>>> -{ >>>> - NetClientState *nc = opaque; >>>> - ssize_t ret; >>>> - >>>> - if (nc->link_down) { >>>> - return size; >>>> - } >>>> - >>>> - if (nc->receive_disabled) { >>>> - return 0; >>>> - } >>>> - >>>> - if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) { >>>> - ret = nc->info->receive_raw(nc, data, size); >>>> - } else { >>>> - ret = nc->info->receive(nc, data, size); >>>> - } >>>> - >>>> - if (ret == 0) { >>>> - nc->receive_disabled = 1; >>>> - } >>>> - >>>> - return ret; >>>> -} >>>> - >>>> void qemu_purge_queued_packets(NetClientState *nc) >>>> { >>>> if (!nc->peer) { >>>> @@ -717,14 +687,18 @@ ssize_t qemu_send_packet_raw(NetClientState >>>> *nc, const uint8_t *buf, int size) >>>> } >>>> >>>> static ssize_t nc_sendv_compat(NetClientState *nc, const struct >>>> iovec *iov, >>>> - int iovcnt) >>>> + int iovcnt, unsigned flags) >>>> { >>>> uint8_t buffer[NET_BUFSIZE]; >>>> size_t offset; >>>> >>>> offset = iov_to_buf(iov, iovcnt, 0, buffer, sizeof(buffer)); >>>> >>>> - return nc->info->receive(nc, buffer, offset); >>>> + if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) { >>>> + return nc->info->receive_raw(nc, buffer, offset); >>>> + } else { >>>> + return nc->info->receive(nc, buffer, offset); >>>> + } >>> >>> Then for net clients that doesn't support receive_iov, an extra memcpy >>> is introduced. This seems unnecessary. And this patch looks independent, >>> so please drop this patch from the series (This can also help to reduce >>> the iteration). I think we may need this after all net clients support >>> receive_iov(). >> >> This patch is required by the next patch which introduce a >> +/* Returns: >> + * >0 - success >> + * 0 - queue packet for future redelivery >> + * <0 - failure (discard packet) >> + */ >> +typedef ssize_t (NetQueueDeliverFunc)(NetClientState *sender, >> + unsigned flags, >> + const struct iovec *iov, >> + int iovcnt, >> + void *opaque); >> >> If we drop this patch, we will need to introduce 2 deliver func, which >> seems not clean and simple. > > Then you can probably skip the iov_to_buf() if iov_cnt is one in the > above code? Seems like a good idea to avoid the extra memcpy, thank you! BTW, can I do this on top of the series if there's no other comments? so that the first 10 patch can be merged as is. if not, I will send another version v12 to fix this. > >> >>> >>> >>>> } >>>> >>>> ssize_t qemu_deliver_packet_iov(NetClientState *sender, >>>> @@ -747,7 +721,7 @@ ssize_t qemu_deliver_packet_iov(NetClientState >>>> *sender, >>>> if (nc->info->receive_iov) { >>>> ret = nc->info->receive_iov(nc, iov, iovcnt); >>>> } else { >>>> - ret = nc_sendv_compat(nc, iov, iovcnt); >>>> + ret = nc_sendv_compat(nc, iov, iovcnt, flags); >>>> } >>>> >>>> if (ret == 0) { >>>> diff --git a/net/queue.c b/net/queue.c >>>> index ebbe2bb..cf8db3a 100644 >>>> --- a/net/queue.c >>>> +++ b/net/queue.c >>>> @@ -152,9 +152,13 @@ static ssize_t qemu_net_queue_deliver(NetQueue >>>> *queue, >>>> size_t size) >>>> { >>>> ssize_t ret = -1; >>>> + struct iovec iov = { >>>> + .iov_base = (void *)data, >>>> + .iov_len = size >>>> + }; >>>> >>>> queue->delivering = 1; >>>> - ret = qemu_deliver_packet(sender, flags, data, size, >>>> queue->opaque); >>>> + ret = qemu_deliver_packet_iov(sender, flags, &iov, 1, >>>> queue->opaque); >>>> queue->delivering = 0; >>>> >>>> return ret; >>> >>> >>> . >>> >> > > . > -- Thanks, Yang.