On Thursday 22 January 2009 00:22:16 Artur Skawina wrote:
> Christian Lamparter wrote:
> > On Wednesday 21 January 2009 20:32:43 Artur Skawina wrote:
> >> Not allocating-on-receive at all worries me a bit. Will test under load. (i already
> >> had instrumented the cb, but the crashes prevented any useful testing).
> >
> > no problem... I'll wait for your data before removing the RFC/RFT tags
>
> That part is probably fine, and i'm just being paranoid. Ignore me.
so, green light? (I'll wait till friday / saturday anyway)
> >>> The net2280 tx path does at least three allocs, one tiny never-changing buffer
> >>>> and two urbs, i'd like to get rid of all of them.
> >>> why? AFAIK kernel memory alloc already provides a good amount of (small) buffer caches,
> >>> so why should stockpile them only for ourself?
> >>>
> >>> You know, 802.11b/g isn't exactly fast by any standards - heck even a 15 year old ethernet NIC
> >>> is easily 5-6 times faster. So, "optimizations" are a bit useless when we have these bottlenecks.
> >> no, i don't expect it do much difference performance-wise; i don't want it to
> >> fail under memory pressure. preallocating ~three small buffers isn't that bad ;)
> >
> > well, the memory pressure is not sooo bad in a (prioritized) kernel thread.
> > After all, the kernel reserves extra space for the kernel only and the OOM killer will become active as well...
> > So unless you got a machine with 8mb (afaik that's the lowest limit linux boots now a days and is
> > still useable!?) and a no/slowwwww swap, you'll have a really hard time to get any shortage of rx urbs at all.
>
> TX, not RX.
> I'll try stealing^Hborrowing the urbs from the refill queue (fixing up
> the rx code to allow for this first, of course).
Ah, well you have to increase the number of urbs in the "rx_list" to a total of 64 (for LM87) / 96 (for usb v1 and the old isl3887 fws)
And then add a check into the p54u_rx_refill so that it will stop as soon as there're 32 urb (with a skb) in the rx_queue.
> >>>> The constant buffer is easy - we can just kmalloc a cacheline-sized chunk on init, and (re)use that.
> >>> only a single constant buffer? are you sure that's a good idea, on dual cores?
> >>> (Or is this a misunderstanding and you plan to have up to 32/64 constant buffers?)
> >> why not? the content never changes, and will only be read by the usb host controller;
> >> the cpu shouldn't even need to see it after the initial setup.
> > Ok, I guess we're talking about different things here.
> > Please, show me a patch, before it gets too confusing ;-)
>
> ok, I was just saying that that all this:
>
> > reg = kmalloc(sizeof(*reg), GFP_ATOMIC);
> > if (!reg) {
> > printk(KERN_INFO "tx_net2280 kmalloc(reg), txqlen = %d\n",
> > skb_queue_len(&priv->common.tx_queue) );
> > return;
> > }
> > [...]
> > reg->port = cpu_to_le16(NET2280_DEV_U32);
> > reg->addr = cpu_to_le32(P54U_DEV_BASE);
> > reg->val = cpu_to_le32(ISL38XX_DEV_INT_DATA);
>
> does not need to happen for every single tx-ed frame.
Ah, yes that's true. what do you say about this...
Instead of using kmalloc in the init procedure, we let gcc already do it.
---
diff --git a/drivers/net/wireless/p54/p54usb.c b/drivers/net/wireless/p54/p54usb.c
index 9b78fee..247376f 100644
--- a/drivers/net/wireless/p54/p54usb.c
+++ b/drivers/net/wireless/p54/p54usb.c
@@ -376,47 +376,35 @@ static void p54u_tx_lm87(struct ieee80211_hw *dev, struct sk_buff *skb)
static void p54u_tx_net2280(struct ieee80211_hw *dev, struct sk_buff *skb)
{
+ const static struct net2280_reg_write reg = {
+ .port = cpu_to_le16(NET2280_DEV_U32),
+ .addr = cpu_to_le32(P54U_DEV_BASE),
+ .val = cpu_to_le32(ISL38XX_DEV_INT_DATA),
+ };
+
struct p54u_priv *priv = dev->priv;
struct urb *int_urb, *data_urb;
struct net2280_tx_hdr *hdr = (void *)skb->data - sizeof(*hdr);
- struct net2280_reg_write *reg;
int err = 0;
- reg = kmalloc(sizeof(*reg), GFP_ATOMIC);
- if (!reg)
- return;
-
int_urb = usb_alloc_urb(0, GFP_ATOMIC);
- if (!int_urb) {
- kfree(reg);
+ if (!int_urb)
return;
- }
data_urb = usb_alloc_urb(0, GFP_ATOMIC);
if (!data_urb) {
- kfree(reg);
usb_free_urb(int_urb);
return;
}
- reg->port = cpu_to_le16(NET2280_DEV_U32);
- reg->addr = cpu_to_le32(P54U_DEV_BASE);
- reg->val = cpu_to_le32(ISL38XX_DEV_INT_DATA);
-
memset(hdr, 0, sizeof(*hdr));
hdr->len = cpu_to_le16(skb->len);
hdr->device_addr = ((struct p54_hdr *) skb->data)->req_id;
usb_fill_bulk_urb(int_urb, priv->udev,
- usb_sndbulkpipe(priv->udev, P54U_PIPE_DEV), reg, sizeof(*reg),
- p54u_tx_dummy_cb, dev);
-
- /*
- * This flag triggers a code path in the USB subsystem that will
- * free what's inside the transfer_buffer after the callback routine
- * has completed.
- */
- int_urb->transfer_flags |= URB_FREE_BUFFER | URB_ZERO_PACKET;
+ usb_sndbulkpipe(priv->udev, P54U_PIPE_DEV),
+ (void *) ®, sizeof(reg), p54u_tx_dummy_cb, dev);
+ int_urb->transfer_flags |= URB_ZERO_PACKET;
usb_fill_bulk_urb(data_urb, priv->udev,
usb_sndbulkpipe(priv->udev, P54U_PIPE_DATA),
---
>
> >>>>> + if (usb_submit_urb(entry, GFP_ATOMIC)) {
> >>>> GFP_KERNEL? [would need dropping rx_queue.lock earlier and retaking in the
> >>>> (hopefully rare) error path]
> >>> why not... I don't remember the real reason why I did this complicated lock, probably
> >> You were already doing this for the skb allocation anyway ;)
> > do you mean the old "init_urbs"?
>
> I meant that you were already dropping a spinlock around one allocation, so it
> seemed odd to not to do that for the other call.
>
>
> > Well the bits I've still in mind about the "complicated lock". Was something about
> > a theroeticall race between p54u_rx_cb, the workqueue and free_urbs.
> >
> > but of course, I've never seen a oops because of it.
> >>> A updated patch is attached (as file)
> >> Will test.
> >> Are the free_urb/get_urb calls necessary? IOW why drop the reference
> >> when preparing the urb, only to grab it again in the completion?
> >
> > Oh, I'm not arguing with Alan Stern about it:.
> > http://lkml.org/lkml/2008/12/6/166
>
> 0) urb is allocated, refcount==1 # and placed on the refill list; p54u_init_urbs()
> 1) p54u_rx_refill()
> 2) urb is anchored refcount==2 # p54u_rx_refill
> 3) submit_urb() refcount==3 # in drivers/usb/core/hcd.c:usb_hcd_submit_urb
> 4) free_urb() refcount==2
> 5) ... usb transfer happens ... refcount==2
> 6) urb is unanchored refcount==1 # in drivers/usb/core/hcd.c:usb_hcd_giveback_urb()
> 7) p54u_rx_cb() # completion is called
> 8) usb_get_urb(urb) refcount==2 # unconditionally called in p54u_rx_cb()
> 9) p54u_rx_cb() returns
> 10) usb_put_urb() refcount==1 # in drivers/usb/core/hcd.c:usb_hcd_giveback_urb()
> 11) urb sits on the refill list
> 12) goto 1.
>
> IOW step 4 causes the refcount to temporarily drop to 1, but as you
> never want the urb freed in the completion, step 8 grabs another reference,
> and the refcount can never become 0.
> (for the skb-reuse case, you anchor and resubmit in step 7 (rc==3),
> then return from completion (rc==2) and restart at step 5.)
>
> Unless i missed something (i'm only looking at the patch).
> So if you omit steps 4 and 8 nothing changes, except that the refcount
> increases by one, in steps 5..7.
> The urbs are freed by p54u_free_rx_refill_list(), which drops the last ref;
> (it would need to get them all though, IOW would have to call
> usb_kill_anchored_urbs() _before_ p54u_free_rx_refill_list(). )
>
>
> Oh, and I just realized urbs are getting lost in the 'unlikely(urb->status)'
> case, both before your patch, and after.
> A simple fix, with your new code, would be to place them on the refill list,
> from where they will be eventually resubmitted.
I hope I addressed all concerns this time...
---
diff --git a/drivers/net/wireless/p54/p54usb.c b/drivers/net/wireless/p54/p54usb.c
index bf264b1..8b8dbc0 100644
--- a/drivers/net/wireless/p54/p54usb.c
+++ b/drivers/net/wireless/p54/p54usb.c
@@ -86,16 +86,18 @@ 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);
if (unlikely(urb->status)) {
dev_kfree_skb_irq(skb);
- return;
+ goto stash_urb;
}
skb_put(skb, urb->actual_length);
+ /* remove firmware/device specific headers in front of the frame */
if (priv->hw_type == P54U_NET2280)
skb_pull(skb, priv->common.tx_hdr_len);
if (priv->common.fw_interface == FW_LM87) {
@@ -103,19 +105,12 @@ static void p54u_rx_cb(struct urb *urb)
skb_put(skb, 4);
}
- 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;
- }
+ if (p54_rx(dev, skb) == 0) {
+ /*
+ * This skb can be reused.
+ * Undo all modifications and resubmit it.
+ */
- info = (struct p54u_rx_info *) skb->cb;
- info->urb = urb;
- info->dev = dev;
- urb->transfer_buffer = skb_tail_pointer(skb);
- urb->context = skb;
- } else {
if (priv->hw_type == P54U_NET2280)
skb_push(skb, priv->common.tx_hdr_len);
if (priv->common.fw_interface == FW_LM87) {
@@ -129,14 +124,46 @@ static void p54u_rx_cb(struct urb *urb)
WARN_ON(1);
urb->transfer_buffer = skb_tail_pointer(skb);
}
+
+ usb_anchor_urb(urb, &priv->submitted);
+ if (usb_submit_urb(urb, GFP_ATOMIC) == 0) {
+ skb_queue_tail(&priv->rx_queue, skb);
+ return ;
+ } else {
+ usb_unanchor_urb(urb);
+ dev_kfree_skb_irq(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);
+
+ /*
+ * This skb CANNOT be reused.
+ * Put the now unused urb into a list and do the refilled later on in
+ * the less critical workqueue thread.
+ * This eases the memory pressure and prevents latency spikes.
+ */
+
+stash_urb:
+ urb->transfer_buffer = NULL;
+ urb->context = NULL;
+
+ spin_lock_irqsave(&priv->rx_refill_lock, flags);
+ list_add_tail(&urb->urb_list, &priv->rx_refill_list);
+ spin_unlock_irqrestore(&priv->rx_refill_lock, flags);
+
+ if (unlikely(urb->status)) {
+ return;
+ }
+
+ if (unlikely(!priv->common.hw->workqueue)) {
+ /*
+ * Huh? mac80211 isn't fully initialized yet?
+ * Please check your system, something bad is going on.
+ */
+ WARN_ON(1);
+ return;
}
+
+ queue_work(priv->common.hw->workqueue, &priv->rx_refill_work);
}
static void p54u_tx_cb(struct urb *urb)
@@ -150,58 +177,112 @@ static void p54u_tx_cb(struct urb *urb)
static void p54u_tx_dummy_cb(struct urb *urb) { }
+static void p54u_free_rx_refill_list(struct ieee80211_hw *dev)
+{
+ struct p54u_priv *priv = dev->priv;
+ struct urb *entry, *tmp;
+ unsigned long flags;
+
+ spin_lock_irqsave(&priv->rx_refill_lock, flags);
+ list_for_each_entry_safe(entry, tmp, &priv->rx_refill_list, urb_list) {
+ list_del(&entry->urb_list);
+ usb_free_urb(entry);
+ }
+ spin_unlock_irqrestore(&priv->rx_refill_lock, flags);
+}
+
static void p54u_free_urbs(struct ieee80211_hw *dev)
{
struct p54u_priv *priv = dev->priv;
+
+ cancel_work_sync(&priv->rx_refill_work);
usb_kill_anchored_urbs(&priv->submitted);
}
-static int p54u_init_urbs(struct ieee80211_hw *dev)
+static int p54u_rx_refill(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;
+ unsigned int filled = 0;
+
+ spin_lock_irqsave(&priv->rx_refill_lock, flags);
+ list_for_each_entry_safe(entry, tmp, &priv->rx_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->rx_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)) {
+ /*
+ * In order to prevent a loop, we put the urb
+ * back at the _front_ of the list, so we can
+ * march on, in out-of-memory situations.
+ */
+
+ spin_lock_irqsave(&priv->rx_refill_lock, flags);
+ list_add(&entry->urb_list, &priv->rx_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);
usb_anchor_urb(entry, &priv->submitted);
- ret = usb_submit_urb(entry, GFP_KERNEL);
- if (ret) {
- skb_unlink(skb, &priv->rx_queue);
+ if (usb_submit_urb(entry, GFP_KERNEL)) {
+ /*
+ * urb submittion failed.
+ * free the associated skb and put the urb back into
+ * the front of the refill list, so we can try our luck
+ * next time.
+ */
+
usb_unanchor_urb(entry);
+ kfree_skb(skb);
+ spin_lock_irqsave(&priv->rx_refill_lock, flags);
+ list_add(&entry->urb_list, &priv->rx_refill_list);
+ } else {
+ skb_queue_tail(&priv->rx_queue, skb);
+ spin_lock_irqsave(&priv->rx_refill_lock, flags);
+ filled++;
+ }
+ }
+ spin_unlock_irqrestore(&priv->rx_refill_lock, flags);
+ return filled ? 0 : -ENOMEM;
+}
+
+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;
}
- usb_free_urb(entry);
- entry = NULL;
+
+ spin_lock_irqsave(&priv->rx_refill_lock, flags);
+ list_add_tail(&entry->urb_list, &priv->rx_refill_list);
+ spin_unlock_irqrestore(&priv->rx_refill_lock, flags);
}
- return 0;
+ p54u_rx_refill(dev);
- err:
- usb_free_urb(entry);
- kfree_skb(skb);
- p54u_free_urbs(dev);
+err:
+ p54u_free_rx_refill_list(dev);
return ret;
}
@@ -878,6 +959,14 @@ static int p54u_upload_firmware_net2280(struct ieee80211_hw *dev)
return err;
}
+static void p54u_rx_refill_work(struct work_struct *work)
+{
+ struct p54u_priv *priv = container_of(work, struct p54u_priv,
+ rx_refill_work);
+
+ p54u_rx_refill(priv->common.hw);
+}
+
static int p54u_open(struct ieee80211_hw *dev)
{
struct p54u_priv *priv = dev->priv;
@@ -888,7 +977,7 @@ static int p54u_open(struct ieee80211_hw *dev)
return err;
}
- priv->common.open = p54u_init_urbs;
+ priv->common.open = p54u_rx_refill;
return 0;
}
@@ -926,6 +1015,9 @@ static int __devinit p54u_probe(struct usb_interface *intf,
priv->intf = intf;
skb_queue_head_init(&priv->rx_queue);
init_usb_anchor(&priv->submitted);
+ INIT_WORK(&priv->rx_refill_work, p54u_rx_refill_work);
+ spin_lock_init(&priv->rx_refill_lock);
+ INIT_LIST_HEAD(&priv->rx_refill_list);
usb_get_dev(udev);
@@ -997,6 +1089,8 @@ static void __devexit p54u_disconnect(struct usb_interface *intf)
if (!dev)
return;
+ p54u_free_rx_refill_list(dev);
+
ieee80211_unregister_hw(dev);
priv = dev->priv;
diff --git a/drivers/net/wireless/p54/p54usb.h b/drivers/net/wireless/p54/p54usb.h
index 8bc5898..04c7258 100644
--- a/drivers/net/wireless/p54/p54usb.h
+++ b/drivers/net/wireless/p54/p54usb.h
@@ -134,6 +134,10 @@ struct p54u_priv {
spinlock_t lock;
struct sk_buff_head rx_queue;
+ spinlock_t rx_refill_lock;
+ struct list_head rx_refill_list;
+ struct work_struct rx_refill_work;
+
struct usb_anchor submitted;
};
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
