From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933550AbcJZP77 (ORCPT ); Wed, 26 Oct 2016 11:59:59 -0400 Received: from mail-by2nam03on0125.outbound.protection.outlook.com ([104.47.42.125]:50423 "EHLO NAM03-BY2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751260AbcJZP76 (ORCPT ); Wed, 26 Oct 2016 11:59:58 -0400 X-Greylist: delayed 3617 seconds by postgrey-1.27 at vger.kernel.org; Wed, 26 Oct 2016 11:59:58 EDT From: Haiyang Zhang To: Vitaly Kuznetsov , "devel@linuxdriverproject.org" CC: "linux-kernel@vger.kernel.org" , "KY Srinivasan" Subject: RE: [PATCH] Drivers: hv: vmbus: Raise retry/wait limits in vmbus_post_msg() Thread-Topic: [PATCH] Drivers: hv: vmbus: Raise retry/wait limits in vmbus_post_msg() Thread-Index: AQHSL3nDSfE2+mehD0eTfcv/1BVeP6C6xzGA Date: Wed, 26 Oct 2016 14:26:20 +0000 Message-ID: References: <1477480307-5546-1-git-send-email-vkuznets@redhat.com> In-Reply-To: <1477480307-5546-1-git-send-email-vkuznets@redhat.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=haiyangz@microsoft.com; x-originating-ip: [72.74.33.140] x-ms-office365-filtering-correlation-id: 2bdf0ef1-8b28-403f-590d-08d3fdac0eea x-microsoft-exchange-diagnostics: 1;BN6PR03MB2481;7:Y3AoKHdKsPzEz5C/t11JrlB/LuqsNFj0glGmcfSPdEjbd0Ton2e9m5CYdW0b5btSzdfMyFGX63ZZVKLxbtIlj/GK9Fzy4W6BbcKqVZeQISHMHmRMQXeeiAd0mBJ1huaBXpS/w9H0TVTutzhpWvz5FcwOSUhAvPg02qDFJWQlNKkSMr9+rpPysykFeBkwnBPb43hQ2ZbZVGu4D6hUdG2PgkMN7O1cnlxk98LPzyyep1ZiObwSqPXnDk9kq/7TnKLjfOQhFJKdWlMF6VtSUnEUe/qoS/zW6JDcKwim4mMjISj8gkFJUvp/+MxjerNAKQoK+4c3grBn1NbNubtYpbDY6V4NxWkIVbsAN3S9x2i5hXvwg7DZb1jYl926U+SbB0ai x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BN6PR03MB2481; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(9452136761055); x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(61425038)(6040176)(6045068)(601004)(2401047)(8121501046)(5005006)(3002001)(10201501046)(6055026)(61426038)(61427038)(6046068)(6072074);SRVR:BN6PR03MB2481;BCL:0;PCL:0;RULEID:;SRVR:BN6PR03MB2481; x-forefront-prvs: 0107098B6C x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(6009001)(7916002)(189002)(13464003)(377454003)(199003)(2950100002)(86362001)(86612001)(10290500002)(3660700001)(5005710100001)(10400500002)(5660300001)(8676002)(19580405001)(8990500004)(2900100001)(11100500001)(19580395003)(5002640100001)(74316002)(101416001)(54356999)(76176999)(77096005)(586003)(122556002)(81156014)(7696004)(81166006)(5001770100001)(97736004)(8936002)(305945005)(68736007)(76576001)(7846002)(106356001)(107886002)(7736002)(9686002)(50986999)(99286002)(3280700002)(10090500001)(66066001)(6116002)(3846002)(2906002)(189998001)(2501003)(87936001)(4326007)(33656002)(102836003)(92566002)(106116001)(4001430100002)(105586002);DIR:OUT;SFP:1102;SCL:1;SRVR:BN6PR03MB2481;H:BLUPR03MB1412.namprd03.prod.outlook.com;FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 X-OriginatorOrg: microsoft.com X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Oct 2016 14:26:21.2130 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47 X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR03MB2481 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id u9QG04jQ001553 > -----Original Message----- > From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com] > Sent: Wednesday, October 26, 2016 7:12 AM > To: devel@linuxdriverproject.org > Cc: linux-kernel@vger.kernel.org; KY Srinivasan ; > Haiyang Zhang > 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 > --- > 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