[PATCH 0/5] Drivers: hv: vmbus

[PATCH 0/5] Drivers: hv: vmbus

[K. Y. Srinivasan]

Cleanup the Hyper-V ring buffer code. Also Implement APIs for
supporting copy-free operations on the read side.

K. Y. Srinivasan (5):
  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: Implement copy-free read APIs

 drivers/hv/ring_buffer.c |   99 ++++++++++++++++++++++++++++++++-------------
 include/linux/hyperv.h   |   33 +++++++++++++++
 2 files changed, 103 insertions(+), 29 deletions(-)

-- 
1.7.4.1

[PATCH 1/5] 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/5] 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/5] 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/5] 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/5] Drivers: hv: vmbus: Implement copy-free read APIs

[K. Y. Srinivasan]

Implement copy-free read APIs.

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

diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index c2c2b2e..c80e1f3 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -444,3 +444,58 @@ int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info,
 
 	return ret;
 }
+
+/*
+ * In-place read functions.
+ */
+bool get_next_pkt_raw(struct vmbus_channel *channel,
+		      struct vmpacket_descriptor **desc)
+{
+	struct hv_ring_buffer_info *ring_info = &channel->inbound;
+	u32 read_loc = ring_info->ring_buffer->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 bytes_avail_toread = hv_get_bytes_to_read(ring_info);
+
+	if (bytes_avail_toread < sizeof(struct vmpacket_descriptor))
+		return false;
+
+	if ((read_loc + sizeof(*desc)) > dsize)
+		return false;
+
+	cur_desc = ring_buffer + read_loc;
+	packetlen = cur_desc->len8 << 3;
+
+	if ((read_loc + packetlen + 8) > (dsize - 1))
+		return false;
+
+	*desc = cur_desc;
+	return true;
+}
+EXPORT_SYMBOL_GPL(get_next_pkt_raw);
+
+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->ring_buffer->read_index;
+	u32 packetlen = desc->len8 << 3;
+	u32 dsize = ring_info->ring_datasize;
+
+	if ((read_loc + packetlen + 8) > dsize)
+		BUG();
+
+	/*
+	 * 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_mb();
+	ring_info->ring_buffer->read_index += packetlen + 8;
+
+	if (hv_need_to_signal_on_read(ring_info))
+		vmbus_set_event(channel);
+}
+EXPORT_SYMBOL_GPL(put_pkt_raw);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index a6b053c..455f3f0 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1035,6 +1035,12 @@ extern int vmbus_recvpacket_raw(struct vmbus_channel *channel,
 				     u32 *buffer_actual_len,
 				     u64 *requestid);
 
+bool get_next_pkt_raw(struct vmbus_channel *channel,
+		      struct vmpacket_descriptor **desc);
+
+void put_pkt_raw(struct vmbus_channel *channel,
+		 struct vmpacket_descriptor *desc);
+
 
 extern void vmbus_ontimer(unsigned long data);
 
-- 
1.7.4.1

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

[Vitaly Kuznetsov]

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

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

Any reason this should be mb()? This is pretty strong and will probably
lead to performace regression ... and, btw, we have another mb() in
hv_ringbuffer_read().

Could you please describe the scenarion you're trying to protect against
so we could search for a better solution?

>
> 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)

-- 
  Vitaly

Re: [PATCH 5/5] Drivers: hv: vmbus: Implement copy-free read APIs

[Vitaly Kuznetsov]

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

> Implement copy-free read APIs.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> ---
>  drivers/hv/ring_buffer.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/hyperv.h   |    6 +++++
>  2 files changed, 61 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> index c2c2b2e..c80e1f3 100644
> --- a/drivers/hv/ring_buffer.c
> +++ b/drivers/hv/ring_buffer.c
> @@ -444,3 +444,58 @@ int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info,
>
>  	return ret;
>  }
> +
> +/*
> + * In-place read functions.
> + */
> +bool get_next_pkt_raw(struct vmbus_channel *channel,
> +		      struct vmpacket_descriptor **desc)

I suggest we make these functions return pointers and use
ERR_PTR()/IS_ERR() when we need to return an error.

> +{
> +	struct hv_ring_buffer_info *ring_info = &channel->inbound;
> +	u32 read_loc = ring_info->ring_buffer->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 bytes_avail_toread = hv_get_bytes_to_read(ring_info);
> +
> +	if (bytes_avail_toread < sizeof(struct vmpacket_descriptor))
> +		return false;
> +
> +	if ((read_loc + sizeof(*desc)) > dsize)
> +		return false;
> +
> +	cur_desc = ring_buffer + read_loc;
> +	packetlen = cur_desc->len8 << 3;
> +
> +	if ((read_loc + packetlen + 8) > (dsize - 1))

Could you please add a define for this '8'?


> +		return false;
> +
> +	*desc = cur_desc;
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(get_next_pkt_raw);
> +
> +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->ring_buffer->read_index;
> +	u32 packetlen = desc->len8 << 3;
> +	u32 dsize = ring_info->ring_datasize;
> +
> +	if ((read_loc + packetlen + 8) > dsize)
> +		BUG();
> +
> +	/*
> +	 * 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_mb();
> +	ring_info->ring_buffer->read_index += packetlen + 8;
> +
> +	if (hv_need_to_signal_on_read(ring_info))
> +		vmbus_set_event(channel);
> +}
> +EXPORT_SYMBOL_GPL(put_pkt_raw);
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index a6b053c..455f3f0 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1035,6 +1035,12 @@ extern int vmbus_recvpacket_raw(struct vmbus_channel *channel,
>  				     u32 *buffer_actual_len,
>  				     u64 *requestid);
>
> +bool get_next_pkt_raw(struct vmbus_channel *channel,
> +		      struct vmpacket_descriptor **desc);
> +
> +void put_pkt_raw(struct vmbus_channel *channel,
> +		 struct vmpacket_descriptor *desc);
> +
>
>  extern void vmbus_ontimer(unsigned long data);

-- 
  Vitaly

RE: [PATCH 5/5] Drivers: hv: vmbus: Implement copy-free read APIs

[KY Srinivasan]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Monday, March 21, 2016 1:29 AM
> To: KY Srinivasan <kys@microsoft.com>
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> jasowang@redhat.com
> Subject: Re: [PATCH 5/5] Drivers: hv: vmbus: Implement copy-free read APIs
> 
> "K. Y. Srinivasan" <kys@microsoft.com> writes:
> 
> > Implement copy-free read APIs.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > ---
> >  drivers/hv/ring_buffer.c |   55
> ++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/hyperv.h   |    6 +++++
> >  2 files changed, 61 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> > index c2c2b2e..c80e1f3 100644
> > --- a/drivers/hv/ring_buffer.c
> > +++ b/drivers/hv/ring_buffer.c
> > @@ -444,3 +444,58 @@ int hv_ringbuffer_read(struct hv_ring_buffer_info
> *inring_info,
> >
> >  	return ret;
> >  }
> > +
> > +/*
> > + * In-place read functions.
> > + */
> > +bool get_next_pkt_raw(struct vmbus_channel *channel,
> > +		      struct vmpacket_descriptor **desc)
> 
> I suggest we make these functions return pointers and use
> ERR_PTR()/IS_ERR() when we need to return an error.

Either we get a pointer to a descriptor or not; there is no other
error here. I will change the signature accordingly.

> 
> > +{
> > +	struct hv_ring_buffer_info *ring_info = &channel->inbound;
> > +	u32 read_loc = ring_info->ring_buffer->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 bytes_avail_toread = hv_get_bytes_to_read(ring_info);
> > +
> > +	if (bytes_avail_toread < sizeof(struct vmpacket_descriptor))
> > +		return false;
> > +
> > +	if ((read_loc + sizeof(*desc)) > dsize)
> > +		return false;
> > +
> > +	cur_desc = ring_buffer + read_loc;
> > +	packetlen = cur_desc->len8 << 3;
> > +
> > +	if ((read_loc + packetlen + 8) > (dsize - 1))
> 
> Could you please add a define for this '8'?

Sure.

K. Y

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

[KY Srinivasan]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Monday, March 21, 2016 1:19 AM
> To: KY Srinivasan <kys@microsoft.com>
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> jasowang@redhat.com; stable@vger.kernel.org
> Subject: Re: [PATCH 3/5] Drivers: hv: vmbus: Fix a bug in
> hv_need_to_signal_on_read()
> 
> "K. Y. Srinivasan" <kys@microsoft.com> writes:
> 
> > We need to issue a full memory barrier prior making a signalling
> > decision.
> 
> Any reason this should be mb()? This is pretty strong and will probably
> lead to performace regression ... and, btw, we have another mb() in
> hv_ringbuffer_read().
> 
> Could you please describe the scenarion you're trying to protect against
> so we could search for a better solution?

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.

Regards,

K. Y

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

[Vitaly Kuznetsov]

KY Srinivasan <kys@microsoft.com> writes:

>> -----Original Message-----
>> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>> Sent: Monday, March 21, 2016 1:19 AM
>> To: KY Srinivasan <kys@microsoft.com>
>> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
>> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
>> jasowang@redhat.com; stable@vger.kernel.org
>> Subject: Re: [PATCH 3/5] Drivers: hv: vmbus: Fix a bug in
>> hv_need_to_signal_on_read()
>> 
>> "K. Y. Srinivasan" <kys@microsoft.com> writes:
>> 
>> > We need to issue a full memory barrier prior making a signalling
>> > decision.
>> 
>> Any reason this should be mb()? This is pretty strong and will probably
>> lead to performace regression ... and, btw, we have another mb() in
>> hv_ringbuffer_read().
>> 
>> Could you please describe the scenarion you're trying to protect against
>> so we could search for a better solution?
>
> 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 these are two reads we can add a lightweight barrier just preventing
compiler from reordering (e.g. smp_rmb()), right?

> 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.

so write and then we read and we need to prevent reordering... not sure
how to get rid on mb() then ...

-- 
  Vitaly

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

[KY Srinivasan]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Tuesday, March 22, 2016 2:56 AM
> To: KY Srinivasan <kys@microsoft.com>
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> jasowang@redhat.com; stable@vger.kernel.org
> Subject: Re: [PATCH 3/5] Drivers: hv: vmbus: Fix a bug in
> hv_need_to_signal_on_read()
> 
> KY Srinivasan <kys@microsoft.com> writes:
> 
> >> -----Original Message-----
> >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> >> Sent: Monday, March 21, 2016 1:19 AM
> >> To: KY Srinivasan <kys@microsoft.com>
> >> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> >> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> >> jasowang@redhat.com; stable@vger.kernel.org
> >> Subject: Re: [PATCH 3/5] Drivers: hv: vmbus: Fix a bug in
> >> hv_need_to_signal_on_read()
> >>
> >> "K. Y. Srinivasan" <kys@microsoft.com> writes:
> >>
> >> > We need to issue a full memory barrier prior making a signalling
> >> > decision.
> >>
> >> Any reason this should be mb()? This is pretty strong and will probably
> >> lead to performace regression ... and, btw, we have another mb() in
> >> hv_ringbuffer_read().
> >>
> >> Could you please describe the scenarion you're trying to protect against
> >> so we could search for a better solution?
> >
> > 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 these are two reads we can add a lightweight barrier just preventing
> compiler from reordering (e.g. smp_rmb()), right?
> 
> > 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.
> 
> so write and then we read and we need to prevent reordering... not sure
> how to get rid on mb() then ...

The other memory barrier in the function (prior to writing the read index)
has been there forever and I am not sure why that needs to be a full barrier.
I feel a read barrier should suffice.

Regards,

K. Y
> 
> --
>   Vitaly

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

[KY Srinivasan]

> -----Original Message-----
> From: KY Srinivasan
> Sent: Tuesday, March 22, 2016 7:37 AM
> To: 'Vitaly Kuznetsov' <vkuznets@redhat.com>
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> jasowang@redhat.com; stable@vger.kernel.org
> Subject: RE: [PATCH 3/5] Drivers: hv: vmbus: Fix a bug in
> hv_need_to_signal_on_read()
> 
> 
> 
> > -----Original Message-----
> > From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> > Sent: Tuesday, March 22, 2016 2:56 AM
> > To: KY Srinivasan <kys@microsoft.com>
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> > jasowang@redhat.com; stable@vger.kernel.org
> > Subject: Re: [PATCH 3/5] Drivers: hv: vmbus: Fix a bug in
> > hv_need_to_signal_on_read()
> >
> > KY Srinivasan <kys@microsoft.com> writes:
> >
> > >> -----Original Message-----
> > >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> > >> Sent: Monday, March 21, 2016 1:19 AM
> > >> To: KY Srinivasan <kys@microsoft.com>
> > >> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > >> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> > >> jasowang@redhat.com; stable@vger.kernel.org
> > >> Subject: Re: [PATCH 3/5] Drivers: hv: vmbus: Fix a bug in
> > >> hv_need_to_signal_on_read()
> > >>
> > >> "K. Y. Srinivasan" <kys@microsoft.com> writes:
> > >>
> > >> > We need to issue a full memory barrier prior making a signalling
> > >> > decision.
> > >>
> > >> Any reason this should be mb()? This is pretty strong and will probably
> > >> lead to performace regression ... and, btw, we have another mb() in
> > >> hv_ringbuffer_read().
> > >>
> > >> Could you please describe the scenarion you're trying to protect against
> > >> so we could search for a better solution?
> > >
> > > 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 these are two reads we can add a lightweight barrier just preventing
> > compiler from reordering (e.g. smp_rmb()), right?
> >
> > > 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.
> >
> > so write and then we read and we need to prevent reordering... not sure
> > how to get rid on mb() then ...
> 
> The other memory barrier in the function (prior to writing the read index)
> has been there forever and I am not sure why that needs to be a full barrier.
> I feel a read barrier should suffice.

I may also look at restructuring these APIs to not always check for signaling.
I will experiment with that scheme to minimize the barrier calls.

K. Y

RE: [PATCH 0/5] Drivers: hv: vmbus

[KY Srinivasan]

> -----Original Message-----
> From: K. Y. Srinivasan [mailto:kys@microsoft.com]
> Sent: Friday, March 18, 2016 6:14 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>
> Subject: [PATCH 0/5] Drivers: hv: vmbus
> 
> Cleanup the Hyper-V ring buffer code. Also Implement APIs for
> supporting copy-free operations on the read side.
> 
> K. Y. Srinivasan (5):
>   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: Implement copy-free read APIs
> 
>  drivers/hv/ring_buffer.c |   99 ++++++++++++++++++++++++++++++++-----
> --------
>  include/linux/hyperv.h   |   33 +++++++++++++++
>  2 files changed, 103 insertions(+), 29 deletions(-)

Greg,

Please drop this series. I am going to send a new patch-set and looking at the changes I
have made, I feel it may not be appropriate to simply bump the version. 

Regards,

K. Y
> 
> --
> 1.7.4.1