> -----Original Message-----
> From: Dexuan Cui
> Sent: Thursday, January 29, 2015 9:03 PM
> To: Vitaly Kuznetsov
> Cc: [email protected]; [email protected]; driverdev-
> [email protected]; [email protected]; [email protected];
> [email protected]; KY Srinivasan; Haiyang Zhang
> Subject: RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on
> ENOMEM
>
> > -----Original Message-----
> > From: Vitaly Kuznetsov [mailto:[email protected]]
> > Sent: Thursday, January 29, 2015 21:22 PM
> > To: Dexuan Cui
> > Cc: [email protected]; [email protected];
> > driverdev- [email protected]; [email protected];
> > [email protected]; [email protected]; KY Srinivasan; Haiyang Zhang
> > Subject: Re: [PATCH 3/3] hv: vmbus_open(): reset the channel state on
> > ENOMEM
> >
> > Dexuan Cui <[email protected]> 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.
> 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.
>
> 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.
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.
Regards,
K. Y
> -----Original Message-----
> From: KY Srinivasan
> Sent: Monday, February 2, 2015 3:42 AM
> To: Dexuan Cui; Vitaly Kuznetsov
> Cc: [email protected]; [email protected]; driverdev-
> [email protected]; [email protected]; [email protected];
> [email protected]; 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: [email protected]; [email protected]; driverdev-
> > [email protected]; [email protected]; [email protected];
> > [email protected]; KY Srinivasan; Haiyang Zhang
> > Subject: RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on
> > ENOMEM
> >
> > > -----Original Message-----
> > > From: Vitaly Kuznetsov [mailto:[email protected]]
> > > Sent: Thursday, January 29, 2015 21:22 PM
> > > To: Dexuan Cui
> > > Cc: [email protected]; [email protected];
> > > driverdev- [email protected]; [email protected];
> > > [email protected]; [email protected]; KY Srinivasan; Haiyang Zhang
> > > Subject: Re: [PATCH 3/3] hv: vmbus_open(): reset the channel state on
> > > ENOMEM
> > >
> > > Dexuan Cui <[email protected]> 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