From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752098AbbA3FDh (ORCPT ); Fri, 30 Jan 2015 00:03:37 -0500 Received: from mail-bl2on0108.outbound.protection.outlook.com ([65.55.169.108]:18432 "EHLO na01-bl2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751735AbbA3FDf convert rfc822-to-8bit (ORCPT ); Fri, 30 Jan 2015 00:03:35 -0500 From: Dexuan Cui To: Vitaly Kuznetsov CC: "gregkh@linuxfoundation.org" , "linux-kernel@vger.kernel.org" , "driverdev-devel@linuxdriverproject.org" , "olaf@aepfle.de" , "apw@canonical.com" , "jasowang@redhat.com" , KY Srinivasan , Haiyang Zhang Subject: RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM Thread-Topic: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM Thread-Index: AQHQO8amul8y1x/ehE+ONgiQmCHxXZzYEp8g Date: Fri, 30 Jan 2015 05:03:23 +0000 Message-ID: References: <1422529370-28261-1-git-send-email-decui@microsoft.com> <87fvat7mcz.fsf@vitty.brq.redhat.com> In-Reply-To: <87fvat7mcz.fsf@vitty.brq.redhat.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [139.226.141.57] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-EOPAttributedMessage: 0 Authentication-Results: spf=pass (sender IP is 206.191.229.84) smtp.mailfrom=decui@microsoft.com; redhat.com; dkim=none (message not signed) header.d=none;redhat.com; dmarc=pass action=none header.from=microsoft.com; X-Forefront-Antispam-Report: CIP:206.191.229.84;CTRY:US;IPV:NLI;EFV:NLI;SFV:NSPM;SFS:(10019020)(6009001)(438002)(51704005)(164054003)(377454003)(13464003)(110136001)(104016003)(106466001)(19580395003)(33656002)(55846006)(66066001)(2656002)(86146001)(19580405001)(6806004)(87936001)(46102003)(106116001)(50986999)(22746005)(54356999)(22756005)(46406003)(23726002)(47776003)(2900100001)(86362001)(77156002)(62966003)(86612001)(102836002)(2920100001)(97756001)(76176999)(50466002)(2950100001)(92566002)(79686002);DIR:OUT;SFP:1102;SCL:1;SRVR:DM2PR0301MB0845;H:064-smtp-out.microsoft.com;FPR:;SPF:Pass;MLV:sfv;LANG:en; X-DmarcStatus-Test: Passed X-DmarcAction-Test: None X-Microsoft-Antispam: UriScan:; X-Microsoft-Antispam: BCL:0;PCL:0;RULEID:(3003004)(3005004);SRVR:DM2PR0301MB0845; X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004);SRVR:DM2PR0301MB0845; X-Forefront-PRVS: 04724A515E X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:;SRVR:DM2PR0301MB0845; X-OriginatorOrg: microsoft.onmicrosoft.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Jan 2015 05:03:31.6548 (UTC) X-MS-Exchange-CrossTenant-Id: 72f988bf-86f1-41af-91ab-2d7cd011db47 X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=72f988bf-86f1-41af-91ab-2d7cd011db47;Ip=[206.191.229.84] X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM2PR0301MB0845 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > -----Original Message----- > From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com] > Sent: Thursday, January 29, 2015 21:22 PM > To: Dexuan Cui > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org; driverdev- > devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com; > jasowang@redhat.com; KY Srinivasan; Haiyang Zhang > Subject: Re: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM > > Dexuan Cui writes: > > > Without this patch, the state is put to CHANNEL_OPENING_STATE, and when > > the driver is loaded next time, vmbus_open() will fail immediately due to > > newchannel->state != CHANNEL_OPEN_STATE. > > The patch makes sense, but I have one small doubt. We call vmbus_open > from probe functions of various devices. E.g. in hyperv-keyboard we > have: > error = vmbus_open(...) > if (error) > goto err_free_mem; > > and we don't call vmbus_close(...) on this path so no > CHANNELMSG_CLOSECHANNEL will be send. Exactly. > Who's gonna retry probe? The user can try 'rmmod' and 'modprobe' the module to re-probe. > Wouldn't it be better to close the channel? Good question! In your example, due to " goto err_free_mem", we don't run vmbus_close(), so the memory allocated for the ringbuffer is actually leaked! Next time when we reload the module, vmbus_open() will allocate new memory for the ringbuffer. KY, Haiyang, Can you please confirm this issue? Thanks, -- Dexuan > > > > > CC: "K. Y. Srinivasan" > > Signed-off-by: Dexuan Cui > > --- > > drivers/hv/channel.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c > > index 2978f5e..26dcf26 100644 > > --- a/drivers/hv/channel.c > > +++ b/drivers/hv/channel.c > > @@ -89,9 +89,10 @@ int vmbus_open(struct vmbus_channel *newchannel, > u32 send_ringbuffer_size, > > out = (void *)__get_free_pages(GFP_KERNEL|__GFP_ZERO, > > get_order(send_ringbuffer_size + recv_ringbuffer_size)); > > > > - if (!out) > > - return -ENOMEM; > > - > > + if (!out) { > > + err = -ENOMEM; > > + goto error0; > > + } > > > > in = (void *)((unsigned long)out + send_ringbuffer_size); > > > > @@ -199,6 +200,7 @@ error0: > > free_pages((unsigned long)out, > > get_order(send_ringbuffer_size + recv_ringbuffer_size)); > > kfree(open_info); > > + newchannel->state = CHANNEL_OPEN_STATE; > > return err; > > } > > EXPORT_SYMBOL_GPL(vmbus_open); > > -- > Vitaly From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from silver.osuosl.org (silver.osuosl.org [140.211.166.136]) by ash.osuosl.org (Postfix) with ESMTP id 8B0DB1BF98B for ; Fri, 30 Jan 2015 05:03:37 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 7FBAA25174 for ; Fri, 30 Jan 2015 05:03:37 +0000 (UTC) Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id MAnoC+4yVRci for ; Fri, 30 Jan 2015 05:03:36 +0000 (UTC) Received: from na01-by2-obe.outbound.protection.outlook.com (mail-by2on0144.outbound.protection.outlook.com [207.46.100.144]) by silver.osuosl.org (Postfix) with ESMTPS id 1A41B1FD1B for ; Fri, 30 Jan 2015 05:03:36 +0000 (UTC) From: Dexuan Cui Subject: RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM Date: Fri, 30 Jan 2015 05:03:23 +0000 Message-ID: References: <1422529370-28261-1-git-send-email-decui@microsoft.com> <87fvat7mcz.fsf@vitty.brq.redhat.com> In-Reply-To: <87fvat7mcz.fsf@vitty.brq.redhat.com> Content-Language: en-US MIME-Version: 1.0 List-Id: Linux Driver Project Developer List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: "devel" To: Vitaly Kuznetsov Cc: "olaf@aepfle.de" , "gregkh@linuxfoundation.org" , "jasowang@redhat.com" , "driverdev-devel@linuxdriverproject.org" , "linux-kernel@vger.kernel.org" , "apw@canonical.com" , Haiyang Zhang > -----Original Message----- > From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com] > Sent: Thursday, January 29, 2015 21:22 PM > To: Dexuan Cui > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org; driverdev- > devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com; > jasowang@redhat.com; KY Srinivasan; Haiyang Zhang > Subject: Re: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM > > Dexuan Cui writes: > > > Without this patch, the state is put to CHANNEL_OPENING_STATE, and when > > the driver is loaded next time, vmbus_open() will fail immediately due to > > newchannel->state != CHANNEL_OPEN_STATE. > > The patch makes sense, but I have one small doubt. We call vmbus_open > from probe functions of various devices. E.g. in hyperv-keyboard we > have: > error = vmbus_open(...) > if (error) > goto err_free_mem; > > and we don't call vmbus_close(...) on this path so no > CHANNELMSG_CLOSECHANNEL will be send. Exactly. > Who's gonna retry probe? The user can try 'rmmod' and 'modprobe' the module to re-probe. > Wouldn't it be better to close the channel? Good question! In your example, due to " goto err_free_mem", we don't run vmbus_close(), so the memory allocated for the ringbuffer is actually leaked! Next time when we reload the module, vmbus_open() will allocate new memory for the ringbuffer. KY, Haiyang, Can you please confirm this issue? Thanks, -- Dexuan > > > > > CC: "K. Y. Srinivasan" > > Signed-off-by: Dexuan Cui > > --- > > drivers/hv/channel.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c > > index 2978f5e..26dcf26 100644 > > --- a/drivers/hv/channel.c > > +++ b/drivers/hv/channel.c > > @@ -89,9 +89,10 @@ int vmbus_open(struct vmbus_channel *newchannel, > u32 send_ringbuffer_size, > > out = (void *)__get_free_pages(GFP_KERNEL|__GFP_ZERO, > > get_order(send_ringbuffer_size + recv_ringbuffer_size)); > > > > - if (!out) > > - return -ENOMEM; > > - > > + if (!out) { > > + err = -ENOMEM; > > + goto error0; > > + } > > > > in = (void *)((unsigned long)out + send_ringbuffer_size); > > > > @@ -199,6 +200,7 @@ error0: > > free_pages((unsigned long)out, > > get_order(send_ringbuffer_size + recv_ringbuffer_size)); > > kfree(open_info); > > + newchannel->state = CHANNEL_OPEN_STATE; > > return err; > > } > > EXPORT_SYMBOL_GPL(vmbus_open); > > -- > Vitaly _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel