Re: [PATCH 1/2] Drivers: hv: vmbus: Fix a Host signaling bug

[Vitaly Kuznetsov <vkuznets@redhat.com>]

"K. Y. Srinivasan" <kys@microsoft.com> writes:

> Currently we have two policies for deciding when to signal the host:
> One based on the ring buffer state and the other based on what the
> VMBUS client driver wants to do. Consider the case when the client
> wants to explicitly control when to signal the host. In this case,
> if the client were to defer signaling, we will not be able to signal
> the host subsequently when the client does want to signal since the
> ring buffer state will prevent the signaling. Implement logic to
> have only one signaling policy in force for a given channel.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> Tested-by: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: <stable@vger.kernel.org> # v4.2+
> ---
>  drivers/hv/channel.c   |   18 ++++++++++++++++++
>  include/linux/hyperv.h |   12 ++++++++++++
>  2 files changed, 30 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 77d2579..c6278c7 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -653,10 +653,19 @@ int vmbus_sendpacket_ctl(struct vmbus_channel *channel, void *buffer,
>  	 *    on the ring. We will not signal if more data is
>  	 *    to be placed.
>  	 *
> +	 * Based on the channel signal state, we will decide
> +	 * which signaling policy will be applied.
> +	 *
>  	 * If we cannot write to the ring-buffer; signal the host
>  	 * even if we may not have written anything. This is a rare
>  	 * enough condition that it should not matter.
>  	 */
> +
> +	if (channel->signal_state)
> +		signal = true;
> +	else
> +		kick_q = true;
> +
>  	if (((ret == 0) && kick_q && signal) || (ret))
>  		vmbus_setevent(channel);
>
> @@ -756,10 +765,19 @@ int vmbus_sendpacket_pagebuffer_ctl(struct vmbus_channel *channel,
>  	 *    on the ring. We will not signal if more data is
>  	 *    to be placed.
>  	 *
> +	 * Based on the channel signal state, we will decide
> +	 * which signaling policy will be applied.
> +	 *
>  	 * If we cannot write to the ring-buffer; signal the host
>  	 * even if we may not have written anything. This is a rare
>  	 * enough condition that it should not matter.
>  	 */
> +
> +	if (channel->signal_state)
> +		signal = true;
> +	else
> +		kick_q = true;
> +
>  	if (((ret == 0) && kick_q && signal) || (ret))
>  		vmbus_setevent(channel);
>
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 437c9c8..7b1af52 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -756,8 +756,20 @@ struct vmbus_channel {
>  	 * link up channels based on their CPU affinity.
>  	 */
>  	struct list_head percpu_list;
> +	/*
> +	 * Host signaling policy: The default policy will be
> +	 * based on the ring buffer state. We will also support
> +	 * a policy where the client driver can have explicit
> +	 * signaling control.
> +	 */
> +	bool signal_state;

Making policy selector 'bool' is counter-intuitive: I suggest we rename
this to somethink like 'signal_explicit' if we're sure there won't be
new policies or (even better in my opinion) introduce a new enum with
these policies, e.g:

       enum hv_channel_policy signal_policy;

where

enum hv_channel_policy {
      HV_CHANNELPOLICY_DEFAULT,
      HV_CHANNELPOLICY_EXPLICIT,
};

>  };
>
> +static inline void set_channel_signal_state(struct vmbus_channel *c, bool state)
> +{
> +	c->signal_state = state;
> +}
> +
>  static inline void set_channel_read_state(struct vmbus_channel *c, bool state)
>  {
>  	c->batched_reading = state;

-- 
  Vitaly