AR6000 netif_queue_stop non stop, Bug?

[Rask Ingemann Lambertsen]

On Fri, Mar 27, 2009 at 10:05:36PM +0300, Ivan Petrov wrote:
> Clean patch attached.
>
> If posible, please apply it to repo.
>
> --
> Regards, Ivan.

> @@ -1278,6 +1280,7 @@
>          connect.EpCallbacks.EpRecv = ar6000_rx;
>          connect.EpCallbacks.EpRecvRefill = ar6000_rx_refill;
>          connect.EpCallbacks.EpSendFull = ar6000_tx_queue_full;
> +        connect.EpCallbacks.EpSendAvail = ar6000_tx_queue_avail;
>              /* set the max queue depth so that our ar6000_tx_queue_full handler gets called.
>               * Linux has the peculiarity of not providing flow control between the
>               * NIC and the network stack. There is no API to indicate that a TX packet
[code continues like this]
             * was sent which could provide some back pressure to the network stack.
             * Under linux you would have to wait till the network stack consumed all sk_buffs
             * before any back-flow kicked in. Which isn't very friendly.
             * So we have to manage this ourselves */
        connect.MaxSendQueueDepth = 32;

   Not a comment about you patch, but since you're hacking on the driver, I
think I'll mention it here. You're not supposed to have a lot of TX packets
queued at the driver level. You should only queue enough that you have time
to prepare a new packet for the hardware while the hardware is still busy
sending the previous packet. If you queue many packets at the driver level,
the network stack has no chance of reordering the packets. It also won't
know that your hardware is already fully occupied after 2-3 calls of
dev->hard_start_xmit(). To me, it looks from the comment above as if the
ar6000 driver is shooting itself in the foot by maintaining a large queue,
because doing so causes exactly the problem that the comment complains
about. netif_stop_queue()/netif_wake_queue() _is_ the flow control.

   If you want a large TX queue, look at dev->tx_queue_len. You can check
the value with ifconfig. Example:

$ /sbin/ifconfig usb0
usb0      Link encap:Ethernet  HWaddr 06:e2:8a:da:03:a9  
          inet addr:192.168.0.202  Bcast:192.168.0.255  Mask:255.255.255.0
          inet6 addr: fe80::4e2:8aff:feda:3a9/64 Scope:Link
          UP BROADCAST RUNNING MULTICAST  MTU:1500  Metric:1
          RX packets:13006 errors:0 dropped:0 overruns:0 frame:0
          TX packets:12510 errors:0 dropped:0 overruns:0 carrier:0
          collisions:0 txqueuelen:1000 
          RX bytes:1044602 (1020.1 KiB)  TX bytes:4824200 (4.6 MiB)

   The default dev->tx_queue_len was raised from 100 to 1000 in 2003-2004
IIRC.

   FWIW, when I wrote the driver for the NE3200 10 Mbps Ethernet card, I
found that a TX buffer large enough to hold three full size packets was
enough to keep the number of queued, unsent packet bytes at least as large
as the number of packet bytes being transmitted by the chip.

> @@ -2317,7 +2315,7 @@
>          /* flush data queues */
>      ar6000_TxDataCleanup(ar);
>  
> -    netif_wake_queue(ar->arNetDev);
> +    netif_start_queue(ar->arNetDev);
>  
>      if ((OPEN_AUTH == ar->arDot11AuthMode) &&
>          (NONE_AUTH == ar->arAuthMode)      &&

   Out of curiousity, what does this change do?

-- 
Rask Ingemann Lambertsen
Danish law requires addresses in e-mail to be logged and stored for a year