Re: [Qemu-devel] [PATCH v11 04/12] net: merge qemu_deliver_packet and qemu_deliver_packet_iov

[Yang Hongyang <yanghy@cn.fujitsu.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 <yanghy@cn.fujitsu.com>
>>>> ---
>>>>    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.