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

[KY Srinivasan]

> -----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.

> 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


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