RE: [PATCH 1/1] Drivers: hv: vmbus: Fix signaling logic in hv_need_to_signal_on_read()

RE: [PATCH 1/1] Drivers: hv: vmbus: Fix signaling logic in hv_need_to_signal_on_read()

[KY Srinivasan]

> -----Original Message-----
> From: K. Y. Srinivasan [mailto:kys@microsoft.com]
> Sent: Saturday, April 2, 2016 3:44 PM
> To: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> vkuznets@redhat.com; jasowang@redhat.com
> Cc: KY Srinivasan <kys@microsoft.com>; stable@vger.kernel.org
> Subject: [PATCH 1/1] Drivers: hv: vmbus: Fix signaling logic in
> hv_need_to_signal_on_read()
> 
> On the consumer side, we have interrupt driven flow management of the
> producer. It is sufficient to base the signaling decision on the
> amount of space that is available to write after the read is complete.
> The current code samples the previous available space and uses this
> in making the signaling decision. This state can be stale and is
> unnecessary. Since the state can be stale, we end up not signaling
> the host (when we should) and this can result in a hang. Fix this
> problem by removing the unnecessary check. I would like to thank
> Arseney Romanenko <arseneyr@microsoft.com> for pointing out this issue.
> 
> Also, issue a full memory barrier before making the signaling descision
> to correctly deal with potential reordering of the write (read index)
> followed by the read of pending_sz.

Greg,

Please drop this; I sent the wrong version of the patch. Sorry for
The confusion.

K. Y
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Tested-by: Dexuan Cui <decui@microsoft.com>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/hv/ring_buffer.c |   20 ++++++++++++++++----
>  1 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> index 5613e2b..e00b632 100644
> --- a/drivers/hv/ring_buffer.c
> +++ b/drivers/hv/ring_buffer.c
> @@ -103,8 +103,7 @@ static bool hv_need_to_signal(u32 old_write, struct
> hv_ring_buffer_info *rbi)
>   *    there is room for the producer to send the pending packet.
>   */
> 
> -static bool hv_need_to_signal_on_read(u32 prev_write_sz,
> -				      struct hv_ring_buffer_info *rbi)
> +static bool hv_need_to_signal_on_read(struct hv_ring_buffer_info *rbi)
>  {
>  	u32 cur_write_sz;
>  	u32 r_size;
> @@ -112,6 +111,19 @@ static bool hv_need_to_signal_on_read(u32
> prev_write_sz,
>  	u32 read_loc = rbi->ring_buffer->read_index;
>  	u32 pending_sz = rbi->ring_buffer->pending_send_sz;
> 
> +	/*
> +	 * Issue a full memory barrier before making the signaling decision.
> +	 * Here is the reason for having this barrier:
> +	 * If the reading of the pend_sz (in this function)
> +	 * were to be reordered and read before we commit the new read
> +	 * index (in the calling function)  we could
> +	 * have a problem. If the host were to set the pending_sz after we
> +	 * have sampled pending_sz and go to sleep before we commit the
> +	 * read index, we could miss sending the interrupt. Issue a full
> +	 * memory barrier to address this.
> +	 */
> +	mb();
> +
>  	/* If the other end is not blocked on write don't bother. */
>  	if (pending_sz == 0)
>  		return false;
> @@ -120,7 +132,7 @@ static bool hv_need_to_signal_on_read(u32
> prev_write_sz,
>  	cur_write_sz = write_loc >= read_loc ? r_size - (write_loc - read_loc)
> :
>  			read_loc - write_loc;
> 
> -	if ((prev_write_sz < pending_sz) && (cur_write_sz >= pending_sz))
> +	if (cur_write_sz >= pending_sz)
>  		return true;
> 
>  	return false;
> @@ -455,7 +467,7 @@ int hv_ringbuffer_read(struct hv_ring_buffer_info
> *inring_info,
>  	/* Update the read index */
>  	hv_set_next_read_location(inring_info, next_read_location);
> 
> -	*signal = hv_need_to_signal_on_read(bytes_avail_towrite,
> inring_info);
> +	*signal = hv_need_to_signal_on_read(inring_info);
> 
>  	return ret;
>  }
> --
> 1.7.4.1

[PATCH 1/1] Drivers: hv: vmbus: Fix signaling logic in hv_need_to_signal_on_read()

[K. Y. Srinivasan]

On the consumer side, we have interrupt driven flow management of the
producer. It is sufficient to base the signaling decision on the
amount of space that is available to write after the read is complete.
The current code samples the previous available space and uses this
in making the signaling decision. This state can be stale and is
unnecessary. Since the state can be stale, we end up not signaling
the host (when we should) and this can result in a hang. Fix this
problem by removing the unnecessary check. I would like to thank
Arseney Romanenko <arseneyr@microsoft.com> for pointing out this issue.

Also, issue a full memory barrier before making the signaling descision
to correctly deal with potential reordering of the write (read index)
followed by the read of pending_sz.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Tested-by: Dexuan Cui <decui@microsoft.com>
Cc: <stable@vger.kernel.org>
---
 drivers/hv/ring_buffer.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 5613e2b..e00b632 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -103,8 +103,7 @@ static bool hv_need_to_signal(u32 old_write, struct hv_ring_buffer_info *rbi)
  *    there is room for the producer to send the pending packet.
  */
 
-static bool hv_need_to_signal_on_read(u32 prev_write_sz,
-				      struct hv_ring_buffer_info *rbi)
+static bool hv_need_to_signal_on_read(struct hv_ring_buffer_info *rbi)
 {
 	u32 cur_write_sz;
 	u32 r_size;
@@ -112,6 +111,19 @@ static bool hv_need_to_signal_on_read(u32 prev_write_sz,
 	u32 read_loc = rbi->ring_buffer->read_index;
 	u32 pending_sz = rbi->ring_buffer->pending_send_sz;
 
+	/*
+	 * Issue a full memory barrier before making the signaling decision.
+	 * Here is the reason for having this barrier:
+	 * If the reading of the pend_sz (in this function)
+	 * were to be reordered and read before we commit the new read
+	 * index (in the calling function)  we could
+	 * have a problem. If the host were to set the pending_sz after we
+	 * have sampled pending_sz and go to sleep before we commit the
+	 * read index, we could miss sending the interrupt. Issue a full
+	 * memory barrier to address this.
+	 */
+	mb();
+
 	/* If the other end is not blocked on write don't bother. */
 	if (pending_sz == 0)
 		return false;
@@ -120,7 +132,7 @@ static bool hv_need_to_signal_on_read(u32 prev_write_sz,
 	cur_write_sz = write_loc >= read_loc ? r_size - (write_loc - read_loc) :
 			read_loc - write_loc;
 
-	if ((prev_write_sz < pending_sz) && (cur_write_sz >= pending_sz))
+	if (cur_write_sz >= pending_sz)
 		return true;
 
 	return false;
@@ -455,7 +467,7 @@ int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info,
 	/* Update the read index */
 	hv_set_next_read_location(inring_info, next_read_location);
 
-	*signal = hv_need_to_signal_on_read(bytes_avail_towrite, inring_info);
+	*signal = hv_need_to_signal_on_read(inring_info);
 
 	return ret;
 }
-- 
1.7.4.1

[PATCH 1/1] Drivers: hv: vmbus: Fix signaling logic in hv_need_to_signal_on_read()

[K. Y. Srinivasan]

On the consumer side, we have interrupt driven flow management of the
producer. It is sufficient to base the signaling decision on the
amount of space that is available to write after the read is complete.
The current code samples the previous available space and uses this
in making the signaling decision. This state can be stale and is
unnecessary. Since the state can be stale, we end up not signaling
the host (when we should) and this can result in a hang. Fix this
problem by removing the unnecessary check. I would like to thank
Arseney Romanenko <arseneyr@microsoft.com> for pointing out this issue.

Also, issue a full memory barrier before making the signaling descision
to correctly deal with potential reordering of the write (read index)
followed by the read of pending_sz.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Tested-by: Dexuan Cui <decui@microsoft.com>
Cc: <stable@vger.kernel.org>
---
 drivers/hv/ring_buffer.c |   26 ++++++++++++++++++++------
 1 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 5613e2b..a40a73a 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -103,15 +103,29 @@ static bool hv_need_to_signal(u32 old_write, struct hv_ring_buffer_info *rbi)
  *    there is room for the producer to send the pending packet.
  */
 
-static bool hv_need_to_signal_on_read(u32 prev_write_sz,
-				      struct hv_ring_buffer_info *rbi)
+static bool hv_need_to_signal_on_read(struct hv_ring_buffer_info *rbi)
 {
 	u32 cur_write_sz;
 	u32 r_size;
-	u32 write_loc = rbi->ring_buffer->write_index;
+	u32 write_loc;
 	u32 read_loc = rbi->ring_buffer->read_index;
-	u32 pending_sz = rbi->ring_buffer->pending_send_sz;
+	u32 pending_sz;
 
+	/*
+	 * Issue a full memory barrier before making the signaling decision.
+	 * Here is the reason for having this barrier:
+	 * If the reading of the pend_sz (in this function)
+	 * were to be reordered and read before we commit the new read
+	 * index (in the calling function)  we could
+	 * have a problem. If the host were to set the pending_sz after we
+	 * have sampled pending_sz and go to sleep before we commit the
+	 * read index, we could miss sending the interrupt. Issue a full
+	 * memory barrier to address this.
+	 */
+	mb();
+
+	pending_sz = rbi->ring_buffer->pending_send_sz;
+	write_loc = rbi->ring_buffer->write_index;
 	/* If the other end is not blocked on write don't bother. */
 	if (pending_sz == 0)
 		return false;
@@ -120,7 +134,7 @@ static bool hv_need_to_signal_on_read(u32 prev_write_sz,
 	cur_write_sz = write_loc >= read_loc ? r_size - (write_loc - read_loc) :
 			read_loc - write_loc;
 
-	if ((prev_write_sz < pending_sz) && (cur_write_sz >= pending_sz))
+	if (cur_write_sz >= pending_sz)
 		return true;
 
 	return false;
@@ -455,7 +469,7 @@ int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info,
 	/* Update the read index */
 	hv_set_next_read_location(inring_info, next_read_location);
 
-	*signal = hv_need_to_signal_on_read(bytes_avail_towrite, inring_info);
+	*signal = hv_need_to_signal_on_read(inring_info);
 
 	return ret;
 }
-- 
1.7.4.1