From 902bca00dc6e3b3ff5fbb1e32e5dbb45d5f30579 Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Sat, 6 Nov 2010 12:36:13 +0100 Subject: firewire: net: count stats.tx_packets and stats.tx_bytes Signed-off-by: Stefan Richter diff --git a/drivers/firewire/net.c b/drivers/firewire/net.c index 18fdd97..e2e968e 100644 --- a/drivers/firewire/net.c +++ b/drivers/firewire/net.c @@ -906,6 +906,7 @@ static int fwnet_send_packet(struct fwnet_packet_task *ptask); static void fwnet_transmit_packet_done(struct fwnet_packet_task *ptask) { struct fwnet_device *dev = ptask->dev; + struct sk_buff *skb = ptask->skb; unsigned long flags; bool free; @@ -916,8 +917,11 @@ static void fwnet_transmit_packet_done(struct fwnet_packet_task *ptask) /* Check whether we or the networking TX soft-IRQ is last user. */ free = (ptask->outstanding_pkts == 0 && !list_empty(&ptask->pt_link)); - if (ptask->outstanding_pkts == 0) + if (ptask->outstanding_pkts == 0) { list_del(&ptask->pt_link); + dev->netdev->stats.tx_packets++; + dev->netdev->stats.tx_bytes += skb->len; + } spin_unlock_irqrestore(&dev->lock, flags); @@ -926,7 +930,6 @@ static void fwnet_transmit_packet_done(struct fwnet_packet_task *ptask) u16 fg_off; u16 datagram_label; u16 lf; - struct sk_buff *skb; /* Update the ptask to point to the next fragment and send it */ lf = fwnet_get_hdr_lf(&ptask->hdr); @@ -953,7 +956,7 @@ static void fwnet_transmit_packet_done(struct fwnet_packet_task *ptask) datagram_label = fwnet_get_hdr_dgl(&ptask->hdr); break; } - skb = ptask->skb; + skb_pull(skb, ptask->max_payload); if (ptask->outstanding_pkts > 1) { fwnet_make_sf_hdr(&ptask->hdr, RFC2374_HDR_INTFRAG, -- cgit v0.10.2 From 7ee11fa8d0a84b05cefe12b0bebc05ab0ea89cd6 Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Sat, 6 Nov 2010 16:57:28 +0100 Subject: firewire: net: fix memory leaks a) fwnet_transmit_packet_done used to poison ptask->pt_link by list_del. If fwnet_send_packet checked later whether it was responsible to clean up (in the border case that the TX soft IRQ was outpaced by the AT-req tasklet on another CPU), it missed this because ptask->pt_link was no longer shown as empty. b) If fwnet_write_complete got an rcode other than RCODE_COMPLETE, we missed to free the skb and ptask entirely. Also, count stats.tx_dropped and stats.tx_errors when rcode != 0. Signed-off-by: Stefan Richter diff --git a/drivers/firewire/net.c b/drivers/firewire/net.c index e2e968e..3a27cee 100644 --- a/drivers/firewire/net.c +++ b/drivers/firewire/net.c @@ -916,9 +916,10 @@ static void fwnet_transmit_packet_done(struct fwnet_packet_task *ptask) /* Check whether we or the networking TX soft-IRQ is last user. */ free = (ptask->outstanding_pkts == 0 && !list_empty(&ptask->pt_link)); + if (free) + list_del(&ptask->pt_link); if (ptask->outstanding_pkts == 0) { - list_del(&ptask->pt_link); dev->netdev->stats.tx_packets++; dev->netdev->stats.tx_bytes += skb->len; } @@ -973,6 +974,31 @@ static void fwnet_transmit_packet_done(struct fwnet_packet_task *ptask) fwnet_free_ptask(ptask); } +static void fwnet_transmit_packet_failed(struct fwnet_packet_task *ptask) +{ + struct fwnet_device *dev = ptask->dev; + unsigned long flags; + bool free; + + spin_lock_irqsave(&dev->lock, flags); + + /* One fragment failed; don't try to send remaining fragments. */ + ptask->outstanding_pkts = 0; + + /* Check whether we or the networking TX soft-IRQ is last user. */ + free = !list_empty(&ptask->pt_link); + if (free) + list_del(&ptask->pt_link); + + dev->netdev->stats.tx_dropped++; + dev->netdev->stats.tx_errors++; + + spin_unlock_irqrestore(&dev->lock, flags); + + if (free) + fwnet_free_ptask(ptask); +} + static void fwnet_write_complete(struct fw_card *card, int rcode, void *payload, size_t length, void *data) { @@ -980,11 +1006,12 @@ static void fwnet_write_complete(struct fw_card *card, int rcode, ptask = data; - if (rcode == RCODE_COMPLETE) + if (rcode == RCODE_COMPLETE) { fwnet_transmit_packet_done(ptask); - else + } else { fw_error("fwnet_write_complete: failed: %x\n", rcode); - /* ??? error recovery */ + fwnet_transmit_packet_failed(ptask); + } } static int fwnet_send_packet(struct fwnet_packet_task *ptask) -- cgit v0.10.2 From 48553011cea504796e513350740781ac6745f556 Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Sat, 6 Nov 2010 23:18:23 +0100 Subject: firewire: net: replace lists by counters The current transmit code does not at all make use of - fwnet_device.packet_list and only very limited use of - fwnet_device.broadcasted_list, - fwnet_device.queued_packets. Their current function is to track whether the TX soft-IRQ finished dealing with an skb when the AT-req tasklet takes over, and to discard pending tx datagrams (if there are any) when the local node is removed. The latter does actually contain a race condition bug with TX soft-IRQ and AT-req tasklet. Instead of these lists and the corresponding link in fwnet_packet_task, - a flag in fwnet_packet_task to track whether fwnet_tx is done, - a counter of queued datagrams in fwnet_device do the job as well. The above mentioned theoretic race condition is resolved by letting fwnet_remove sleep until all datagrams were flushed. It may sleep almost arbitrarily long since fwnet_remove is executed in the context of a multithreaded (concurrency managed) workqueue. The type of max_payload is changed to u16 here to avoid waste in struct fwnet_packet_task. This value cannot exceed 4096 per IEEE 1394:2008 table 16-18 (or 32678 per specification of packet headers, if there is ever going to be something else than beta mode). Signed-off-by: Stefan Richter diff --git a/drivers/firewire/net.c b/drivers/firewire/net.c index 3a27cee..d2d488b 100644 --- a/drivers/firewire/net.c +++ b/drivers/firewire/net.c @@ -7,6 +7,7 @@ */ #include +#include #include #include #include @@ -169,15 +170,8 @@ struct fwnet_device { struct fw_address_handler handler; u64 local_fifo; - /* List of packets to be sent */ - struct list_head packet_list; - /* - * List of packets that were broadcasted. When we get an ISO interrupt - * one of them has been sent - */ - struct list_head broadcasted_list; - /* List of packets that have been sent but not yet acked */ - struct list_head sent_list; + /* Number of tx datagrams that have been queued but not yet acked */ + int queued_datagrams; struct list_head peer_list; struct fw_card *card; @@ -195,7 +189,7 @@ struct fwnet_peer { unsigned pdg_size; /* pd_list size */ u16 datagram_label; /* outgoing datagram label */ - unsigned max_payload; /* includes RFC2374_FRAG_HDR_SIZE overhead */ + u16 max_payload; /* includes RFC2374_FRAG_HDR_SIZE overhead */ int node_id; int generation; unsigned speed; @@ -203,22 +197,18 @@ struct fwnet_peer { /* This is our task struct. It's used for the packet complete callback. */ struct fwnet_packet_task { - /* - * ptask can actually be on dev->packet_list, dev->broadcasted_list, - * or dev->sent_list depending on its current state. - */ - struct list_head pt_link; struct fw_transaction transaction; struct rfc2734_header hdr; struct sk_buff *skb; struct fwnet_device *dev; int outstanding_pkts; - unsigned max_payload; u64 fifo_addr; u16 dest_node; + u16 max_payload; u8 generation; u8 speed; + u8 enqueued; }; /* @@ -915,9 +905,9 @@ static void fwnet_transmit_packet_done(struct fwnet_packet_task *ptask) ptask->outstanding_pkts--; /* Check whether we or the networking TX soft-IRQ is last user. */ - free = (ptask->outstanding_pkts == 0 && !list_empty(&ptask->pt_link)); + free = (ptask->outstanding_pkts == 0 && ptask->enqueued); if (free) - list_del(&ptask->pt_link); + dev->queued_datagrams--; if (ptask->outstanding_pkts == 0) { dev->netdev->stats.tx_packets++; @@ -986,9 +976,9 @@ static void fwnet_transmit_packet_failed(struct fwnet_packet_task *ptask) ptask->outstanding_pkts = 0; /* Check whether we or the networking TX soft-IRQ is last user. */ - free = !list_empty(&ptask->pt_link); + free = ptask->enqueued; if (free) - list_del(&ptask->pt_link); + dev->queued_datagrams--; dev->netdev->stats.tx_dropped++; dev->netdev->stats.tx_errors++; @@ -1069,9 +1059,11 @@ static int fwnet_send_packet(struct fwnet_packet_task *ptask) spin_lock_irqsave(&dev->lock, flags); /* If the AT tasklet already ran, we may be last user. */ - free = (ptask->outstanding_pkts == 0 && list_empty(&ptask->pt_link)); + free = (ptask->outstanding_pkts == 0 && !ptask->enqueued); if (!free) - list_add_tail(&ptask->pt_link, &dev->broadcasted_list); + ptask->enqueued = true; + else + dev->queued_datagrams--; spin_unlock_irqrestore(&dev->lock, flags); @@ -1086,9 +1078,11 @@ static int fwnet_send_packet(struct fwnet_packet_task *ptask) spin_lock_irqsave(&dev->lock, flags); /* If the AT tasklet already ran, we may be last user. */ - free = (ptask->outstanding_pkts == 0 && list_empty(&ptask->pt_link)); + free = (ptask->outstanding_pkts == 0 && !ptask->enqueued); if (!free) - list_add_tail(&ptask->pt_link, &dev->sent_list); + ptask->enqueued = true; + else + dev->queued_datagrams--; spin_unlock_irqrestore(&dev->lock, flags); @@ -1350,10 +1344,12 @@ static netdev_tx_t fwnet_tx(struct sk_buff *skb, struct net_device *net) max_payload += RFC2374_FRAG_HDR_SIZE; } + dev->queued_datagrams++; + spin_unlock_irqrestore(&dev->lock, flags); ptask->max_payload = max_payload; - INIT_LIST_HEAD(&ptask->pt_link); + ptask->enqueued = 0; fwnet_send_packet(ptask); @@ -1487,14 +1483,9 @@ static int fwnet_probe(struct device *_dev) dev->broadcast_rcv_context = NULL; dev->broadcast_xmt_max_payload = 0; dev->broadcast_xmt_datagramlabel = 0; - dev->local_fifo = FWNET_NO_FIFO_ADDR; - - INIT_LIST_HEAD(&dev->packet_list); - INIT_LIST_HEAD(&dev->broadcasted_list); - INIT_LIST_HEAD(&dev->sent_list); + dev->queued_datagrams = 0; INIT_LIST_HEAD(&dev->peer_list); - dev->card = card; dev->netdev = net; @@ -1552,7 +1543,7 @@ static int fwnet_remove(struct device *_dev) struct fwnet_peer *peer = dev_get_drvdata(_dev); struct fwnet_device *dev = peer->dev; struct net_device *net; - struct fwnet_packet_task *ptask, *pt_next; + int i; mutex_lock(&fwnet_device_mutex); @@ -1570,21 +1561,9 @@ static int fwnet_remove(struct device *_dev) dev->card); fw_iso_context_destroy(dev->broadcast_rcv_context); } - list_for_each_entry_safe(ptask, pt_next, - &dev->packet_list, pt_link) { - dev_kfree_skb_any(ptask->skb); - kmem_cache_free(fwnet_packet_task_cache, ptask); - } - list_for_each_entry_safe(ptask, pt_next, - &dev->broadcasted_list, pt_link) { - dev_kfree_skb_any(ptask->skb); - kmem_cache_free(fwnet_packet_task_cache, ptask); - } - list_for_each_entry_safe(ptask, pt_next, - &dev->sent_list, pt_link) { - dev_kfree_skb_any(ptask->skb); - kmem_cache_free(fwnet_packet_task_cache, ptask); - } + for (i = 0; dev->queued_datagrams && i < 5; i++) + ssleep(1); + WARN_ON(dev->queued_datagrams); list_del(&dev->dev_link); free_netdev(net); -- cgit v0.10.2 From b2268830f5cf29d94b3e4a2af0b795a8f28776fe Mon Sep 17 00:00:00 2001 From: Stefan Richter Date: Sun, 14 Nov 2010 14:35:40 +0100 Subject: firewire: net: throttle TX queue before running out of tlabels This prevents firewire-net from submitting write requests in fast succession until failure due to all 64 transaction labels were used up for unfinished split transactions. The netif_stop/wake_queue API is used for this purpose. Without this stop/wake mechanism, datagrams were simply lost whenever the tlabel pool was exhausted. Plus, tlabel exhaustion by firewire-net also prevented other unrelated outbound transactions to be initiated. The chosen queue depth was checked by me to hit the maximum possible throughput with an OS X peer whose receive DMA is good enough to never reject requests due to busy inbound request FIFO. Current Linux peers show a mixed picture of -5%...+15% change in bandwidth; their current bottleneck are RCODE_BUSY situations (fewer or more, depending on TX queue depth) due to too small AR buffer in firewire-ohci. Maxim Levitsky tested this change with similar watermarks with a Linux peer and some pending firewire-ohci improvements that address the RCODE_BUSY problem and confirmed that these TX queue limits are good. Note: This removes some netif_wake_queue from reception code paths. They were apparently copy&paste artefacts from a nonsensical netif_wake_queue use in the older eth1394 driver. This belongs only into the transmit path. Signed-off-by: Stefan Richter Tested-by: Maxim Levitsky diff --git a/drivers/firewire/net.c b/drivers/firewire/net.c index d2d488b..1a467a9 100644 --- a/drivers/firewire/net.c +++ b/drivers/firewire/net.c @@ -27,8 +27,14 @@ #include #include -#define FWNET_MAX_FRAGMENTS 25 /* arbitrary limit */ -#define FWNET_ISO_PAGE_COUNT (PAGE_SIZE < 16 * 1024 ? 4 : 2) +/* rx limits */ +#define FWNET_MAX_FRAGMENTS 30 /* arbitrary, > TX queue depth */ +#define FWNET_ISO_PAGE_COUNT (PAGE_SIZE < 16*1024 ? 4 : 2) + +/* tx limits */ +#define FWNET_MAX_QUEUED_DATAGRAMS 20 /* < 64 = number of tlabels */ +#define FWNET_MIN_QUEUED_DATAGRAMS 10 /* should keep AT DMA busy enough */ +#define FWNET_TX_QUEUE_LEN FWNET_MAX_QUEUED_DATAGRAMS /* ? */ #define IEEE1394_BROADCAST_CHANNEL 31 #define IEEE1394_ALL_NODES (0xffc0 | 0x003f) @@ -640,8 +646,6 @@ static int fwnet_finish_incoming_packet(struct net_device *net, net->stats.rx_packets++; net->stats.rx_bytes += skb->len; } - if (netif_queue_stopped(net)) - netif_wake_queue(net); return 0; @@ -650,8 +654,6 @@ static int fwnet_finish_incoming_packet(struct net_device *net, net->stats.rx_dropped++; dev_kfree_skb_any(skb); - if (netif_queue_stopped(net)) - netif_wake_queue(net); return -ENOENT; } @@ -783,15 +785,10 @@ static int fwnet_incoming_packet(struct fwnet_device *dev, __be32 *buf, int len, * Datagram is not complete, we're done for the * moment. */ - spin_unlock_irqrestore(&dev->lock, flags); - - return 0; + retval = 0; fail: spin_unlock_irqrestore(&dev->lock, flags); - if (netif_queue_stopped(net)) - netif_wake_queue(net); - return retval; } @@ -891,6 +888,13 @@ static void fwnet_free_ptask(struct fwnet_packet_task *ptask) kmem_cache_free(fwnet_packet_task_cache, ptask); } +/* Caller must hold dev->lock. */ +static void dec_queued_datagrams(struct fwnet_device *dev) +{ + if (--dev->queued_datagrams == FWNET_MIN_QUEUED_DATAGRAMS) + netif_wake_queue(dev->netdev); +} + static int fwnet_send_packet(struct fwnet_packet_task *ptask); static void fwnet_transmit_packet_done(struct fwnet_packet_task *ptask) @@ -907,7 +911,7 @@ static void fwnet_transmit_packet_done(struct fwnet_packet_task *ptask) /* Check whether we or the networking TX soft-IRQ is last user. */ free = (ptask->outstanding_pkts == 0 && ptask->enqueued); if (free) - dev->queued_datagrams--; + dec_queued_datagrams(dev); if (ptask->outstanding_pkts == 0) { dev->netdev->stats.tx_packets++; @@ -978,7 +982,7 @@ static void fwnet_transmit_packet_failed(struct fwnet_packet_task *ptask) /* Check whether we or the networking TX soft-IRQ is last user. */ free = ptask->enqueued; if (free) - dev->queued_datagrams--; + dec_queued_datagrams(dev); dev->netdev->stats.tx_dropped++; dev->netdev->stats.tx_errors++; @@ -1063,7 +1067,7 @@ static int fwnet_send_packet(struct fwnet_packet_task *ptask) if (!free) ptask->enqueued = true; else - dev->queued_datagrams--; + dec_queued_datagrams(dev); spin_unlock_irqrestore(&dev->lock, flags); @@ -1082,7 +1086,7 @@ static int fwnet_send_packet(struct fwnet_packet_task *ptask) if (!free) ptask->enqueued = true; else - dev->queued_datagrams--; + dec_queued_datagrams(dev); spin_unlock_irqrestore(&dev->lock, flags); @@ -1248,6 +1252,15 @@ static netdev_tx_t fwnet_tx(struct sk_buff *skb, struct net_device *net) struct fwnet_peer *peer; unsigned long flags; + spin_lock_irqsave(&dev->lock, flags); + + /* Can this happen? */ + if (netif_queue_stopped(dev->netdev)) { + spin_unlock_irqrestore(&dev->lock, flags); + + return NETDEV_TX_BUSY; + } + ptask = kmem_cache_alloc(fwnet_packet_task_cache, GFP_ATOMIC); if (ptask == NULL) goto fail; @@ -1266,9 +1279,6 @@ static netdev_tx_t fwnet_tx(struct sk_buff *skb, struct net_device *net) proto = hdr_buf.h_proto; dg_size = skb->len; - /* serialize access to peer, including peer->datagram_label */ - spin_lock_irqsave(&dev->lock, flags); - /* * Set the transmission type for the packet. ARP packets and IP * broadcast packets are sent via GASP. @@ -1290,7 +1300,7 @@ static netdev_tx_t fwnet_tx(struct sk_buff *skb, struct net_device *net) peer = fwnet_peer_find_by_guid(dev, be64_to_cpu(guid)); if (!peer || peer->fifo == FWNET_NO_FIFO_ADDR) - goto fail_unlock; + goto fail; generation = peer->generation; dest_node = peer->node_id; @@ -1344,7 +1354,8 @@ static netdev_tx_t fwnet_tx(struct sk_buff *skb, struct net_device *net) max_payload += RFC2374_FRAG_HDR_SIZE; } - dev->queued_datagrams++; + if (++dev->queued_datagrams == FWNET_MAX_QUEUED_DATAGRAMS) + netif_stop_queue(dev->netdev); spin_unlock_irqrestore(&dev->lock, flags); @@ -1355,9 +1366,9 @@ static netdev_tx_t fwnet_tx(struct sk_buff *skb, struct net_device *net) return NETDEV_TX_OK; - fail_unlock: - spin_unlock_irqrestore(&dev->lock, flags); fail: + spin_unlock_irqrestore(&dev->lock, flags); + if (ptask) kmem_cache_free(fwnet_packet_task_cache, ptask); @@ -1403,7 +1414,7 @@ static void fwnet_init_dev(struct net_device *net) net->addr_len = FWNET_ALEN; net->hard_header_len = FWNET_HLEN; net->type = ARPHRD_IEEE1394; - net->tx_queue_len = 10; + net->tx_queue_len = FWNET_TX_QUEUE_LEN; } /* caller must hold fwnet_device_mutex */ -- cgit v0.10.2