[PATCH] tun: Persistent devices can get stuck in xoff state

[PATCH] tun: Persistent devices can get stuck in xoff state

[Max Krasnyansky]

The scenario goes like this. App stops reading from tun/tap.
TX queue gets full and driver does netif_stop_queue().
App closes fd and TX queue gets flushed as part of the cleanup.
Next time the app opens tun/tap and starts reading from it but
the xoff state is not cleared. We're stuck.
Normally xoff state is cleared when netdev is brought up. But
in the case of persistent devices this happens only during
initial setup.

The fix is trivial. If device is already up when an app opens
it we clear xoff state and that gets things moving again.

Signed-off-by: Max Krasnyansky <maxk@qualcomm.com>
---
 drivers/net/tun.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 5b5d875..e665f72 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -576,6 +576,11 @@ static int tun_set_iff(struct file *file, struct ifreq *ifr)
 	file->private_data = tun;
 	tun->attached = 1;
 
+	/* Make sure persistent devices do not get stuck in
+	 * xoff state */
+	if (netif_running(tun->dev))
+		netif_wake_queue(tun->dev);
+
 	strcpy(ifr->ifr_name, tun->dev->name);
 	return 0;
 
-- 
1.5.5.1

Re: [PATCH] tun: Persistent devices can get stuck in xoff state

[Christian Borntraeger]

Am Mittwoch, 9. Juli 2008 schrieb Max Krasnyansky:
> The scenario goes like this. App stops reading from tun/tap.
> TX queue gets full and driver does netif_stop_queue().
> App closes fd and TX queue gets flushed as part of the cleanup.
> Next time the app opens tun/tap and starts reading from it but
> the xoff state is not cleared. We're stuck.
> Normally xoff state is cleared when netdev is brought up. But
> in the case of persistent devices this happens only during
> initial setup.

Thats interesting. I believe we have seen exactly this behaviour
with KVM and lots of preallocated tap devices. 
[...]
> +++ b/drivers/net/tun.c
> @@ -576,6 +576,11 @@ static int tun_set_iff(struct file *file, struct ifreq 
*ifr)
>  	file->private_data = tun;
>  	tun->attached = 1;
> 
> +	/* Make sure persistent devices do not get stuck in
> +	 * xoff state */
> +	if (netif_running(tun->dev))
> +		netif_wake_queue(tun->dev);
> +
>  	strcpy(ifr->ifr_name, tun->dev->name);
>  	return 0;

I think that patch looks ok, but I am curious why you dont clear the xoff 
state on application close at the same time when the TX queue gets flushed?

Christian

Re: [PATCH] tun: Persistent devices can get stuck in xoff state

[Christian Borntraeger]

Am Mittwoch, 9. Juli 2008 schrieb Max Krasnyansky:
> The scenario goes like this. App stops reading from tun/tap.
> TX queue gets full and driver does netif_stop_queue().
> App closes fd and TX queue gets flushed as part of the cleanup.
> Next time the app opens tun/tap and starts reading from it but
> the xoff state is not cleared. We're stuck.
> Normally xoff state is cleared when netdev is brought up. But
> in the case of persistent devices this happens only during
> initial setup.

I was able to reproduce the problem. The patch seems to fix it.

Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>

Re: [PATCH] tun: Persistent devices can get stuck in xoff state

[Max Krasnyansky]

Christian Borntraeger wrote:
> Am Mittwoch, 9. Juli 2008 schrieb Max Krasnyansky:
>> The scenario goes like this. App stops reading from tun/tap.
>> TX queue gets full and driver does netif_stop_queue().
>> App closes fd and TX queue gets flushed as part of the cleanup.
>> Next time the app opens tun/tap and starts reading from it but
>> the xoff state is not cleared. We're stuck.
>> Normally xoff state is cleared when netdev is brought up. But
>> in the case of persistent devices this happens only during
>> initial setup.
> 
> Thats interesting. I believe we have seen exactly this behaviour
> with KVM and lots of preallocated tap devices. 
Yeah, it's interesting given that it's (the bug that is) been there for 
at least 2-3 years now :).

> [...]
>> +++ b/drivers/net/tun.c
>> @@ -576,6 +576,11 @@ static int tun_set_iff(struct file *file, struct ifreq 
> *ifr)
>>  	file->private_data = tun;
>>  	tun->attached = 1;
>>
>> +	/* Make sure persistent devices do not get stuck in
>> +	 * xoff state */
>> +	if (netif_running(tun->dev))
>> +		netif_wake_queue(tun->dev);
>> +
>>  	strcpy(ifr->ifr_name, tun->dev->name);
>>  	return 0;
> 
> I think that patch looks ok, but I am curious why you dont clear the xoff 
> state on application close at the same time when the TX queue gets flushed?
Why bother. I mean the packets will be dropped anyway.

Max

Re: [PATCH] tun: Persistent devices can get stuck in xoff state

[Max Krasnyansky]

Christian Borntraeger wrote:
> Am Mittwoch, 9. Juli 2008 schrieb Max Krasnyansky:
>> The scenario goes like this. App stops reading from tun/tap.
>> TX queue gets full and driver does netif_stop_queue().
>> App closes fd and TX queue gets flushed as part of the cleanup.
>> Next time the app opens tun/tap and starts reading from it but
>> the xoff state is not cleared. We're stuck.
>> Normally xoff state is cleared when netdev is brought up. But
>> in the case of persistent devices this happens only during
>> initial setup.
> 
> I was able to reproduce the problem. The patch seems to fix it.
> 
> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>

Oh it definitely fixes that problem. It's very easy to reproduce by just 
suspending (ctrl-z) an app that is ready from the tun, waiting a bit for 
the queue to fill up and then killing the app. I have a simple test app 
that simulates this. It's just that in real life that does not happen 
very often. ie Applications either constantly read from tun or they 
close it.

Thanks a lot for testing and confirming.

Max

Re: [PATCH] tun: Persistent devices can get stuck in xoff state

[David Miller]

From: Max Krasnyansky <maxk@qualcomm.com>
Date: Wed, 09 Jul 2008 14:19:28 -0700

> Christian Borntraeger wrote:
> > Am Mittwoch, 9. Juli 2008 schrieb Max Krasnyansky:
> >> The scenario goes like this. App stops reading from tun/tap.
> >> TX queue gets full and driver does netif_stop_queue().
> >> App closes fd and TX queue gets flushed as part of the cleanup.
> >> Next time the app opens tun/tap and starts reading from it but
> >> the xoff state is not cleared. We're stuck.
> >> Normally xoff state is cleared when netdev is brought up. But
> >> in the case of persistent devices this happens only during
> >> initial setup.
> > 
> > I was able to reproduce the problem. The patch seems to fix it.
> > 
> > Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> Oh it definitely fixes that problem. It's very easy to reproduce by just 
> suspending (ctrl-z) an app that is ready from the tun, waiting a bit for 
> the queue to fill up and then killing the app. I have a simple test app 
> that simulates this. It's just that in real life that does not happen 
> very often. ie Applications either constantly read from tun or they 
> close it.
> 
> Thanks a lot for testing and confirming.

I've applied this, thanks.

But I had to apply it by hand.  You made this against an older
tree, because in the current code there is a get_net() call
right before the new lines you added but in your patch context
it isn't there.

Re: [PATCH] tun: Persistent devices can get stuck in xoff state

[Max Krasnyansky]

David Miller wrote:
> From: Max Krasnyansky <maxk@qualcomm.com>
> Date: Wed, 09 Jul 2008 14:19:28 -0700
> 
>> Christian Borntraeger wrote:
>>> Am Mittwoch, 9. Juli 2008 schrieb Max Krasnyansky:
>>>> The scenario goes like this. App stops reading from tun/tap.
>>>> TX queue gets full and driver does netif_stop_queue().
>>>> App closes fd and TX queue gets flushed as part of the cleanup.
>>>> Next time the app opens tun/tap and starts reading from it but
>>>> the xoff state is not cleared. We're stuck.
>>>> Normally xoff state is cleared when netdev is brought up. But
>>>> in the case of persistent devices this happens only during
>>>> initial setup.
>>> I was able to reproduce the problem. The patch seems to fix it.
>>>
>>> Tested-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Oh it definitely fixes that problem. It's very easy to reproduce by just 
>> suspending (ctrl-z) an app that is ready from the tun, waiting a bit for 
>> the queue to fill up and then killing the app. I have a simple test app 
>> that simulates this. It's just that in real life that does not happen 
>> very often. ie Applications either constantly read from tun or they 
>> close it.
>>
>> Thanks a lot for testing and confirming.
> 
> I've applied this, thanks.
> 
> But I had to apply it by hand.  You made this against an older
> tree, because in the current code there is a get_net() call
> right before the new lines you added but in your patch context
> it isn't there.

Hmm, I though I used latest git tree. Oh, turns out it was 2.6.25.4. 
Sorry. Thanx for fixing it.

Max