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

[Jason Wang <jasowang@redhat.com>]

On 09/22/2015 04:21 PM, Yang Hongyang wrote:
>
>
> 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?

Better squash the changes in this patch or another independent patch
before this patch. (We don't want to break bisection).

> so that the first 10 patch can be merged as is. if not, I will send
> another
> version v12 to fix this.

Post V12 or if you wish, I can fix this myself :)

Thanks