tuntap: Overload handling

tuntap: Overload handling

[Sebastian Pöhn]

I am having a look on the tun driver to realize an userspace network
driver ( TAP + UIO ). Maybe that's not the use-case tun is intended
for.

What I've noticed is that in tun.c Line 741

static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)

/* Limit the number of packets queued by dividing txq length with the
 * number of queues.
 */
if (skb_queue_len(&tfile->socket.sk->sk_receive_queue)
 >= dev->tx_queue_len / tun->numqueues)
goto drop;

If a frame can not be tx it is dropped by the driver.
Wouldn't it be more correct to netif_tx_stop_queue() so that packet
drops are performed by the overlying traffic control code?

Of course this is not very likely in virtual environments but as soon
as any real network hop is involved it could be important.

(I also had a look on some two year old version of tun.c. There
queue/tx stopping was done correctly.)

---

Re: tuntap: Overload handling

[Eric Dumazet]

On Thu, 2013-02-14 at 12:50 +0100, Sebastian Pöhn wrote:
> I am having a look on the tun driver to realize an userspace network
> driver ( TAP + UIO ). Maybe that's not the use-case tun is intended
> for.
> 
> What I've noticed is that in tun.c Line 741
> 
> static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
> 
> /* Limit the number of packets queued by dividing txq length with the
>  * number of queues.
>  */
> if (skb_queue_len(&tfile->socket.sk->sk_receive_queue)
>  >= dev->tx_queue_len / tun->numqueues)
> goto drop;
> 
> If a frame can not be tx it is dropped by the driver.
> Wouldn't it be more correct to netif_tx_stop_queue() so that packet
> drops are performed by the overlying traffic control code?
> 
> Of course this is not very likely in virtual environments but as soon
> as any real network hop is involved it could be important.
> 
> (I also had a look on some two year old version of tun.c. There
> queue/tx stopping was done correctly.)

You should ask Michael S. Tsirkin, as he removed the flow control
in commit 5d097109257c03a71845729f8db6b5770c4bbedc
(tun: only queue packets on device)

Re: tuntap: Overload handling

[Michael S. Tsirkin]

On Thu, Feb 14, 2013 at 08:32:27AM -0800, Eric Dumazet wrote:
> On Thu, 2013-02-14 at 12:50 +0100, Sebastian Pöhn wrote:
> > I am having a look on the tun driver to realize an userspace network
> > driver ( TAP + UIO ). Maybe that's not the use-case tun is intended
> > for.
> > 
> > What I've noticed is that in tun.c Line 741
> > 
> > static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
> > 
> > /* Limit the number of packets queued by dividing txq length with the
> >  * number of queues.
> >  */
> > if (skb_queue_len(&tfile->socket.sk->sk_receive_queue)
> >  >= dev->tx_queue_len / tun->numqueues)
> > goto drop;
> > 
> > If a frame can not be tx it is dropped by the driver.
> > Wouldn't it be more correct to netif_tx_stop_queue() so that packet
> > drops are performed by the overlying traffic control code?
> > 
> > Of course this is not very likely in virtual environments but as soon
> > as any real network hop is involved it could be important.
> > 
> > (I also had a look on some two year old version of tun.c. There
> > queue/tx stopping was done correctly.)

Hmm so ~1000 packets in the tun queue is not enough?
You always have the option to increase it some more ...

> You should ask Michael S. Tsirkin, as he removed the flow control
> in commit 5d097109257c03a71845729f8db6b5770c4bbedc
> (tun: only queue packets on device)
> 

Eric in the past you said the following things
(http://lkml.indiana.edu/hypermail/linux/kernel/1204.1/00784.html)
> > In your case I would just not use qdisc at all, like other virtual
> > devices.
...
> > Anyway, with a 500 packet limit in TUN queue itself, qdisc layer should
> > be always empty. Whats the point storing more than 500 packets for a
> > device ? Thats a latency killer.
you don't think this applies, anymore?

-- 
MST

Re: tuntap: Overload handling

[Eric Dumazet]

On Thu, 2013-02-14 at 18:42 +0200, Michael S. Tsirkin wrote:

> Hmm so ~1000 packets in the tun queue is not enough?
> You always have the option to increase it some more ...
> 
> > You should ask Michael S. Tsirkin, as he removed the flow control
> > in commit 5d097109257c03a71845729f8db6b5770c4bbedc
> > (tun: only queue packets on device)
> > 
> 
> Eric in the past you said the following things
> (http://lkml.indiana.edu/hypermail/linux/kernel/1204.1/00784.html)
> > > In your case I would just not use qdisc at all, like other virtual
> > > devices.
> ...
> > > Anyway, with a 500 packet limit in TUN queue itself, qdisc layer should
> > > be always empty. Whats the point storing more than 500 packets for a
> > > device ? Thats a latency killer.
> you don't think this applies, anymore?
> 

Users have the choice to setup a qdisc or not.

Having no qdisc can help raw performance, at the expense of bufferbloat.
Thats all I was saying.

It seems tun.c has no longer the possibility to effectively use a qdisc,
(allowing the queue to buildup at qdisc layer)

Re: tuntap: Overload handling

[Sebastian Pöhn]

Ok, thanks. So if you think of tuntap as a virtual device (and that's
what it is) then the drop behavior is quite a valid way.

So I'll gonna try to use the "old" version with qdisc support and
extend it be the features I need.

On Thu, Feb 14, 2013 at 6:01 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2013-02-14 at 18:42 +0200, Michael S. Tsirkin wrote:
>
>> Hmm so ~1000 packets in the tun queue is not enough?
>> You always have the option to increase it some more ...
>>
>> > You should ask Michael S. Tsirkin, as he removed the flow control
>> > in commit 5d097109257c03a71845729f8db6b5770c4bbedc
>> > (tun: only queue packets on device)
>> >
>>
>> Eric in the past you said the following things
>> (http://lkml.indiana.edu/hypermail/linux/kernel/1204.1/00784.html)
>> > > In your case I would just not use qdisc at all, like other virtual
>> > > devices.
>> ...
>> > > Anyway, with a 500 packet limit in TUN queue itself, qdisc layer should
>> > > be always empty. Whats the point storing more than 500 packets for a
>> > > device ? Thats a latency killer.
>> you don't think this applies, anymore?
>>
>
> Users have the choice to setup a qdisc or not.
>
> Having no qdisc can help raw performance, at the expense of bufferbloat.
> Thats all I was saying.
>
> It seems tun.c has no longer the possibility to effectively use a qdisc,
> (allowing the queue to buildup at qdisc layer)
>
>
>

Re: tuntap: Overload handling

[Michael S. Tsirkin]

On Thu, Feb 14, 2013 at 09:01:30AM -0800, Eric Dumazet wrote:
> On Thu, 2013-02-14 at 18:42 +0200, Michael S. Tsirkin wrote:
> 
> > Hmm so ~1000 packets in the tun queue is not enough?
> > You always have the option to increase it some more ...
> > 
> > > You should ask Michael S. Tsirkin, as he removed the flow control
> > > in commit 5d097109257c03a71845729f8db6b5770c4bbedc
> > > (tun: only queue packets on device)
> > > 
> > 
> > Eric in the past you said the following things
> > (http://lkml.indiana.edu/hypermail/linux/kernel/1204.1/00784.html)
> > > > In your case I would just not use qdisc at all, like other virtual
> > > > devices.
> > ...
> > > > Anyway, with a 500 packet limit in TUN queue itself, qdisc layer should
> > > > be always empty. Whats the point storing more than 500 packets for a
> > > > device ? Thats a latency killer.
> > you don't think this applies, anymore?
> > 
> 
> Users have the choice to setup a qdisc or not.
> 
> Having no qdisc can help raw performance, at the expense of bufferbloat.
> Thats all I was saying.
> 
> It seems tun.c has no longer the possibility to effectively use a qdisc,
> (allowing the queue to buildup at qdisc layer)
> 

But, userspace is in no position to decide whether using
the qdisc is a good or a bad thing.
The issue I tried to solve is that with tun, it's trivially easy for
userspace to lock up resources forever.
Simply not stopping the qdisc is probably the simplest solution.

An alternative is to orphan the skbs before we queue them.
At some point I posted a proposal doing exactly this
subj of "net: orphan queued skbs if device tx can stall".
Do you think it's worth revisiting this?

Also - does anyone know of a testcase showing there's a problem
with the simplest solution we now have in place?

-- 
MST

Re: tuntap: Overload handling

[Sebastian Pöhn]

On Sun, 2013-02-17 at 15:24 +0200, Michael S. Tsirkin wrote:
> On Thu, Feb 14, 2013 at 09:01:30AM -0800, Eric Dumazet wrote:
> > On Thu, 2013-02-14 at 18:42 +0200, Michael S. Tsirkin wrote:
> > 
> > > Hmm so ~1000 packets in the tun queue is not enough?
> > > You always have the option to increase it some more ...
> > > 
> > > > You should ask Michael S. Tsirkin, as he removed the flow control
> > > > in commit 5d097109257c03a71845729f8db6b5770c4bbedc
> > > > (tun: only queue packets on device)
> > > > 
> > > 
> > > Eric in the past you said the following things
> > > (http://lkml.indiana.edu/hypermail/linux/kernel/1204.1/00784.html)
> > > > > In your case I would just not use qdisc at all, like other virtual
> > > > > devices.
> > > ...
> > > > > Anyway, with a 500 packet limit in TUN queue itself, qdisc layer should
> > > > > be always empty. Whats the point storing more than 500 packets for a
> > > > > device ? Thats a latency killer.
> > > you don't think this applies, anymore?
> > > 
> > 
> > Users have the choice to setup a qdisc or not.
> > 
> > Having no qdisc can help raw performance, at the expense of bufferbloat.
> > Thats all I was saying.
> > 
> > It seems tun.c has no longer the possibility to effectively use a qdisc,
> > (allowing the queue to buildup at qdisc layer)
> > 
> 
> But, userspace is in no position to decide whether using
> the qdisc is a good or a bad thing.
> The issue I tried to solve is that with tun, it's trivially easy for
> userspace to lock up resources forever.
> Simply not stopping the qdisc is probably the simplest solution.
> 
> An alternative is to orphan the skbs before we queue them.
> At some point I posted a proposal doing exactly this
> subj of "net: orphan queued skbs if device tx can stall".
> Do you think it's worth revisiting this?
> 
> Also - does anyone know of a testcase showing there's a problem
> with the simplest solution we now have in place?
> 

I think the solution is good as it is. Of course if you want to do odd
things with it like me - it's not, but that's not its usual use-case.

Re: tuntap: Overload handling

[Michael S. Tsirkin]

On Sun, Feb 17, 2013 at 05:08:13PM +0100, Sebastian Pöhn wrote:
> On Sun, 2013-02-17 at 15:24 +0200, Michael S. Tsirkin wrote:
> > On Thu, Feb 14, 2013 at 09:01:30AM -0800, Eric Dumazet wrote:
> > > On Thu, 2013-02-14 at 18:42 +0200, Michael S. Tsirkin wrote:
> > > 
> > > > Hmm so ~1000 packets in the tun queue is not enough?
> > > > You always have the option to increase it some more ...
> > > > 
> > > > > You should ask Michael S. Tsirkin, as he removed the flow control
> > > > > in commit 5d097109257c03a71845729f8db6b5770c4bbedc
> > > > > (tun: only queue packets on device)
> > > > > 
> > > > 
> > > > Eric in the past you said the following things
> > > > (http://lkml.indiana.edu/hypermail/linux/kernel/1204.1/00784.html)
> > > > > > In your case I would just not use qdisc at all, like other virtual
> > > > > > devices.
> > > > ...
> > > > > > Anyway, with a 500 packet limit in TUN queue itself, qdisc layer should
> > > > > > be always empty. Whats the point storing more than 500 packets for a
> > > > > > device ? Thats a latency killer.
> > > > you don't think this applies, anymore?
> > > > 
> > > 
> > > Users have the choice to setup a qdisc or not.
> > > 
> > > Having no qdisc can help raw performance, at the expense of bufferbloat.
> > > Thats all I was saying.
> > > 
> > > It seems tun.c has no longer the possibility to effectively use a qdisc,
> > > (allowing the queue to buildup at qdisc layer)
> > > 
> > 
> > But, userspace is in no position to decide whether using
> > the qdisc is a good or a bad thing.
> > The issue I tried to solve is that with tun, it's trivially easy for
> > userspace to lock up resources forever.
> > Simply not stopping the qdisc is probably the simplest solution.
> > 
> > An alternative is to orphan the skbs before we queue them.
> > At some point I posted a proposal doing exactly this
> > subj of "net: orphan queued skbs if device tx can stall".
> > Do you think it's worth revisiting this?
> > 
> > Also - does anyone know of a testcase showing there's a problem
> > with the simplest solution we now have in place?
> > 
> 
> I think the solution is good as it is. Of course if you want to do odd
> things with it like me - it's not, but that's not its usual use-case.

Tap+UIO seems actually pretty close to a VM case.
Do you know it's not good for your usecase, or do you speculate?
What's the tx queue length in your setup?

-- 
MST

Re: tuntap: Overload handling

[Sebastian Pöhn]

On Sun, 2013-02-17 at 18:18 +0200, Michael S. Tsirkin wrote:
> On Sun, Feb 17, 2013 at 05:08:13PM +0100, Sebastian Pöhn wrote:
> > On Sun, 2013-02-17 at 15:24 +0200, Michael S. Tsirkin wrote:
> > > On Thu, Feb 14, 2013 at 09:01:30AM -0800, Eric Dumazet wrote:
> > > > On Thu, 2013-02-14 at 18:42 +0200, Michael S. Tsirkin wrote:
> > > > 
> > > > > Hmm so ~1000 packets in the tun queue is not enough?
> > > > > You always have the option to increase it some more ...
> > > > > 
> > > > > > You should ask Michael S. Tsirkin, as he removed the flow control
> > > > > > in commit 5d097109257c03a71845729f8db6b5770c4bbedc
> > > > > > (tun: only queue packets on device)
> > > > > > 
> > > > > 
> > > > > Eric in the past you said the following things
> > > > > (http://lkml.indiana.edu/hypermail/linux/kernel/1204.1/00784.html)
> > > > > > > In your case I would just not use qdisc at all, like other virtual
> > > > > > > devices.
> > > > > ...
> > > > > > > Anyway, with a 500 packet limit in TUN queue itself, qdisc layer should
> > > > > > > be always empty. Whats the point storing more than 500 packets for a
> > > > > > > device ? Thats a latency killer.
> > > > > you don't think this applies, anymore?
> > > > > 
> > > > 
> > > > Users have the choice to setup a qdisc or not.
> > > > 
> > > > Having no qdisc can help raw performance, at the expense of bufferbloat.
> > > > Thats all I was saying.
> > > > 
> > > > It seems tun.c has no longer the possibility to effectively use a qdisc,
> > > > (allowing the queue to buildup at qdisc layer)
> > > > 
> > > 
> > > But, userspace is in no position to decide whether using
> > > the qdisc is a good or a bad thing.
> > > The issue I tried to solve is that with tun, it's trivially easy for
> > > userspace to lock up resources forever.
> > > Simply not stopping the qdisc is probably the simplest solution.
> > > 
> > > An alternative is to orphan the skbs before we queue them.
> > > At some point I posted a proposal doing exactly this
> > > subj of "net: orphan queued skbs if device tx can stall".
> > > Do you think it's worth revisiting this?
> > > 
> > > Also - does anyone know of a testcase showing there's a problem
> > > with the simplest solution we now have in place?
> > > 
> > 
> > I think the solution is good as it is. Of course if you want to do odd
> > things with it like me - it's not, but that's not its usual use-case.
> 
> Tap+UIO seems actually pretty close to a VM case.
In this case, no. What I have is a over-provisioned wan line. If you
have double the load you can handle - having a QoS system which is
slowing down your clients is essential, dropping in a device driver is
the last you may want.
> Do you know it's not good for your usecase, or do you speculate?
Well the scheme I work in is like this: The wan line says 'Hey I have
some bandwidth, give me some traffic.' So it's not a good idea that the
network subsystem is blowing in a lot of traffic.
> What's the tx queue length in your setup?
No decided yet.
> 
But again I think it's not the aim of tuntap to satisfy my exotic usage.
I'll gonna do some changes in it anyway.

Re: tuntap: Overload handling

[Eric Dumazet]

On Sun, 2013-02-17 at 15:24 +0200, Michael S. Tsirkin wrote:

> But, userspace is in no position to decide whether using
> the qdisc is a good or a bad thing.
> The issue I tried to solve is that with tun, it's trivially easy for
> userspace to lock up resources forever.
> Simply not stopping the qdisc is probably the simplest solution.
> 
> An alternative is to orphan the skbs before we queue them.
> At some point I posted a proposal doing exactly this
> subj of "net: orphan queued skbs if device tx can stall".
> Do you think it's worth revisiting this?


Its trivially easy for userspace to consume all resources, with or
without tuntap.

Say, a regular Gbps ethernet device.

We need some flow control at some point, unless we deal with device
with unlimited bandwidth.

If we orphan skbs too soon, how can we limit a single sender to flood
the device ? Even a single TCP flow could do that. TCP Small Queues
prevent this, but relies on proper skb orphaning.

If the tuntap problem is that skb can sit there and are never consumed,
its not a bug in the producer (the sender), but a problem with the
receiver (the consumer of the queue). Some kind of cleanup is needed.

Lets take the qdisc analogy :

Codel for instance is able to drop packets that are sitting too long in
queue. skbs are not orphaned until they are freed.