> -----Original Message----- > From: Vitaly Kuznetsov [mailto:vkuznets@xxxxxxxxxx] > Sent: Wednesday, October 26, 2016 7:12 AM > To: devel@xxxxxxxxxxxxxxxxxxxxxx > Cc: linux-kernel@xxxxxxxxxxxxxxx; KY Srinivasan <kys@xxxxxxxxxxxxx>; > Haiyang Zhang <haiyangz@xxxxxxxxxxxxx> > Subject: [PATCH] Drivers: hv: vmbus: Raise retry/wait limits in > vmbus_post_msg() > > DoS protection conditions were altered in WS2016 and now it's easy to > get > -EAGAIN returned from vmbus_post_msg() (e.g. when we try changing MTU on > a > netvsc device in a loop). All vmbus_post_msg() callers don't retry the > operation and we usually end up with a non-functional device or crash. > > While host's DoS protection conditions are unknown to me my tests show > that > it can take up to 46 attempts to send a message after changing udelay() > to > mdelay() and caping msec at '256', this means we can wait up to 10 > seconds > before the message is sent so we need to use msleep() instead. Almost > all > vmbus_post_msg() callers are ready to sleep but there is one special > case: > vmbus_initiate_unload() which can be called from interrupt/NMI context > and > we can't sleep there. I'm also not sure about the lonely > vmbus_send_tl_connect_request() which has no in-tree users but its > external > users are most likely waiting for the host to reply so sleeping there is > also appropriate. > > Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> > --- > drivers/hv/channel.c | 17 +++++++++-------- > drivers/hv/channel_mgmt.c | 10 ++++++---- > drivers/hv/connection.c | 19 ++++++++++++------- > drivers/hv/hyperv_vmbus.h | 2 +- > 4 files changed, 28 insertions(+), 20 deletions(-) > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c > index 16f91c8..28ca66e 100644 > --- a/drivers/hv/channel.c > +++ b/drivers/hv/channel.c > @@ -180,7 +180,7 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 > send_ringbuffer_size, > spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags); > > ret = vmbus_post_msg(open_msg, > - sizeof(struct vmbus_channel_open_channel)); > + sizeof(struct vmbus_channel_open_channel), true); > > if (ret != 0) { > err = ret; > @@ -232,7 +232,7 @@ int vmbus_send_tl_connect_request(const uuid_le > *shv_guest_servie_id, > conn_msg.guest_endpoint_id = *shv_guest_servie_id; > conn_msg.host_service_id = *shv_host_servie_id; > > - return vmbus_post_msg(&conn_msg, sizeof(conn_msg)); > + return vmbus_post_msg(&conn_msg, sizeof(conn_msg), true); > } > EXPORT_SYMBOL_GPL(vmbus_send_tl_connect_request); > > @@ -418,7 +418,7 @@ int vmbus_establish_gpadl(struct vmbus_channel > *channel, void *kbuffer, > spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags); > > ret = vmbus_post_msg(gpadlmsg, msginfo->msgsize - > - sizeof(*msginfo)); > + sizeof(*msginfo), true); > if (ret != 0) > goto cleanup; > > @@ -432,8 +432,8 @@ int vmbus_establish_gpadl(struct vmbus_channel > *channel, void *kbuffer, > gpadl_body->gpadl = next_gpadl_handle; > > ret = vmbus_post_msg(gpadl_body, > - submsginfo->msgsize - > - sizeof(*submsginfo)); > + submsginfo->msgsize - sizeof(*submsginfo), > + true); > if (ret != 0) > goto cleanup; > > @@ -484,8 +484,8 @@ int vmbus_teardown_gpadl(struct vmbus_channel > *channel, u32 gpadl_handle) > list_add_tail(&info->msglistentry, > &vmbus_connection.chn_msg_list); > spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags); > - ret = vmbus_post_msg(msg, > - sizeof(struct vmbus_channel_gpadl_teardown)); > + ret = vmbus_post_msg(msg, sizeof(struct > vmbus_channel_gpadl_teardown), > + true); > > if (ret) > goto post_msg_err; > @@ -556,7 +556,8 @@ static int vmbus_close_internal(struct vmbus_channel > *channel) > msg->header.msgtype = CHANNELMSG_CLOSECHANNEL; > msg->child_relid = channel->offermsg.child_relid; > > - ret = vmbus_post_msg(msg, sizeof(struct > vmbus_channel_close_channel)); > + ret = vmbus_post_msg(msg, sizeof(struct > vmbus_channel_close_channel), > + true); > > if (ret) { > pr_err("Close failed: close post msg return is %d\n", ret); > diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c > index 96a85cd..160d92e 100644 > --- a/drivers/hv/channel_mgmt.c > +++ b/drivers/hv/channel_mgmt.c > @@ -321,7 +321,8 @@ static void vmbus_release_relid(u32 relid) > memset(&msg, 0, sizeof(struct vmbus_channel_relid_released)); > msg.child_relid = relid; > msg.header.msgtype = CHANNELMSG_RELID_RELEASED; > - vmbus_post_msg(&msg, sizeof(struct vmbus_channel_relid_released)); > + vmbus_post_msg(&msg, sizeof(struct vmbus_channel_relid_released), > + true); > } > > void hv_event_tasklet_disable(struct vmbus_channel *channel) > @@ -728,7 +729,8 @@ void vmbus_initiate_unload(bool crash) > init_completion(&vmbus_connection.unload_event); > memset(&hdr, 0, sizeof(struct vmbus_channel_message_header)); > hdr.msgtype = CHANNELMSG_UNLOAD; > - vmbus_post_msg(&hdr, sizeof(struct vmbus_channel_message_header)); > + vmbus_post_msg(&hdr, sizeof(struct vmbus_channel_message_header), > + !crash); > > /* > * vmbus_initiate_unload() is also called on crash and the crash > can be > @@ -1116,8 +1118,8 @@ int vmbus_request_offers(void) > msg->msgtype = CHANNELMSG_REQUESTOFFERS; > > > - ret = vmbus_post_msg(msg, > - sizeof(struct vmbus_channel_message_header)); > + ret = vmbus_post_msg(msg, sizeof(struct > vmbus_channel_message_header), > + true); > if (ret != 0) { > pr_err("Unable to request offers - %d\n", ret); > > diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c > index 78e6368..da1feb4 100644 > --- a/drivers/hv/connection.c > +++ b/drivers/hv/connection.c > @@ -110,7 +110,8 @@ static int vmbus_negotiate_version(struct > vmbus_channel_msginfo *msginfo, > spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags); > > ret = vmbus_post_msg(msg, > - sizeof(struct vmbus_channel_initiate_contact)); > + sizeof(struct vmbus_channel_initiate_contact), > + true); > if (ret != 0) { > spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags); > list_del(&msginfo->msglistentry); > @@ -434,12 +435,12 @@ void vmbus_on_event(unsigned long data) > /* > * vmbus_post_msg - Send a msg on the vmbus's message connection > */ > -int vmbus_post_msg(void *buffer, size_t buflen) > +int vmbus_post_msg(void *buffer, size_t buflen, bool can_sleep) > { > union hv_connection_id conn_id; > int ret = 0; > int retries = 0; > - u32 usec = 1; > + u32 msec = 1; > > conn_id.asu32 = 0; > conn_id.u.id = VMBUS_MESSAGE_CONNECTION_ID; > @@ -449,7 +450,7 @@ int vmbus_post_msg(void *buffer, size_t buflen) > * insufficient resources. Retry the operation a couple of > * times before giving up. > */ > - while (retries < 20) { > + while (retries < 100) { > ret = hv_post_message(conn_id, 1, buffer, buflen); > > switch (ret) { > @@ -472,9 +473,13 @@ int vmbus_post_msg(void *buffer, size_t buflen) > } > > retries++; > - udelay(usec); > - if (usec < 2048) > - usec *= 2; > + if (can_sleep) > + msleep(msec); > + else > + mdelay(msec); > + > + if (msec < 256) > + msec *= 2; The change looks fine. I knew, in case of initializing large trunk of receive/send buffers, vmbus_establish_gpadl() is called many times, and retrying of 1ms or more is needed often. So, using milliseconds delay/sleep is good. Thanks, Reviewed-by: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx> _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
