Re: [Qemu-devel] [PATCH v2 15/15] net: invoke qemu_can_send_packet only before net queue sending function

[Paolo Bonzini]

Il 24/05/2012 06:05, Zhi Yong Wu ha scritto:
> On Thu, May 24, 2012 at 12:00 AM, Paolo Bonzini <address@hidden> wrote:
>> Il 23/05/2012 17:14, address@hidden ha scritto:
>>> From: Zhi Yong Wu <address@hidden>
>>>
>>> Signed-off-by: Zhi Yong Wu <address@hidden>
>>> ---
>>>  net/queue.c      |    4 ++--
>>>  net/slirp.c      |    7 -------
>>>  net/tap.c        |    2 +-
>>>  slirp/if.c       |    5 -----
>>>  slirp/libslirp.h |    1 -
>>>  5 files changed, 3 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/net/queue.c b/net/queue.c
>>> index 0afd783..d2e57de 100644
>>> --- a/net/queue.c
>>> +++ b/net/queue.c
>>> @@ -176,7 +176,7 @@ ssize_t qemu_net_queue_send(NetQueue *queue,
>>>  {
>>>      ssize_t ret;
>>>
>>> -    if (queue->delivering) {
>>> +    if (queue->delivering || !qemu_can_send_packet(sender)) {
>>>          return qemu_net_queue_append(queue, sender, flags, data, size, 
>>> NULL);
>>>      }
>>>
>>> @@ -200,7 +200,7 @@ ssize_t qemu_net_queue_send_iov(NetQueue *queue,
>>>  {
>>>      ssize_t ret;
>>>
>>> -    if (queue->delivering) {
>>> +    if (queue->delivering || !qemu_can_send_packet(sender)) {
>>>          return qemu_net_queue_append_iov(queue, sender, flags, iov, 
>>> iovcnt, NULL);
>>>      }
>>>
>>> diff --git a/net/slirp.c b/net/slirp.c
>>> index a6ede2b..248f7ff 100644
>>> --- a/net/slirp.c
>>> +++ b/net/slirp.c
>>> @@ -96,13 +96,6 @@ static void slirp_smb_cleanup(SlirpState *s);
>>>  static inline void slirp_smb_cleanup(SlirpState *s) { }
>>>  #endif
>>>
>>> -int slirp_can_output(void *opaque)
>>> -{
>>> -    SlirpState *s = opaque;
>>> -
>>> -    return qemu_can_send_packet(&s->nc);
>>> -}
>>> -
>>>  void slirp_output(void *opaque, const uint8_t *pkt, int pkt_len)
>>>  {
>>>      SlirpState *s = opaque;
>>> diff --git a/net/tap.c b/net/tap.c
>>> index 65f45b8..7b1992b 100644
>>> --- a/net/tap.c
>>> +++ b/net/tap.c
>>> @@ -210,7 +210,7 @@ static void tap_send(void *opaque)
>>>          if (size == 0) {
>>>              tap_read_poll(s, 0);
>>>          }
>>> -    } while (size > 0 && qemu_can_send_packet(&s->nc));
>>> +    } while (size > 0);
>>
>> Can you explain this?  Also, have you benchmarked the change to see what
> Its code execution flow is like below:
> tap_send --> qemu_send_packet_async
> ->qemu_send_packet_async_with_flags ->qemu_net_queue_send
> 
> So it will finally invoke qemu_can_send_packet to determine if it can
> send packets. this code change delay this determination.

But you will copy packets uselessly.  The code before the patch simply
left them on the tap file descriptor.  This is better because it
involves the kernel in flow control.  You are introducing "bufferbloat".

>> Also, can you explain why you didn't implement this?
> Hub can now do its own flow control if it provides its can_recieve.

But you didn't add can_receive.

> Why need we add some counts to track in-flight packets?

To implement can_receive.

Paolo