Re: [PATCH V2 2/3 net-next] Drivers: hv: vmbus: Fix a bug in the signalling logic with kick_q

[Jason Wang <jasowang@redhat.com>]

On Thu, Mar 12, 2015 at 3:04 AM, K. Y. Srinivasan <kys@microsoft.com> 
wrote:
> When the caller specifies that signalling should be deferred, we need 
> to
> address the case where we are not able to place the current packet 
> because
> the buffer is full. In this case, we will signal the host as some 
> packets
> may have been placed on the ring buffer.
> I would like to thank Jason Wang <jasowang@redhat.com> for pointing
> out this issue.
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
>  drivers/hv/channel.c |   32 ++++++++++++++++++++++++++++++++
>  1 files changed, 32 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index e58cdb7..ae06ba9 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -614,8 +614,24 @@ int vmbus_sendpacket_ctl(struct vmbus_channel 
> *channel, void *buffer,
>  
>  	ret = hv_ringbuffer_write(&channel->outbound, bufferlist, 3, 
> &signal);
>  
> +	/*
> +	 * Here is the logic for signalling the host:
> +	 * 1. If the host is already draining the ringbuffer,
> +	 *    don't signal. This is indicated by the parameter
> +	 *    "signal".
> +	 *
> +	 * 2. If we are not able to write, signal if kick_q is false.
> +	 *    kick_q being false indicates that we may have placed zero or
> +	 *    more packets with more packets to come. We will signal in
> +	 *    this case even if potentially we may have not placed any
> +	 *    packet. This is a rare enough condition that it should not
> +	 *    matter.
> +	 */
> +
>  	if ((ret == 0) && kick_q && signal)
>  		vmbus_setevent(channel);
> +	else if ((ret != 0) && !kick_q)
> +		vmbus_setevent(channel);
>  
>  	return ret;
>  }
> @@ -705,8 +721,24 @@ int vmbus_sendpacket_pagebuffer_ctl(struct 
> vmbus_channel *channel,
>  
>  	ret = hv_ringbuffer_write(&channel->outbound, bufferlist, 3, 
> &signal);
>  
> +	/*
> +	 * Here is the logic for signalling the host:
> +	 * 1. If the host is already draining the ringbuffer,
> +	 *    don't signal. This is indicated by the parameter
> +	 *    "signal".
> +	 *
> +	 * 2. If we are not able to write, signal if kick_q is false.
> +	 *    kick_q being false indicates that we may have placed zero or
> +	 *    more packets with more packets to come. We will signal in
> +	 *    this case even if potentially we may have not placed any
> +	 *    packet. This is a rare enough condition that it should not
> +	 *    matter.
> +	 */
> +
>  	if ((ret == 0) && kick_q && signal)
>  		vmbus_setevent(channel);
> +	else if ((ret != 0) && !kick_q)
> +		vmbus_setevent(channel);
>  
>  	return ret;
>  }
> -- 

Looks like we need to kick unconditionally here. Consider we may get 
-EAGAIN when we want to send the last skb (kick_q is true) from the 
list. We need kick host in this case.

Btw, another method is let the driver to decide e.g exporting the 
vmbus_setevent() and call it in netvsc_start_xmit().