Re: [RFC][RFT][PATCH] p54usb: rx refill revamp

[Artur Skawina <art.08.09@gmail.com>]

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