Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751581AbaDWVqy (ORCPT ); Wed, 23 Apr 2014 17:46:54 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:33937 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751162AbaDWVqY (ORCPT ); Wed, 23 Apr 2014 17:46:24 -0400 Date: Thu, 24 Apr 2014 00:45:58 +0300 From: Dan Carpenter To: "K. Y. Srinivasan" Cc: davem@davemloft.net, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, devel@linuxdriverproject.org, olaf@aepfle.de, apw@canonical.com, jasowang@redhat.com Subject: Re: [PATCH V1 net-next 1/1] hyperv: Enable sendbuf mechanism on the send path Message-ID: <20140423214557.GA26890@mwanda> References: <1398288285-16268-1-git-send-email-kys@microsoft.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1398288285-16268-1-git-send-email-kys@microsoft.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: acsinet22.oracle.com [141.146.126.238] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org TLDR; Style nits and we should return -ENOMEM on error instead of success. On Wed, Apr 23, 2014 at 02:24:45PM -0700, K. Y. Srinivasan wrote: > 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 > Reviewed-by: Haiyang Zhang > --- > 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(-) > > diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h > index d1f7826..4b7df5a 100644 > --- a/drivers/net/hyperv/hyperv_net.h > +++ b/drivers/net/hyperv/hyperv_net.h > @@ -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; > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c > index bbee446..c041f63 100644 > --- a/drivers/net/hyperv/netvsc.c > +++ b/drivers/net/hyperv/netvsc.c > @@ -28,6 +28,7 @@ > #include > #include > #include > +#include > > #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 > + * 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; > + 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) ". > + 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; > + } > + 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. > } > > -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; > } > > + /* 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); > + ret = -ENOMEM; > + goto cleanup; > + } > + > + /* 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"); > + 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, > + 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. > + > + /* 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"... > + 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; > + > + 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); > + if (net_device->send_section_map == NULL) > + goto cleanup; Set "ret = -ENOMEM;" > + > goto exit; > These bunny hops are super annoying. Just return success here. Why does code have to be all tangled up? > 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) > { > @@ -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. > 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); > + 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 > 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; > diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c > index c76b665..939e3af 100644 > --- a/drivers/net/hyperv/netvsc_drv.c > +++ b/drivers/net/hyperv/netvsc_drv.c > @@ -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); > } > > -- > 1.7.4.1 > > _______________________________________________ > devel mailing list > devel@linuxdriverproject.org > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/