Re: [PATCH] net: Disable queueing when carrier is lost

[Tommy Christensen <tommy.christensen@tpack.net>]

Herbert Xu wrote:
> You're right.  However, the netif_queue_stopped check doesn't make sense.
> If the queue isn't stopped, you'd be leaving the qdisc open which means
> that it can fill up later on.  Remember the problem is not just with NIC
> drivers stopping the queue explicitly, some NICs simply won't process the
> TX rings when the carrier is off.

I know. I've given this a lot of thought already - and failed to come
up with a solution for everything.

Regarding the sort of NICs you mention, I came to the conclusion that
we shouldn't try to fix this in the stack. They indicate that they can
take more packets (by NOT calling netif_stop_queue), so we should obey
this and leave it to the drivers to handle things, e.g. by dropping the
packets if needed. Otherwise we deprive the drivers the option of
maintaining their tx_carrier_errors statistics, for instance. And,
theoretically, a driver could even depend on having its hard_start_xmit
invoked in order to kickstart its TX engine.

As stated earlier: if a driver holds on to packets "indefinitely", it
is buggy. This should be fixed, rather than the core having to jump
through loops trying to remedy things.

So, I am reluctant to drop this check for netif_queue_stopped. Doing so
would needlessly impact the drivers that work fine already (or at least
should be handling this situation).

> The other concern I have is that this code can call dev_activate
> or dev_deactivate twice in a row.  As far as I can tell this is
> harmless for the moment but it would be nice if we can avoid it.

Agreed, this isn't ideal. However, if this is the only downside, it
is something that I'd be happy to live with. I don't see any easy
way to avoid it.

Ideas, anyone?


-Tommy