[RFC,6/8] tap: Drop tap_can_send

This callback is called by main loop before polling s->fd, if it returns
false, the fd will not be polled in this iteration.

This is redundant with checks inside read callback. After this patch,
the data will be sent to peer when it arrives. If the device can't
receive, it will be queued to incoming_queue, and when the device status
changes, this queue will be flushed.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 net/tap.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

On 29/04/2015 12:37, Fam Zheng wrote:
> @@ -190,10 +181,13 @@ static void tap_send(void *opaque)
>      TAPState *s = opaque;
>      int size;
>      int packets = 0;
> +    bool can_send = true;
>  
> -    while (qemu_can_send_packet(&s->nc)) {
> +    while (can_send) {

This is effectively a while (true) since can_send is:

- assigned to true on entry to the first iteration

- true on the next iterations as well or you would have exited.

Paolo

On Wed, Apr 29, 2015 at 6:37 PM, Fam Zheng <famz@redhat.com> wrote:
> This callback is called by main loop before polling s->fd, if it 
> returns
> false, the fd will not be polled in this iteration.
> 
> This is redundant with checks inside read callback. After this patch,
> the data will be sent to peer when it arrives. If the device can't
> receive, it will be queued to incoming_queue, and when the device 
> status
> changes, this queue will be flushed.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  net/tap.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/net/tap.c b/net/tap.c
> index 968df46..2ddf570 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -61,14 +61,12 @@ typedef struct TAPState {
>  
>  static int launch_script(const char *setup_script, const char 
> *ifname, int fd);
>  
> -static int tap_can_send(void *opaque);
>  static void tap_send(void *opaque);
>  static void tap_writable(void *opaque);
>  
>  static void tap_update_fd_handler(TAPState *s)
>  {
> -    qemu_set_fd_handler2(s->fd,
> -                         s->read_poll && s->enabled ? tap_can_send : 
> NULL,
> +    qemu_set_fd_handler2(s->fd, NULL,
>                           s->read_poll && s->enabled ? tap_send     : 
> NULL,
>                           s->write_poll && s->enabled ? tap_writable 
> : NULL,
>                           s);
> @@ -165,13 +163,6 @@ static ssize_t tap_receive(NetClientState *nc, 
> const uint8_t *buf, size_t size)
>      return tap_write_packet(s, iov, 1);
>  }
>  
> -static int tap_can_send(void *opaque)
> -{
> -    TAPState *s = opaque;
> -
> -    return qemu_can_send_packet(&s->nc);
> -}
> -
>  #ifndef __sun__
>  ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen)
>  {
> @@ -190,10 +181,13 @@ static void tap_send(void *opaque)
>      TAPState *s = opaque;
>      int size;
>      int packets = 0;
> +    bool can_send = true;
>  
> -    while (qemu_can_send_packet(&s->nc)) {
> +    while (can_send) {
>          uint8_t *buf = s->buf;
>  
> +        can_send = qemu_can_send_packet(&s->nc);
> +
>          size = tap_read_packet(s->fd, s->buf, sizeof(s->buf));
>          if (size <= 0) {
>              break;
> @@ -204,8 +198,13 @@ static void tap_send(void *opaque)
>              size -= s->host_vnet_hdr_len;
>          }
>  
> +        /* If !can_send, we will want to disable the read poll, but 
> we still
> +         * need the send completion callback to enable it again, 
> which is a
> +         * sign of peer becoming ready.  So call the send function
> +         * regardlessly of can_send.
> +         */

It was probably not safe to depend on sent_cb to re-enable the polling. 
Since the packet could be purged in some conditions (e.g 
net_vm_change_state_handler()). So tap_send_completed won't be called 
in this case.

> 
>          size = qemu_send_packet_async(&s->nc, buf, size, 
> tap_send_completed);
> -        if (size == 0) {
> +        if (size == 0 || !can_send) {
>              tap_read_poll(s, false);
>              break;
>          } else if (size < 0) {
> -- 
> 1.9.3
>

On 30/04/2015 10:59, Jason Wang wrote:
>>
>>  
>> +        /* If !can_send, we will want to disable the read poll, but
>> we still
>> +         * need the send completion callback to enable it again,
>> which is a
>> +         * sign of peer becoming ready.  So call the send function
>> +         * regardlessly of can_send.
>> +         */
> 
> It was probably not safe to depend on sent_cb to re-enable the polling.
> Since the packet could be purged in some conditions (e.g
> net_vm_change_state_handler()). So tap_send_completed won't be called in
> this case.

Doesn't qemu_net_queue_purge also call the sent_cb?

Paolo

On Thu, Apr 30, 2015 at 6:32 PM, Paolo Bonzini <pbonzini@redhat.com> 
wrote:
> 
> 
> On 30/04/2015 10:59, Jason Wang wrote:
>>> 
>>>   
>>>  +        /* If !can_send, we will want to disable the read poll, 
>>> but
>>>  we still
>>>  +         * need the send completion callback to enable it again,
>>>  which is a
>>>  +         * sign of peer becoming ready.  So call the send function
>>>  +         * regardlessly of can_send.
>>>  +         */
>>  
>>  It was probably not safe to depend on sent_cb to re-enable the 
>> polling.
>>  Since the packet could be purged in some conditions (e.g
>>  net_vm_change_state_handler()). So tap_send_completed won't be 
>> called in
>>  this case.
> 
> Doesn't qemu_net_queue_purge also call the sent_cb?
> 
> Paolo

I was wrong. It calls.