This patch fixes a long standing issue in p54usb.
Under high memory pressure, dev_alloc_skb couldn't always allocate a
replacement skb. In such situations, we had to free the associated urb.
And over the time all urbs were eventually gone altogether and
obviously the device remained mute from then on.
---
diff --git a/drivers/net/wireless/p54/p54usb.c b/drivers/net/wireless/p54/p54usb.c
index bf264b1..e7b99bf 100644
--- a/drivers/net/wireless/p54/p54usb.c
+++ b/drivers/net/wireless/p54/p54usb.c
@@ -86,6 +86,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);
@@ -96,6 +97,7 @@ static void p54u_rx_cb(struct urb *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,47 @@ 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.
+ */
+
+ 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);
+
+ /*
+ * Don't let the usb stack free the queued urb after this completion
+ * callback has finished.
+ */
+ usb_get_urb(urb);
+
+ 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 +178,115 @@ static void p54u_tx_cb(struct urb *urb)
static void p54u_tx_dummy_cb(struct urb *urb) { }
+static void p54u_rx_refill_free_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;
+
usb_kill_anchored_urbs(&priv->submitted);
+ cancel_work_sync(&priv->rx_refill_work);
+ p54u_rx_refill_free_list(dev);
}
-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, flags2;
- while (skb_queue_len(&priv->rx_queue) < 32) {
+ 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;
+
+ 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);
+ 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)) {
+ /*
+ * 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.
+ */
+
+ __skb_unlink(skb, &priv->rx_queue);
+ spin_unlock_irqrestore(&priv->rx_queue.lock, flags);
+ kfree_skb(skb);
+ spin_lock_irqsave(&priv->rx_refill_lock, flags);
+ list_add(&entry->urb_list, &priv->rx_refill_list);
+ spin_unlock_irqrestore(&priv->rx_refill_lock, flags);
}
+ spin_unlock_irqrestore(&priv->rx_queue.lock, flags2);
usb_free_urb(entry);
- entry = NULL;
+ spin_lock_irqsave(&priv->rx_refill_lock, flags);
}
-
+ spin_unlock_irqrestore(&priv->rx_refill_lock, flags);
return 0;
+}
- err:
- usb_free_urb(entry);
- kfree_skb(skb);
- p54u_free_urbs(dev);
+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->rx_refill_lock, flags);
+ list_add_tail(&entry->urb_list, &priv->rx_refill_list);
+ spin_unlock_irqrestore(&priv->rx_refill_lock, flags);
+ }
+
+ p54u_rx_refill(dev);
+
+err:
+ p54u_rx_refill_free_list(dev);
return ret;
}
@@ -878,6 +963,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;
@@ -926,6 +1019,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);
Christian Lamparter wrote:
> On Thursday 22 January 2009 16:43:20 Artur Skawina wrote:
>> Christian Lamparter wrote:
>>> On Thursday 22 January 2009 00:22:16 Artur Skawina wrote:
>>> + 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);
>> please do not add WARN_ON's unless you're actually interested in the
>> stacktrace, In this case it's a usb completion, so in most cases the
>> backtrace isn't very interesting, wouldn't a printk be enough?
>> [i was hitting this when testing, and it took several seconds to
>> get all the data to the console]
>
> Ahh, wait!
>
> In fact we "should" call BUG_ON here, as mac80211 is not fully initialized at
> this point and we might have accidently submitted a dataframe to the stack.
> (Of course, this attempt by the device to send garbage to the stack is
> caught by the common-code... so no oops here)
Wouldn't you then want to catch it _before_ p54_rx()?
> However, I wonder if the WARN_ON gets triggered under normal operation or not.
> (Just in case, no it does not trigger with the ISL3887 chips)
i have never seen it, after the initial 32 times.
As-is, it currently triggers on every init however...
artur
On Thursday 22 January 2009 16:52:44 Artur Skawina wrote:
> Christian Lamparter wrote:
> > On Thursday 22 January 2009 06:40:56 Artur Skawina wrote:
> > was this with your patch to use rx_refills urb pool for tx, or without?
>
> just your patch plus the fixes that i needed to get it to work, all
> of them were in that email. The subject said RFT... ;)
> [the line #s were off, because of some extra printks logging the queue len]
>
all right, could you please put a commit message in your mail as well?
Regards,
Chr
Christian Lamparter wrote:
>> This patch makes the usb rx path alloc-less (except for the actual urb
>> submission call) which is good, but i wonder if we should try a GFP_NOWAIT
>> allocation, and only fallback if that one fails.
> Not necessary, we waste quite a lot memory by filling the rx ring with 32 useable packets.
> So there should be no shortage (anymore).
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).
>> 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 ;)
> In fact, if you have more than one GHz in your box, you should let your CPU do the
> encryption/decryption instead of the 30Mhz ARM CPU....
> this will give you a better latency for next to nothing.
BTW i tested both w/ hw encryption and w/o and both worked; saw no difference
in throughput, but didn't benchmark yet.
And no, i don't have >1GHz, the target system has probably 1/4 of that available
when it's idle, and much less when it's under load. Also i'd like to be able to
connect the device to a small fanless brick and have it do it's work (if i can find
a usable 2.6-based one, that is).
>> 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.
>> As to the urbs, i originally wanted to put (at least one of) them in the skb
>> headroom. But the fact that the skb can be freed before the completions run
>> makes that impossible.
> Not only that, but you'll shift the alloc stuff to mac80211, which uses GFP_ATOMIC to expand the head,
> if it's necessary.
increasing the allocation by one struct urb wouldn't make much difference and
avoid a kmalloc, but this doesn't matter as the lifetime of the skbs prohibits
such scheme.
>> Do you have a git tree, or some kind of patch queue, with all the pending p54 patches?
> No, In fact, Linville do all the accouting in wireless-testing :-D already.
ok, will pick them up from the list, last time i checked they weren't in
wireless-testing.
>> Working on top of wireless-testing makes it harder to test.
>> What was this patch made against?
> Strange? It should be apply cleanly on top of wireless-testing... well, give Linville some time to catch up ;-)
I just need to take in all of -rc?, which i wouldn't normally run on the
production machine, and forward port a dozen+ local branches; and all of
this just for one driver. Not a problem, it just means it takes a few days
between tests.
>>> +static void p54u_rx_refill_free_list(struct ieee80211_hw *dev)
>> the name is a bit misleading...
>> s/p54u_rx_refill_free_list/p54u_free_rx_refill_list/ ?
> dunno, it's more a namespace thing( easier to copy, paste & remember).
> but on the other hand, p54u_free_rx is better for the eyes.
rx_refill_free_list suggests that it, well, refills some list, while it
does the exact opposite.
>>>> usb_anchor_urb(entry, &priv->submitted);
>>> + 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 ;)
> 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?
p54u_free_rx_refill_list() is what frees them anyway.
artur
On Thursday 22 January 2009 06:40:56 Artur Skawina wrote:
> Artur Skawina wrote:
> > Christian Lamparter wrote:
> >> A updated patch is attached (as file)
> >
> > Will test.
>
> Did a quick test of your patch on top of
> current w-t/pending
> +
> "p54usb: fix packet loss with first generation devices"
> + this:
>
> diff --git a/drivers/net/wireless/p54/p54usb.c b/drivers/net/wireless/p54/p54usb.c
> index f754798..20818d6 100644
> --- a/drivers/net/wireless/p54/p54usb.c
> +++ b/drivers/net/wireless/p54/p54usb.c
> @@ -92,7 +92,7 @@ static void p54u_rx_cb(struct urb *urb)
>
> if (unlikely(urb->status)) {
> dev_kfree_skb_irq(skb);
> - return;
> + goto stash_urb;
> }
>
> skb_put(skb, urb->actual_length);
> @@ -141,7 +141,7 @@ static void p54u_rx_cb(struct urb *urb)
> * the less critical workqueue thread.
> * This eases the memory pressure and prevents latency spikes.
> */
> -
> +stash_urb:
> urb->transfer_buffer = NULL;
> urb->context = NULL;
>
> That didn't work, various symptoms such as hostapd getting stuck, other unrelated
> usb devices breaking, machine frozen etc.
>
> After applying the changes below it seemed to work alright, but i only tested for
> a few minutes.
> [line #s are wrong]
>
> diff --git a/drivers/net/wireless/p54/p54usb.c b/drivers/net/wireless/p54/p54usb.c
> index 20818d6..643d517 100644
> --- a/drivers/net/wireless/p54/p54usb.c
> +++ b/drivers/net/wireless/p54/p54usb.c
> @@ -160,7 +173,7 @@ stash_urb:
> * Huh? mac80211 isn't fully initialized yet?
> * Please check your system, something bad is going on.
> */
> - WARN_ON(1);
> + printk(KERN_WARNING "p54u_rx_cb workqueue missing\n");
> return;
> }
>
> @@ -196,9 +210,9 @@ static void p54u_free_urbs(struct ieee80211_hw *dev)
> {
> struct p54u_priv *priv = dev->priv;
>
> cancel_work_sync(&priv->rx_refill_work);
> - p54u_free_rx_refill_list(dev);
> usb_kill_anchored_urbs(&priv->submitted);
> + p54u_free_rx_refill_list(dev);
> }
>
> static int p54u_rx_refill(struct ieee80211_hw *dev)
> @@ -281,9 +297,9 @@ static int p54u_init_urbs(struct ieee80211_hw *dev)
> }
>
> p54u_rx_refill(dev);
> -
> err:
> - p54u_free_rx_refill_list(dev);
> + if (ret)
> + p54u_free_rx_refill_list(dev);
> return ret;
> }
>
> The 'workqueue-missing' warnings appear on module init; there are several
> p54u_init_urbs(), p54u_free_urbs() call sequences.
> p54u_free_urbs() triggers the unlikely(urb->status) path while killing
> the urbs, and the first time this happens you get 32 warnings.
> After that everything's fine.
the workqueue is available as soon as ieee80211_register_hw completed,
however before we can do that, we have to get the eeprom for the MAC...
The reason why this works without "warning" is that we recycle the skb
of the eeprom readback... so the queue_work is never executed on start up
and if it is then something is very wrong with the device itself.
> I put the machine under some load and while there was usually 28..31 RX
> urbs queued, at one point the refilling stalled and the number went down
> from 31 to 1, then 6, slowly grew to ~30, then fell to 13, then recovered.
> All of this within ~5 mins of testing, w/ only a small amount of http
> traffic generated by the client. it looks like i will have to try
> allocating in the interrupt after all.
was this with your patch to use rx_refills urb pool for tx, or without?
Regards,
Chr
On Thursday 22 January 2009 16:43:20 Artur Skawina wrote:
> Christian Lamparter wrote:
> > On Thursday 22 January 2009 00:22:16 Artur Skawina wrote:
> > + 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);
>
> please do not add WARN_ON's unless you're actually interested in the
> stacktrace, In this case it's a usb completion, so in most cases the
> backtrace isn't very interesting, wouldn't a printk be enough?
> [i was hitting this when testing, and it took several seconds to
> get all the data to the console]
Ahh, wait!
In fact we "should" call BUG_ON here, as mac80211 is not fully initialized at
this point and we might have accidently submitted a dataframe to the stack.
(Of course, this attempt by the device to send garbage to the stack is
caught by the common-code... so no oops here)
However, I wonder if the WARN_ON gets triggered under normal operation or not.
(Just in case, no it does not trigger with the ISL3887 chips)
Regards,
Chr
Christian Lamparter wrote:
> On Wednesday 21 January 2009 20:32:43 Artur Skawina wrote:
>> Christian Lamparter wrote:
>>>> This patch makes the usb rx path alloc-less (except for the actual urb
>>>> submission call) which is good, but i wonder if we should try a GFP_NOWAIT
>>>> allocation, and only fallback if that one fails.
>>> Not necessary, we waste quite a lot memory by filling the rx ring with 32 useable packets.
>>> So there should be no shortage (anymore).
>> 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.
>>> 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).
>>> In fact, if you have more than one GHz in your box, you should let your CPU do the
>>> encryption/decryption instead of the 30Mhz ARM CPU....
>>> this will give you a better latency for next to nothing.
>> BTW i tested both w/ hw encryption and w/o and both worked; saw no difference
>> in throughput, but didn't benchmark yet.
>> And no, i don't have >1GHz, the target system has probably 1/4 of that available
>> when it's idle, and much less when it's under load. Also i'd like to be able to
>> connect the device to a small fanless brick and have it do it's work (if i can find
>> a usable 2.6-based one, that is).
>
> well, the latency is usually about 0.1 - 0.2 msec better.
> However you'll get a big improvement if you change the MTU...
> As a ethernet device, the default is at 1500 octets, however the limit for WLAN is somewhere at 2274.
Good to know. As most packets will go over a ~1500 link upstream anyway
i'd rather not pay the pmtu discovery cost ;)
>>>> 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.
>>>>> + 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.
artur
Christian Lamparter wrote:
> well, I took a quick look into the usb code...
> (I know this isn't "usb_poison_anchored_urbs", or usb_kill_anchored_urbs,
> but they have to use this ones!)
>
> void usb_kill_urb(struct urb *urb)
> {
> might_sleep();
> if (!(urb && urb->dev && urb->ep))
> return;
> atomic_inc(&urb->reject);
>
> usb_hcd_unlink_urb(urb, -ENOENT);
> wait_event(usb_kill_urb_queue, atomic_read(&urb->use_count) == 0);
>
> atomic_dec(&urb->reject);
> }
>
> vs.
>
> void usb_poison_urb(struct urb *urb)
> {
> might_sleep();
> if (!(urb && urb->dev && urb->ep))
> return;
> atomic_inc(&urb->reject);
>
> usb_hcd_unlink_urb(urb, -ENOENT);
> wait_event(usb_kill_urb_queue, atomic_read(&urb->use_count) == 0);
> }
>
> it looks like usb_poison_urb doesn't do what I though it does...
> In fact the way I see it now... there's no advantage if we use it,
> we can stick usb_kill_anchored_urb, right?
the difference is that after an urb is killed it can be resubmitted;
after it's poisoned it will always fail w/ -EPERM.
usb_poison_anchored_urbs() will not only poison all anchored urbs
but also mark the anchor itself -- and the usb_anchor_urb() call will
then also poison every urb it anchors.
So once you have called usb_poison_anchored_urbs(A), you can count
on every single urb that either already was or is subsequently
anchored to A to always fail on submission.
And any urbs that were already submitted at the time of the
usb_poison_anchored_urbs(A) call will have gone through the completion.
artur
On Wednesday 21 January 2009 17:04:36 Artur Skawina wrote:
> Yes, that's one of the things that is (well, was) on my todo list after
> reading p54usb.c, so i obviously like the idea ;)
well, this patch is not so "new" anymore, I posted it a while a go.
But then we found a different, smaller fix and this work _stalled_.
> I'll write down some of the other things as they are related to this.
> It will take at least a few days for me do do and properly test, hence
> if you find the comments below useful i won't mind. ;)
not at all ;)
> This patch makes the usb rx path alloc-less (except for the actual urb
> submission call) which is good, but i wonder if we should try a GFP_NOWAIT
> allocation, and only fallback if that one fails.
Not necessary, we waste quite a lot memory by filling the rx ring with 32 useable packets.
So there should be no shortage (anymore).
> 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.
In fact, if you have more than one GHz in your box, you should let your CPU do the
encryption/decryption instead of the 30Mhz ARM CPU....
this will give you a better latency for next to nothing.
> 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?)
> As to the urbs, i originally wanted to put (at least one of) them in the skb
> headroom. But the fact that the skb can be freed before the completions run
> makes that impossible.
Not only that, but you'll shift the alloc stuff to mac80211, which uses GFP_ATOMIC to expand the head,
if it's necessary.
> Do you have a git tree, or some kind of patch queue, with all the pending p54 patches?
No, In fact, Linville do all the accouting in wireless-testing :-D already.
> Working on top of wireless-testing makes it harder to test.
> What was this patch made against?
Strange? It should be apply cleanly on top of wireless-testing... well, give Linville some time to catch up ;-)
> > static void p54u_tx_cb(struct urb *urb)
> > @@ -150,58 +178,115 @@ static void p54u_tx_cb(struct urb *urb)
> >
> > static void p54u_tx_dummy_cb(struct urb *urb) { }
> >
> > +static void p54u_rx_refill_free_list(struct ieee80211_hw *dev)
>
> the name is a bit misleading...
> s/p54u_rx_refill_free_list/p54u_free_rx_refill_list/ ?
dunno, it's more a namespace thing( easier to copy, paste & remember).
but on the other hand, p54u_free_rx is better for the eyes.
> > +static int p54u_rx_refill(struct ieee80211_hw *dev)
> > {
> > struct p54u_priv *priv = dev->priv;
> > + struct urb* entry, *tmp;
> > + unsigned long flags, flags2;
> >
> > + 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;
> > +
> > + 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 (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;
> > + spin_lock_irqsave(&priv->rx_queue.lock, flags2);
> > + __skb_queue_tail(&priv->rx_queue, skb);
> >
> > usb_anchor_urb(entry, &priv->submitted);
> > + 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
because of resume/suspend (which does not work, in case you're looking for "real" work :-D ).
> > + /*
> > + * 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.
> > + */
> > +
> > + __skb_unlink(skb, &priv->rx_queue);
> > + spin_unlock_irqrestore(&priv->rx_queue.lock, flags);
> > + kfree_skb(skb);
> > + spin_lock_irqsave(&priv->rx_refill_lock, flags);
> > + list_add(&entry->urb_list, &priv->rx_refill_list);
> > + spin_unlock_irqrestore(&priv->rx_refill_lock, flags);
>
> 'entry' is now both anchored in priv->submitted and in the rx_refill_list.
thats right, in the error path the entry has to be unachored.
A updated patch is attached (as file)
Regards,
Chr
Christian Lamparter wrote:
> On Thursday 22 January 2009 16:52:44 Artur Skawina wrote:
>> Christian Lamparter wrote:
>>> On Thursday 22 January 2009 06:40:56 Artur Skawina wrote:
>>> was this with your patch to use rx_refills urb pool for tx, or without?
>> just your patch plus the fixes that i needed to get it to work, all
>> of them were in that email. The subject said RFT... ;)
>> [the line #s were off, because of some extra printks logging the queue len]
>>
> all right, could you please put a commit message in your mail as well?
I thought you had already merged the changes, but i see you only took the
first hunk. There's no point in splitting the commit, as the intermediate
result wouldn't work. You can add my s-o-b, if you feel that's necessary.
This last version seems fine, just one thing: I can't convince myself
that not queuing the work after an urb fails with urb->status==true is
safe -- what if some temporary error condition causes the rx queue to
drain? Nothing will resubmit the urbs.
Wouldn't a usb_poison_anchored_urbs() instead of usb_kill_anchored_urbs()
in p54u_free_urbs() prevent p54u_rx_refill from resubmitting, and that early
return in the completion could then go? Or did i miss a case where it's
needed, other than stop()?
artur
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;
};
On Wednesday 21 January 2009 20:32:43 Artur Skawina wrote:
> Christian Lamparter wrote:
> >> This patch makes the usb rx path alloc-less (except for the actual urb
> >> submission call) which is good, but i wonder if we should try a GFP_NOWAIT
> >> allocation, and only fallback if that one fails.
> > Not necessary, we waste quite a lot memory by filling the rx ring with 32 useable packets.
> > So there should be no shortage (anymore).
>
> 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
> >> 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.
The only alternative, is to do it in a tasklet, however we can't use GFP_KERNEL there...
But let's wait for the results, "this is my theory" and it could be wrong (again). ;-)
> > In fact, if you have more than one GHz in your box, you should let your CPU do the
> > encryption/decryption instead of the 30Mhz ARM CPU....
> > this will give you a better latency for next to nothing.
> BTW i tested both w/ hw encryption and w/o and both worked; saw no difference
> in throughput, but didn't benchmark yet.
> And no, i don't have >1GHz, the target system has probably 1/4 of that available
> when it's idle, and much less when it's under load. Also i'd like to be able to
> connect the device to a small fanless brick and have it do it's work (if i can find
> a usable 2.6-based one, that is).
well, the latency is usually about 0.1 - 0.2 msec better.
However you'll get a big improvement if you change the MTU...
As a ethernet device, the default is at 1500 octets, however the limit for WLAN is somewhere at 2274.
> >> 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 ;-)
> >> As to the urbs, i originally wanted to put (at least one of) them in the skb
> >> headroom. But the fact that the skb can be freed before the completions run
> >> makes that impossible.
> > Not only that, but you'll shift the alloc stuff to mac80211, which uses GFP_ATOMIC to expand the head,
> > if it's necessary.
>
> increasing the allocation by one struct urb wouldn't make much difference and
> avoid a kmalloc, but this doesn't matter as the lifetime of the skbs prohibits
> such scheme.
well, to flog a dead horse a bit more urb struct is 176 bytes on x64...
And as far as I know the "worst-case" is that mac80211 has to copy the
whole packet to add more headroom, which eventually will trigger
more truesize bugs to appear?!! (don't know, maybe)
> >> Do you have a git tree, or some kind of patch queue, with all the pending p54 patches?
> > No, In fact, Linville do all the accouting in wireless-testing :-D already.
>
> ok, will pick them up from the list, last time i checked they weren't in
> wireless-testing.
well, Linville just updated the tree... however the p54usb urb_zero_packet stuff isn't there yet?!
> >> Working on top of wireless-testing makes it harder to test.
> >> What was this patch made against?
> > Strange? It should be apply cleanly on top of wireless-testing... well, give Linville some time to catch up ;-)
>
> I just need to take in all of -rc?, which i wouldn't normally run on the
> production machine, and forward port a dozen+ local branches; and all of
> this just for one driver. Not a problem, it just means it takes a few days
> between tests.
hmm, you should be able to (re)use your old kernel... all you have to do, is to get a "clone" from /wireless-testing/
and run make M=wireless-testing/drivers/net/wireless/p54... that should do the trick and you have a pair of new
modules (if you build p54common & p54usb only), as long as no one changes the API.
> >>> +static void p54u_rx_refill_free_list(struct ieee80211_hw *dev)
> >> the name is a bit misleading...
> >> s/p54u_rx_refill_free_list/p54u_free_rx_refill_list/ ?
> > dunno, it's more a namespace thing( easier to copy, paste & remember).
> > but on the other hand, p54u_free_rx is better for the eyes.
>
> rx_refill_free_list suggests that it, well, refills some list, while it
> does the exact opposite.
oh, p54u_rx_refill_ (pause) _free_list (the structure itself is called rx_refill_list as well)...
So yeah, we can bash over this as well...
> >>>> usb_anchor_urb(entry, &priv->submitted);
> >>> + 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"?
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
Regards,
Chr
Artur Skawina wrote:
> Christian Lamparter wrote:
>> A updated patch is attached (as file)
>
> Will test.
Did a quick test of your patch on top of
current w-t/pending
+
"p54usb: fix packet loss with first generation devices"
+ this:
diff --git a/drivers/net/wireless/p54/p54usb.c b/drivers/net/wireless/p54/p54usb.c
index f754798..20818d6 100644
--- a/drivers/net/wireless/p54/p54usb.c
+++ b/drivers/net/wireless/p54/p54usb.c
@@ -92,7 +92,7 @@ static void p54u_rx_cb(struct urb *urb)
if (unlikely(urb->status)) {
dev_kfree_skb_irq(skb);
- return;
+ goto stash_urb;
}
skb_put(skb, urb->actual_length);
@@ -141,7 +141,7 @@ static void p54u_rx_cb(struct urb *urb)
* the less critical workqueue thread.
* This eases the memory pressure and prevents latency spikes.
*/
-
+stash_urb:
urb->transfer_buffer = NULL;
urb->context = NULL;
That didn't work, various symptoms such as hostapd getting stuck, other unrelated
usb devices breaking, machine frozen etc.
After applying the changes below it seemed to work alright, but i only tested for
a few minutes.
[line #s are wrong]
diff --git a/drivers/net/wireless/p54/p54usb.c b/drivers/net/wireless/p54/p54usb.c
index 20818d6..643d517 100644
--- a/drivers/net/wireless/p54/p54usb.c
+++ b/drivers/net/wireless/p54/p54usb.c
@@ -160,7 +173,7 @@ stash_urb:
* Huh? mac80211 isn't fully initialized yet?
* Please check your system, something bad is going on.
*/
- WARN_ON(1);
+ printk(KERN_WARNING "p54u_rx_cb workqueue missing\n");
return;
}
@@ -196,9 +210,9 @@ static void p54u_free_urbs(struct ieee80211_hw *dev)
{
struct p54u_priv *priv = dev->priv;
cancel_work_sync(&priv->rx_refill_work);
- p54u_free_rx_refill_list(dev);
usb_kill_anchored_urbs(&priv->submitted);
+ p54u_free_rx_refill_list(dev);
}
static int p54u_rx_refill(struct ieee80211_hw *dev)
@@ -281,9 +297,9 @@ static int p54u_init_urbs(struct ieee80211_hw *dev)
}
p54u_rx_refill(dev);
-
err:
- p54u_free_rx_refill_list(dev);
+ if (ret)
+ p54u_free_rx_refill_list(dev);
return ret;
}
The 'workqueue-missing' warnings appear on module init; there are several
p54u_init_urbs(), p54u_free_urbs() call sequences.
p54u_free_urbs() triggers the unlikely(urb->status) path while killing
the urbs, and the first time this happens you get 32 warnings.
After that everything's fine.
I put the machine under some load and while there was usually 28..31 RX
urbs queued, at one point the refilling stalled and the number went down
from 31 to 1, then 6, slowly grew to ~30, then fell to 13, then recovered.
All of this within ~5 mins of testing, w/ only a small amount of http
traffic generated by the client. it looks like i will have to try
allocating in the interrupt after all.
artur
On Thursday 22 January 2009 22:45:44 Artur Skawina wrote:
> Christian Lamparter wrote:
> > On Thursday 22 January 2009 16:43:20 Artur Skawina wrote:
> >> Christian Lamparter wrote:
> >>> On Thursday 22 January 2009 00:22:16 Artur Skawina wrote:
> >>> + 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);
> >> please do not add WARN_ON's unless you're actually interested in the
> >> stacktrace, In this case it's a usb completion, so in most cases the
> >> backtrace isn't very interesting, wouldn't a printk be enough?
> >> [i was hitting this when testing, and it took several seconds to
> >> get all the data to the console]
> >
> > Ahh, wait!
> >
> > In fact we "should" call BUG_ON here, as mac80211 is not fully initialized at
> > this point and we might have accidently submitted a dataframe to the stack.
> > (Of course, this attempt by the device to send garbage to the stack is
> > caught by the common-code... so no oops here)
>
> Wouldn't you then want to catch it _before_ p54_rx()?
Well neither the device's MAC/BBP nor radio is initialized... so its garbage.
but I hoped the
+ if (unlikely(urb->status)) {
+ return;
+ }
if (unlikely(!priv->common.hw->workqueue)) {
/*
resolved this issue when usb_kill_anchored_urbs is called...
(the urb completion callback is always called, even if we don't want it...
that's why we don't reschedule a p54u_rx_refill_work and wait until urb
with a good status arrives.. )
> > However, I wonder if the WARN_ON gets triggered under normal operation or not.
> > (Just in case, no it does not trigger with the ISL3887 chips)
>
> i have never seen it, after the initial 32 times.
> As-is, it currently triggers on every init however...
well, it clearly shouldn't do that....
Off topic:
On Thursday 22 January 2009 16:43:20 Artur Skawina wrote:
> Christian Lamparter wrote:
> > On Thursday 22 January 2009 00:22:16 Artur Skawina wrote:
> >> Christian Lamparter wrote:
> >>> 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.
>
> apparently there are archs where dmaing from not-kmalloced areas doesn't work
> that well, this mostly applies to the stack, but i'd rather be safe and
> stick to a kmalloc buffer. one allocation on device init isn't worth avoiding.
agreed, the static definition is not a good idea and the usb stack also recommends
to use usb_buffer_alloc over kmalloc to avoid DMA bounce buffers usage etc...
Regards,
Chr
Christian Lamparter wrote:
> On Thursday 22 January 2009 20:19:16 Artur Skawina wrote:
>> This last version seems fine, just one thing: I can't convince myself
>> that not queuing the work after an urb fails with urb->status==true is
>> safe -- what if some temporary error condition causes the rx queue to
>> drain? Nothing will resubmit the urbs.
> well, the usb->status has to be "=! 0" 32 times in a row.
> So either the device, the system, or both have more serious problem and need
> some user attention/reset. However yes a few more unlikely paths wont hurt. ;-)
>
>> Wouldn't a usb_poison_anchored_urbs() instead of usb_kill_anchored_urbs()
>> in p54u_free_urbs() prevent p54u_rx_refill from resubmitting, and that early
>> return in the completion could then go? Or did i miss a case where it's
>> needed, other than stop()?
> size of the patch? because then we have to rewrite the p54u_start and
> p54u_stop to go a different path for ifdown/ifup (poison/unpoison) or
> suspend / disconnect (here we probably want kill).
>
> But if you want to do that, you're welcome your post patches.
How about this? Can you see anything wrong w/ it? Survives a quick test here.
Yes, unpoisoning the urbs would make things much more complicated.
(mostly because usb_anchor_urb() poisons the urb, while usb_unanchor_urb()
doesn't unpoison, so it would either have to done manually (extra locking
to get the state of the anchor itself) or the un-/poisoning rules become
quite complex)
This simple approach frees all urbs on stop() and reallocates them on open().
All urbs go through the completion, and all are moved to the refill list,
even the ones that failed. If they are not supposed to be resubmitted, all
currently in flight ones are killed and poisoned, and when they arrive in
p54u_rx_refill() the submission will fail.
artur
[patch vs your last one]
diff --git a/drivers/net/wireless/p54/p54usb.c b/drivers/net/wireless/p54/p54usb.c
index 32534c1..f0191fd 100644
--- a/drivers/net/wireless/p54/p54usb.c
+++ b/drivers/net/wireless/p54/p54usb.c
@@ -150,16 +150,12 @@ stash_urb:
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);
+ printk(KERN_WARNING "p54u_rx_cb(): workqueue missing\n");
return;
}
@@ -195,8 +191,15 @@ static void p54u_free_urbs(struct ieee80211_hw *dev)
{
struct p54u_priv *priv = dev->priv;
+ usb_poison_anchored_urbs(&priv->submitted);
cancel_work_sync(&priv->rx_refill_work);
- usb_kill_anchored_urbs(&priv->submitted);
+ p54u_free_rx_refill_list(dev);
+ /*
+ * Unpoison the anchor itself; the old urbs are already gone,
+ * p54u_rx_cb() has moved them all to the refill list.
+ * Prevents new urbs from being poisoned when anchored.
+ */
+ usb_unpoison_anchored_urbs(&priv->submitted);
}
static int p54u_rx_refill(struct ieee80211_hw *dev)
@@ -205,6 +208,7 @@ static int p54u_rx_refill(struct ieee80211_hw *dev)
struct urb *entry, *tmp;
unsigned long flags;
unsigned int filled = 0;
+ int ret = 0;
spin_lock_irqsave(&priv->rx_refill_lock, flags);
list_for_each_entry_safe(entry, tmp, &priv->rx_refill_list, urb_list) {
@@ -237,7 +241,8 @@ static int p54u_rx_refill(struct ieee80211_hw *dev)
info->dev = dev;
usb_anchor_urb(entry, &priv->submitted);
- if (usb_submit_urb(entry, GFP_KERNEL)) {
+ ret=usb_submit_urb(entry, GFP_KERNEL);
+ if (ret) {
/*
* urb submittion failed.
* free the associated skb and put the urb back into
@@ -249,6 +254,8 @@ static int p54u_rx_refill(struct ieee80211_hw *dev)
kfree_skb(skb);
spin_lock_irqsave(&priv->rx_refill_lock, flags);
list_add(&entry->urb_list, &priv->rx_refill_list);
+ if (ret==-EPERM) /* urb has been poisoned */
+ break; /* no point in trying to submit the rest */
} else {
skb_queue_tail(&priv->rx_queue, skb);
spin_lock_irqsave(&priv->rx_refill_lock, flags);
@@ -280,9 +287,9 @@ static int p54u_init_urbs(struct ieee80211_hw *dev)
}
p54u_rx_refill(dev);
-
err:
- p54u_free_rx_refill_list(dev);
+ if (ret)
+ p54u_free_rx_refill_list(dev);
return ret;
}
@@ -979,7 +986,7 @@ static int p54u_open(struct ieee80211_hw *dev)
return err;
}
- priv->common.open = p54u_rx_refill;
+ priv->common.open = p54u_init_urbs;
return 0;
}
Christian Lamparter wrote:
> This patch fixes a long standing issue in p54usb.
>
> Under high memory pressure, dev_alloc_skb couldn't always allocate a
> replacement skb. In such situations, we had to free the associated urb.
> And over the time all urbs were eventually gone altogether and
> obviously the device remained mute from then on.
Yes, that's one of the things that is (well, was) on my todo list after
reading p54usb.c, so i obviously like the idea ;)
I'll write down some of the other things as they are related to this.
It will take at least a few days for me do do and properly test, hence
if you find the comments below useful i won't mind. ;)
This patch makes the usb rx path alloc-less (except for the actual urb
submission call) which is good, but i wonder if we should try a GFP_NOWAIT
allocation, and only fallback if that one fails. Was going to do some
measuring before implementing it, to see if starvation was a risk. Maybe
only alloc in irq if the rx queue shrinks too much.
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. The constant buffer is
easy - we can just kmalloc a cacheline-sized chunk on init, and (re)use that.
As to the urbs, i originally wanted to put (at least one of) them in the skb
headroom. But the fact that the skb can be freed before the completions run
makes that impossible. That leaves keeping them around on a list and reusing
them. And as if you maintain a list for the rx path it comes naturally to
reuse it for tx as well. (obviously if the list is empty we have to create a
new urb, but the common case should be zero allocations).
Do you have a git tree, or some kind of patch queue, with all the pending p54
patches? Working on top of wireless-testing makes it harder to test.
What was this patch made against?
artur
[a few comments inline below]
> + if (p54_rx(dev, skb) == 0) {
> + /*
> + * This skb can be reused.
> + * Undo all modifications and resubmit it.
> + */
>
> if (priv->hw_type == P54U_NET2280)
> skb_push(skb, priv->common.tx_hdr_len);
> if (priv->common.fw_interface == FW_LM87) {
> @@ -129,14 +124,47 @@ 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);
> + }
> }
> +
> + /*
> + * 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.
> + */
> +
> + 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);
> +
> + /*
> + * Don't let the usb stack free the queued urb after this completion
> + * callback has finished.
> + */
> + usb_get_urb(urb);
> +
> + 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 +178,115 @@ static void p54u_tx_cb(struct urb *urb)
>
> static void p54u_tx_dummy_cb(struct urb *urb) { }
>
> +static void p54u_rx_refill_free_list(struct ieee80211_hw *dev)
the name is a bit misleading...
s/p54u_rx_refill_free_list/p54u_free_rx_refill_list/ ?
> +{
> + 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;
> +
> usb_kill_anchored_urbs(&priv->submitted);
> + cancel_work_sync(&priv->rx_refill_work);
> + p54u_rx_refill_free_list(dev);
> }
>
> +static int p54u_rx_refill(struct ieee80211_hw *dev)
> {
> struct p54u_priv *priv = dev->priv;
> + struct urb* entry, *tmp;
> + unsigned long flags, flags2;
>
> + 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;
> +
> + 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 (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;
> + spin_lock_irqsave(&priv->rx_queue.lock, flags2);
> + __skb_queue_tail(&priv->rx_queue, skb);
>
> usb_anchor_urb(entry, &priv->submitted);
> + if (usb_submit_urb(entry, GFP_ATOMIC)) {
GFP_KERNEL? [would need dropping rx_queue.lock earlier and retaking in the
(hopefully rare) error path]
> + /*
> + * 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.
> + */
> +
> + __skb_unlink(skb, &priv->rx_queue);
> + spin_unlock_irqrestore(&priv->rx_queue.lock, flags);
> + kfree_skb(skb);
> + spin_lock_irqsave(&priv->rx_refill_lock, flags);
> + list_add(&entry->urb_list, &priv->rx_refill_list);
> + spin_unlock_irqrestore(&priv->rx_refill_lock, flags);
'entry' is now both anchored in priv->submitted and in the rx_refill_list.
also, a ref will be dropped by the free_urb() below.
> }
> + spin_unlock_irqrestore(&priv->rx_queue.lock, flags2);
> usb_free_urb(entry);
> + spin_lock_irqsave(&priv->rx_refill_lock, flags);
> }
> + spin_unlock_irqrestore(&priv->rx_refill_lock, flags);
> return 0;
> +}
>>> that not queuing the work after an urb fails with urb->status==true is
>>> safe -- what if some temporary error condition causes the rx queue to
>>> drain? Nothing will resubmit the urbs.
Here's an updated version, hopefully the extra commentary makes it
a bit more obvious. I added the allocate-in-irq fallback too.
Tested, including the fallback path, works.
The !priv->common.hw->workqueue path only triggers when freeing the urbs
after reading the eeprom (ie while killing/poisoning the urbs), hence it is
completely harmless and i would replace it w/ just:
if (priv->common.hw->workqueue)
queue_work(priv->common.hw->workqueue, &priv->rx_refill_work);
Other than that, patch seems ready.
Thanks,
artur
commit c93ede1be61a0db0a904424738afe3b75425a782
Author: Artur Skawina <[email protected]>
Date: Thu Jan 22 21:01:32 2009 +0100
Signed-off-by: Artur Skawina <[email protected]>
diff --git a/drivers/net/wireless/p54/p54usb.c b/drivers/net/wireless/p54/p54usb.c
index 32534c1..872ace1 100644
--- a/drivers/net/wireless/p54/p54usb.c
+++ b/drivers/net/wireless/p54/p54usb.c
@@ -80,6 +80,15 @@ static struct usb_device_id p54u_table[] __devinitdata = {
MODULE_DEVICE_TABLE(usb, p54u_table);
+/*
+ * Every successfully submitted RX urb goes through this completion
+ * function. Each urb must be added to the refill list after being
+ * processed, this includes the failed/canceled ones (status!=0).
+ * p54u_rx_refill() will take care of resubmitting them later.
+ * Alternatively, an urb can be reanchored and resubmitted, it will
+ * then come back here again, after the I/O is done.
+ */
+
static void p54u_rx_cb(struct urb *urb)
{
struct sk_buff *skb = (struct sk_buff *) urb->context;
@@ -124,7 +133,7 @@ static void p54u_rx_cb(struct urb *urb)
WARN_ON(1);
urb->transfer_buffer = skb_tail_pointer(skb);
}
-
+resubmit_urb: /* Now both the urb and the skb are as good as new */
usb_anchor_urb(urb, &priv->submitted);
if (usb_submit_urb(urb, GFP_ATOMIC) == 0) {
skb_queue_tail(&priv->rx_queue, skb);
@@ -133,11 +142,32 @@ static void p54u_rx_cb(struct urb *urb)
usb_unanchor_urb(urb);
dev_kfree_skb_irq(skb);
}
+ } else {
+ /*
+ * This skb has been given away to the mac80211 layer.
+ * We should put the urb on the refill list, where it
+ * can be linked to a newly allocated skb later.
+ * Except, if the workqueue that does the refilling can't
+ * keep up, we'll try a bit harder and attempt to obtain
+ * a new skb right here.
+ */
+ if (skb_queue_len(&priv->rx_queue)<4) {
+ skb = dev_alloc_skb(priv->common.rx_mtu + 32);
+ if (skb) {
+ info = (struct p54u_rx_info *)skb->cb;
+ info->urb = urb;
+ info->dev = dev;
+ /* Update the urb to use the new skb */
+ urb->transfer_buffer = skb_tail_pointer(skb);
+ urb->context = skb;
+ goto resubmit_urb;
+ }
+ }
}
/*
* This skb CANNOT be reused.
- * Put the now unused urb into a list and do the refilled later on in
+ * Put the now unused urb into a list and do the refill later on in
* the less critical workqueue thread.
* This eases the memory pressure and prevents latency spikes.
*/
@@ -150,16 +180,12 @@ stash_urb:
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);
+ printk(KERN_WARNING "p54u_rx_cb(): workqueue missing\n");
return;
}
@@ -195,8 +221,21 @@ static void p54u_free_urbs(struct ieee80211_hw *dev)
{
struct p54u_priv *priv = dev->priv;
+ usb_poison_anchored_urbs(&priv->submitted);
+ /*
+ * By now every RX urb has either finished or been cancelled;
+ * the p54u_rx_cb() completion has placed it on the refill
+ * list; any attempts to resubmit from p54u_rx_refill(),
+ * which could still be scheduled to run, will fail.
+ */
cancel_work_sync(&priv->rx_refill_work);
- usb_kill_anchored_urbs(&priv->submitted);
+ p54u_free_rx_refill_list(dev);
+ /*
+ * Unpoison the anchor itself; the old urbs are already gone,
+ * p54u_rx_cb() has moved them all to the refill list.
+ * Prevents new urbs from being poisoned when anchored.
+ */
+ usb_unpoison_anchored_urbs(&priv->submitted);
}
static int p54u_rx_refill(struct ieee80211_hw *dev)
@@ -205,6 +244,7 @@ static int p54u_rx_refill(struct ieee80211_hw *dev)
struct urb *entry, *tmp;
unsigned long flags;
unsigned int filled = 0;
+ int ret = 0;
spin_lock_irqsave(&priv->rx_refill_lock, flags);
list_for_each_entry_safe(entry, tmp, &priv->rx_refill_list, urb_list) {
@@ -237,7 +277,8 @@ static int p54u_rx_refill(struct ieee80211_hw *dev)
info->dev = dev;
usb_anchor_urb(entry, &priv->submitted);
- if (usb_submit_urb(entry, GFP_KERNEL)) {
+ ret=usb_submit_urb(entry, GFP_KERNEL);
+ if (ret) {
/*
* urb submittion failed.
* free the associated skb and put the urb back into
@@ -249,6 +290,8 @@ static int p54u_rx_refill(struct ieee80211_hw *dev)
kfree_skb(skb);
spin_lock_irqsave(&priv->rx_refill_lock, flags);
list_add(&entry->urb_list, &priv->rx_refill_list);
+ if (ret==-EPERM) /* urb has been poisoned */
+ break; /* no point in trying to submit the rest */
} else {
skb_queue_tail(&priv->rx_queue, skb);
spin_lock_irqsave(&priv->rx_refill_lock, flags);
@@ -280,9 +323,9 @@ static int p54u_init_urbs(struct ieee80211_hw *dev)
}
p54u_rx_refill(dev);
-
err:
- p54u_free_rx_refill_list(dev);
+ if (ret)
+ p54u_free_rx_refill_list(dev);
return ret;
}
@@ -979,7 +1022,7 @@ static int p54u_open(struct ieee80211_hw *dev)
return err;
}
- priv->common.open = p54u_rx_refill;
+ priv->common.open = p54u_init_urbs;
return 0;
}
Christian Lamparter wrote:
> 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)
see the email i sent after testing -- rx queue starvation seems indeed possible.
I'll send a patch after i figure out and test a solution (hopefully only
allocating the skb if the skb_queue_len is low will fix it).
>> 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.
Why would you want to increase the number? I'll send a patch, after this one
becomes stable.
>> 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.
apparently there are archs where dmaing from not-kmalloced areas doesn't work
that well, this mostly applies to the stack, but i'd rather be safe and
stick to a kmalloc buffer. one allocation on device init isn't worth avoiding.
> + 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);
please do not add WARN_ON's unless you're actually interested in the
stacktrace, In this case it's a usb completion, so in most cases the
backtrace isn't very interesting, wouldn't a printk be enough?
[i was hitting this when testing, and it took several seconds to
get all the data to the console]
> I hope I addressed all concerns this time...
I'll send a patch later today ;)
artur
Christian Lamparter wrote:
> This patch fixes a long standing issue in p54usb.
>=20
> Under high memory pressure, dev_alloc_skb couldn't always allocate a=20
> replacement skb. In such situations, we had to free the associated ur=
b.
> And over the time all urbs were eventually gone altogether and=20
> obviously the device remained mute from then on.
With this patch, I get lots of missing rx_refill_* messages such as
drivers/net/wireless/p54/p54usb.c:1024: error: =E2=80=98struct p54u_pri=
v=E2=80=99 has no member named
=E2=80=98rx_refill_work=E2=80=99
drivers/net/wireless/p54/p54usb.c:1025: error: =E2=80=98struct p54u_pri=
v=E2=80=99 has no member named
=E2=80=98rx_refill_lock=E2=80=99
drivers/net/wireless/p54/p54usb.c:1026: error: =E2=80=98struct p54u_pri=
v=E2=80=99 has no member named
=E2=80=98rx_refill_list=E2=80=99
Did I miss a patch? I did a search for the string "rx_refill" in my ent=
ire mailbox and
found only this mail and Artur's reply.
Larry
On Wednesday 21 January 2009 21:06:54 Larry Finger wrote:
> Christian Lamparter wrote:
> > This patch fixes a long standing issue in p54usb.
> >=20
> > Under high memory pressure, dev_alloc_skb couldn't always allocate =
a=20
> > replacement skb. In such situations, we had to free the associated =
urb.
> > And over the time all urbs were eventually gone altogether and=20
> > obviously the device remained mute from then on.
>=20
> With this patch, I get lots of missing rx_refill_* messages such as
>=20
> drivers/net/wireless/p54/p54usb.c:1024: error: =E2=80=98struct p54u_p=
riv=E2=80=99 has no member named
> =E2=80=98rx_refill_work=E2=80=99
> drivers/net/wireless/p54/p54usb.c:1025: error: =E2=80=98struct p54u_p=
riv=E2=80=99 has no member named
> =E2=80=98rx_refill_lock=E2=80=99
> drivers/net/wireless/p54/p54usb.c:1026: error: =E2=80=98struct p54u_p=
riv=E2=80=99 has no member named
> =E2=80=98rx_refill_list=E2=80=99
>=20
> Did I miss a patch? I did a search for the string "rx_refill" in my e=
ntire mailbox and
> found only this mail and Artur's reply.
>=20
No, you don't miss anything... here's the other p54usb.h change.
Sorry!
Regards,
Chr
---
diff --git a/drivers/net/wireless/p54/p54usb.h b/drivers/net/wireless/p=
54/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 {
=20
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;
};
=20
On Thursday 22 January 2009 20:19:16 Artur Skawina wrote:
> Christian Lamparter wrote:
> > On Thursday 22 January 2009 16:52:44 Artur Skawina wrote:
> >> Christian Lamparter wrote:
> >>> On Thursday 22 January 2009 06:40:56 Artur Skawina wrote:
> >>> was this with your patch to use rx_refills urb pool for tx, or without?
> >> just your patch plus the fixes that i needed to get it to work, all
> >> of them were in that email. The subject said RFT... ;)
> >> [the line #s were off, because of some extra printks logging the queue len]
> >>
> > all right, could you please put a commit message in your mail as well?
>
> I thought you had already merged the changes, but i see you only took the
> first hunk. There's no point in splitting the commit, as the intermediate
> result wouldn't work. You can add my s-o-b, if you feel that's necessary.
> This last version seems fine, just one thing: I can't convince myself
> that not queuing the work after an urb fails with urb->status==true is
> safe -- what if some temporary error condition causes the rx queue to
> drain? Nothing will resubmit the urbs.
well, the usb->status has to be "=! 0" 32 times in a row.
So either the device, the system, or both have more serious problem and need
some user attention/reset. However yes a few more unlikely paths wont hurt. ;-)
> Wouldn't a usb_poison_anchored_urbs() instead of usb_kill_anchored_urbs()
> in p54u_free_urbs() prevent p54u_rx_refill from resubmitting, and that early
> return in the completion could then go? Or did i miss a case where it's
> needed, other than stop()?
size of the patch? because then we have to rewrite the p54u_start and
p54u_stop to go a different path for ifdown/ifup (poison/unpoison) or
suspend / disconnect (here we probably want kill).
But if you want to do that, you're welcome your post patches.
Regards,
Chr
On Thursday 22 January 2009 23:05:37 Artur Skawina wrote:
> Christian Lamparter wrote:
> > On Thursday 22 January 2009 20:19:16 Artur Skawina wrote:
> >> This last version seems fine, just one thing: I can't convince myself
> >> that not queuing the work after an urb fails with urb->status==true is
> >> safe -- what if some temporary error condition causes the rx queue to
> >> drain? Nothing will resubmit the urbs.
> > well, the usb->status has to be "=! 0" 32 times in a row.
> > So either the device, the system, or both have more serious problem and need
> > some user attention/reset. However yes a few more unlikely paths wont hurt. ;-)
> >
> >> Wouldn't a usb_poison_anchored_urbs() instead of usb_kill_anchored_urbs()
> >> in p54u_free_urbs() prevent p54u_rx_refill from resubmitting, and that early
> >> return in the completion could then go? Or did i miss a case where it's
> >> needed, other than stop()?
> > size of the patch? because then we have to rewrite the p54u_start and
> > p54u_stop to go a different path for ifdown/ifup (poison/unpoison) or
> > suspend / disconnect (here we probably want kill).
> >
> > But if you want to do that, you're welcome your post patches.
>
> How about this? Can you see anything wrong w/ it? Survives a quick test here.
>
> Yes, unpoisoning the urbs would make things much more complicated.
> (mostly because usb_anchor_urb() poisons the urb, while usb_unanchor_urb()
> doesn't unpoison, so it would either have to done manually (extra locking
> to get the state of the anchor itself) or the un-/poisoning rules become
> quite complex)
>
> This simple approach frees all urbs on stop() and reallocates them on open().
> All urbs go through the completion, and all are moved to the refill list,
> even the ones that failed. If they are not supposed to be resubmitted, all
> currently in flight ones are killed and poisoned, and when they arrive in
> p54u_rx_refill() the submission will fail.
>
> artur
>
well, I took a quick look into the usb code...
(I know this isn't "usb_poison_anchored_urbs", or usb_kill_anchored_urbs,
but they have to use this ones!)
void usb_kill_urb(struct urb *urb)
{
might_sleep();
if (!(urb && urb->dev && urb->ep))
return;
atomic_inc(&urb->reject);
usb_hcd_unlink_urb(urb, -ENOENT);
wait_event(usb_kill_urb_queue, atomic_read(&urb->use_count) == 0);
atomic_dec(&urb->reject);
}
vs.
void usb_poison_urb(struct urb *urb)
{
might_sleep();
if (!(urb && urb->dev && urb->ep))
return;
atomic_inc(&urb->reject);
usb_hcd_unlink_urb(urb, -ENOENT);
wait_event(usb_kill_urb_queue, atomic_read(&urb->use_count) == 0);
}
it looks like usb_poison_urb doesn't do what I though it does...
In fact the way I see it now... there's no advantage if we use it,
we can stick usb_kill_anchored_urb, right?
Well, let's make a 2nd RFC/RFT tomorrow.
Good night,
Chr
Christian Lamparter wrote:
> On Thursday 22 January 2009 06:40:56 Artur Skawina wrote:
> was this with your patch to use rx_refills urb pool for tx, or without?
just your patch plus the fixes that i needed to get it to work, all
of them were in that email. The subject said RFT... ;)
[the line #s were off, because of some extra printks logging the queue len]
artur