Skip to contentSkip to conversation

hyperv: Enable sendbuf mechanism on the send path

PATCH In review K. Y. Srinivasan 4 participants 7 messages Apr 23, 2014 – Apr 30, 2014

Subsystems (3)
Networking Network drivers Hyper-V/Azure core and drivers
lore
Share & tools
lore

K. Y. Srinivasan

author
#0 lore raw

PATCH 1/1 hyperv: Enable sendbuf mechanism on the send path

We send packets using a copy-free mechanism (this is the Guest to Host transport via VMBUS). While this is obviously optimal for large packets,
it may not be optimal for small packets. Hyper-V host supports
a second mechanism for sending packets that is "copy based". We implement that mechanism in this patch.

In this version of the patch I have addressed a comment from David Miller.

With this patch (and all of the other offload and VRSS patches), we are now able to almost saturate a 10G interface between Linux VMs on Hyper-V
on different hosts - close to 9 Gbps as measured via iperf.

Signed-off-by K. Y. Srinivasan <[email protected]>
Reviewed-by Haiyang Zhang <[email protected]>
---
 drivers/net/hyperv/hyperv_net.h |   14 +++
 drivers/net/hyperv/netvsc.c     |  226 +++++++++++++++++++++++++++++++++++++--
 drivers/net/hyperv/netvsc_drv.c |    3 +-
 3 files changed, 234 insertions(+), 9 deletions(-)
drivers/net/hyperv/hyperv_net.h +14 -0
index d1f7826..4b7df5a 100644
@@ -140,6 +140,8 @@ struct hv_netvsc_packet {
 	void *send_completion_ctx;
 	void (*send_completion)(void *context);
 
+	u32 send_buf_index;
+
 	/* This points to the memory after page_buf */
 	struct rndis_message *rndis_msg;
 

@@ -582,6 +584,9 @@ struct nvsp_message {
 
 #define NETVSC_RECEIVE_BUFFER_SIZE		(1024*1024*16)	/* 16MB */
 #define NETVSC_RECEIVE_BUFFER_SIZE_LEGACY	(1024*1024*15)  /* 15MB */
+#define NETVSC_SEND_BUFFER_SIZE			(1024 * 1024)   /* 1MB */
+#define NETVSC_INVALID_INDEX			-1
+
 
 #define NETVSC_RECEIVE_BUFFER_ID		0xcafe
 

@@ -607,6 +612,15 @@ struct netvsc_device {
 	u32 recv_section_cnt;
 	struct nvsp_1_receive_buffer_section *recv_section;
 
+	/* Send buffer allocated by us */
+	void *send_buf;
+	u32 send_buf_size;
+	u32 send_buf_gpadl_handle;
+	u32 send_section_cnt;
+	u32 send_section_size;
+	unsigned long *send_section_map;
+	int map_words;
+
 	/* Used for NetVSP initialization protocol */
 	struct completion channel_init_wait;
 	struct nvsp_message channel_init_pkt;
drivers/net/hyperv/netvsc.c +218 -8

306 lines not on this page · raw message

drivers/net/hyperv/netvsc_drv.c +2 -1
index c76b665..939e3af 100644
@@ -236,10 +236,11 @@ static void netvsc_xmit_completion(void *context)
 	struct hv_netvsc_packet *packet = (struct hv_netvsc_packet *)context;
 	struct sk_buff *skb = (struct sk_buff *)
 		(unsigned long)packet->send_completion_tid;
+	u32 index = packet->send_buf_index;
 
 	kfree(packet);
 
-	if (skb)
+	if (skb && (index == NETVSC_INVALID_INDEX))
 		dev_kfree_skb_any(skb);
 }
Signature · 1 line
1.7.4.1
#1 lore raw

TLDR; Style nits and we should return -ENOMEM on error instead of success.

Quoted 87 lines · K. Y. Srinivasan

We send packets using a copy-free mechanism (this is the Guest to Host transport via VMBUS). While this is obviously optimal for large packets,
it may not be optimal for small packets. Hyper-V host supports
a second mechanism for sending packets that is "copy based". We implement that mechanism in this patch.

In this version of the patch I have addressed a comment from David Miller.

With this patch (and all of the other offload and VRSS patches), we are now able to almost saturate a 10G interface between Linux VMs on Hyper-V
on different hosts - close to 9 Gbps as measured via iperf.

Signed-off-by K. Y. Srinivasan <[email protected]>
Reviewed-by Haiyang Zhang <[email protected]>
---
 drivers/net/hyperv/hyperv_net.h |   14 +++
 drivers/net/hyperv/netvsc.c     |  226 +++++++++++++++++++++++++++++++++++++--
 drivers/net/hyperv/netvsc_drv.c |    3 +-
 3 files changed, 234 insertions(+), 9 deletions(-)
drivers/net/hyperv/hyperv_net.h +14 -0
index d1f7826..4b7df5a 100644
@@ -140,6 +140,8 @@ struct hv_netvsc_packet {
 	void *send_completion_ctx;
 	void (*send_completion)(void *context);
 
+	u32 send_buf_index;
+
 	/* This points to the memory after page_buf */
 	struct rndis_message *rndis_msg;
 

@@ -582,6 +584,9 @@ struct nvsp_message {
 
 #define NETVSC_RECEIVE_BUFFER_SIZE		(1024*1024*16)	/* 16MB */
 #define NETVSC_RECEIVE_BUFFER_SIZE_LEGACY	(1024*1024*15)  /* 15MB */
+#define NETVSC_SEND_BUFFER_SIZE			(1024 * 1024)   /* 1MB */
+#define NETVSC_INVALID_INDEX			-1
+
 
 #define NETVSC_RECEIVE_BUFFER_ID		0xcafe
 

@@ -607,6 +612,15 @@ struct netvsc_device {
 	u32 recv_section_cnt;
 	struct nvsp_1_receive_buffer_section *recv_section;
 
+	/* Send buffer allocated by us */
+	void *send_buf;
+	u32 send_buf_size;
+	u32 send_buf_gpadl_handle;
+	u32 send_section_cnt;
+	u32 send_section_size;
+	unsigned long *send_section_map;
+	int map_words;
+
 	/* Used for NetVSP initialization protocol */
 	struct completion channel_init_wait;
 	struct nvsp_message channel_init_pkt;
drivers/net/hyperv/netvsc.c +5 -1
index bbee446..c041f63 100644
@@ -28,6 +28,7 @@
 #include <linux/slab.h>
 #include <linux/netdevice.h>
 #include <linux/if_ether.h>
+#include <asm/sync_bitops.h>
 
 #include "hyperv_net.h"
 

@@ -80,7 +81,7 @@ get_in_err:
 }
 
 
-static int netvsc_destroy_recv_buf(struct netvsc_device *net_device)
+static int netvsc_destroy_buf(struct netvsc_device *net_device)
 {
 	struct nvsp_message *revoke_packet;
 	int ret = 0;

@@ -146,10 +147,62 @@ static int netvsc_destroy_recv_buf(struct netvsc_device *net_device)
 		net_device->recv_section = NULL;
 	}
 
+	/* Deal with the send buffer we may have setup.
+	 * If we got a  send section size, it means we received a
+	 * SendsendBufferComplete msg (ie sent

drivers/net/hyperv/netvsc.c static int netvsc_destroy_recv_buf(struct netvsc_device *net_device) · line 162

Quoted 10 lines
drivers/net/hyperv/netvsc.c +10 -0
+	 * NvspMessage1TypeSendReceiveBuffer msg) therefore, we need
+	 * to send a revoke msg here
+	 */
+	if (net_device->send_section_size) {
+		/* Send the revoke receive buffer */
+		revoke_packet = &net_device->revoke_packet;
+		memset(revoke_packet, 0, sizeof(struct nvsp_message));
+
+		revoke_packet->hdr.msg_type =
+			NVSP_MSG1_TYPE_REVOKE_SEND_BUF;

This fits in 80 characters.

		revoke_packet->hdr.msg_type = NVSP_MSG1_TYPE_REVOKE_SEND_BUF;

drivers/net/hyperv/netvsc.c static int netvsc_destroy_recv_buf(struct netvsc_device *net_device) · line 173

Quoted 11 lines
drivers/net/hyperv/netvsc.c +11 -0
+		revoke_packet->msg.v1_msg.revoke_recv_buf.id = 0;
+
+		ret = vmbus_sendpacket(net_device->dev->channel,
+				       revoke_packet,
+				       sizeof(struct nvsp_message),
+				       (unsigned long)revoke_packet,
+				       VM_PKT_DATA_INBAND, 0);
+		/* If we failed here, we might as well return and
+		 * have a leak rather than continue and a bugchk
+		 */
+		if (ret != 0) {

I have never been a big fan of these double negative conditions. It's simpler and more normal style to say: "if (ret) { ". There are some cases where it makes sense to compare against zero. For example, if you are dealing with a variable as a number then you would say "if (x == 0) ... else if (x == 1) ". Also for strcmp() then you should use == 0 and != 0. But for error codes it's better it's more idiomatic to just say "if (ret) ".

Quoted 19 lines
drivers/net/hyperv/netvsc.c +19 -0
+			netdev_err(ndev, "unable to send "
+				   "revoke send buffer to netvsp\n");
+			return ret;
+		}
+	}
+	/* Teardown the gpadl on the vsp end */
+	if (net_device->send_buf_gpadl_handle) {
+		ret = vmbus_teardown_gpadl(net_device->dev->channel,
+					   net_device->send_buf_gpadl_handle);
+
+		/* If we failed here, we might as well return and have a leak
+		 * rather than continue and a bugchk
+		 */
+		if (ret != 0) {
+			netdev_err(ndev,
+				   "unable to teardown send buffer's gpadl\n");
+			return ret;
+		}
+		net_device->recv_buf_gpadl_handle = 0;

drivers/net/hyperv/netvsc.c static int netvsc_destroy_recv_buf(struct netvsc_device *net_device) · line 202

Quoted 10 lines
drivers/net/hyperv/netvsc.c +9 -0
+	}
+	if (net_device->send_buf) {
+		/* Free up the receive buffer */
+		free_pages((unsigned long)net_device->send_buf,
+			   get_order(net_device->send_buf_size));
+		net_device->send_buf = NULL;
+	}
+	kfree(net_device->send_section_map);
+
 	return ret;

This patch doesn't introduce this, but it's better to say:

	return 0;

That is more clear so you don't have to back trace and verify what "ret" is.

Quoted 9 lines
drivers/net/hyperv/netvsc.c +1 -1
 }
 
-static int netvsc_init_recv_buf(struct hv_device *device)
+static int netvsc_init_buf(struct hv_device *device)
 {
 	int ret = 0;
 	int t;

@@ -248,10 +301,90 @@ static int netvsc_init_recv_buf(struct hv_device *device)
 		goto cleanup;

drivers/net/hyperv/netvsc.c static int netvsc_init_recv_buf(struct hv_device *device) · line 311

Quoted 10 lines
drivers/net/hyperv/netvsc.c +8 -0
 	}
 
+	/* Now setup the send buffer.
+	 */
+	net_device->send_buf =
+		(void *)__get_free_pages(GFP_KERNEL|__GFP_ZERO,
+					 get_order(net_device->send_buf_size));
+	if (!net_device->send_buf) {
+		netdev_err(ndev, "unable to allocate send "
+			   "buffer of size %d\n", net_device->send_buf_size);

This string actually fits into 80 characters fine as-is.

		netdev_err(ndev, "unable to allocate send buffer of size %d\n",
			   net_device->send_buf_size);
Quoted 4 lines
drivers/net/hyperv/netvsc.c +4 -0
+		ret = -ENOMEM;
+		goto cleanup;
+	}
+

drivers/net/hyperv/netvsc.c static int netvsc_init_recv_buf(struct hv_device *device) · line 325

Quoted 10 lines
drivers/net/hyperv/netvsc.c +10 -0
+	/* Establish the gpadl handle for this buffer on this
+	 * channel.  Note: This call uses the vmbus connection rather
+	 * than the channel to establish the gpadl handle.
+	 */
+	ret = vmbus_establish_gpadl(device->channel, net_device->send_buf,
+				    net_device->send_buf_size,
+				    &net_device->send_buf_gpadl_handle);
+	if (ret != 0) {
+		netdev_err(ndev,
+			   "unable to establish send buffer's gpadl\n");

Same. Just put it on one line.

		netdev_err(ndev, "unable to establish send buffer's gpadl\n");
Quoted 15 lines
drivers/net/hyperv/netvsc.c +15 -0
+		goto cleanup;
+	}
+
+	/* Notify the NetVsp of the gpadl handle */
+	init_packet = &net_device->channel_init_pkt;
+	memset(init_packet, 0, sizeof(struct nvsp_message));
+	init_packet->hdr.msg_type = NVSP_MSG1_TYPE_SEND_SEND_BUF;
+	init_packet->msg.v1_msg.send_recv_buf.gpadl_handle =
+		net_device->send_buf_gpadl_handle;
+	init_packet->msg.v1_msg.send_recv_buf.id = 0;
+
+	/* Send the gpadl notification request */
+	ret = vmbus_sendpacket(device->channel, init_packet,
+			       sizeof(struct nvsp_message),
+			       (unsigned long)init_packet,

drivers/net/hyperv/netvsc.c static int netvsc_init_recv_buf(struct hv_device *device) · line 350

Quoted 10 lines
drivers/net/hyperv/netvsc.c +10 -0
+			       VM_PKT_DATA_INBAND,
+			       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
+	if (ret != 0) {
+		netdev_err(ndev,
+			   "unable to send send buffer's gpadl to netvsp\n");
+		goto cleanup;
+	}
+
+	t = wait_for_completion_timeout(&net_device->channel_init_wait, 5*HZ);
+	BUG_ON(t == 0);

Is there no way to just return an error code here? We may as well use wait_for_completion() without the timeout if we're just going to call BUG_ON() anyway.

drivers/net/hyperv/netvsc.c static int netvsc_init_recv_buf(struct hv_device *device) · line 354

Quoted 4 lines
drivers/net/hyperv/netvsc.c +4 -0
+
+	/* Check the response */
+	if (init_packet->msg.v1_msg.
+	    send_send_buf_complete.status != NVSP_STAT_SUCCESS) {

Better to break it up like this:

	if (init_packet->msg.v1_msg.send_send_buf_complete.status !=
	    NVSP_STAT_SUCCESS) {

There has to be a better name for "send_send_buf_complete"...

Quoted 16 lines
drivers/net/hyperv/netvsc.c +16 -0
+		netdev_err(ndev, "Unable to complete send buffer "
+			   "initialization with NetVsp - status %d\n",
+			   init_packet->msg.v1_msg.
+			   send_recv_buf_complete.status);
+		ret = -EINVAL;
+		goto cleanup;
+	}
+
+	/* Parse the response */
+	net_device->send_section_size = init_packet->msg.
+				v1_msg.send_send_buf_complete.section_size;
+
+	/* Section count is simply the size divided by the section size.
+	 */
+	net_device->send_section_cnt =
+		net_device->send_buf_size/net_device->send_section_size;

drivers/net/hyperv/netvsc.c static int netvsc_init_recv_buf(struct hv_device *device) · line 380

Quoted 10 lines
drivers/net/hyperv/netvsc.c +10 -0
+
+	dev_info(&device->device, "Send section size: %d, Section count:%d\n",
+		 net_device->send_section_size, net_device->send_section_cnt);
+
+	/* Setup state for managing the send buffer. */
+	net_device->map_words = DIV_ROUND_UP(net_device->send_section_cnt,
+					     BITS_PER_LONG);
+
+	net_device->send_section_map =
+		kzalloc(net_device->map_words * sizeof(ulong), GFP_KERNEL);

Use kcalloc().

	net_device->send_section_map = kcalloc(net_device->map_words,
					       sizeof(ulong), GFP_KERNEL);

drivers/net/hyperv/netvsc.c static int netvsc_init_recv_buf(struct hv_device *device) · line 382

Quoted 2 lines
drivers/net/hyperv/netvsc.c +2 -0
+	if (net_device->send_section_map == NULL)
+		goto cleanup;

Set "ret = -ENOMEM;"

drivers/net/hyperv/netvsc.c static int netvsc_init_recv_buf(struct hv_device *device) · line 385

Quoted 3 lines
drivers/net/hyperv/netvsc.c +1 -0
+
 	goto exit;

These bunny hops are super annoying. Just return success here. Why does code have to be all tangled up?

Quoted 111 lines
drivers/net/hyperv/netvsc.c +60 -3
 cleanup:
-	netvsc_destroy_recv_buf(net_device);
+	netvsc_destroy_buf(net_device);
 
 exit:
 	return ret;

@@ -369,8 +502,9 @@ static int netvsc_connect_vsp(struct hv_device *device)
 		net_device->recv_buf_size = NETVSC_RECEIVE_BUFFER_SIZE_LEGACY;
 	else
 		net_device->recv_buf_size = NETVSC_RECEIVE_BUFFER_SIZE;
+	net_device->send_buf_size = NETVSC_SEND_BUFFER_SIZE;
 
-	ret = netvsc_init_recv_buf(device);
+	ret = netvsc_init_buf(device);
 
 cleanup:
 	return ret;

@@ -378,7 +512,7 @@ cleanup:
 
 static void netvsc_disconnect_vsp(struct netvsc_device *net_device)
 {
-	netvsc_destroy_recv_buf(net_device);
+	netvsc_destroy_buf(net_device);
 }
 
 /*

@@ -440,6 +574,12 @@ static inline u32 hv_ringbuf_avail_percent(
 	return avail_write * 100 / ring_info->ring_datasize;
 }
 
+static inline void netvsc_free_send_slot(struct netvsc_device *net_device,
+					 u32 index)
+{
+	sync_change_bit(index, net_device->send_section_map);
+}
+
 static void netvsc_send_completion(struct netvsc_device *net_device,
 				   struct hv_device *device,
 				   struct vmpacket_descriptor *packet)

@@ -447,6 +587,7 @@ static void netvsc_send_completion(struct netvsc_device *net_device,
 	struct nvsp_message *nvsp_packet;
 	struct hv_netvsc_packet *nvsc_packet;
 	struct net_device *ndev;
+	u32 send_index;
 
 	ndev = net_device->ndev;
 

@@ -477,6 +618,9 @@ static void netvsc_send_completion(struct netvsc_device *net_device,
 
 		/* Notify the layer above us */
 		if (nvsc_packet) {
+			send_index = nvsc_packet->send_buf_index;
+			if (send_index != NETVSC_INVALID_INDEX)
+				netvsc_free_send_slot(net_device, send_index);
 			q_idx = nvsc_packet->q_idx;
 			channel = nvsc_packet->channel;
 			nvsc_packet->send_completion(nvsc_packet->

@@ -504,6 +648,52 @@ static void netvsc_send_completion(struct netvsc_device *net_device,
 
 }
 
+static u32 netvsc_get_next_send_section(struct netvsc_device *net_device)
+{
+	unsigned long index;
+	u32 max_words = net_device->map_words;
+	unsigned long *map_addr = (unsigned long *)net_device->send_section_map;
+	u32 section_cnt = net_device->send_section_cnt;
+	int ret_val = NETVSC_INVALID_INDEX;
+	int i;
+	int prev_val;
+
+	for (i = 0; i < max_words; i++) {
+		if (!~(map_addr[i]))
+			continue;
+		index = ffz(map_addr[i]);
+		prev_val = sync_test_and_set_bit(index, &map_addr[i]);
+		if (prev_val)
+			continue;
+		if ((index + (i * BITS_PER_LONG)) >= section_cnt)
+			break;
+		ret_val = (index + (i * BITS_PER_LONG));
+		break;
+	}
+	return ret_val;
+}
+
+u32 netvsc_copy_to_send_buf(struct netvsc_device *net_device,
+			    unsigned int section_index,
+			    struct hv_netvsc_packet *packet)
+{
+	char *start = net_device->send_buf;
+	char *dest = (start + (section_index * net_device->send_section_size));
+	int i;
+	u32 msg_size = 0;
+
+	for (i = 0; i < packet->page_buf_cnt; i++) {
+		char *src = phys_to_virt(packet->page_buf[i].pfn << PAGE_SHIFT);
+		u32 offset = packet->page_buf[i].offset;
+		u32 len = packet->page_buf[i].len;
+
+		memcpy(dest, (src + offset), len);
+		msg_size += len;
+		dest += len;
+	}
+	return msg_size;
+}
+
 int netvsc_send(struct hv_device *device,
 			struct hv_netvsc_packet *packet)
 {

drivers/net/hyperv/netvsc.c int netvsc_send(struct hv_device *device, · line 710

Quoted 8 lines
drivers/net/hyperv/netvsc.c +4 -0
@@ -513,6 +703,10 @@ int netvsc_send(struct hv_device *device,
 	struct net_device *ndev;
 	struct vmbus_channel *out_channel = NULL;
 	u64 req_id;
+	unsigned int section_index = NETVSC_INVALID_INDEX;
+	u32 msg_size = 0;
+	struct sk_buff *skb;
+

Don't put consecutive blank lines.

Quoted 14 lines
drivers/net/hyperv/netvsc.c +7 -1
 	net_device = get_outbound_net_device(device);
 	if (!net_device)

@@ -528,10 +722,26 @@ int netvsc_send(struct hv_device *device,
 		sendMessage.msg.v1_msg.send_rndis_pkt.channel_type = 1;
 	}
 
-	/* Not using send buffer section */
+	/* Attempt to send via sendbuf */
+	if (packet->total_data_buflen < net_device->send_section_size) {
+		section_index = netvsc_get_next_send_section(net_device);
+		if (section_index != NETVSC_INVALID_INDEX) {
+			msg_size = netvsc_copy_to_send_buf(net_device,
+							   section_index,
+							   packet);

drivers/net/hyperv/netvsc.c int netvsc_send(struct hv_device *device, · line 741

Quoted 10 lines
drivers/net/hyperv/netvsc.c +10 -0
+			skb = (struct sk_buff *)
+			      (unsigned long)packet->send_completion_tid;
+			if (skb)
+				dev_kfree_skb_any(skb);
+			packet->page_buf_cnt = 0;
+		}
+	}
+	packet->send_buf_index = section_index;
+
+

Don't put consective blank lines.

regards,
dan carpenter

Quoted 32 lines
drivers/net/hyperv/netvsc.c +2 -2
 	sendMessage.msg.v1_msg.send_rndis_pkt.send_buf_section_index =
-		0xFFFFFFFF;
-	sendMessage.msg.v1_msg.send_rndis_pkt.send_buf_section_size = 0;
+		section_index;
+	sendMessage.msg.v1_msg.send_rndis_pkt.send_buf_section_size = msg_size;
 
 	if (packet->send_completion)
 		req_id = (ulong)packet;
drivers/net/hyperv/netvsc_drv.c +2 -1
index c76b665..939e3af 100644
@@ -236,10 +236,11 @@ static void netvsc_xmit_completion(void *context)
 	struct hv_netvsc_packet *packet = (struct hv_netvsc_packet *)context;
 	struct sk_buff *skb = (struct sk_buff *)
 		(unsigned long)packet->send_completion_tid;
+	u32 index = packet->send_buf_index;
 
 	kfree(packet);
 
-	if (skb)
+	if (skb && (index == NETVSC_INVALID_INDEX))
 		dev_kfree_skb_any(skb);
 }
Signature · 6 lines
1.7.4.1

_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
#2 lore raw
In reply to K. Y. Srinivasan
Quoted 3 lines · K. Y. Srinivasan
 drivers/net/hyperv/hyperv_net.h |   14 +++
 drivers/net/hyperv/netvsc.c     |  226 +++++++++++++++++++++++++++++++++++++--

drivers/net/hyperv/netvsc_drv.c | 3 +-> 3 files changed, 234 insertions(+), 9 deletions(-)

I just looked over netvsc.c and it could definitely use a more consistent coding style.

Your use of goto exit/cleanup in some functions and returning directly on errors in others could use a cleanup. Please consider doing that while you are touching those files.

author
#3 lore raw
Quoted 19 lines

-----Original Message-----
From: Andev [mailto:[email protected]]
Sent: Thursday, April 24, 2014 2:50 PM
To: KY Srinivasan
Cc: [email protected]; [email protected]; LKML; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH V1 net-next 1/1] hyperv: Enable sendbuf mechanism on the send path

Quoted 2 lines · K. Y. Srinivasan
 drivers/net/hyperv/hyperv_net.h |   14 +++
 drivers/net/hyperv/netvsc.c     |  226

+++++++++++++++++++++++++++++++++++++--

Quoted 1 line

drivers/net/hyperv/netvsc_drv.c | 3 +-> 3 files changed, 234

Quoted 8 lines

insertions(+), 9 deletions(-)

I just looked over netvsc.c and it could definitely use a more consistent coding style.

Your use of goto exit/cleanup in some functions and returning directly on errors in others could use a cleanup. Please consider doing that while you are touching those files.

Will do. The most recent changes I made to netvsc.c, I think was consistent with the existing code; going forward we will certainly move towards a more consistent coding style.

Regards,

K. Y

#4 lore raw
Quoted 8 lines · KY Srinivasan
Quoted 4 lines

From: Andev [mailto:[email protected]]
Your use of goto exit/cleanup in some functions and returning directly on errors in others could use a cleanup. Please consider doing that while you are touching those files.

Will do. The most recent changes I made to netvsc.c, I think was consistent with the existing code; going forward we will certainly move towards a more consistent coding style.

It scares me when you talk about being consistent with the existing code... Just do it the correct way.

1) Don't do the "return ret;" if you know ret is zero.

2) Replace:

	ret = vmbus_sendpacket(...);

	return ret;

with
return vmbus_sendpacket(...);

3) Don't do "goto cleanup;" when "return ret;" will suffice. The do-nothing goto is misleading because you assume it will cleanup somthing. Some people used to misread CodingStyle to think that all functions should only have one return but I have updated it so it is more clear.

regards,
dan carpenter

author
#5 lore raw
In reply to K. Y. Srinivasan
Quoted 12 lines

-----Original Message-----
From: K. Y. Srinivasan [mailto:[email protected]]
Sent: Wednesday, April 23, 2014 2:25 PM
To: [email protected]; [email protected]; linux- [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: KY Srinivasan
Subject: [PATCH V1 net-next 1/1] hyperv: Enable sendbuf mechanism on the send path

Quoted 7 lines

We send packets using a copy-free mechanism (this is the Guest to Host transport via VMBUS). While this is obviously optimal for large packets, it may not be optimal for small packets. Hyper-V host supports a second mechanism for sending packets that is "copy based". We implement that mechanism in this patch.

In this version of the patch I have addressed a comment from David Miller.

David,

Please let me know if there is some other issue you want addressed in this patch.

Regards,

K. Y

maintainer
#6 lore raw

From: KY Srinivasan <[email protected]>
Date: Wed, 30 Apr 2014 14:17:04 +0000

Quoted 1 line

Please let me know if there is some other issue you want addressed in this patch.

If the patch isn't in patchwork, I'm not going to apply it and you need to simply resubmit it.