LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM
@ 2015-01-29 11:02 Dexuan Cui
  2015-01-29 13:22 ` Vitaly Kuznetsov
  0 siblings, 1 reply; 5+ messages in thread
From: Dexuan Cui @ 2015-01-29 11:02 UTC (permalink / raw)
  To: gregkh, linux-kernel, driverdev-devel, olaf, apw, jasowang, kys; +Cc: haiyangz

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.

CC: "K. Y. Srinivasan" <kys@microsoft.com>
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
 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);
-- 
1.9.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM
  2015-01-29 11:02 [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM Dexuan Cui
@ 2015-01-29 13:22 ` Vitaly Kuznetsov
  2015-01-30  5:03   ` Dexuan Cui
  0 siblings, 1 reply; 5+ messages in thread
From: Vitaly Kuznetsov @ 2015-01-29 13:22 UTC (permalink / raw)
  To: Dexuan Cui
  Cc: gregkh, linux-kernel, driverdev-devel, olaf, apw, jasowang, kys,
	haiyangz

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. Who's gonna retry probe? Wouldn't
it be better to close the channel?

>
> CC: "K. Y. Srinivasan" <kys@microsoft.com>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM
  2015-01-29 13:22 ` Vitaly Kuznetsov
@ 2015-01-30  5:03   ` Dexuan Cui
  2015-02-01 19:42     ` KY Srinivasan
  0 siblings, 1 reply; 5+ messages in thread
From: Dexuan Cui @ 2015-01-30  5:03 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: gregkh, linux-kernel, driverdev-devel, olaf, apw, jasowang,
	KY Srinivasan, 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 <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.
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" <kys@microsoft.com>
> > Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > ---
> >  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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM
  2015-01-30  5:03   ` Dexuan Cui
@ 2015-02-01 19:42     ` KY Srinivasan
  2015-02-02  2:37       ` Dexuan Cui
  0 siblings, 1 reply; 5+ messages in thread
From: KY Srinivasan @ 2015-02-01 19:42 UTC (permalink / raw)
  To: Dexuan Cui, Vitaly Kuznetsov
  Cc: gregkh, linux-kernel, driverdev-devel, olaf, apw, jasowang,
	Haiyang Zhang



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

> 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



^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM
  2015-02-01 19:42     ` KY Srinivasan
@ 2015-02-02  2:37       ` Dexuan Cui
  0 siblings, 0 replies; 5+ messages in thread
From: Dexuan Cui @ 2015-02-02  2:37 UTC (permalink / raw)
  To: KY Srinivasan, Vitaly Kuznetsov
  Cc: gregkh, linux-kernel, driverdev-devel, olaf, apw, jasowang,
	Haiyang Zhang

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2015-02-02  2:38 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-29 11:02 [PATCH 3/3] hv: vmbus_open(): reset the channel state on ENOMEM Dexuan Cui
2015-01-29 13:22 ` Vitaly Kuznetsov
2015-01-30  5:03   ` Dexuan Cui
2015-02-01 19:42     ` KY Srinivasan
2015-02-02  2:37       ` Dexuan Cui

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).