linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/5] Drivers: hv: ring_buffer: do some code cleanup and optimization
@ 2015-12-08 17:07 Vitaly Kuznetsov
  2015-12-08 17:07 ` [PATCH v2 1/5] Drivers: hv: ring_buffer.c: fix comment style Vitaly Kuznetsov
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Vitaly Kuznetsov @ 2015-12-08 17:07 UTC (permalink / raw)
  To: devel; +Cc: K. Y. Srinivasan, Haiyang Zhang, linux-kernel

Changes since v1: added PATCH 4/5 and 5/5.

1) Fix comment style in ring_buffer.c
2) Remove unneeded smp_read_barrier_depends() from hyperv.h
3) Unify hv_ringbuffer_peek/read() functions
4) Unify vmbus_recvpacket()/vmbus_recvpacket_raw() functions
5) Eliminate hv_ringbuffer_peek().

Vitaly Kuznetsov (5):
  Drivers: hv: ring_buffer.c: fix comment style
  Drivers: hv: ring_buffer: remove stray smp_read_barrier_depends()
  Drivers: hv: ring_buffer: remove code duplication from
    hv_ringbuffer_peek/read()
  Drivers: hv: remove code duplication between
    vmbus_recvpacket()/vmbus_recvpacket_raw()
  Drivers: hv: ring_buffer: eliminate hv_ringbuffer_peek()

 drivers/hv/channel.c      |  81 ++++-------------
 drivers/hv/hyperv_vmbus.h |  11 +--
 drivers/hv/ring_buffer.c  | 218 ++++++++++++++--------------------------------
 include/linux/hyperv.h    |   2 -
 4 files changed, 83 insertions(+), 229 deletions(-)

-- 
2.4.3


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 1/5] Drivers: hv: ring_buffer.c: fix comment style
  2015-12-08 17:07 [PATCH v2 0/5] Drivers: hv: ring_buffer: do some code cleanup and optimization Vitaly Kuznetsov
@ 2015-12-08 17:07 ` Vitaly Kuznetsov
  2015-12-08 17:07 ` [PATCH v2 2/5] Drivers: hv: ring_buffer: remove stray smp_read_barrier_depends() Vitaly Kuznetsov
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Vitaly Kuznetsov @ 2015-12-08 17:07 UTC (permalink / raw)
  To: devel; +Cc: K. Y. Srinivasan, Haiyang Zhang, linux-kernel

Convert 6+-string comments repeating function names to normal kernel-style
comments and fix a couple of other comment style issues. No textual or
functional changes intended.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/ring_buffer.c | 135 +++++++++--------------------------------------
 1 file changed, 26 insertions(+), 109 deletions(-)

diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 70a1a9a..7bca513 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -112,9 +112,7 @@ 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;
 
-	/*
-	 * If the other end is not blocked on write don't bother.
-	 */
+	/* If the other end is not blocked on write don't bother. */
 	if (pending_sz == 0)
 		return false;
 
@@ -128,12 +126,7 @@ static bool hv_need_to_signal_on_read(u32 prev_write_sz,
 	return false;
 }
 
-/*
- * hv_get_next_write_location()
- *
- * Get the next write location for the specified ring buffer
- *
- */
+/* 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)
 {
@@ -142,12 +135,7 @@ hv_get_next_write_location(struct hv_ring_buffer_info *ring_info)
 	return next;
 }
 
-/*
- * hv_set_next_write_location()
- *
- * Set the next write location for the specified ring buffer
- *
- */
+/* Set the next write location for the specified ring buffer. */
 static inline void
 hv_set_next_write_location(struct hv_ring_buffer_info *ring_info,
 		     u32 next_write_location)
@@ -155,11 +143,7 @@ hv_set_next_write_location(struct hv_ring_buffer_info *ring_info,
 	ring_info->ring_buffer->write_index = next_write_location;
 }
 
-/*
- * hv_get_next_read_location()
- *
- * Get the next read location for the specified ring buffer
- */
+/* Get the next read location for the specified ring buffer. */
 static inline u32
 hv_get_next_read_location(struct hv_ring_buffer_info *ring_info)
 {
@@ -169,10 +153,8 @@ hv_get_next_read_location(struct hv_ring_buffer_info *ring_info)
 }
 
 /*
- * hv_get_next_readlocation_withoffset()
- *
  * Get the next read location + offset for the specified ring buffer.
- * This allows the caller to skip
+ * This allows the caller to skip.
  */
 static inline u32
 hv_get_next_readlocation_withoffset(struct hv_ring_buffer_info *ring_info,
@@ -186,13 +168,7 @@ hv_get_next_readlocation_withoffset(struct hv_ring_buffer_info *ring_info,
 	return next;
 }
 
-/*
- *
- * hv_set_next_read_location()
- *
- * Set the next read location for the specified ring buffer
- *
- */
+/* Set the next read location for the specified ring buffer. */
 static inline void
 hv_set_next_read_location(struct hv_ring_buffer_info *ring_info,
 		    u32 next_read_location)
@@ -201,12 +177,7 @@ hv_set_next_read_location(struct hv_ring_buffer_info *ring_info,
 }
 
 
-/*
- *
- * hv_get_ring_buffer()
- *
- * Get the start of the ring buffer
- */
+/* Get the start of the ring buffer. */
 static inline void *
 hv_get_ring_buffer(struct hv_ring_buffer_info *ring_info)
 {
@@ -214,25 +185,14 @@ hv_get_ring_buffer(struct hv_ring_buffer_info *ring_info)
 }
 
 
-/*
- *
- * hv_get_ring_buffersize()
- *
- * Get the size of the ring buffer
- */
+/* Get the size of the ring buffer. */
 static inline u32
 hv_get_ring_buffersize(struct hv_ring_buffer_info *ring_info)
 {
 	return ring_info->ring_datasize;
 }
 
-/*
- *
- * hv_get_ring_bufferindices()
- *
- * Get the read and write indices as u64 of the specified ring buffer
- *
- */
+/* Get the read and write indices as u64 of the specified ring buffer. */
 static inline u64
 hv_get_ring_bufferindices(struct hv_ring_buffer_info *ring_info)
 {
@@ -240,12 +200,8 @@ hv_get_ring_bufferindices(struct hv_ring_buffer_info *ring_info)
 }
 
 /*
- *
- * hv_copyfrom_ringbuffer()
- *
  * Helper routine to copy to source from ring buffer.
  * Assume there is enough room. Handles wrap-around in src case only!!
- *
  */
 static u32 hv_copyfrom_ringbuffer(
 	struct hv_ring_buffer_info	*ring_info,
@@ -277,12 +233,8 @@ static u32 hv_copyfrom_ringbuffer(
 
 
 /*
- *
- * hv_copyto_ringbuffer()
- *
  * Helper routine to copy from source to ring buffer.
  * Assume there is enough room. Handles wrap-around in dest case only!!
- *
  */
 static u32 hv_copyto_ringbuffer(
 	struct hv_ring_buffer_info	*ring_info,
@@ -308,13 +260,7 @@ static u32 hv_copyto_ringbuffer(
 	return start_write_offset;
 }
 
-/*
- *
- * hv_ringbuffer_get_debuginfo()
- *
- * Get various debug metrics for the specified ring buffer
- *
- */
+/* Get various debug metrics for the specified ring buffer. */
 void hv_ringbuffer_get_debuginfo(struct hv_ring_buffer_info *ring_info,
 			    struct hv_ring_buffer_debug_info *debug_info)
 {
@@ -337,13 +283,7 @@ void hv_ringbuffer_get_debuginfo(struct hv_ring_buffer_info *ring_info,
 	}
 }
 
-/*
- *
- * hv_ringbuffer_init()
- *
- *Initialize the ring buffer
- *
- */
+/* Initialize the ring buffer. */
 int hv_ringbuffer_init(struct hv_ring_buffer_info *ring_info,
 		   void *buffer, u32 buflen)
 {
@@ -356,9 +296,7 @@ int hv_ringbuffer_init(struct hv_ring_buffer_info *ring_info,
 	ring_info->ring_buffer->read_index =
 		ring_info->ring_buffer->write_index = 0;
 
-	/*
-	 * Set the feature bit for enabling flow control.
-	 */
+	/* Set the feature bit for enabling flow control. */
 	ring_info->ring_buffer->feature_bits.value = 1;
 
 	ring_info->ring_size = buflen;
@@ -369,24 +307,12 @@ int hv_ringbuffer_init(struct hv_ring_buffer_info *ring_info,
 	return 0;
 }
 
-/*
- *
- * hv_ringbuffer_cleanup()
- *
- * Cleanup the ring buffer
- *
- */
+/* Cleanup the ring buffer. */
 void hv_ringbuffer_cleanup(struct hv_ring_buffer_info *ring_info)
 {
 }
 
-/*
- *
- * hv_ringbuffer_write()
- *
- * Write to the ring buffer
- *
- */
+/* Write to the ring buffer. */
 int hv_ringbuffer_write(struct hv_ring_buffer_info *outring_info,
 		    struct kvec *kv_list, u32 kv_count, bool *signal)
 {
@@ -411,10 +337,11 @@ int hv_ringbuffer_write(struct hv_ring_buffer_info *outring_info,
 				&bytes_avail_toread,
 				&bytes_avail_towrite);
 
-
-	/* If there is only room for the packet, assume it is full. */
-	/* Otherwise, the next time around, we think the ring buffer */
-	/* is empty since the read index == write index */
+	/*
+	 * If there is only room for the packet, assume it is full.
+	 * Otherwise, the next time around, we think the ring buffer
+	 * is empty since the read index == write index.
+	 */
 	if (bytes_avail_towrite <= totalbytes_towrite) {
 		spin_unlock_irqrestore(&outring_info->ring_lock, flags);
 		return -EAGAIN;
@@ -454,13 +381,7 @@ int hv_ringbuffer_write(struct hv_ring_buffer_info *outring_info,
 }
 
 
-/*
- *
- * hv_ringbuffer_peek()
- *
- * Read without advancing the read index
- *
- */
+/* Read without advancing the read index. */
 int hv_ringbuffer_peek(struct hv_ring_buffer_info *Inring_info,
 		   void *Buffer, u32 buflen)
 {
@@ -497,13 +418,7 @@ int hv_ringbuffer_peek(struct hv_ring_buffer_info *Inring_info,
 }
 
 
-/*
- *
- * hv_ringbuffer_read()
- *
- * Read and advance the read index
- *
- */
+/* Read and advance the read index. */
 int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info, void *buffer,
 		   u32 buflen, u32 offset, bool *signal)
 {
@@ -542,9 +457,11 @@ int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info, void *buffer,
 						sizeof(u64),
 						next_read_location);
 
-	/* 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 */
+	/*
+	 * 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.
+	 */
 	mb();
 
 	/* Update the read index */
-- 
2.4.3


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 2/5] Drivers: hv: ring_buffer: remove stray smp_read_barrier_depends()
  2015-12-08 17:07 [PATCH v2 0/5] Drivers: hv: ring_buffer: do some code cleanup and optimization Vitaly Kuznetsov
  2015-12-08 17:07 ` [PATCH v2 1/5] Drivers: hv: ring_buffer.c: fix comment style Vitaly Kuznetsov
@ 2015-12-08 17:07 ` Vitaly Kuznetsov
  2015-12-08 17:07 ` [PATCH v2 3/5] Drivers: hv: ring_buffer: remove code duplication from hv_ringbuffer_peek/read() Vitaly Kuznetsov
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Vitaly Kuznetsov @ 2015-12-08 17:07 UTC (permalink / raw)
  To: devel; +Cc: K. Y. Srinivasan, Haiyang Zhang, linux-kernel

smp_read_barrier_depends() does nothing on almost all arcitectures
including x86 and having it in the beginning of
hv_get_ringbuffer_availbytes() does not provide any guarantees anyway.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 include/linux/hyperv.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 8fdc17b..1777dc8 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -141,8 +141,6 @@ hv_get_ringbuffer_availbytes(struct hv_ring_buffer_info *rbi,
 {
 	u32 read_loc, write_loc, dsize;
 
-	smp_read_barrier_depends();
-
 	/* Capture the read/write indices before they changed */
 	read_loc = rbi->ring_buffer->read_index;
 	write_loc = rbi->ring_buffer->write_index;
-- 
2.4.3


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 3/5] Drivers: hv: ring_buffer: remove code duplication from hv_ringbuffer_peek/read()
  2015-12-08 17:07 [PATCH v2 0/5] Drivers: hv: ring_buffer: do some code cleanup and optimization Vitaly Kuznetsov
  2015-12-08 17:07 ` [PATCH v2 1/5] Drivers: hv: ring_buffer.c: fix comment style Vitaly Kuznetsov
  2015-12-08 17:07 ` [PATCH v2 2/5] Drivers: hv: ring_buffer: remove stray smp_read_barrier_depends() Vitaly Kuznetsov
@ 2015-12-08 17:07 ` Vitaly Kuznetsov
  2015-12-08 17:07 ` [PATCH v2 4/5] Drivers: hv: remove code duplication between vmbus_recvpacket()/vmbus_recvpacket_raw() Vitaly Kuznetsov
  2015-12-08 17:07 ` [PATCH v2 5/5] Drivers: hv: ring_buffer: eliminate hv_ringbuffer_peek() Vitaly Kuznetsov
  4 siblings, 0 replies; 6+ messages in thread
From: Vitaly Kuznetsov @ 2015-12-08 17:07 UTC (permalink / raw)
  To: devel; +Cc: K. Y. Srinivasan, Haiyang Zhang, linux-kernel

hv_ringbuffer_peek() does the same as hv_ringbuffer_read() without
advancing the read index. The only functional change this patch brings
is moving hv_need_to_signal_on_read() call under the ring_lock but this
function is just a couple of comparisons.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/ring_buffer.c | 68 ++++++++++++++++++------------------------------
 1 file changed, 25 insertions(+), 43 deletions(-)

diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 7bca513..07f9408 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -380,47 +380,9 @@ int hv_ringbuffer_write(struct hv_ring_buffer_info *outring_info,
 	return 0;
 }
 
-
-/* Read without advancing the read index. */
-int hv_ringbuffer_peek(struct hv_ring_buffer_info *Inring_info,
-		   void *Buffer, u32 buflen)
-{
-	u32 bytes_avail_towrite;
-	u32 bytes_avail_toread;
-	u32 next_read_location = 0;
-	unsigned long flags;
-
-	spin_lock_irqsave(&Inring_info->ring_lock, flags);
-
-	hv_get_ringbuffer_availbytes(Inring_info,
-				&bytes_avail_toread,
-				&bytes_avail_towrite);
-
-	/* Make sure there is something to read */
-	if (bytes_avail_toread < buflen) {
-
-		spin_unlock_irqrestore(&Inring_info->ring_lock, flags);
-
-		return -EAGAIN;
-	}
-
-	/* Convert to byte offset */
-	next_read_location = hv_get_next_read_location(Inring_info);
-
-	next_read_location = hv_copyfrom_ringbuffer(Inring_info,
-						Buffer,
-						buflen,
-						next_read_location);
-
-	spin_unlock_irqrestore(&Inring_info->ring_lock, flags);
-
-	return 0;
-}
-
-
-/* Read and advance the read index. */
-int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info, void *buffer,
-		   u32 buflen, u32 offset, bool *signal)
+static inline int __hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info,
+				       void *buffer, u32 buflen, u32 offset,
+				       bool *signal, bool advance)
 {
 	u32 bytes_avail_towrite;
 	u32 bytes_avail_toread;
@@ -452,6 +414,9 @@ int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info, void *buffer,
 						buflen,
 						next_read_location);
 
+	if (!advance)
+		goto out_unlock;
+
 	next_read_location = hv_copyfrom_ringbuffer(inring_info,
 						&prev_indices,
 						sizeof(u64),
@@ -467,9 +432,26 @@ int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info, void *buffer,
 	/* Update the read index */
 	hv_set_next_read_location(inring_info, next_read_location);
 
-	spin_unlock_irqrestore(&inring_info->ring_lock, flags);
-
 	*signal = hv_need_to_signal_on_read(bytes_avail_towrite, inring_info);
 
+out_unlock:
+	spin_unlock_irqrestore(&inring_info->ring_lock, flags);
 	return 0;
 }
+
+/* Read from ring buffer without advancing the read index. */
+int hv_ringbuffer_peek(struct hv_ring_buffer_info *inring_info,
+		       void *buffer, u32 buflen)
+{
+	return __hv_ringbuffer_read(inring_info, buffer, buflen,
+				    0, NULL, false);
+}
+
+/* Read from ring buffer and advance the read index. */
+int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info,
+		       void *buffer, u32 buflen, u32 offset,
+		       bool *signal)
+{
+	return __hv_ringbuffer_read(inring_info, buffer, buflen,
+				    offset, signal, true);
+}
-- 
2.4.3


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 4/5] Drivers: hv: remove code duplication between vmbus_recvpacket()/vmbus_recvpacket_raw()
  2015-12-08 17:07 [PATCH v2 0/5] Drivers: hv: ring_buffer: do some code cleanup and optimization Vitaly Kuznetsov
                   ` (2 preceding siblings ...)
  2015-12-08 17:07 ` [PATCH v2 3/5] Drivers: hv: ring_buffer: remove code duplication from hv_ringbuffer_peek/read() Vitaly Kuznetsov
@ 2015-12-08 17:07 ` Vitaly Kuznetsov
  2015-12-08 17:07 ` [PATCH v2 5/5] Drivers: hv: ring_buffer: eliminate hv_ringbuffer_peek() Vitaly Kuznetsov
  4 siblings, 0 replies; 6+ messages in thread
From: Vitaly Kuznetsov @ 2015-12-08 17:07 UTC (permalink / raw)
  To: devel; +Cc: K. Y. Srinivasan, Haiyang Zhang, linux-kernel

vmbus_recvpacket() and vmbus_recvpacket_raw() are almost identical but
there are two discrepancies:
1) vmbus_recvpacket() doesn't propagate errors from hv_ringbuffer_read()
   which looks like it is not desired.
2) There is an error message printed in packetlen > bufferlen case in
   vmbus_recvpacket(). I'm removing it as it is usless for users to see
   such messages and /vmbus_recvpacket_raw() doesn't have it.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/channel.c | 65 ++++++++++++++++++----------------------------------
 1 file changed, 22 insertions(+), 43 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index c4dcab0..c72271d 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -881,8 +881,10 @@ EXPORT_SYMBOL_GPL(vmbus_sendpacket_multipagebuffer);
  *
  * Mainly used by Hyper-V drivers.
  */
-int vmbus_recvpacket(struct vmbus_channel *channel, void *buffer,
-			u32 bufferlen, u32 *buffer_actual_len, u64 *requestid)
+static inline int
+__vmbus_recvpacket(struct vmbus_channel *channel, void *buffer,
+		   u32 bufferlen, u32 *buffer_actual_len, u64 *requestid,
+		   bool raw)
 {
 	struct vmpacket_descriptor desc;
 	u32 packetlen;
@@ -900,27 +902,34 @@ int vmbus_recvpacket(struct vmbus_channel *channel, void *buffer,
 		return 0;
 
 	packetlen = desc.len8 << 3;
-	userlen = packetlen - (desc.offset8 << 3);
+	if (!raw)
+		userlen = packetlen - (desc.offset8 << 3);
+	else
+		userlen = packetlen;
 
 	*buffer_actual_len = userlen;
 
-	if (userlen > bufferlen) {
-
-		pr_err("Buffer too small - got %d needs %d\n",
-			   bufferlen, userlen);
-		return -ETOOSMALL;
-	}
+	if (userlen > bufferlen)
+		return -ENOBUFS;
 
 	*requestid = desc.trans_id;
 
 	/* Copy over the packet to the user buffer */
 	ret = hv_ringbuffer_read(&channel->inbound, buffer, userlen,
-			     (desc.offset8 << 3), &signal);
+				 raw ? 0 : desc.offset8 << 3, &signal);
 
 	if (signal)
 		vmbus_setevent(channel);
 
-	return 0;
+	return ret;
+}
+
+int vmbus_recvpacket(struct vmbus_channel *channel, void *buffer,
+		     u32 bufferlen, u32 *buffer_actual_len,
+		     u64 *requestid)
+{
+	return __vmbus_recvpacket(channel, buffer, bufferlen,
+				  buffer_actual_len, requestid, false);
 }
 EXPORT_SYMBOL(vmbus_recvpacket);
 
@@ -931,37 +940,7 @@ int vmbus_recvpacket_raw(struct vmbus_channel *channel, void *buffer,
 			      u32 bufferlen, u32 *buffer_actual_len,
 			      u64 *requestid)
 {
-	struct vmpacket_descriptor desc;
-	u32 packetlen;
-	int ret;
-	bool signal = false;
-
-	*buffer_actual_len = 0;
-	*requestid = 0;
-
-
-	ret = hv_ringbuffer_peek(&channel->inbound, &desc,
-			     sizeof(struct vmpacket_descriptor));
-	if (ret != 0)
-		return 0;
-
-
-	packetlen = desc.len8 << 3;
-
-	*buffer_actual_len = packetlen;
-
-	if (packetlen > bufferlen)
-		return -ENOBUFS;
-
-	*requestid = desc.trans_id;
-
-	/* Copy over the entire packet to the user buffer */
-	ret = hv_ringbuffer_read(&channel->inbound, buffer, packetlen, 0,
-				 &signal);
-
-	if (signal)
-		vmbus_setevent(channel);
-
-	return ret;
+	return __vmbus_recvpacket(channel, buffer, bufferlen,
+				  buffer_actual_len, requestid, true);
 }
 EXPORT_SYMBOL_GPL(vmbus_recvpacket_raw);
-- 
2.4.3


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 5/5] Drivers: hv: ring_buffer: eliminate hv_ringbuffer_peek()
  2015-12-08 17:07 [PATCH v2 0/5] Drivers: hv: ring_buffer: do some code cleanup and optimization Vitaly Kuznetsov
                   ` (3 preceding siblings ...)
  2015-12-08 17:07 ` [PATCH v2 4/5] Drivers: hv: remove code duplication between vmbus_recvpacket()/vmbus_recvpacket_raw() Vitaly Kuznetsov
@ 2015-12-08 17:07 ` Vitaly Kuznetsov
  4 siblings, 0 replies; 6+ messages in thread
From: Vitaly Kuznetsov @ 2015-12-08 17:07 UTC (permalink / raw)
  To: devel; +Cc: K. Y. Srinivasan, Haiyang Zhang, linux-kernel

Currently, there is only one user for hv_ringbuffer_read()/
hv_ringbuffer_peak() functions and the usage of these functions is:
- insecure as we drop ring_lock between them, someone else (in theory
  only) can acquire it in between;
- non-optimal as we do a number of things (acquire/release the above
  mentioned lock, calculate available space on the ring, ...) twice and
  this path is performance-critical.

Remove hv_ringbuffer_peek() moving the logic from __vmbus_recvpacket() to
hv_ringbuffer_read().

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 drivers/hv/channel.c      | 30 ++--------------------
 drivers/hv/hyperv_vmbus.h | 11 +++-----
 drivers/hv/ring_buffer.c  | 65 +++++++++++++++++++++++++++--------------------
 3 files changed, 42 insertions(+), 64 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index c72271d..a4c800d 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -886,37 +886,11 @@ __vmbus_recvpacket(struct vmbus_channel *channel, void *buffer,
 		   u32 bufferlen, u32 *buffer_actual_len, u64 *requestid,
 		   bool raw)
 {
-	struct vmpacket_descriptor desc;
-	u32 packetlen;
-	u32 userlen;
 	int ret;
 	bool signal = false;
 
-	*buffer_actual_len = 0;
-	*requestid = 0;
-
-
-	ret = hv_ringbuffer_peek(&channel->inbound, &desc,
-			     sizeof(struct vmpacket_descriptor));
-	if (ret != 0)
-		return 0;
-
-	packetlen = desc.len8 << 3;
-	if (!raw)
-		userlen = packetlen - (desc.offset8 << 3);
-	else
-		userlen = packetlen;
-
-	*buffer_actual_len = userlen;
-
-	if (userlen > bufferlen)
-		return -ENOBUFS;
-
-	*requestid = desc.trans_id;
-
-	/* Copy over the packet to the user buffer */
-	ret = hv_ringbuffer_read(&channel->inbound, buffer, userlen,
-				 raw ? 0 : desc.offset8 << 3, &signal);
+	ret = hv_ringbuffer_read(&channel->inbound, buffer, bufferlen,
+				 buffer_actual_len, requestid, &signal, raw);
 
 	if (signal)
 		vmbus_setevent(channel);
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 3782636..d5ee081 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -614,14 +614,9 @@ int hv_ringbuffer_write(struct hv_ring_buffer_info *ring_info,
 		    struct kvec *kv_list,
 		    u32 kv_count, bool *signal);
 
-int hv_ringbuffer_peek(struct hv_ring_buffer_info *ring_info, void *buffer,
-		   u32 buflen);
-
-int hv_ringbuffer_read(struct hv_ring_buffer_info *ring_info,
-		   void *buffer,
-		   u32 buflen,
-		   u32 offset, bool *signal);
-
+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);
 
 void hv_ringbuffer_get_debuginfo(struct hv_ring_buffer_info *ring_info,
 			    struct hv_ring_buffer_debug_info *debug_info);
diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 07f9408..b53702c 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -380,30 +380,59 @@ int hv_ringbuffer_write(struct hv_ring_buffer_info *outring_info,
 	return 0;
 }
 
-static inline int __hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info,
-				       void *buffer, u32 buflen, u32 offset,
-				       bool *signal, bool advance)
+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;
 	unsigned long flags;
+	struct vmpacket_descriptor desc;
+	u32 offset;
+	u32 packetlen;
+	int ret = 0;
 
 	if (buflen <= 0)
 		return -EINVAL;
 
 	spin_lock_irqsave(&inring_info->ring_lock, flags);
 
+	*buffer_actual_len = 0;
+	*requestid = 0;
+
 	hv_get_ringbuffer_availbytes(inring_info,
 				&bytes_avail_toread,
 				&bytes_avail_towrite);
 
 	/* Make sure there is something to read */
-	if (bytes_avail_toread < buflen) {
-		spin_unlock_irqrestore(&inring_info->ring_lock, flags);
+	if (bytes_avail_toread < sizeof(desc)) {
+		/*
+		 * No error is set when there is even no header, drivers are
+		 * supposed to analyze buffer_actual_len.
+		 */
+		goto out_unlock;
+	}
 
-		return -EAGAIN;
+	next_read_location = hv_get_next_read_location(inring_info);
+	next_read_location = hv_copyfrom_ringbuffer(inring_info, &desc,
+						    sizeof(desc),
+						    next_read_location);
+
+	offset = raw ? 0 : (desc.offset8 << 3);
+	packetlen = (desc.len8 << 3) - offset;
+	*buffer_actual_len = packetlen;
+	*requestid = desc.trans_id;
+
+	if (bytes_avail_toread < packetlen + offset) {
+		ret = -EAGAIN;
+		goto out_unlock;
+	}
+
+	if (packetlen > buflen) {
+		ret = -ENOBUFS;
+		goto out_unlock;
 	}
 
 	next_read_location =
@@ -411,12 +440,9 @@ static inline int __hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info,
 
 	next_read_location = hv_copyfrom_ringbuffer(inring_info,
 						buffer,
-						buflen,
+						packetlen,
 						next_read_location);
 
-	if (!advance)
-		goto out_unlock;
-
 	next_read_location = hv_copyfrom_ringbuffer(inring_info,
 						&prev_indices,
 						sizeof(u64),
@@ -436,22 +462,5 @@ static inline int __hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info,
 
 out_unlock:
 	spin_unlock_irqrestore(&inring_info->ring_lock, flags);
-	return 0;
-}
-
-/* Read from ring buffer without advancing the read index. */
-int hv_ringbuffer_peek(struct hv_ring_buffer_info *inring_info,
-		       void *buffer, u32 buflen)
-{
-	return __hv_ringbuffer_read(inring_info, buffer, buflen,
-				    0, NULL, false);
-}
-
-/* Read from ring buffer and advance the read index. */
-int hv_ringbuffer_read(struct hv_ring_buffer_info *inring_info,
-		       void *buffer, u32 buflen, u32 offset,
-		       bool *signal)
-{
-	return __hv_ringbuffer_read(inring_info, buffer, buflen,
-				    offset, signal, true);
+	return ret;
 }
-- 
2.4.3


^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2015-12-08 17:08 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-08 17:07 [PATCH v2 0/5] Drivers: hv: ring_buffer: do some code cleanup and optimization Vitaly Kuznetsov
2015-12-08 17:07 ` [PATCH v2 1/5] Drivers: hv: ring_buffer.c: fix comment style Vitaly Kuznetsov
2015-12-08 17:07 ` [PATCH v2 2/5] Drivers: hv: ring_buffer: remove stray smp_read_barrier_depends() Vitaly Kuznetsov
2015-12-08 17:07 ` [PATCH v2 3/5] Drivers: hv: ring_buffer: remove code duplication from hv_ringbuffer_peek/read() Vitaly Kuznetsov
2015-12-08 17:07 ` [PATCH v2 4/5] Drivers: hv: remove code duplication between vmbus_recvpacket()/vmbus_recvpacket_raw() Vitaly Kuznetsov
2015-12-08 17:07 ` [PATCH v2 5/5] Drivers: hv: ring_buffer: eliminate hv_ringbuffer_peek() Vitaly Kuznetsov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).