[PATCH 0/7] Drivers: hv: vmbus: Cleanup the ring buffer code

[PATCH 0/7] Drivers: hv: vmbus: Cleanup the ring buffer code

[K. Y. Srinivasan]

Cleanup and fix a bug in the ring buffer code. Also implement
APIs for in place consumption of received packets.

K. Y. Srinivasan (7):
  Drivers: hv: vmbus: Introduce functions for estimating room in the
    ring buffer
  Drivers: hv: vmbus: Use READ_ONCE() to read variables that are
    volatile
  Drivers: hv: vmbus: Fix a bug in hv_need_to_signal_on_read()
  Drivers: hv: vmbus: Use the new virt_xx barrier code
  Drivers: hv: vmbus: Export the vmbus_set_event() API
  Drivers: hv: vmbus: Move some ring buffer functions to hyperv.h
  Drivers: hv: vmbus: Implement APIs to support "in place" consumption
    of vmbus packets

 drivers/hv/connection.c   |    1 +
 drivers/hv/hyperv_vmbus.h |    2 -
 drivers/hv/ring_buffer.c  |   79 ++++-------------------
 include/linux/hyperv.h    |  156 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 169 insertions(+), 69 deletions(-)

-- 
1.7.4.1

[PATCH 1/7] Drivers: hv: vmbus: Introduce functions for estimating room in the ring buffer

[K. Y. Srinivasan]

Introduce separate functions for estimating how much can be read from
and written to the ring buffer.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/ring_buffer.c |   24 ++++--------------------
 include/linux/hyperv.h   |   27 +++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 085003a..902375b 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -38,8 +38,6 @@ void hv_begin_read(struct hv_ring_buffer_info *rbi)
 
 u32 hv_end_read(struct hv_ring_buffer_info *rbi)
 {
-	u32 read;
-	u32 write;
 
 	rbi->ring_buffer->interrupt_mask = 0;
 	mb();
@@ -49,9 +47,7 @@ u32 hv_end_read(struct hv_ring_buffer_info *rbi)
 	 * If it is not, we raced and we need to process new
 	 * incoming messages.
 	 */
-	hv_get_ringbuffer_availbytes(rbi, &read, &write);
-
-	return read;
+	return hv_get_bytes_to_read(rbi);
 }
 
 /*
@@ -106,18 +102,13 @@ static bool hv_need_to_signal(u32 old_write, 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 read_loc = rbi->ring_buffer->read_index;
 	u32 pending_sz = rbi->ring_buffer->pending_send_sz;
 
 	/* If the other end is not blocked on write don't bother. */
 	if (pending_sz == 0)
 		return false;
 
-	r_size = rbi->ring_datasize;
-	cur_write_sz = write_loc >= read_loc ? r_size - (write_loc - read_loc) :
-			read_loc - write_loc;
+	cur_write_sz = hv_get_bytes_to_write(rbi);
 
 	if (cur_write_sz >= pending_sz)
 		return true;
@@ -317,7 +308,6 @@ int hv_ringbuffer_write(struct hv_ring_buffer_info *outring_info,
 {
 	int i = 0;
 	u32 bytes_avail_towrite;
-	u32 bytes_avail_toread;
 	u32 totalbytes_towrite = 0;
 
 	u32 next_write_location;
@@ -333,9 +323,7 @@ int hv_ringbuffer_write(struct hv_ring_buffer_info *outring_info,
 	if (lock)
 		spin_lock_irqsave(&outring_info->ring_lock, flags);
 
-	hv_get_ringbuffer_availbytes(outring_info,
-				&bytes_avail_toread,
-				&bytes_avail_towrite);
+	bytes_avail_towrite = hv_get_bytes_to_write(outring_info);
 
 	/*
 	 * If there is only room for the packet, assume it is full.
@@ -386,7 +374,6 @@ int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info,
 		       void *buffer, u32 buflen, u32 *buffer_actual_len,
 		       u64 *requestid, bool *signal, bool raw)
 {
-	u32 bytes_avail_towrite;
 	u32 bytes_avail_toread;
 	u32 next_read_location = 0;
 	u64 prev_indices = 0;
@@ -402,10 +389,7 @@ int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info,
 	*buffer_actual_len = 0;
 	*requestid = 0;
 
-	hv_get_ringbuffer_availbytes(inring_info,
-				&bytes_avail_toread,
-				&bytes_avail_towrite);
-
+	bytes_avail_toread = hv_get_bytes_to_read(inring_info);
 	/* Make sure there is something to read */
 	if (bytes_avail_toread < sizeof(desc)) {
 		/*
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index ecd81c3..a6b053c 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -151,6 +151,33 @@ hv_get_ringbuffer_availbytes(struct hv_ring_buffer_info *rbi,
 	*read = dsize - *write;
 }
 
+static inline u32 hv_get_bytes_to_read(struct hv_ring_buffer_info *rbi)
+{
+	u32 read_loc, write_loc, dsize, read;
+
+	dsize = rbi->ring_datasize;
+	read_loc = rbi->ring_buffer->read_index;
+	write_loc = READ_ONCE(rbi->ring_buffer->write_index);
+
+	read = write_loc >= read_loc ? (write_loc - read_loc) :
+		(dsize - read_loc) + write_loc;
+
+	return read;
+}
+
+static inline u32 hv_get_bytes_to_write(struct hv_ring_buffer_info *rbi)
+{
+	u32 read_loc, write_loc, dsize, write;
+
+	dsize = rbi->ring_datasize;
+	read_loc = READ_ONCE(rbi->ring_buffer->read_index);
+	write_loc = rbi->ring_buffer->write_index;
+
+	write = write_loc >= read_loc ? dsize - (write_loc - read_loc) :
+		read_loc - write_loc;
+	return write;
+}
+
 /*
  * VMBUS version is 32 bit entity broken up into
  * two 16 bit quantities: major_number. minor_number.
-- 
1.7.4.1

[PATCH 2/7] Drivers: hv: vmbus: Use READ_ONCE() to read variables that are volatile

[K. Y. Srinivasan]

Use the READ_ONCE macro to access variabes that can change asynchronously.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/ring_buffer.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 902375b..2919395 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -69,7 +69,7 @@ u32 hv_end_read(struct hv_ring_buffer_info *rbi)
 static bool hv_need_to_signal(u32 old_write, struct hv_ring_buffer_info *rbi)
 {
 	mb();
-	if (rbi->ring_buffer->interrupt_mask)
+	if (READ_ONCE(rbi->ring_buffer->interrupt_mask))
 		return false;
 
 	/* check interrupt_mask before read_index */
@@ -78,7 +78,7 @@ static bool hv_need_to_signal(u32 old_write, struct hv_ring_buffer_info *rbi)
 	 * This is the only case we need to signal when the
 	 * ring transitions from being empty to non-empty.
 	 */
-	if (old_write == rbi->ring_buffer->read_index)
+	if (old_write == READ_ONCE(rbi->ring_buffer->read_index))
 		return true;
 
 	return false;
@@ -102,8 +102,9 @@ static bool hv_need_to_signal(u32 old_write, 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 pending_sz = rbi->ring_buffer->pending_send_sz;
+	u32 pending_sz;
 
+	pending_sz = READ_ONCE(rbi->ring_buffer->pending_send_sz);
 	/* If the other end is not blocked on write don't bother. */
 	if (pending_sz == 0)
 		return false;
-- 
1.7.4.1

[PATCH 3/7] Drivers: hv: vmbus: Fix a bug in hv_need_to_signal_on_read()

[K. Y. Srinivasan]

We need to issue a full memory barrier prior making a signalling decision.

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

diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 2919395..67dc245 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -104,6 +104,7 @@ static bool hv_need_to_signal_on_read(struct hv_ring_buffer_info *rbi)
 	u32 cur_write_sz;
 	u32 pending_sz;
 
+	mb();
 	pending_sz = READ_ONCE(rbi->ring_buffer->pending_send_sz);
 	/* If the other end is not blocked on write don't bother. */
 	if (pending_sz == 0)
-- 
1.7.4.1

[PATCH 4/7] Drivers: hv: vmbus: Use the new virt_xx barrier code

[K. Y. Srinivasan]

Use the virt_xx barriers that have been defined for use in virtual machines.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/ring_buffer.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 67dc245..c2c2b2e 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -33,14 +33,14 @@
 void hv_begin_read(struct hv_ring_buffer_info *rbi)
 {
 	rbi->ring_buffer->interrupt_mask = 1;
-	mb();
+	virt_mb();
 }
 
 u32 hv_end_read(struct hv_ring_buffer_info *rbi)
 {
 
 	rbi->ring_buffer->interrupt_mask = 0;
-	mb();
+	virt_mb();
 
 	/*
 	 * Now check to see if the ring buffer is still empty.
@@ -68,12 +68,12 @@ u32 hv_end_read(struct hv_ring_buffer_info *rbi)
 
 static bool hv_need_to_signal(u32 old_write, struct hv_ring_buffer_info *rbi)
 {
-	mb();
+	virt_mb();
 	if (READ_ONCE(rbi->ring_buffer->interrupt_mask))
 		return false;
 
 	/* check interrupt_mask before read_index */
-	rmb();
+	virt_rmb();
 	/*
 	 * This is the only case we need to signal when the
 	 * ring transitions from being empty to non-empty.
@@ -104,7 +104,7 @@ static bool hv_need_to_signal_on_read(struct hv_ring_buffer_info *rbi)
 	u32 cur_write_sz;
 	u32 pending_sz;
 
-	mb();
+	virt_mb();
 	pending_sz = READ_ONCE(rbi->ring_buffer->pending_send_sz);
 	/* If the other end is not blocked on write don't bother. */
 	if (pending_sz == 0)
@@ -359,7 +359,7 @@ int hv_ringbuffer_write(struct hv_ring_buffer_info *outring_info,
 					     sizeof(u64));
 
 	/* Issue a full memory barrier before updating the write index */
-	mb();
+	virt_mb();
 
 	/* Now, update the write location */
 	hv_set_next_write_location(outring_info, next_write_location);
@@ -435,7 +435,7 @@ int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info,
 	 * the writer may start writing to the read area once the read index
 	 * is updated.
 	 */
-	mb();
+	virt_mb();
 
 	/* Update the read index */
 	hv_set_next_read_location(inring_info, next_read_location);
-- 
1.7.4.1

[PATCH 5/7] Drivers: hv: vmbus: Export the vmbus_set_event() API

[K. Y. Srinivasan]

In preparation for moving some ring buffer functionality out of the
vmbus driver, export the API for signaling the host.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/connection.c   |    1 +
 drivers/hv/hyperv_vmbus.h |    2 --
 include/linux/hyperv.h    |    1 +
 3 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index d02f137..fcf8a02 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -495,3 +495,4 @@ void vmbus_set_event(struct vmbus_channel *channel)
 
 	hv_do_hypercall(HVCALL_SIGNAL_EVENT, channel->sig_event, NULL);
 }
+EXPORT_SYMBOL_GPL(vmbus_set_event);
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 28e9df9..8cbd630 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -678,8 +678,6 @@ void vmbus_disconnect(void);
 
 int vmbus_post_msg(void *buffer, size_t buflen);
 
-void vmbus_set_event(struct vmbus_channel *channel);
-
 void vmbus_on_event(unsigned long data);
 void vmbus_on_msg_dpc(unsigned long data);
 
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index a6b053c..4adeb6e 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1365,4 +1365,5 @@ extern __u32 vmbus_proto_version;
 
 int vmbus_send_tl_connect_request(const uuid_le *shv_guest_servie_id,
 				  const uuid_le *shv_host_servie_id);
+void vmbus_set_event(struct vmbus_channel *channel);
 #endif /* _HYPERV_H */
-- 
1.7.4.1

[PATCH 6/7] Drivers: hv: vmbus: Move some ring buffer functions to hyperv.h

[K. Y. Srinivasan]

In preparation for implementing APIs for in-place consumption of VMBUS
packets, movve some ring buffer functionality into hyperv.h

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/ring_buffer.c |   42 ------------------------------------------
 include/linux/hyperv.h   |   42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index c2c2b2e..253311b 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -84,40 +84,6 @@ static bool hv_need_to_signal(u32 old_write, struct hv_ring_buffer_info *rbi)
 	return false;
 }
 
-/*
- * To optimize the flow management on the send-side,
- * when the sender is blocked because of lack of
- * sufficient space in the ring buffer, potential the
- * consumer of the ring buffer can signal the producer.
- * This is controlled by the following parameters:
- *
- * 1. pending_send_sz: This is the size in bytes that the
- *    producer is trying to send.
- * 2. The feature bit feat_pending_send_sz set to indicate if
- *    the consumer of the ring will signal when the ring
- *    state transitions from being full to a state where
- *    there is room for the producer to send the pending packet.
- */
-
-static bool hv_need_to_signal_on_read(struct hv_ring_buffer_info *rbi)
-{
-	u32 cur_write_sz;
-	u32 pending_sz;
-
-	virt_mb();
-	pending_sz = READ_ONCE(rbi->ring_buffer->pending_send_sz);
-	/* If the other end is not blocked on write don't bother. */
-	if (pending_sz == 0)
-		return false;
-
-	cur_write_sz = hv_get_bytes_to_write(rbi);
-
-	if (cur_write_sz >= pending_sz)
-		return true;
-
-	return false;
-}
-
 /* Get the next write location for the specified ring buffer. */
 static inline u32
 hv_get_next_write_location(struct hv_ring_buffer_info *ring_info)
@@ -169,14 +135,6 @@ hv_set_next_read_location(struct hv_ring_buffer_info *ring_info,
 }
 
 
-/* Get the start of the ring buffer. */
-static inline void *
-hv_get_ring_buffer(struct hv_ring_buffer_info *ring_info)
-{
-	return (void *)ring_info->ring_buffer->buffer;
-}
-
-
 /* Get the size of the ring buffer. */
 static inline u32
 hv_get_ring_buffersize(struct hv_ring_buffer_info *ring_info)
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 4adeb6e..8fc9b09 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1366,4 +1366,46 @@ extern __u32 vmbus_proto_version;
 int vmbus_send_tl_connect_request(const uuid_le *shv_guest_servie_id,
 				  const uuid_le *shv_host_servie_id);
 void vmbus_set_event(struct vmbus_channel *channel);
+
+/* Get the start of the ring buffer. */
+static inline void *
+hv_get_ring_buffer(struct hv_ring_buffer_info *ring_info)
+{
+	return (void *)ring_info->ring_buffer->buffer;
+}
+
+/*
+ * To optimize the flow management on the send-side,
+ * when the sender is blocked because of lack of
+ * sufficient space in the ring buffer, potential the
+ * consumer of the ring buffer can signal the producer.
+ * This is controlled by the following parameters:
+ *
+ * 1. pending_send_sz: This is the size in bytes that the
+ *    producer is trying to send.
+ * 2. The feature bit feat_pending_send_sz set to indicate if
+ *    the consumer of the ring will signal when the ring
+ *    state transitions from being full to a state where
+ *    there is room for the producer to send the pending packet.
+ */
+
+static inline  bool hv_need_to_signal_on_read(struct hv_ring_buffer_info *rbi)
+{
+	u32 cur_write_sz;
+	u32 pending_sz;
+
+	virt_mb();
+	pending_sz = READ_ONCE(rbi->ring_buffer->pending_send_sz);
+	/* If the other end is not blocked on write don't bother. */
+	if (pending_sz == 0)
+		return false;
+
+	cur_write_sz = hv_get_bytes_to_write(rbi);
+
+	if (cur_write_sz >= pending_sz)
+		return true;
+
+	return false;
+}
+
 #endif /* _HYPERV_H */
-- 
1.7.4.1

[PATCH 7/7] Drivers: hv: vmbus: Implement APIs to support "in place" consumption of vmbus packets

[K. Y. Srinivasan]

Implement APIs for in-place consumption of vmbus packets. Currently, each
packet is copied and processed one at a time and as part of processing
each packet we potentially may signal the host (if it is waiting for
room to produce a packet).

These APIs help batched in-place processing of vmbus packets.
We also optimize host signaling by having a separate API to signal
the end of in-place consumption. With netvsc using these APIs,
on an iperf run on average I see about 20X reduction in checks to
signal the host.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/ring_buffer.c |    1 +
 include/linux/hyperv.h   |   86 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+), 0 deletions(-)

diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 253311b..a2a38ab 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -132,6 +132,7 @@ hv_set_next_read_location(struct hv_ring_buffer_info *ring_info,
 		    u32 next_read_location)
 {
 	ring_info->ring_buffer->read_index = next_read_location;
+	ring_info->priv_read_index = next_read_location;
 }
 
 
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 8fc9b09..3fadbaf 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -126,6 +126,8 @@ struct hv_ring_buffer_info {
 
 	u32 ring_datasize;		/* < ring_size */
 	u32 ring_data_startoffset;
+	u32 priv_write_index;
+	u32 priv_read_index;
 };
 
 /*
@@ -1408,4 +1410,88 @@ static inline  bool hv_need_to_signal_on_read(struct hv_ring_buffer_info *rbi)
 	return false;
 }
 
+/*
+ * An API to support in-place processing of incoming VMBUS packets.
+ */
+#define VMBUS_PKT_TRAILER	8
+
+static inline struct vmpacket_descriptor *
+get_next_pkt_raw(struct vmbus_channel *channel)
+{
+	struct hv_ring_buffer_info *ring_info = &channel->inbound;
+	u32 read_loc = ring_info->priv_read_index;
+	void *ring_buffer = hv_get_ring_buffer(ring_info);
+	struct vmpacket_descriptor *cur_desc;
+	u32 packetlen;
+	u32 dsize = ring_info->ring_datasize;
+	u32 delta = read_loc - ring_info->ring_buffer->read_index;
+	u32 bytes_avail_toread = (hv_get_bytes_to_read(ring_info) - delta);
+
+	if (bytes_avail_toread < sizeof(struct vmpacket_descriptor))
+		return NULL;
+
+	if ((read_loc + sizeof(*cur_desc)) > dsize)
+		return NULL;
+
+	cur_desc = ring_buffer + read_loc;
+	packetlen = cur_desc->len8 << 3;
+
+	/*
+	 * If the packet under consideration is wrapping around,
+	 * return failure.
+	 */
+	if ((read_loc + packetlen + VMBUS_PKT_TRAILER) > (dsize - 1))
+		return NULL;
+
+	return cur_desc;
+}
+
+/*
+ * A helper function to step through packets "in-place"
+ * This API is to be called after each successful call
+ * get_next_pkt_raw().
+ */
+static inline void put_pkt_raw(struct vmbus_channel *channel,
+				struct vmpacket_descriptor *desc)
+{
+	struct hv_ring_buffer_info *ring_info = &channel->inbound;
+	u32 read_loc = ring_info->priv_read_index;
+	u32 packetlen = desc->len8 << 3;
+	u32 dsize = ring_info->ring_datasize;
+
+	if ((read_loc + packetlen + VMBUS_PKT_TRAILER) > dsize)
+		BUG();
+	/*
+	 * Include the packet trailer.
+	 */
+	ring_info->priv_read_index += packetlen + VMBUS_PKT_TRAILER;
+}
+
+/*
+ * This call commits the read index and potentially signals the host.
+ * Here is the pattern for using the "in-place" consumption APIs:
+ *
+ * while (get_next_pkt_raw() {
+ *	process the packet "in-place";
+ *	put_pkt_raw();
+ * }
+ * if (packets processed in place)
+ *	commit_rd_index();
+ */
+static inline void commit_rd_index(struct vmbus_channel *channel)
+{
+	struct hv_ring_buffer_info *ring_info = &channel->inbound;
+	/*
+	 * Make sure all reads are done before we update the read index since
+	 * the writer may start writing to the read area once the read index
+	 * is updated.
+	 */
+	virt_rmb();
+	ring_info->ring_buffer->read_index = ring_info->priv_read_index;
+
+	if (hv_need_to_signal_on_read(ring_info))
+		vmbus_set_event(channel);
+}
+
+
 #endif /* _HYPERV_H */
-- 
1.7.4.1

Re: [PATCH 3/7] Drivers: hv: vmbus: Fix a bug in hv_need_to_signal_on_read()

[Greg KH]

On Wed, Mar 23, 2016 at 05:53:53PM -0700, K. Y. Srinivasan wrote:
> We need to issue a full memory barrier prior making a signalling decision.
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/hv/ring_buffer.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)

why is this burried in a 7 patch series?  Don't you want this in
4.6-final?

Please break this out into a separate patch, and redo this series.

thanks,

greg k-h

Re: [PATCH 3/7] Drivers: hv: vmbus: Fix a bug in hv_need_to_signal_on_read()

[Greg KH]

On Wed, Mar 23, 2016 at 05:53:53PM -0700, K. Y. Srinivasan wrote:
> We need to issue a full memory barrier prior making a signalling decision.
> 
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/hv/ring_buffer.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> index 2919395..67dc245 100644
> --- a/drivers/hv/ring_buffer.c
> +++ b/drivers/hv/ring_buffer.c
> @@ -104,6 +104,7 @@ static bool hv_need_to_signal_on_read(struct hv_ring_buffer_info *rbi)
>  	u32 cur_write_sz;
>  	u32 pending_sz;
>  
> +	mb();

And, are you sure this is correct?  You better document the heck out of
this, why it's here, and what it is protecting.  "raw" mb() calls are
really rare for good reason.

thanks,

greg k-h

Re: [PATCH 2/7] Drivers: hv: vmbus: Use READ_ONCE() to read variables that are volatile

[Greg KH]

On Wed, Mar 23, 2016 at 05:53:52PM -0700, K. Y. Srinivasan wrote:
> Use the READ_ONCE macro to access variabes that can change asynchronously.

Why?  What is this "fixing"?

thanks,

greg k-h

RE: [PATCH 3/7] Drivers: hv: vmbus: Fix a bug in hv_need_to_signal_on_read()

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, March 31, 2016 5:00 PM
> To: KY Srinivasan <kys@microsoft.com>
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> olaf@aepfle.de; apw@canonical.com; vkuznets@redhat.com;
> jasowang@redhat.com; stable@vger.kernel.org
> Subject: Re: [PATCH 3/7] Drivers: hv: vmbus: Fix a bug in
> hv_need_to_signal_on_read()
> 
> On Wed, Mar 23, 2016 at 05:53:53PM -0700, K. Y. Srinivasan wrote:
> > We need to issue a full memory barrier prior making a signalling decision.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/hv/ring_buffer.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> why is this burried in a 7 patch series?  Don't you want this in
> 4.6-final?
> 
> Please break this out into a separate patch, and redo this series.

Will do.

Thanks,

K. Y
> 
> thanks,
> 
> greg k-h

RE: [PATCH 3/7] Drivers: hv: vmbus: Fix a bug in hv_need_to_signal_on_read()

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, March 31, 2016 5:00 PM
> To: KY Srinivasan <kys@microsoft.com>
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> olaf@aepfle.de; apw@canonical.com; vkuznets@redhat.com;
> jasowang@redhat.com; stable@vger.kernel.org
> Subject: Re: [PATCH 3/7] Drivers: hv: vmbus: Fix a bug in
> hv_need_to_signal_on_read()
> 
> On Wed, Mar 23, 2016 at 05:53:53PM -0700, K. Y. Srinivasan wrote:
> > We need to issue a full memory barrier prior making a signalling decision.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/hv/ring_buffer.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> > index 2919395..67dc245 100644
> > --- a/drivers/hv/ring_buffer.c
> > +++ b/drivers/hv/ring_buffer.c
> > @@ -104,6 +104,7 @@ static bool hv_need_to_signal_on_read(struct
> hv_ring_buffer_info *rbi)
> >  	u32 cur_write_sz;
> >  	u32 pending_sz;
> >
> > +	mb();
> 
> And, are you sure this is correct?  You better document the heck out of
> this, why it's here, and what it is protecting.  "raw" mb() calls are
> really rare for good reason.

Yes; I am sure I need this barrier. This is a lockless ringbuffer code where
the reader  updates the read index while the writer only updates the
write index and they each sample the index modified by the other end to
make decisions.  Here is the reason for having this barrier:

"If the reading of the pend_sz (in the function hv_need_to_signal_on_read)
were to be reordered and read before we commit the new read index 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."

I will add the necessary documentation here.

Regards,

K. Y 

RE: [PATCH 2/7] Drivers: hv: vmbus: Use READ_ONCE() to read variables that are volatile

[KY Srinivasan]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, March 31, 2016 5:01 PM
> To: KY Srinivasan <kys@microsoft.com>
> Cc: linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> olaf@aepfle.de; apw@canonical.com; vkuznets@redhat.com;
> jasowang@redhat.com
> Subject: Re: [PATCH 2/7] Drivers: hv: vmbus: Use READ_ONCE() to read
> variables that are volatile
> 
> On Wed, Mar 23, 2016 at 05:53:52PM -0700, K. Y. Srinivasan wrote:
> > Use the READ_ONCE macro to access variabes that can change
> asynchronously.
> 
> Why?  What is this "fixing"?

This is to prevent the compiler optimizations and moving the code. This is the
recommended mechanism for reading volatile variables. 

Regards,

K. Y
> 
> thanks,
> 
> greg k-h