RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM

[Dexuan Cui]

> -----Original Message-----
> From: KY Srinivasan
> Sent: Monday, February 2, 2015 3:42 AM
> To: Dexuan Cui; Vitaly Kuznetsov
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; driverdev-
> devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx;
> jasowang@xxxxxxxxxx; 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@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; driverdev-
> > devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx;
> > jasowang@xxxxxxxxxx; 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@xxxxxxxxxx]
> > > Sent: Thursday, January 29, 2015 21:22 PM
> > > To: Dexuan Cui
> > > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > > driverdev- devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx;
> > > apw@xxxxxxxxxxxxx; jasowang@xxxxxxxxxx; KY Srinivasan; Haiyang Zhang
> > > Subject: Re: [PATCH 3/3] hv: vmbus_open(): reset the channel state on
> > > ENOMEM
> > >
> > > Dexuan Cui <decui@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/