From: Dexuan Cui <decui@microsoft.com>
To: KY Srinivasan <kys@microsoft.com>, Vitaly Kuznetsov <vkuznets@redhat.com>
CC: "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "driverdev-devel@linuxdriverproject.org" 
	<driverdev-devel@linuxdriverproject.org>,
        "olaf@aepfle.de" <olaf@aepfle.de>,
        "apw@canonical.com" <apw@canonical.com>,
        "jasowang@redhat.com" <jasowang@redhat.com>,
        Haiyang Zhang <haiyangz@microsoft.com>
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+ONgiQmCHxXZzYEp8ggAQkKQCAAHJckA==
Date: Mon, 2 Feb 2015 02:37:48 +0000
Message-ID: <F792CF86EFE20D4AB8064279AFBA51C61F31D93D@SIXPRD3002MB028.064d.mgd.msft.net>
References: <1422529370-28261-1-git-send-email-decui@microsoft.com>
 <87fvat7mcz.fsf@vitty.brq.redhat.com>
 <F792CF86EFE20D4AB8064279AFBA51C61EA9B9A2@SIXPRD3002MB028.064d.mgd.msft.net>
 <BY2PR0301MB0711B5F3CD481610B22D3993A03F0@BY2PR0301MB0711.namprd03.prod.outlook.com>
In-Reply-To: <BY2PR0301MB0711B5F3CD481610B22D3993A03F0@BY2PR0301MB0711.namprd03.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Feb 2015 02:38:06.4445
 (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: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3618
Lines: 94

> -----Original Message-----
> From: KY Srinivasan
> Sent: Monday, February 2, 2015 3:42 AM
> To: Dexuan Cui; Vitaly Kuznetsov
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org; driverdev-
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> jasowang@redhat.com; Haiyang Zhang
> Subject: RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM
> > -----Original Message-----
> > From: Dexuan Cui
> > Sent: Thursday, January 29, 2015 9:03 PM
> > 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
> >
> > > -----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 <decui@microsoft.com> 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.
> 
> If vmbus_open() fails, depending on where the failure occurred, the necessary
> rollback is expected to have
> been done in the vmbus_open() function itself. In vmbus_open function  the last
> thing we do is to send the request
> to the host to open the channel. If this fails, then there is no need to close the
> channel.
Got it.

> > 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!
> We close the channel only if the open() of the channel succeeded.
Got it.

> >
> > 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.
> 
> I must be missing something here. When vmbus_open() fails, we will rollback the
> state and free up the memory
> allocated for the ring buffer.
KY,
Sorry, I didn't check the code carefully...
You're correct. There is no issue here.

> 
> As I look at the vmbus_open() code I do see an issue with regards cleaning up
> the gpadl state and I am sending a patch for this
> shortly. I will base this on top of the this series from Dexuan.
Thanks, KY!
I'm not sure you meant the below line at the end of vmbus_open():
        if (err == 0)
                newchannel->state = CHANNEL_OPENED_STATE;
        ELSE
	???

-- Dexuan
--
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/