On Wed, 30 May 2012 10:40:31 +0200
Eric Dumazet [off-list ref] wrote:

On Wed, 2012-05-30 at 09:06 +0900, Hiroaki SHIMODA wrote:

quoted

While reading the bql code, I have some questions.

1) dql_completed() and dql_queued() can be called concurrently,
   so dql->num_queued could change while processing
   dql_completed().
   Is it intentional to refer num_queued from "dql->" each time ?

not sure it can have problems, but doing the read once is indeed a good
plan.

quoted

2) From the comment in the code
   *   - The queue was over-limit in the previous interval and
   *     when enqueuing it was possible that all queued data
   *     had been consumed.

   and

   * Queue was not starved, check if the limit can be decreased.
   * A decrease is only considered if the queue has been busy in
   * the whole interval (the check above). 

   the calculation of all_prev_completed should take into account
   completed == dql->prev_num_queued case ?
   On current implementation, limit shrinks easily and some NIC
   hit TX stalls.
   To mitigate TX stalls, should we fix all_prev_completed rather
   than individual driver ?

Not sure what you mean

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.

I thought excess limit decrement induces the TX stalls.

quoted

3) limit calculation fails to consider integer wrap around in
   one place ?

Yes

quoted

Here is the patch what I meant.

diff --git a/lib/dynamic_queue_limits.c b/lib/dynamic_queue_limits.c

@@ -11,22 +11,27 @@
 #include <linux/dynamic_queue_limits.h>
 
 #define POSDIFF(A, B) ((A) > (B) ? (A) - (B) : 0)
+#define POSDIFFI(A, B) ((int)((A) - (B)) > 0 ? (A) - (B) : 0)
+#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, num_queued;
+	bool all_prev_completed;
+
+	num_queued = dql->num_queued;

I suggest :

	num_queued = ACCESS_ONCE(dql->num_queued);
	
Or else compiler is free to do whatever he wants.

Thank you for your suggestion.