Re: Serializing hard_start_xmit() (was: 3c509 ethernet going down.)

NIIBE Yutaka (gniibe@mri.co.jp)
Wed, 29 Jan 1997 21:27:38 +0900


Alan Cox writes:
> The big thing as I see it is that we don't want to slow the kernel down
> by serializing it (especially when the SMP is fine grained). We want to
> change the driver semantics a little so that you get the expected behaviour
> in all cases. The dev->tbusy TX/RX threading being the issue.

Yes.

BTW, here is small cosmetic changes of net/core/dev.c (against
2.1.24). This slightly improves the code.

Changes:
(1) New function dev_queue_retransmit(). Call this function when retransmit
the packet.

(2) In do_dev_queue_xmit(), distinguish normal kernel level and
software interrupt level by intr_count.

If it is normal kernel level and dev->tbusy==0, the queue must be
empty, so we can send the packet (don't need checking the queue).

If it is software kernel level, this thread is the only one.
Hence, we don't need another locking mechanism.

(3) Guarantee that hard_start_xmit() is called atomically with dev->tbusy==0.
(This can make network drivers simple.)

Thanks,

-- 
NIIBE Yutaka

--- dev.c.orig +++ dev.c Wed Jan 29 20:46:35 1997 @@ -53,6 +53,7 @@ #include <asm/uaccess.h> #include <asm/system.h> #include <asm/bitops.h> +#include <asm/smp_lock.h> #include <linux/config.h> #include <linux/types.h> #include <linux/kernel.h> @@ -424,34 +425,12 @@ /* * Send (or queue for sending) a packet. * - * IMPORTANT: When this is called to resend frames. The caller MUST - * already have locked the sk_buff. Apart from that we do the - * rest of the magic. */ static void do_dev_queue_xmit(struct sk_buff *skb, struct device *dev, int pri) { unsigned long flags; struct sk_buff_head *list; - int retransmission = 0; /* used to say if the packet should go */ - /* at the front or the back of the */ - /* queue - front is a retransmit try */ - -#if CONFIG_SKB_CHECK - IS_SKB(skb); -#endif - - /* - * Negative priority is used to flag a frame that is being pulled from the - * queue front as a retransmit attempt. It therefore goes back on the queue - * start on a failure. - */ - - if (pri < 0) - { - pri = -pri-1; - retransmission = 1; - } #ifdef CONFIG_NET_DEBUG if (pri >= DEV_NUMBUFFS) @@ -478,50 +457,72 @@ save_flags(flags); - /* - * If this isn't a retransmission, use the first packet instead. - * Note: We don't do strict priority ordering here. We will in - * fact kick the queue that is our priority. The dev_tint reload - * does strict priority queueing. In effect what we are doing here - * is to add some random jitter to the queues and to do so by - * saving clocks. Doing a perfect priority queue isn't a good idea - * as you get some fascinating timing interactions. - */ - - if (!retransmission) + /* avoid overrunning the device queue.. */ + if (skb_queue_len(list) > dev->tx_queue_len) { - /* avoid overrunning the device queue.. */ - if (skb_queue_len(list) > dev->tx_queue_len) - { - dev_kfree_skb(skb, FREE_WRITE); - return; - } - - /* copy outgoing packets to any sniffer packet handlers */ - if (dev_nit) - queue_xmit_nit(skb,dev); + dev_kfree_skb(skb, FREE_WRITE); + return; + } + /* copy outgoing packets to any sniffer packet handlers */ + if (dev_nit) + queue_xmit_nit(skb,dev); + + if (intr_count == 0) { + /* NORMAL kernel level. (We can sleep here) */ + int tbusy; + + start_bh_atomic(); + lock_kernel(); + if ((tbusy = dev->tbusy) == 0) + tbusy = dev->hard_start_xmit(skb, dev); + unlock_kernel(); + end_bh_atomic(); + + if (tbusy == 0) + return 0; + } else { + /* Software Interrupt level: Send it. */ + + /* + * Use the first packet instead. + * Note: We don't do strict priority ordering here. We will in + * fact kick the queue that is our priority. The dev_tint reload + * does strict priority queueing. In effect what we are doing here + * is to add some random jitter to the queues and to do so by + * saving clocks. Doing a perfect priority queue isn't a good idea + * as you get some fascinating timing interactions. + */ if (skb_queue_len(list)) { + unsigned long flags; + save_flags(flags); cli(); __skb_queue_tail(list, skb); skb = __skb_dequeue(list); restore_flags(flags); } - } - if (dev->hard_start_xmit(skb, dev) == 0) { - /* - * Packet is now solely the responsibility of the driver - */ - return; + if (dev->tbusy == 0 && dev->hard_start_xmit(skb, dev) == 0) + return 0; } /* * Transmission failed, put skb back into a list. Once on the list it's safe and * no longer device locked (it can be freed safely from the device queue) */ - cli(); - __skb_queue_head(list,skb); - restore_flags(flags); + skb_queue_head(list,skb); +} + +/* + * Retransmit a packet. + */ + +static void dev_queue_retransmit(struct sk_buff *skb, struct device *dev, + struct sk_buff_head *head) +{ + if (dev->hard_start_xmit(skb, dev) == 0) + return; + + skb_queue_head(head, skb); } /* @@ -532,8 +533,6 @@ { struct device *dev = skb->dev; - start_bh_atomic(); - #if CONFIG_SKB_CHECK IS_SKB(skb); #endif @@ -554,10 +553,7 @@ if (dev->rebuild_header) { if (dev->rebuild_header(skb)) - { - end_bh_atomic(); return 0; - } } else printk("%s: !skb->arp & !rebuild_header!\n", dev->name); @@ -575,7 +571,6 @@ skb->dev = dev = net_alias_main_dev(dev); do_dev_queue_xmit(skb, dev, skb->priority); - end_bh_atomic(); return 0; } @@ -907,7 +902,7 @@ * Feed them to the output stage and if it fails * indicate they re-queue at the front. */ - do_dev_queue_xmit(skb,dev,-i - 1); + dev_queue_retransmit(skb, dev, head); /* * If we can take no more then stop here. */