On Thursday 18 December 2008 03:28:54 Larry Finger wrote:
> Christian Lamparter wrote:
> > hmm, I wonder why it's a "order:1" allocation, even on x86-64 the skb_shared_struct is less than 300 bytes
> > and the maximum rx_mtu size about 3240 so there should be room left.... of course, it's not really a big
> > deal for p54, since we don't have to support frames larger RTS or Fragmentation threshold anyway...
> > but what about 11n devices? aren't they suffer from the same problems under load?
>
> The actual size requested is 520 bytes bigger than what is asked for plus the L1
> cache alignment, which is getting close to 4000 bytes. If I missed something, it
> could be over 4096.
>
> Larry
Oh well, looks like iwlwifi had this problems as well:
http://git.kernel.org/?p=linux/kernel/git/linville/wireless-testing.git;a=commit;h=7b52beeab30692f4b92c8efc9f806794c6b03473
anyway, I've attached an early RFC, in which the RX refilling is offloaded into a workqueue.
The locking is a maybe a bit overkill, and make C=2 produces a warning, (not to mention checkpatch.pl ;) )
but it didn't crash/freeze/burn here and Its really late here.
Regards,
Chr
diff -Nurp a/drivers/net/wireless/p54/p54usb.c b/drivers/net/wireless/p54/p54usb.c
--- a/drivers/net/wireless/p54/p54usb.c 2008-12-19 00:31:46.000000000 +0100
+++ b/drivers/net/wireless/p54/p54usb.c 2008-12-19 02:42:05.000000000 +0100
@@ -85,6 +85,7 @@ static void p54u_rx_cb(struct urb *urb)
struct p54u_rx_info *info = (struct p54u_rx_info *)skb->cb;
struct ieee80211_hw *dev = info->dev;
struct p54u_priv *priv = dev->priv;
+ unsigned long flags;
skb_unlink(skb, &priv->rx_queue);
@@ -103,17 +104,17 @@ static void p54u_rx_cb(struct urb *urb)
}
if (p54_rx(dev, skb)) {
- skb = dev_alloc_skb(priv->common.rx_mtu + 32);
- if (unlikely(!skb)) {
- /* TODO check rx queue length and refill *somewhere* */
- return;
- }
-
- info = (struct p54u_rx_info *) skb->cb;
- info->urb = urb;
- info->dev = dev;
- urb->transfer_buffer = skb_tail_pointer(skb);
- urb->context = skb;
+ spin_lock_irqsave(&priv->refill_lock, flags);
+ list_add_tail(&urb->urb_list, &priv->refill_list);
+ spin_unlock_irqrestore(&priv->refill_lock, flags);
+ urb->transfer_buffer = NULL;
+ urb->context = NULL;
+
+ /*
+ * don't let the usb stack free the urb.
+ */
+ usb_get_urb(urb);
+ schedule_work(&priv->work);
} else {
if (priv->hw_type == P54U_NET2280)
skb_push(skb, priv->common.tx_hdr_len);
@@ -128,13 +129,13 @@ static void p54u_rx_cb(struct urb *urb)
WARN_ON(1);
urb->transfer_buffer = skb_tail_pointer(skb);
}
- }
- skb_queue_tail(&priv->rx_queue, skb);
- usb_anchor_urb(urb, &priv->submitted);
- if (usb_submit_urb(urb, GFP_ATOMIC)) {
- skb_unlink(skb, &priv->rx_queue);
- usb_unanchor_urb(urb);
- dev_kfree_skb_irq(skb);
+
+ usb_anchor_urb(urb, &priv->submitted);
+ if (usb_submit_urb(urb, GFP_ATOMIC)) {
+ usb_unanchor_urb(urb);
+ dev_kfree_skb_irq(skb);
+ } else
+ skb_queue_tail(&priv->rx_queue, skb);
}
}
@@ -152,58 +153,101 @@ static void p54u_tx_cb(struct urb *urb)
static void p54u_tx_dummy_cb(struct urb *urb) { }
+static void p54u_free_refill(struct ieee80211_hw *dev)
+{
+ struct p54u_priv *priv = dev->priv;
+ struct urb *entry, *tmp;
+ unsigned long flags;
+
+ spin_lock_irqsave(&priv->refill_lock, flags);
+ list_for_each_entry_safe(entry, tmp, &priv->refill_list, urb_list) {
+ list_del(&entry->urb_list);
+ usb_free_urb(entry);
+ }
+ spin_unlock_irqrestore(&priv->refill_lock, flags);
+}
+
static void p54u_free_urbs(struct ieee80211_hw *dev)
{
struct p54u_priv *priv = dev->priv;
usb_kill_anchored_urbs(&priv->submitted);
+ cancel_work_sync(&priv->work);
+ p54u_free_refill(dev);
}
-static int p54u_init_urbs(struct ieee80211_hw *dev)
+static int p54u_refill_rx(struct ieee80211_hw *dev)
{
struct p54u_priv *priv = dev->priv;
- struct urb *entry = NULL;
- struct sk_buff *skb;
- struct p54u_rx_info *info;
- int ret = 0;
+ struct urb* entry, *tmp;
+ unsigned long flags, flags2;
+
+ spin_lock_irqsave(&priv->refill_lock, flags);
+ list_for_each_entry_safe(entry, tmp, &priv->refill_list, urb_list) {
+ struct p54u_rx_info *info;
+ struct sk_buff *skb;
- while (skb_queue_len(&priv->rx_queue) < 32) {
+ list_del(&entry->urb_list);
+ spin_unlock_irqrestore(&priv->refill_lock, flags);
skb = __dev_alloc_skb(priv->common.rx_mtu + 32, GFP_KERNEL);
- if (!skb) {
- ret = -ENOMEM;
- goto err;
- }
- entry = usb_alloc_urb(0, GFP_KERNEL);
- if (!entry) {
- ret = -ENOMEM;
- goto err;
+
+ if (unlikely(!skb)) {
+ spin_lock_irqsave(&priv->refill_lock, flags);
+ list_add(&entry->urb_list, &priv->refill_list);
+ continue;
}
usb_fill_bulk_urb(entry, priv->udev,
usb_rcvbulkpipe(priv->udev, P54U_PIPE_DATA),
skb_tail_pointer(skb),
priv->common.rx_mtu + 32, p54u_rx_cb, skb);
+
info = (struct p54u_rx_info *) skb->cb;
info->urb = entry;
info->dev = dev;
- skb_queue_tail(&priv->rx_queue, skb);
+ spin_lock_irqsave(&priv->rx_queue.lock, flags2);
+ __skb_queue_tail(&priv->rx_queue, skb);
usb_anchor_urb(entry, &priv->submitted);
- ret = usb_submit_urb(entry, GFP_KERNEL);
- if (ret) {
- skb_unlink(skb, &priv->rx_queue);
- usb_unanchor_urb(entry);
- goto err;
+ if (usb_submit_urb(entry, GFP_ATOMIC)) {
+ __skb_unlink(skb, &priv->rx_queue);
+ spin_unlock_irqrestore(&priv->rx_queue.lock, flags2);
+ spin_lock_irqsave(&priv->refill_lock, flags);
+ list_add_tail(&entry->urb_list, &priv->refill_list);
+ spin_unlock_irqrestore(&priv->refill_lock, flags);
+ kfree_skb(skb);
}
+ spin_unlock_irqrestore(&priv->rx_queue.lock, flags2);
usb_free_urb(entry);
- entry = NULL;
+ spin_lock_irqsave(&priv->refill_lock, flags);
}
-
+ spin_unlock_irqrestore(&priv->refill_lock, flags);
return 0;
+}
+
+static int p54u_init_urbs(struct ieee80211_hw *dev)
+{
+ struct p54u_priv *priv = dev->priv;
+ struct urb *entry;
+ unsigned long flags;
+ int ret = 0;
+ int i;
+
+ for (i = 0; i < 32; i++) {
+ entry = usb_alloc_urb(0, GFP_KERNEL);
+ if (!entry) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ spin_lock_irqsave(&priv->refill_lock, flags);
+ list_add_tail(&entry->urb_list, &priv->refill_list);
+ spin_unlock_irqrestore(&priv->refill_lock, flags);
+ }
+
+ p54u_refill_rx(dev);
err:
- usb_free_urb(entry);
- kfree_skb(skb);
- p54u_free_urbs(dev);
+ p54u_free_refill(dev);
return ret;
}
@@ -834,6 +878,12 @@ static int p54u_upload_firmware_net2280(
return err;
}
+static void p54u_work(struct work_struct *work)
+{
+ struct p54u_priv *priv = container_of(work, struct p54u_priv, work);
+ p54u_refill_rx(priv->common.hw);
+}
+
static int p54u_open(struct ieee80211_hw *dev)
{
struct p54u_priv *priv = dev->priv;
@@ -924,6 +974,9 @@ static int __devinit p54u_probe(struct u
skb_queue_head_init(&priv->rx_queue);
init_usb_anchor(&priv->submitted);
+ INIT_WORK(&priv->work, p54u_work);
+ spin_lock_init(&priv->refill_lock);
+ INIT_LIST_HEAD(&priv->refill_list);
p54u_open(dev);
err = p54_read_eeprom(dev);
diff -Nurp a/drivers/net/wireless/p54/p54usb.h b/drivers/net/wireless/p54/p54usb.h
--- a/drivers/net/wireless/p54/p54usb.h 2008-12-19 00:31:48.000000000 +0100
+++ b/drivers/net/wireless/p54/p54usb.h 2008-12-19 01:33:06.000000000 +0100
@@ -134,6 +134,9 @@ struct p54u_priv {
spinlock_t lock;
struct sk_buff_head rx_queue;
struct usb_anchor submitted;
+ spinlock_t refill_lock;
+ struct list_head refill_list;
+ struct work_struct work;
};
#endif /* P54USB_H */
