Re: Strange latency spikes/TX network stalls on Sun Fire X4150(x86) and e1000e

[dave taht <dave.taht@gmail.com>]

On 05/30/2012 07:49 AM, Eric Dumazet wrote:
> On Wed, 2012-05-30 at 20:29 +0900, Hiroaki SHIMODA wrote:
>> On Wed, 30 May 2012 13:08:27 +0200
>> Eric Dumazet<eric.dumazet@gmail.com>  wrote:
>>
>>> On Wed, 2012-05-30 at 19:43 +0900, Hiroaki SHIMODA wrote:
>>>
>>>> While examining ping problem, below pattern is often observed.
>>>>
>>>>                                                 TIME
>>>>         dql_queued()         dql_completed()     |
>>>>        a) initial state                          |
>>>>                                                  |
>>>>        b) X bytes queued                         V
>>>>
>>>>        c) Y bytes queued
>>>>                             d) X bytes completed
>>>>        e) Z bytes queued
>>>>                             f) Y bytes completed
>>>>
>>>> a) dql->limit has already some value and there is no in-flight packet.
>>>> b) X bytes queued.
>>>> c) Y bytes queued and excess limit.
>>>> d) X bytes completed and dql->prev_ovlimit is set and also
>>>>     dql->prev_num_queued is set Y.
>>>> e) Z bytes queued.
>>>> f) Y bytes completed. inprogress and prev_inprogress are true.
>>>>
>>>> At f), if I read the comment correctly, all_prev_completed becomes
>>>> true and limit should be increased. But POSDIFF() ignores
>>>> (A == B) case, so limit is decreased.
>>> Which POSDIFF(), because there are many ;)
>> I mean,
>> 	all_prev_completed = POSDIFF(completed, dql->prev_num_queued);
>>
>>> By the way, given complexity of this I suggest you split your ideas in
>>> independent patches.
>> In this case, here is the patch what I thinking.
>>
>> diff --git a/lib/dynamic_queue_limits.c b/lib/dynamic_queue_limits.c
>> @@ -11,12 +11,14 @@
>>   #include<linux/dynamic_queue_limits.h>
>>
>>   #define POSDIFF(A, B) ((A)>  (B) ? (A) - (B) : 0)
>> +#define #define AFTER_EQ(A, B) ((int)((A) - (B))>= 0)
>>
>>   /* Records completed count and recalculates the queue limit */
>>   void dql_completed(struct dql *dql, unsigned int count)
>>   {
>>   	unsigned int inprogress, prev_inprogress, limit;
>> -	unsigned int ovlimit, all_prev_completed, completed;
>> +	unsigned int ovlimit, completed;
>> +	bool all_prev_completed;
>>
>>   	/* Can't complete more than what's in queue */
>>   	BUG_ON(count>  dql->num_queued - dql->num_completed);
>> @@ -26,7 +28,7 @@ void dql_completed(struct dql *dql, unsigned int count)
>>   	ovlimit = POSDIFF(dql->num_queued - dql->num_completed, limit);
>>   	inprogress = dql->num_queued - completed;
>>   	prev_inprogress = dql->prev_num_queued - dql->num_completed;
>> -	all_prev_completed = POSDIFF(completed, dql->prev_num_queued);
>> +	all_prev_completed = AFTER_EQ(completed, dql->prev_num_queued);
>>
>>   	if ((ovlimit&&  !inprogress) ||
>>   	    (dql->prev_ovlimit&&  all_prev_completed)) {
> I am fine with this one.
>
> Can you send official patches please ?

While this looks encouraging, BQL presently overbuffers by about a 
factor of 2 in low (sub 100Mbit) scenarios
on the hardware I have available to me.

I look forward to re-running benchmarks with this patch however.

>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html